This PR fixes DataSize class according to ISO/IEC 8000 spec (see Binary prefix)
Comment From: sbrannen
At a glance, this looks like a series of breaking changes in the API that has existed since Spring Framework 5.1, and such breaking changes are not acceptable.
If we were to add ISO 8000 support, it would have to be "in addition" to the existing feature set.
Comment From: newzubakhin
Ok, I'll implement something like ISODataSize class with a corresponding converter and tests. Should I close this PR?
Comment From: snicoll
@newzubakhin please hold on. As Sam indicated, the current proposal is a breaking change and we won't and can't change DataSize
that way. Before you spend more time on the topic, I'd like to get a chance to review the suggestion and the impacts of having two separate DataSize
types.
Comment From: snicoll
Thanks for the PR @newzubakhin. We've discussed this and decided to keep DataSize
in its current form and clarify its Javadoc, see #23697
Comment From: sstock
I understand keeping the existing DataSize
behavior for backward compatibility. However I encourage adding a standards compliant variation as suggested by @newzubakhin. The #23697 documentation certainly helps with direct consumers of the class. But does not alleviate the need to replicate such clarification to values parsed by DataSize
, e.g. Spring Boot externalized configuration. The current behavior creates confusion as The International System of Units (9th edition, section 3 Decimal multiples and sub-multiples of SI units) states "The SI prefixes refer strictly to powers of 10. They should not be used to indicate powers of 2 (for example, one kilobit represents 1000 bits and not 1024 bits)." This wording is mirrored by NIST's SI prefixes summary.
Comment From: vpavic
As this was declined due to being a breaking change back in 2019, could you reconsider it for 6.0
?
IMO it's very unfortunate to have a piece of javadoc that basically redefines something as well established as the SI prefixes. If there's a configuration property of type DataSize
annotated with @DataSizeUnit(DataUnit.KILOBYTES)
that should be quite self explanatory.
Comment From: vbezhenar
My suggestion is to support both forms, like DataUnit.KILOBYTES, DataUnit.KIBIBYTES, etc. KILOBYTES will not change from current meaning, but @Deprecated
probably should be added, so people would avoid it. And support parsing "KiB" suffix as well.
So basically does not break backwards compatibility but adds support for proper units.
For me it was surprising to find out this terminology used in Spring. I think at this point all technical usage should adhere to ISO.
Comment From: vpavic
As someone who got burned by this in a project that surfaced DataSize
through configuration properties, I'd kindly ask the team again to reconsider this. Not respecting (and redefining) the SI prefixes only has the potential to cause confusion, and I can only echo what @sstock already pointed out:
The https://github.com/spring-projects/spring-framework/issues/23697 documentation certainly helps with direct consumers of the class. But does not alleviate the need to replicate such clarification to values parsed by
DataSize
, e.g. Spring Boot externalized configuration.