From 2c5f9e0782b09c9415c90c609d56e3fc593283ce Mon Sep 17 00:00:00 2001 From: Matthias Keck Date: Wed, 18 Dec 2024 08:55:29 +0100 Subject: [PATCH 1/5] Fixes default value scaling in case of missing Numeric annotation - Using Numeric annotation is strongly advised and missing annotations get reported at startup. So no one should define an Amount field without it - But when it is missing, AmountProperty#contributeToTable will default to (15, 0) and so we should also apply scale 0 as default to the sql default value Fixes: SIRI-1041 --- src/main/java/sirius/db/mixing/properties/AmountProperty.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/sirius/db/mixing/properties/AmountProperty.java b/src/main/java/sirius/db/mixing/properties/AmountProperty.java index 0db7c6dfa..c003820ba 100644 --- a/src/main/java/sirius/db/mixing/properties/AmountProperty.java +++ b/src/main/java/sirius/db/mixing/properties/AmountProperty.java @@ -146,7 +146,7 @@ public String getColumnDefaultValue() { // else a schema change will be issued. NumberFormat format = getAnnotation(Numeric.class).map(numeric -> { return new NumberFormat(numeric.scale(), RoundingMode.HALF_UP, NLS.getMachineFormatSymbols(), false, null); - }).orElse(NumberFormat.MACHINE_THREE_DECIMAL_PLACES); + }).orElse(NumberFormat.MACHINE_NO_DECIMAL_PLACES); return Amount.of((BigDecimal) defaultData).toString(format).asString(); } From 538ef8e5cc835b09f60585be141d76052eaa7e16 Mon Sep 17 00:00:00 2001 From: Matthias Keck Date: Wed, 18 Dec 2024 10:28:18 +0100 Subject: [PATCH 2/5] Creates a failing testcase for amount scaling Fixes: SIRI-1041 --- .../java/sirius/db/jdbc/DataTypesEntity.java | 4 +++- .../kotlin/sirius/db/jdbc/DataTypesTest.kt | 19 ++++++++++++++++++- 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/src/test/java/sirius/db/jdbc/DataTypesEntity.java b/src/test/java/sirius/db/jdbc/DataTypesEntity.java index ee513499b..8f38b2b07 100644 --- a/src/test/java/sirius/db/jdbc/DataTypesEntity.java +++ b/src/test/java/sirius/db/jdbc/DataTypesEntity.java @@ -20,6 +20,8 @@ public class DataTypesEntity extends SQLEntity { + public static final int AMOUNT_SCALE = 3; + public enum TestEnum { Test1, TestTest, Test2 } @@ -44,7 +46,7 @@ public enum TestEnum { @DefaultValue("300") @NullAllowed - @Numeric(precision = 20, scale = 3) + @Numeric(precision = 20, scale = AMOUNT_SCALE) private Amount amountValue = Amount.NOTHING; @DefaultValue("2018-01-01") diff --git a/src/test/kotlin/sirius/db/jdbc/DataTypesTest.kt b/src/test/kotlin/sirius/db/jdbc/DataTypesTest.kt index d67a84b08..ceb7c9b24 100644 --- a/src/test/kotlin/sirius/db/jdbc/DataTypesTest.kt +++ b/src/test/kotlin/sirius/db/jdbc/DataTypesTest.kt @@ -14,6 +14,7 @@ import sirius.kernel.SiriusExtension import sirius.kernel.commons.Amount import sirius.kernel.commons.Value import sirius.kernel.di.std.Part +import java.math.RoundingMode import java.time.Duration import kotlin.test.assertEquals import kotlin.test.assertFalse @@ -86,8 +87,24 @@ class DataTypesTest { } @Test - fun `default values work`() { + fun `AmountProperty scaling works as excepted`() { + var test = DataTypesEntity() + val unscaledValue = Value.of("1,2345") + + val amountProperty = test.descriptor.getProperty("amountValue") + amountProperty.parseValue(test, unscaledValue) + oma.update(test) + test = oma.refreshOrFail(test) + val expectedAmount = unscaledValue.amount.round(DataTypesEntity.AMOUNT_SCALE, RoundingMode.HALF_UP) + assertEquals(expectedAmount, test.amountValue) + // Storing the same value twice must not trigger a change + amountProperty.parseValue(test, unscaledValue) + assertFalse { test.isAnyMappingChanged } + } + + @Test + fun `default values work`() { var test = DataTypesEntity() oma.update(test) From e12083c066a7b5b5659a89c68efd43835d232fdf Mon Sep 17 00:00:00 2001 From: Matthias Keck Date: Wed, 18 Dec 2024 10:31:53 +0100 Subject: [PATCH 3/5] Applies persistance scale on amounts - when transforming values from user input or import input - round by the amount property field defined scale so the current field value has the same scale as persisted and loaded data Fixes: SIRI-1041 --- .../db/mixing/properties/AmountProperty.java | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/src/main/java/sirius/db/mixing/properties/AmountProperty.java b/src/main/java/sirius/db/mixing/properties/AmountProperty.java index c003820ba..35e43834f 100644 --- a/src/main/java/sirius/db/mixing/properties/AmountProperty.java +++ b/src/main/java/sirius/db/mixing/properties/AmountProperty.java @@ -92,7 +92,7 @@ public void create(EntityDescriptor descriptor, @Override public Object transformValue(Value value) { if (value.isFilled()) { - return NLS.parseUserString(Amount.class, value.asString()); + return NLS.parseUserString(Amount.class, value.asString()).round(getPersistanceNumberFormat()); } if (this.isNullable() || defaultValue.isEmptyString()) { return Amount.NOTHING; @@ -106,7 +106,7 @@ protected Object transformValueFromImport(Value value) { if (value.getAmount().isEmpty() && !this.isNullable()) { return defaultValue.getAmount(); } - return value.get(); + return ((Amount) value.get()).round(getPersistanceNumberFormat()); } if (value.isFilled()) { @@ -118,10 +118,10 @@ protected Object transformValueFromImport(Value value) { private Amount parseWithNLS(@Nonnull Value value) { try { - return Amount.ofMachineString(value.asString()); + return Amount.ofMachineString(value.asString()).round(getPersistanceNumberFormat()); } catch (IllegalArgumentException originalFormatException) { try { - return Amount.ofUserString(value.asString()); + return Amount.ofUserString(value.asString()).round(getPersistanceNumberFormat()); } catch (Exception ignored) { throw originalFormatException; } @@ -144,10 +144,15 @@ public String getColumnDefaultValue() { } // the resulting string needs to match the string representation in the DB exactly, // else a schema change will be issued. - NumberFormat format = getAnnotation(Numeric.class).map(numeric -> { + NumberFormat format = getPersistanceNumberFormat(); + return Amount.of((BigDecimal) defaultData).toString(format).asString(); + } + + @Nonnull + private NumberFormat getPersistanceNumberFormat() { + return getAnnotation(Numeric.class).map(numeric -> { return new NumberFormat(numeric.scale(), RoundingMode.HALF_UP, NLS.getMachineFormatSymbols(), false, null); }).orElse(NumberFormat.MACHINE_NO_DECIMAL_PLACES); - return Amount.of((BigDecimal) defaultData).toString(format).asString(); } @Override From 5ec3512377eb6b1849cf763552376e989d7c6000 Mon Sep 17 00:00:00 2001 From: Matthias Keck Date: Wed, 18 Dec 2024 12:41:41 +0100 Subject: [PATCH 4/5] Only apply amount property scaling in case a Numeric annotation exists - so mongo can keep on going with "unlimited" scale and not annotated fields - and adds test case for scaling Fixes: SIRI-1041 --- .../db/mixing/properties/AmountProperty.java | 25 ++++++++++---- .../mongo/properties/MongoAmountEntity.java | 15 ++++++++ .../properties/MongoAmountPropertyTest.kt | 34 +++++++++++++++++++ 3 files changed, 67 insertions(+), 7 deletions(-) diff --git a/src/main/java/sirius/db/mixing/properties/AmountProperty.java b/src/main/java/sirius/db/mixing/properties/AmountProperty.java index 35e43834f..41ae406a4 100644 --- a/src/main/java/sirius/db/mixing/properties/AmountProperty.java +++ b/src/main/java/sirius/db/mixing/properties/AmountProperty.java @@ -38,6 +38,7 @@ import java.math.BigDecimal; import java.math.RoundingMode; import java.sql.Types; +import java.util.Optional; import java.util.function.Consumer; /** @@ -92,7 +93,7 @@ public void create(EntityDescriptor descriptor, @Override public Object transformValue(Value value) { if (value.isFilled()) { - return NLS.parseUserString(Amount.class, value.asString()).round(getPersistanceNumberFormat()); + return applyNumericRounding(NLS.parseUserString(Amount.class, value.asString())); } if (this.isNullable() || defaultValue.isEmptyString()) { return Amount.NOTHING; @@ -100,13 +101,23 @@ public Object transformValue(Value value) { return defaultValue.getAmount(); } + /** + * In case the property is annotated with {@link Numeric}, the value is rounded according to the annotation. + * + * @param amount the amount + * @return the rounded amount if the property is annotated with {@link Numeric}, otherwise the amount itself + */ + private Amount applyNumericRounding(Amount amount) { + return getAnnotatedNumberFormat().map(amount::round).orElse(amount); + } + @Override protected Object transformValueFromImport(Value value) { if (value.is(Amount.class)) { if (value.getAmount().isEmpty() && !this.isNullable()) { return defaultValue.getAmount(); } - return ((Amount) value.get()).round(getPersistanceNumberFormat()); + return applyNumericRounding((Amount) value.get()); } if (value.isFilled()) { @@ -118,10 +129,10 @@ protected Object transformValueFromImport(Value value) { private Amount parseWithNLS(@Nonnull Value value) { try { - return Amount.ofMachineString(value.asString()).round(getPersistanceNumberFormat()); + return applyNumericRounding(Amount.ofMachineString(value.asString())); } catch (IllegalArgumentException originalFormatException) { try { - return Amount.ofUserString(value.asString()).round(getPersistanceNumberFormat()); + return applyNumericRounding(Amount.ofUserString(value.asString())); } catch (Exception ignored) { throw originalFormatException; } @@ -144,15 +155,15 @@ public String getColumnDefaultValue() { } // the resulting string needs to match the string representation in the DB exactly, // else a schema change will be issued. - NumberFormat format = getPersistanceNumberFormat(); + NumberFormat format = getAnnotatedNumberFormat().orElse(NumberFormat.MACHINE_NO_DECIMAL_PLACES); return Amount.of((BigDecimal) defaultData).toString(format).asString(); } @Nonnull - private NumberFormat getPersistanceNumberFormat() { + private Optional getAnnotatedNumberFormat() { return getAnnotation(Numeric.class).map(numeric -> { return new NumberFormat(numeric.scale(), RoundingMode.HALF_UP, NLS.getMachineFormatSymbols(), false, null); - }).orElse(NumberFormat.MACHINE_NO_DECIMAL_PLACES); + }); } @Override diff --git a/src/test/java/sirius/db/mongo/properties/MongoAmountEntity.java b/src/test/java/sirius/db/mongo/properties/MongoAmountEntity.java index 157633be8..0bbcb3af5 100644 --- a/src/test/java/sirius/db/mongo/properties/MongoAmountEntity.java +++ b/src/test/java/sirius/db/mongo/properties/MongoAmountEntity.java @@ -9,14 +9,21 @@ package sirius.db.mongo.properties; import sirius.db.mixing.Mapping; +import sirius.db.mixing.annotations.Numeric; import sirius.db.mongo.MongoEntity; import sirius.kernel.commons.Amount; public class MongoAmountEntity extends MongoEntity { + public static final int AMOUNT_SCALE = 3; + public static final Mapping TEST_AMOUNT = Mapping.named("testAmount"); private Amount testAmount = Amount.NOTHING; + public static final Mapping SCALED_AMOUNT = Mapping.named("scaledAmount"); + @Numeric(precision = 20, scale = AMOUNT_SCALE) + private Amount scaledAmount = Amount.NOTHING; + public Amount getTestAmount() { return testAmount; } @@ -24,4 +31,12 @@ public Amount getTestAmount() { public void setTestAmount(Amount testAmount) { this.testAmount = testAmount; } + + public Amount getScaledAmount() { + return scaledAmount; + } + + public void setScaledAmount(Amount scaledAmount) { + this.scaledAmount = scaledAmount; + } } diff --git a/src/test/kotlin/sirius/db/mongo/properties/MongoAmountPropertyTest.kt b/src/test/kotlin/sirius/db/mongo/properties/MongoAmountPropertyTest.kt index 2edc7680d..217a9fa3a 100644 --- a/src/test/kotlin/sirius/db/mongo/properties/MongoAmountPropertyTest.kt +++ b/src/test/kotlin/sirius/db/mongo/properties/MongoAmountPropertyTest.kt @@ -13,8 +13,11 @@ import org.junit.jupiter.api.extension.ExtendWith import sirius.db.mongo.Mango import sirius.kernel.SiriusExtension import sirius.kernel.commons.Amount +import sirius.kernel.commons.Value import sirius.kernel.di.std.Part +import java.math.RoundingMode import kotlin.test.assertEquals +import kotlin.test.assertFalse @ExtendWith(SiriusExtension::class) class MongoAmountPropertyTest { @@ -23,9 +26,31 @@ class MongoAmountPropertyTest { val values = listOf(-3.77, Double.MAX_VALUE, 0.00001, -0.00001) for (value in values) { assertEquals(Amount.of(value), saveAndRead(Amount.of(value))) + assertEquals(Amount.of(value), saveAndReadUsingPropertyParsing(Value.of(value))) } } + @Test + fun `property autoscaling of amount fields works`() { + var test = MongoAmountEntity() + val unscaledValue = Value.of("1,2345") + + val scaledAmountProperty = test.descriptor.getProperty("scaledAmount") + scaledAmountProperty.parseValue(test, unscaledValue) + val testAmountProperty = test.descriptor.getProperty("testAmount") + testAmountProperty.parseValue(test, unscaledValue) + mango.update(test) + test = mango.refreshOrFail(test) + val expectedAmount = unscaledValue.amount.round(MongoAmountEntity.AMOUNT_SCALE, RoundingMode.HALF_UP) + assertEquals(expectedAmount, test.scaledAmount) + assertEquals(unscaledValue.amount, test.testAmount) + + // Storing the same value twice must not trigger a change + scaledAmountProperty.parseValue(test, unscaledValue) + testAmountProperty.parseValue(test, unscaledValue) + assertFalse { test.isAnyMappingChanged } + } + companion object { @Part private lateinit var mango: Mango @@ -37,5 +62,14 @@ class MongoAmountPropertyTest { mongoAmountEntity = mango.refreshOrFail(mongoAmountEntity) return mongoAmountEntity.testAmount } + + private fun saveAndReadUsingPropertyParsing(value: Value): Amount { + var mongoAmountEntity = MongoAmountEntity() + val amountProperty = mongoAmountEntity.descriptor.getProperty("testAmount") + amountProperty.parseValueFromImport(mongoAmountEntity, value) + mango.update(mongoAmountEntity) + mongoAmountEntity = mango.refreshOrFail(mongoAmountEntity) + return mongoAmountEntity.testAmount + } } } From a55f1638ed089c7b9ce705e87a6e199f44ca5d47 Mon Sep 17 00:00:00 2001 From: Matthias Keck Date: Wed, 18 Dec 2024 14:44:24 +0100 Subject: [PATCH 5/5] Fixes test entity so existing tests do not break Fixes: SIRI-1041 --- src/test/java/sirius/db/mongo/properties/MongoAmountEntity.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/test/java/sirius/db/mongo/properties/MongoAmountEntity.java b/src/test/java/sirius/db/mongo/properties/MongoAmountEntity.java index 0bbcb3af5..2c4eff43e 100644 --- a/src/test/java/sirius/db/mongo/properties/MongoAmountEntity.java +++ b/src/test/java/sirius/db/mongo/properties/MongoAmountEntity.java @@ -9,6 +9,7 @@ package sirius.db.mongo.properties; import sirius.db.mixing.Mapping; +import sirius.db.mixing.annotations.NullAllowed; import sirius.db.mixing.annotations.Numeric; import sirius.db.mongo.MongoEntity; import sirius.kernel.commons.Amount; @@ -21,6 +22,7 @@ public class MongoAmountEntity extends MongoEntity { private Amount testAmount = Amount.NOTHING; public static final Mapping SCALED_AMOUNT = Mapping.named("scaledAmount"); + @NullAllowed @Numeric(precision = 20, scale = AMOUNT_SCALE) private Amount scaledAmount = Amount.NOTHING;