What I'm trying to achieve:

@ConfigurationProperties
record ApplicationProps(Instant instant) {}

with the following application.properties:

instant = 1680612880417

where the value is millis since the epoch, see Instant.ofEpochMilli().

Edit: Currently this fails with a

...
Caused by: java.lang.IllegalArgumentException: Parse attempt failed for value [123456789]
    at org.springframework.format.support.FormattingConversionService$ParserConverter.convert(FormattingConversionService.java:224) ~[spring-context-6.0.6.jar:6.0.6]
    at org.springframework.core.convert.support.ConversionUtils.invokeConverter(ConversionUtils.java:41) ~[spring-core-6.0.6.jar:6.0.6]
    ... 117 common frames omitted
Caused by: java.time.format.DateTimeParseException: Text '123456789' could not be parsed at index 0

Perhaps this is a good candidate for a default binding.

Considerations: - Why are we choosing Instant.ofEpochMilli() and not Instant.ofEpochSecond()? It just felt more natural. Is this uncertainty reason enough to refuse this request? That said, there already is a registered-by-default Converter<Long, Instant> which uses ofEpochMilli. Therefore, an additional Formatter would be consistent with the already-existing Converter. - Do we also need to add more similar formatters for types like java.util.Date or java.sql.Timestamp? I do no think think so as neither of them has a simple ofEpochMilli()-like creator method or clear semantics. Other java.time also are not good targets for this unless OffsetDateTime/ZonedDateTime is considered okay given a default timezone.

Comment From: snicoll

Why are we choosing Instant.ofEpochMilli() and not Instant.ofEpochSecond()?

Because the usual way of representing a "time" is by using the number of ms, not seconds. I understand that you'd prefer seconds but that's not what the default representation is at the moment: the seconds variant is merely a shortcut for those who don't need ms precision.

Both inputs are numbers so it's impossible for us to know which format you want to use automatically. I can't even suggest to configure a custom converter, unless you want to trump the default one for every fields.

Having said all of that, I am not sure I understand what you'd expect from us. Can you clarify?

Comment From: JanecekPetr

I understand that you'd prefer seconds

Sorry, maybe I need to rephrase my question as that was not what I meant. I absolutely prefer millis and I agree. I was just adding considerations and alternatives that someone might ask for instead.

Can you clarify?

Sure! (Should I also edit the original question?)

Currently it is not possible to use a timestamp as a property of type Instant. I'd like Spring to parse that, ideally by default. My suggestion is for you to add this capability to the existing InstantFormatter.

If you try the code I suggested above, it fails with:

...
Caused by: org.springframework.beans.TypeMismatchException: Failed to convert value of type 'java.lang.String' to required type 'java.time.Instant'; Failed to convert from type [java.lang.String] to type [@org.springframework.beans.factory.annotation.Value java.time.Instant] for value '123456789'
    at org.springframework.beans.TypeConverterSupport.convertIfNecessary(TypeConverterSupport.java:79) ~[spring-beans-6.0.6.jar:6.0.6]
    at org.springframework.beans.factory.support.DefaultListableBeanFactory.doResolveDependency(DefaultListableBeanFactory.java:1353) ~[spring-beans-6.0.6.jar:6.0.6]
    at org.springframework.beans.factory.support.DefaultListableBeanFactory.resolveDependency(DefaultListableBeanFactory.java:1325) ~[spring-beans-6.0.6.jar:6.0.6]
    at org.springframework.beans.factory.support.ConstructorResolver.resolveAutowiredArgument(ConstructorResolver.java:885) ~[spring-beans-6.0.6.jar:6.0.6]
    at org.springframework.beans.factory.support.ConstructorResolver.createArgumentArray(ConstructorResolver.java:789) ~[spring-beans-6.0.6.jar:6.0.6]
    ... 110 common frames omitted
Caused by: org.springframework.core.convert.ConversionFailedException: Failed to convert from type [java.lang.String] to type [@org.springframework.beans.factory.annotation.Value java.time.Instant] for value '123456789'
    at org.springframework.core.convert.support.ConversionUtils.invokeConverter(ConversionUtils.java:47) ~[spring-core-6.0.6.jar:6.0.6]
    at org.springframework.core.convert.support.GenericConversionService.convert(GenericConversionService.java:192) ~[spring-core-6.0.6.jar:6.0.6]
    at org.springframework.beans.TypeConverterDelegate.convertIfNecessary(TypeConverterDelegate.java:129) ~[spring-beans-6.0.6.jar:6.0.6]
    at org.springframework.beans.TypeConverterSupport.convertIfNecessary(TypeConverterSupport.java:73) ~[spring-beans-6.0.6.jar:6.0.6]
    ... 114 common frames omitted
Caused by: java.lang.IllegalArgumentException: Parse attempt failed for value [123456789]
    at org.springframework.format.support.FormattingConversionService$ParserConverter.convert(FormattingConversionService.java:224) ~[spring-context-6.0.6.jar:6.0.6]
    at org.springframework.core.convert.support.ConversionUtils.invokeConverter(ConversionUtils.java:41) ~[spring-core-6.0.6.jar:6.0.6]
    ... 117 common frames omitted
