Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Addition of fallback patterns to temporal parser loses cause in Spring 5.3.5 #26777

Closed
petergphillips opened this issue Apr 8, 2021 · 7 comments
Assignees
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: regression A bug that is also a regression
Milestone

Comments

@petergphillips
Copy link

petergphillips commented Apr 8, 2021

Affects: 5.3.5

We've just pulled in the latest spring framework as a result of a spring boot upgrade to 2.4.4 and noticed that some of our unit tests are now failing as the error messages have now changed.

When we detect a validation failure our code calls e.getMostSpecificCause().getMessage() to get the reason for the failure. So for input value of 2019-30-17 we expect to get the error message of

Invalid value for MonthOfYear (valid values 1 - 12): 30

as that is what the root DateTimeException message is set to.

However it appears that changes for #20292 create a brand new DateTimeParseException without passing in the cause into the constructor on creation. This can be seen at

and should be changed to pass through the existing DateTimeException cause.

This means that the error message we now get is

Unable to parse date time value "2019-30-17" using configuration from @org.springframework.format.annotation.DateTimeFormat(pattern="", style="SS", iso=DATE, fallbackPatterns={})

which isn't nearly as human readable as the original, divulges inner spring annotations, and isn't suitable for end users. I understand that there is a desire to output the fallbackPatterns in case there are multiple specified, but I would have expected the underlying cause to be preserved so that applications can choose which cause to present to the user. Furthermore I wouldn't expect patch versions of spring to change the validation error messages.

In our spring boot code the field is annotated with

@DateTimeFormat(iso = ISO.DATE) @RequestParam(value = "from_date", required = false) LocalDate fromDate
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Apr 8, 2021
@jhoeller jhoeller added in: core Issues in core modules (aop, beans, core, context, expression) type: regression A bug that is also a regression and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Apr 9, 2021
@jhoeller jhoeller added this to the 5.3.6 milestone Apr 9, 2021
@jhoeller jhoeller self-assigned this Apr 9, 2021
@sbrannen sbrannen assigned sbrannen and unassigned jhoeller Apr 9, 2021
@sbrannen
Copy link
Member

Thanks for bringing this to our attention!

This has been fixed in 22d9012 and will be available in Spring Framework 5.3.6.

@petergphillips
Copy link
Author

thanks for the quick fix @sbrannen. Whilst not affecting us I did also notice that

throw new ParseException(
String.format("Unable to parse date time value \"%s\" using configuration from %s", text, this.source),
ex.getErrorOffset());
was added at the same time as the other change and is also missing the root cause too.

@sbrannen
Copy link
Member

was added at the same time as the other change and is also missing the root cause too.

I actually noticed that on my own (unfortunately after the 5.3.6 release), created #26804 to address it, and just came back here to point people to that issue as well... only to notice your comment after the fact. 😉

Cheers!

sbrannen added a commit that referenced this issue Apr 16, 2021
The support for fallback parsing patterns in @DateTimeFormat introduced
in gh-20292 introduced a regression in that the original cause of the
parsing exception was no longer retained.

gh-26777 addressed that regression for `java.time` support; whereas,
this commit addresses that regression for legacy Date types.

This commit ensures that the original ParseException is set as the
cause for any newly created ParseException, thereby retaining the
original exception as the root cause.

Closes gh-26804
@aboyer1213
Copy link

aboyer1213 commented Apr 18, 2021

*Edit: turns out my JAVA_HOME was actually set to open jdk 11 which explains the below situation. JDK 11 must use {} instead of [].

I notice when I run the build from command line, the DateTimeParseException contains the fallbackPatterns={} (with curly braces) like in this original issue's description.

When running the gradle test In eclipse, the exception contains the fallbackPatterns=[] (with square brackets)

The asserts in DateTimeFormattingTests.isoLocalDateWithInvalidFormat() 0f31830 and DateFormattingTests.styleDateWithInvalidFormat() 22d9012 assert on [], so the test is failing for me by command-line, but passing in IDE. I've tried to make sure I am running same jdk for both (open jdk 8), but I might have something incorrectly configured in my development environment. Has anyone had this issue before?

sbrannen added a commit that referenced this issue Apr 18, 2021
Prior to this commit, two tests for exception handling regarding
@DateTimeFormat processing only passed on Java 8. This is due to the
fact that the toString() implementation for annotations changed in Java
9. Specifically, the representation for arrays changed from [] to {},
and strings are enclosed in double quotes beginning with Java 9.

This commit ensures that the affected @DateTimeFormat tests pass on
Java 9+, by making the assertions more lenient regarding toString()
output for annotations.

See gh-26777
See gh-26804
@sbrannen
Copy link
Member

@aboyer1213, thank you for bringing this to my attention!

