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

Period conversion does not support a ChronoUnit of WEEKS but supports weeks in ISO-8601 (e.g. P2W) and simple (e.g. 2w) formats #22225

Closed
AlexFalappa opened this issue Jul 5, 2020 · 1 comment
Assignees
Labels
type: bug A general bug
Milestone

Comments

@AlexFalappa
Copy link
Contributor

It may be worth adding to the documentation (chapter 2.8.8) that ChronoUnit.WEEKS is not supported in @PeriodUnit annotation to specify how to interpret unqualified integer values for Period configuration properties fields.

Specifying weeks fails with a root cause of:

...
Caused by: java.lang.IllegalArgumentException: '2' is not a valid simple period
	at org.springframework.boot.convert.PeriodStyle$1.parse(PeriodStyle.java:59) ~[spring-boot-2.3.1.RELEASE.jar:2.3.1.RELEASE]
	at org.springframework.boot.convert.StringToPeriodConverter.convert(StringToPeriodConverter.java:65) ~[spring-boot-2.3.1.RELEASE.jar:2.3.1.RELEASE]
	at org.springframework.boot.convert.StringToPeriodConverter.convert(StringToPeriodConverter.java:50) ~[spring-boot-2.3.1.RELEASE.jar:2.3.1.RELEASE]
	at org.springframework.core.convert.support.ConversionUtils.invokeConverter(ConversionUtils.java:41) ~[spring-core-5.2.7.RELEASE.jar:5.2.7.RELEASE]
	... 57 common frames omitted
Caused by: java.lang.IllegalArgumentException: Unsupported unit Weeks
	at org.springframework.boot.convert.PeriodStyle$Unit.fromChronoUnit(PeriodStyle.java:273) ~[spring-boot-2.3.1.RELEASE.jar:2.3.1.RELEASE]
	at org.springframework.boot.convert.PeriodStyle$1.parse(PeriodStyle.java:47) ~[spring-boot-2.3.1.RELEASE.jar:2.3.1.RELEASE]
	... 60 common frames omitted
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jul 5, 2020
@wilkinsona
Copy link
Member

wilkinsona commented Jul 6, 2020

Thanks, @AlexFalappa. This looks like a bug to me. We support weeks with the ISO-8601 and simple formats. Unless I've missed something, there's no reason why they're not also supported when specify an int and a ChronoUnit of weeks.

Period has no accessor for weeks but I don't believe we need one. For String -> Period conversion we don't access any of the components of the period. For Period -> String conversion we only need years, months, and days. I think we can safely add WEEKS to PeriodStyle.Unit that supports parsing but not printing.

@wilkinsona wilkinsona added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Jul 6, 2020
@wilkinsona wilkinsona changed the title Document supported ChronoUnit values for @PeriodUnit annotation Period conversion does not support a ChronoUnit of WEEKS but supports weeks in ISO-8601 (e.g. P2W) and simple (e.g. 2w) formats Jul 6, 2020
@wilkinsona wilkinsona changed the title Period conversion does not support a ChronoUnit of WEEKS but supports weeks in ISO-8601 (e.g. P2W) and simple (e.g. 2w) formats Period conversion does not support a ChronoUnit of WEEKS but supports weeks in ISO-8601 (e.g. P2W) and simple (e.g. 2w) formats Jul 6, 2020
@wilkinsona wilkinsona added this to the 2.3.x milestone Jul 6, 2020
@wilkinsona wilkinsona self-assigned this Jul 6, 2020
@wilkinsona wilkinsona modified the milestones: 2.3.x, 2.3.2 Jul 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

No branches or pull requests

3 participants