This is an automated email from the ASF dual-hosted git repository. danhaywood pushed a commit to branch CAUSEWAY-3689 in repository https://gitbox.apache.org/repos/asf/causeway.git
commit 94dc03a2b14bb26f88cbf9b674125fc47242343f Author: danhaywood <[email protected]> AuthorDate: Thu Feb 22 16:06:26 2024 +0000 CAUSEWAY-3689: adds config param to disallow grouping (thousands) separator when parsing BigDecimals ... because these are often used as money, risk of misinterpretation --- .../value/semantics/ValueSemanticsAbstract.java | 25 +++++++++-- .../core/config/CausewayConfiguration.java | 23 ++++++++++ .../valuesemantics/BigDecimalValueSemantics.java | 5 ++- .../valuesemantics/DoubleValueSemantics.java | 2 +- .../valuesemantics/FloatValueSemantics.java | 2 +- .../BigDecimalValueSemanticsProviderTest.java | 51 +++++++++++++++++++++- 6 files changed, 101 insertions(+), 7 deletions(-) diff --git a/api/applib/src/main/java/org/apache/causeway/applib/value/semantics/ValueSemanticsAbstract.java b/api/applib/src/main/java/org/apache/causeway/applib/value/semantics/ValueSemanticsAbstract.java index 017e679545..e29d65a7e0 100644 --- a/api/applib/src/main/java/org/apache/causeway/applib/value/semantics/ValueSemanticsAbstract.java +++ b/api/applib/src/main/java/org/apache/causeway/applib/value/semantics/ValueSemanticsAbstract.java @@ -21,6 +21,7 @@ package org.apache.causeway.applib.value.semantics; import java.math.BigDecimal; import java.math.BigInteger; import java.text.DecimalFormat; +import java.text.DecimalFormatSymbols; import java.text.NumberFormat; import java.text.ParseException; import java.text.ParsePosition; @@ -223,20 +224,38 @@ ValueSemanticsProvider<T> { return Optional.empty(); } try { - return parseDecimal(context, input) + return parseDecimal(context, input, GroupingSeparatorWhenParsePolicy.ALLOW) .map(BigDecimal::toBigIntegerExact); } catch (final NumberFormatException | ArithmeticException e) { throw new TextEntryParseException("Not an integer value " + text, e); } } + protected enum GroupingSeparatorWhenParsePolicy { + ALLOW, + DISALLOW, + ; + } + protected Optional<BigDecimal> parseDecimal( - final @Nullable ValueSemanticsProvider.Context context, - final @Nullable String text) { + final @Nullable Context context, + final @Nullable String text, + final GroupingSeparatorWhenParsePolicy parsePolicy) { val input = _Strings.blankToNullOrTrim(text); if(input==null) { return Optional.empty(); } + + if (parsePolicy == GroupingSeparatorWhenParsePolicy.DISALLOW) { + UserLocale userLocale = getUserLocale(context); + val decimalFormatSymbols = new DecimalFormatSymbols(userLocale.getNumberFormatLocale()); + char groupingSeparator = decimalFormatSymbols.getGroupingSeparator(); + if (input.contains(""+groupingSeparator)) { + throw new TextEntryParseException("Invalid value '" + input + "'; do not use the '" + groupingSeparator + "' grouping separator"); + } + } + + val format = getNumberFormat(context, FormatUsageFor.PARSING); format.setParseBigDecimal(true); diff --git a/core/config/src/main/java/org/apache/causeway/core/config/CausewayConfiguration.java b/core/config/src/main/java/org/apache/causeway/core/config/CausewayConfiguration.java index 067b072e45..d6a5eb8d70 100644 --- a/core/config/src/main/java/org/apache/causeway/core/config/CausewayConfiguration.java +++ b/core/config/src/main/java/org/apache/causeway/core/config/CausewayConfiguration.java @@ -3263,6 +3263,29 @@ public class CausewayConfiguration { * either {@link Digits#fraction()} or an ORM semantic such as the (JPA) {@link Column#scale()}. */ private Integer minScale = null; + + /** + * A common use of {@link java.math.BigDecimal} is as a money value. In some locales (eg English), the + * "," (comma) is the grouping (thousands) separator wihle the "." (period) acts as a + * decimal point, but in others (eg France, Italy) it is the other way around. + * + * <p> + * Surprisingly perhaps, a string such as "123,99", when parsed ((by {@link java.text.DecimalFormat}) + * in an English locale, is not rejected but instead is evaluated as the value 12399L. That's almost + * certainly not what the end-user would have expected, and results in a money value 100x too large. + * </p> + * + * <p> + * The purpose of this configuration property is to remove the confusion by simply disallowing the + * thousands separator from being part of the input string. + * </p> + * + * <p> + * For maximum safety, allowing the grouping separator is disallowed, but the alternate (original) + * behaviour can be reinstated by setting this config property back to <code>true</code>. + * </p> + */ + private boolean allowGroupingSeparatorWhenParse = false; } private final Kroki kroki = new Kroki(); diff --git a/core/metamodel/src/main/java/org/apache/causeway/core/metamodel/valuesemantics/BigDecimalValueSemantics.java b/core/metamodel/src/main/java/org/apache/causeway/core/metamodel/valuesemantics/BigDecimalValueSemantics.java index 628ad9bf79..fe30215c9e 100644 --- a/core/metamodel/src/main/java/org/apache/causeway/core/metamodel/valuesemantics/BigDecimalValueSemantics.java +++ b/core/metamodel/src/main/java/org/apache/causeway/core/metamodel/valuesemantics/BigDecimalValueSemantics.java @@ -130,7 +130,10 @@ implements @Override public BigDecimal parseTextRepresentation(final ValueSemanticsProvider.Context context, final String text) { - return super.parseDecimal(context, text) + val parsePolicy = causewayConfiguration.getValueTypes().getBigDecimal().isAllowGroupingSeparatorWhenParse() + ? GroupingSeparatorWhenParsePolicy.ALLOW + : GroupingSeparatorWhenParsePolicy.DISALLOW; + return super.parseDecimal(context, text, parsePolicy) .orElse(null); } diff --git a/core/metamodel/src/main/java/org/apache/causeway/core/metamodel/valuesemantics/DoubleValueSemantics.java b/core/metamodel/src/main/java/org/apache/causeway/core/metamodel/valuesemantics/DoubleValueSemantics.java index 8eccd6ddda..8f849e3013 100644 --- a/core/metamodel/src/main/java/org/apache/causeway/core/metamodel/valuesemantics/DoubleValueSemantics.java +++ b/core/metamodel/src/main/java/org/apache/causeway/core/metamodel/valuesemantics/DoubleValueSemantics.java @@ -101,7 +101,7 @@ implements @Override public Double parseTextRepresentation(final Context context, final String text) { - return _Doubles.convertToDouble(super.parseDecimal(context, text)) + return _Doubles.convertToDouble(super.parseDecimal(context, text, GroupingSeparatorWhenParsePolicy.ALLOW)) .orElse(null); } diff --git a/core/metamodel/src/main/java/org/apache/causeway/core/metamodel/valuesemantics/FloatValueSemantics.java b/core/metamodel/src/main/java/org/apache/causeway/core/metamodel/valuesemantics/FloatValueSemantics.java index 7dc81d1660..7157cd6284 100644 --- a/core/metamodel/src/main/java/org/apache/causeway/core/metamodel/valuesemantics/FloatValueSemantics.java +++ b/core/metamodel/src/main/java/org/apache/causeway/core/metamodel/valuesemantics/FloatValueSemantics.java @@ -101,7 +101,7 @@ implements @Override public Float parseTextRepresentation(final Context context, final String text) { - return _Floats.convertToFloat(super.parseDecimal(context, text)) + return _Floats.convertToFloat(super.parseDecimal(context, text, GroupingSeparatorWhenParsePolicy.ALLOW)) .orElse(null); } diff --git a/core/metamodel/src/test/java/org/apache/causeway/core/metamodel/facets/value/BigDecimalValueSemanticsProviderTest.java b/core/metamodel/src/test/java/org/apache/causeway/core/metamodel/facets/value/BigDecimalValueSemanticsProviderTest.java index 5f1e187ded..def7f3795f 100644 --- a/core/metamodel/src/test/java/org/apache/causeway/core/metamodel/facets/value/BigDecimalValueSemanticsProviderTest.java +++ b/core/metamodel/src/test/java/org/apache/causeway/core/metamodel/facets/value/BigDecimalValueSemanticsProviderTest.java @@ -18,8 +18,13 @@ */ package org.apache.causeway.core.metamodel.facets.value; +import lombok.val; + import java.math.BigDecimal; +import org.apache.causeway.core.config.CausewayConfiguration; + +import org.assertj.core.api.Assertions; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -40,7 +45,9 @@ extends ValueSemanticsProviderAbstractTestCase<BigDecimal> { bigDecimal = new BigDecimal("34132.199"); allowMockAdapterToReturn(bigDecimal); - setSemantics(value = new BigDecimalValueSemantics()); + BigDecimalValueSemantics valueSemantics = new BigDecimalValueSemantics(); + valueSemantics.setCausewayConfiguration(new CausewayConfiguration(null, null)); + setSemantics(value = valueSemantics); } @Test @@ -58,6 +65,48 @@ extends ValueSemanticsProviderAbstractTestCase<BigDecimal> { } } + @Test + void parseInvalidStringWithGroupingSeparator() throws Exception { + try { + value.parseTextRepresentation(null, "123,999.01"); + fail(); + } catch (final TextEntryParseException expected) { + } + } + + @Test + void parseValidStringWithGroupingSeparatorIfConfiguredToAllow() throws Exception { + val causewayConfiguration = new CausewayConfiguration(null, null); + causewayConfiguration.getValueTypes().getBigDecimal().setAllowGroupingSeparatorWhenParse(true); + value.setCausewayConfiguration(causewayConfiguration); + + value.parseTextRepresentation(null, "123,999.01"); + } + + @Test + void demonstrateTheRiskOfAllowingGroupingSeparatorIfConfiguredToAllow() throws Exception { + + // default disallows grouping separator + try { + value.parseTextRepresentation(null, "1239,99"); + fail(); + } catch (final TextEntryParseException expected) { + } + + // but if we allow it... + val causewayConfiguration = new CausewayConfiguration(null, null); + causewayConfiguration.getValueTypes().getBigDecimal().setAllowGroupingSeparatorWhenParse(true); + value.setCausewayConfiguration(causewayConfiguration); + + BigDecimal bigDecimal = value.parseTextRepresentation(null, "1239,99"); + Assertions.assertThat(bigDecimal).isEqualTo(new BigDecimal(123999)); + } + + @Test + void parseValidStringWithNoGroupingSeparator() throws Exception { + value.parseTextRepresentation(null, "123999.01"); + } + @Test void titleOf() { assertEquals("34,132.199", value.titlePresentation(null, bigDecimal));
