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

Detection of DurationStyle.ISO8601 does not support lower-case input #32218

Closed
valentine-dev opened this issue Sep 2, 2022 · 5 comments
Closed
Labels
status: superseded An issue that has been superseded by another

Comments

@valentine-dev
Copy link
Contributor

valentine-dev commented Sep 2, 2022

Bug Report

Environment

Version of Spring Boot

2.7.3

Version of Java

11

Expected (what is in the Reference Documentation)

The standard ISO-8601 format used by java.time.Duration

from https://docs.spring.io/spring-boot/docs/current/reference/html/features.html#features.external-config.typesafe-configuration-properties.conversion.durations

Actual Behavior

Some format used by java.time.Duration is not working in Spring Boot app, like "pt25.234s" demonstrated in the example application at https://github.com/valentine-dev/spring-boot-duration-style-iso8601

Suggestion Solution

  1. Add Pattern.CASE_INSENSITIVE as the second argument to the Pattern.compile method at Line 87 of DurationStyle.java
  2. Use the following pattern string in java.time.Duration source code (Line 151 ~ 152 in the file from JDK 11) at Line 65 of DurationStyle.java:

"([-+]?)P(?:([-+]?[0-9]+)D)?" +
"(T(?:([-+]?[0-9]+)H)?(?:([-+]?[0-9]+)M)?(?:([-+]?[0-9]+)(?:[.,]([0-9]{0,9}))?S)?)?"

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Sep 2, 2022
@mdeinum
Copy link
Contributor

mdeinum commented Sep 2, 2022

Not sure it is a bug.

From the ISO-8601 spec (emphasize is mine)

The capital letters P, Y, M, W, D, T, H, M, and S are designators for each of the date and time elements and are not replaced

It explicitly mentions capital letters. So one could argue that Spring (Boot) might adhere to strictly to the standard as opposed the java.time.Duration class?

@wilkinsona
Copy link
Member

Given that we say we use the format that is used by java.time.Duration, I think it's reasonable to expect the detect and detectAndParse methods to align with Duration's own parsing.

I'm not sure that we need to go as far as using the same pattern as Duration does. Our simpler pattern used case-insensitively may be enough for our detection needs.

@wilkinsona wilkinsona added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Sep 2, 2022
@wilkinsona wilkinsona changed the title DurationStyle.ISO8601 does not work the way described in the Reference Documentation Detection of DurationStyle.ISO8601 does not support lower-case input Sep 2, 2022
@wilkinsona wilkinsona added this to the 2.6.x milestone Sep 2, 2022
@valentine-dev
Copy link
Contributor Author

valentine-dev commented Sep 2, 2022

Given that we say we use the format that is used by java.time.Duration, I think it's reasonable to expect the detect and detectAndParse methods to align with Duration's own parsing.

I'm not sure that we need to go as far as using the same pattern as Duration does. Our simpler pattern used case-insensitively may be enough for our detection needs.

Thanks for the feedback!

Add a lower-case P in the current simpler pattern may be enough - update "^[+-]?P.*$" to be "^[+-]?[pP].*$".
I would be happy to fix this small bug if needed. @wilkinsona

@valentine-dev
Copy link
Contributor Author

valentine-dev commented Sep 2, 2022

PR created at #32223 to fix this bug - please have a review.

@philwebb
Copy link
Member

philwebb commented Sep 3, 2022

Closing in favor of PR #32223. Thanks @valentine-dev!

@philwebb philwebb closed this as completed Sep 3, 2022
@philwebb philwebb closed this as not planned Won't fix, can't repro, duplicate, stale Sep 3, 2022
@philwebb philwebb added status: superseded An issue that has been superseded by another and removed type: bug A general bug labels Sep 3, 2022
@philwebb philwebb removed this from the 2.6.x milestone Sep 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: superseded An issue that has been superseded by another
Projects
None yet
Development

No branches or pull requests

5 participants