spring-projects / spring-boot

Spring Boot helps you to create Spring-powered, production-grade applications and services with absolute minimum fuss.
https://spring.io/projects/spring-boot
Apache License 2.0
75.23k stars 40.7k forks source link

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

Closed valentine-dev closed 2 years ago

valentine-dev commented 2 years ago

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)?)?"

mdeinum commented 2 years ago

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 commented 2 years ago

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.

valentine-dev commented 2 years ago

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 commented 2 years ago

PR created at https://github.com/spring-projects/spring-boot/pull/32223 to fix this bug - please have a review.

philwebb commented 2 years ago

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