I've fixed the tests in e489706, and the following excerpt from the commit message explains what's going on (which you already discerned on your own).

Prior to this commit, two tests for exception handling regarding
@DateTimeFormat processing only passed on Java 8. This is due to the
fact that the toString() implementation for annotations changed in Java
9. Specifically, the representation for arrays changed from [] to {},
and strings are enclosed in double quotes beginning with Java 9.

This commit ensures that the affected @DateTimeFormat tests pass on
Java 9+, by making the assertions more lenient regarding toString()
output for annotations.

@aboyer1213
Copy link

@sbrannen Thanks for the detailed explanation! I cloned the repo for the first time yesterday, so I figured it had to be something with my local environment, but I'm glad it was useful for potential future Java upgrades too. Thanks again!

@sbrannen
Copy link
Member

@sbrannen Thanks for the detailed explanation!

You're welcome.

I cloned the repo for the first time yesterday, so I figured it had to be something with my local environment, but I'm glad it was useful for potential future Java upgrades too.

Well, we actually have CI builds against JDK 11 and 15. Thus, those tests should have failed our CI builds, but they didn't -- for example, JDK 11 build 364 should have failed.

Thus, your report of the test failure on JDK 11 helped us realize that something is wrong with our JDK 11 and 15 CI builds, which we'll be fixing in the coming week or so.

Thanks again!

Thank YOU! (for helping us realize our CI builds are broken)

Zoran0104 pushed a commit to Zoran0104/spring-framework that referenced this issue Aug 20, 2021
The support for fallback parsing patterns in @DateTimeFormat introduced
in spring-projectsgh-20292 introduced a regression in that the original cause of the
parsing exception was no longer retained.

This commit ensures that the original DateTimeParseException is set as
the cause for any newly created DateTimeParseException, thereby
retaining the original exception as the root cause.

Closes spring-projectsgh-26777
Zoran0104 pushed a commit to Zoran0104/spring-framework that referenced this issue Aug 20, 2021
The support for fallback parsing patterns in @DateTimeFormat introduced
in spring-projectsgh-20292 introduced a regression in that the original cause of the
parsing exception was no longer retained.

spring-projectsgh-26777 addressed that regression for `java.time` support; whereas,
this commit addresses that regression for legacy Date types.

This commit ensures that the original ParseException is set as the
cause for any newly created ParseException, thereby retaining the
original exception as the root cause.

Closes spring-projectsgh-26804
Zoran0104 pushed a commit to Zoran0104/spring-framework that referenced this issue Aug 20, 2021
Prior to this commit, two tests for exception handling regarding
@DateTimeFormat processing only passed on Java 8. This is due to the
fact that the toString() implementation for annotations changed in Java
9. Specifically, the representation for arrays changed from [] to {},
and strings are enclosed in double quotes beginning with Java 9.

This commit ensures that the affected @DateTimeFormat tests pass on
Java 9+, by making the assertions more lenient regarding toString()
output for annotations.

See spring-projectsgh-26777
See spring-projectsgh-26804
lxbzmy pushed a commit to lxbzmy/spring-framework that referenced this issue Mar 26, 2022
The support for fallback parsing patterns in @DateTimeFormat introduced
in spring-projectsgh-20292 introduced a regression in that the original cause of the
parsing exception was no longer retained.

This commit ensures that the original DateTimeParseException is set as
the cause for any newly created DateTimeParseException, thereby
retaining the original exception as the root cause.

Closes spring-projectsgh-26777
lxbzmy pushed a commit to lxbzmy/spring-framework that referenced this issue Mar 26, 2022
The support for fallback parsing patterns in @DateTimeFormat introduced
in spring-projectsgh-20292 introduced a regression in that the original cause of the
parsing exception was no longer retained.

spring-projectsgh-26777 addressed that regression for `java.time` support; whereas,
this commit addresses that regression for legacy Date types.

This commit ensures that the original ParseException is set as the
cause for any newly created ParseException, thereby retaining the
original exception as the root cause.

Closes spring-projectsgh-26804
lxbzmy pushed a commit to lxbzmy/spring-framework that referenced this issue Mar 26, 2022
Prior to this commit, two tests for exception handling regarding
@DateTimeFormat processing only passed on Java 8. This is due to the
fact that the toString() implementation for annotations changed in Java
9. Specifically, the representation for arrays changed from [] to {},
and strings are enclosed in double quotes beginning with Java 9.

This commit ensures that the affected @DateTimeFormat tests pass on
Java 9+, by making the assertions more lenient regarding toString()
output for annotations.

See spring-projectsgh-26777
See spring-projectsgh-26804
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: regression A bug that is also a regression
Projects
None yet
Development

No branches or pull requests

5 participants