Skip to content

Ensure InstantFormatter can properly deserialize ISO-formatted dates#23895

Closed
monosoul wants to merge 3 commits into
spring-projects:masterfrom
monosoul:bugfix/instant-formatter-iso-parse
Closed

Ensure InstantFormatter can properly deserialize ISO-formatted dates#23895
monosoul wants to merge 3 commits into
spring-projects:masterfrom
monosoul:bugfix/instant-formatter-iso-parse

Conversation

@monosoul

@monosoul monosoul commented Oct 30, 2019

Copy link
Copy Markdown

At the moment InstantFormatter can serialize an Instant that is far in the future (or in the past), but cannot properly deserialize it, because in this case ISO-formatted instant will start with +/--sign. This PR fixes this issue, while maintaining the previous contract.
Also, it introduces tests for InstantFormatter 🙂

@pivotal-issuemaster

pivotal-issuemaster commented Oct 30, 2019

Copy link
Copy Markdown

@monosoul Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-issuemaster

pivotal-issuemaster commented Oct 30, 2019

Copy link
Copy Markdown

@monosoul Thank you for signing the Contributor License Agreement!

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Oct 30, 2019
@monosoul

monosoul commented Oct 30, 2019

Copy link
Copy Markdown
Author

@jhoeller as per our discussion at Joker conf, here's a PR to fix InstantFormatter

@sbrannen sbrannen added in: core Issues in core modules (aop, beans, core, context, expression) type: bug A general bug labels Oct 30, 2019
sbrannen
sbrannen requested changes Oct 30, 2019
View reviewed changes
Comment thread ...ontext/src/test/java/org/springframework/format/datetime/standard/InstantFormatterTests.java
Comment thread ...ontext/src/test/java/org/springframework/format/datetime/standard/InstantFormatterTests.java Outdated
@monosoul

monosoul commented Oct 30, 2019

Copy link
Copy Markdown
Author

@sbrannen all fixed!

@sbrannen sbrannen self-assigned this Oct 30, 2019
@sbrannen sbrannen added this to the 5.2.2 milestone Oct 30, 2019
@sbrannen

sbrannen commented Oct 30, 2019

Copy link
Copy Markdown
Member

Thanks for making the changes.

We'll hold off on merging this into master until after 5.2.1 has been released.

@sbrannen sbrannen removed the status: waiting-for-triage An issue we've not yet triaged or decided on label Oct 30, 2019
@sbrannen sbrannen changed the title InstantFormatter ISO-formatted date parsing bug fix Ensure InstantFormatter can properly deserialze ISO-formatted dates Oct 30, 2019
@sbrannen

sbrannen commented Oct 30, 2019

Copy link
Copy Markdown
Member

By the way, nice use of custom ArgumentsProvider implementations for JUnit Jupiter. 👍

@monosoul

monosoul commented Oct 30, 2019

Copy link
Copy Markdown
Author

@sbrannen thank you!

Regarding the test names (locale part), I just forgot to include this part of the contract into the test names. Should I fix it?

@sbrannen

sbrannen commented Oct 30, 2019

Copy link
Copy Markdown
Member

Regarding the test names (locale part), I just forgot to include this part of the contract into the test names. Should I fix it?

No. I think it's OK like it is.

@sbrannen sbrannen changed the title Ensure InstantFormatter can properly deserialze ISO-formatted dates Ensure InstantFormatter can properly deserialize ISO-formatted dates Nov 4, 2019
@sbrannen

sbrannen commented Nov 13, 2019

Copy link
Copy Markdown
Member

For future reference, please make sure you execute ./gradlew check before submitting a PR.

[ant:checkstyle] [ERROR] /Users/XXX/spring-framework/spring-context/src/test/java/org/springframework/format/datetime/standard/InstantFormatterTests.java:1: File does not end with a newline. [NewlineAtEndOfFile]
[ant:checkstyle] [ERROR] /Users/XXX/spring-framework/spring-context/src/test/java/org/springframework/format/datetime/standard/InstantFormatterTests.java:25: Wrong order for 'java.text.ParseException' import. [ImportOrder]

I'll fix those locally before merging.

@monosoul

monosoul commented Nov 14, 2019

Copy link
Copy Markdown
Author

Oh, sorry for that. I can fix it myself if you didn't do that yet.

@sbrannen

sbrannen commented Nov 14, 2019

Copy link
Copy Markdown
Member

Oh, sorry for that. I can fix it myself if you didn't do that yet.

No worries. I've already handled that locally.

@sbrannen sbrannen closed this in a0e4ac3 Nov 14, 2019
sbrannen added a commit that referenced this pull request Nov 14, 2019
@sbrannen
Polish contribution
8186b77 
See gh-23895
@sbrannen

sbrannen commented Nov 14, 2019

Copy link
Copy Markdown
Member

This has been merged into master.

Thanks

@spring-projects-issues spring-projects-issues added status: backported An issue that has been backported to maintenance branches and removed for: backport-to-5.1.x labels Nov 14, 2019
@spring-projects-issues spring-projects-issues mentioned this pull request Nov 14, 2019
Closed
sbrannen pushed a commit to sbrannen/spring-framework that referenced this pull request Nov 14, 2019
@sbrannen
Inverse condition to fix ISO-formatted Instant parsing
c9a6f42 
Prior to this commit, InstantFormatter was able to properly serialize
an Instant that is far in the future (or in the past), but it could not
properly deserialize it, because in such scenarios an ISO-formatted
Instant starts with a +/- sign.

This commit fixes this issue, while maintaining the previous contract,
and also introduces tests for InstantFormatter.

Closes spring-projectsgh-23895
sbrannen added a commit to sbrannen/spring-framework that referenced this pull request Nov 14, 2019
@sbrannen
Polish contribution
40ac055 
See spring-projectsgh-23895
sbrannen added a commit to sbrannen/spring-framework that referenced this pull request Nov 14, 2019
@sbrannen
Convert InstantFormatterTests to JUnit 4
96a1a0d 
See spring-projectsgh-23895
sbrannen added a commit to sbrannen/spring-framework that referenced this pull request Nov 14, 2019
@sbrannen
Polish contribution
8df3afd 
See spring-projectsgh-23895
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sbrannen sbrannen sbrannen approved these changes

Assignees

@sbrannen sbrannen

Labels

in: core Issues in core modules (aop, beans, core, context, expression) status: backported An issue that has been backported to maintenance branches type: bug A general bug

Projects

None yet

Milestone

5.2.2

Development

Successfully merging this pull request may close these issues.

4 participants

@monosoul @pivotal-issuemaster @sbrannen @spring-projects-issues