Caused by: java.time.format.DateTimeParseException: Text '123456789' could not be parsed at index 0
    at java.base/java.time.format.DateTimeFormatter.parseResolved0(DateTimeFormatter.java:2052) ~[na:na]
    at java.base/java.time.format.DateTimeFormatter.parse(DateTimeFormatter.java:1954) ~[na:na]
    at java.base/java.time.Instant.parse(Instant.java:397) ~[na:na]
    at org.springframework.format.datetime.standard.InstantFormatter.parse(InstantFormatter.java:50) ~[spring-context-6.0.6.jar:6.0.6]
    at org.springframework.format.datetime.standard.InstantFormatter.parse(InstantFormatter.java:40) ~[spring-context-6.0.6.jar:6.0.6]
    at org.springframework.format.support.FormattingConversionService$ParserConverter.convert(FormattingConversionService.java:218) ~[spring-context-6.0.6.jar:6.0.6]
    ... 118 common frames omitted

Comment From: snicoll

Oh. I think the considerations have confused me. Yes, I agree supporting instant would be nice and I don't think we should bother about the seconds variant.

Comment From: wilkinsona

@snicoll Given the involvement of org.springframework.format.datetime.standard.InstantFormatter, should this be transferred to Framework?

Comment From: snicoll

Yes. We do have issues to move some of the converters that we have in Spring Boot already so that makes sense.

Comment From: RemusRD

Hey! Can I take this one? :)

Comment From: simonbasle

@RemusRD I think it is worth a shot yes. From my perspective it should be possible to retrofit this parsing into existing InstantFormatter. I have the following test to suggest in DateTimeFormattingTests.java, but please include other more direct unit tests as well:


    @Test
    void testBindInstantAsLongEpochMillis() {
        MutablePropertyValues propertyValues = new MutablePropertyValues();
        propertyValues.add("instant", 1234L);
        binder.bind(propertyValues);
        assertThat(binder.getBindingResult().getErrorCount()).isZero();
        assertThat(binder.getBindingResult().getRawFieldValue("instant"))
                .isInstanceOf(Instant.class)
                .isEqualTo(Instant.ofEpochMilli(1234L));
        assertThat(binder.getBindingResult().getFieldValue("instant"))
                .hasToString("1970-01-01T00:00:01.234Z");
    }

Comment From: RemusRD

@RemusRD I think it is worth a shot yes. From my perspective it should be possible to retrofit this parsing into existing InstantFormatter. I have the following test to suggest in DateTimeFormattingTests.java, but please include other more direct unit tests as well:

java @Test void testBindInstantAsLongEpochMillis() { MutablePropertyValues propertyValues = new MutablePropertyValues(); propertyValues.add("instant", 1234L); binder.bind(propertyValues); assertThat(binder.getBindingResult().getErrorCount()).isZero(); assertThat(binder.getBindingResult().getRawFieldValue("instant")) .isInstanceOf(Instant.class) .isEqualTo(Instant.ofEpochMilli(1234L)); assertThat(binder.getBindingResult().getFieldValue("instant")) .hasToString("1970-01-01T00:00:01.234Z"); }

I already started working on this! I added your test which seems to be already passing, should I keep it?

Also, would be great if you could share with me any other merge requests that are already implemented similar to this one so I can take inspiration :) from.

Comment From: RemusRD

I have added more tests and the functionality to the InstantFormatter.java however, I have found if I use RandomInstantProvider.java in my tests, the values provided will overflow the Long.class making my tests fail, any suggestion on how to approach this? I could create a new provider, or limit the current one to be from Long.MIN to Long.Max

@ParameterizedTest
@ArgumentsSource(RandomInstantProvider.class)
void should_parse_into_an_Instant_from_epoch_mili(Instant input) throws ParseException {
    String inputS = String.valueOf(input.toEpochMilli()); // toEpochMilli() will overflow and make the test fail
    Instant expected = Instant.ofEpochMilli(Long.parseLong(inputS));

    Instant actual = instantFormatter.parse(inputS, null);

    assertThat(actual).isEqualTo(expected);
}

Comment From: simonbasle

yeah RandomInstantProvider is not a good fit because it will create Instants that fall off the limit of epochMillis (due to using seconds and not millis). You could take inspiration from it to create a similar provider which uses ofEpochMillis(Long.MIN_VALUE)/ofEpochMilli(Long.MAX_VALUE). I would also advise to truncateTo(ChronoUnit.MILLIS) each generated Instant.

Comment From: simonbasle

any progress @RemusRD ?

Comment From: RemusRD

any progress @RemusRD ?

Submitting the PR shortly, I added tests into the InstantFormatter, wondering if it would make sense to do a more end to end test? Like checking it works from parsing a yaml or something like that.

Comment From: RemusRD

@simonbasle I have opened the PR, but not sure how I can link it here? https://github.com/spring-projects/spring-framework/pull/30546

Comment From: simonbasle

  • superseded by gh-30546