bugfix 319 fix invalid BigDecimal serialization because of locale #320
Conversation
Signed-off-by: Simulant <[email protected]>
There was a problem hiding this comment.
Thanks for the contribution @Simulant87! Just one minor change requested and then we can merge
| @@ -49,7 +50,7 @@ public AbstractNumberDeserializer(Class<T> clazz, Customization customization) { | |||
|
|
|||
| final JsonbNumberFormatter numberFormat = getCustomization().getDeserializeNumberFormatter(); | |||
| //consider synchronizing on format instance or per thread cache. | |||
| final NumberFormat format = NumberFormat.getInstance(jsonbContext.getConfigProperties().getLocale(numberFormat.getLocale())); | |||
| final NumberFormat format = NumberFormat.getInstance((Locale.ENGLISH)); | |||
| ((DecimalFormat)format).applyPattern(numberFormat.getFormat()); | |||
| format.setParseIntegerOnly(integerOnly); | |||
There was a problem hiding this comment.
going with the same train of thought not allowing commas in JSON numbers, I think we should also call format.setGroupingUsed(false) here, because people may try to use a number format like ###,##0.00 which could output a number like 123,456.78 and should be serialized as 123456.78 according to RFC 7159
There was a problem hiding this comment.
I updated this and also the test to cover it. But there are other test failing:
org.eclipse.yasson.customization.NumberFormatTest#testDeserializer
org.eclipse.yasson.customization.NumberFormatTest#testDeserializeWithoutClassLevelFormatter
I am unsure if I should just adopt the expected behavior/input data to my new implementation to let them pass as this changes current behavior.
There was a problem hiding this comment.
while I think this behavior proposed in this PR is technically correct, some existing users may be depending on the behavior and it would be good to not break such users in a micro release.
Since we have a major/minor release of JSON-B on the horizon, I am thinking we should defer this PR to a later release. Is that OK with you @Simulant87 or do you urgently need this change?
There was a problem hiding this comment.
That is OK with me and I totally agree that because of the changed behavior it should be part of a major- and not part of a minor-release. I am not depending an a fix.
Signed-off-by: Simulant <[email protected]>
fix #319