WW-4871 Fixes StringConverter and improves it's tests

[yasserzamani]

No description provided.

[aleksandr-m]

Please use { } even if it is one line.

[aleksandr-m]

Tests are failing.

[yasserzamani]

Thanks! both fixed :)

[lukaszlenart]

I think we are good ... @aleksandr-m what's your opinion?

[aleksandr-m]

Where 15 comes from? Shouldn't it be 16?

[yasserzamani]

Ouch! I mislead max fraction digits with max precision while they're different. thank you. fixed.

[aleksandr-m]

As a quick fix it looks ok. In the future it would be good to make this configurable somehow (defaults, exact digits, settings from locale).

[lukaszlenart]

As a quick fix it looks ok. In the future it would be good to make this configurable somehow (defaults, exact digits, settings from locale).

I would start with this and then add more options when requested

[yasserzamani]

Tests for convert back also improved within NumberConverter's tests

[aleksandr-m]

@yasserzamani Can you explain where these numbers coming from? How do you find them?

[yasserzamani]

For BigDecimal theoretically there is no limit for count of fraction digits [1] but technically setMaximumFractionDigits cannot get bigger than Integer.MAX_VALUE.

For Double and Float, I converted Double.MIN_VALUE and Float.MIN_VALUE to a not scientific notated string by java 7 8 and 9 and found that numbers.

[1] http://download.oracle.com/javase/6/docs/api/java/math/BigDecimal.html

[yasserzamani]

@aleksandr-m what do you think about keeping max integer for all instead:

if (BigDecimal.class.isInstance(value) || 
    Double.class.isInstance(value) || Float.class.isInstance(value)) {
          format.setMinimumFractionDigits(1);
          format.setMaximumFractionDigits(Integer.MAX_VALUE);
}

[lukaszlenart]

I think that's ok

[yasserzamani]

👍 Fixed

[aleksandr-m]

Maybe we shouldn't format numbers at all. Just replace , and . according to locale. WDYT?

[lukaszlenart]

Maybe we shouldn't format numbers at all. Just replace , and . according to locale. WDYT?

You mean calling .toString and replacing . with , when needed?

[lukaszlenart]

Something like this:

        if (Number.class.isInstance(value)) {
            String str = value.toString();
            DecimalFormatSymbols otherSymbols = new DecimalFormatSymbols(locale);
            return str.replace('.', otherSymbols.getDecimalSeparator());
        } else {
            return Objects.toString(value, null);
        }

[aleksandr-m]

@lukaszlenart Yes. The if (Number.class.isInstance(value)) should have same conditions as in number converter. And if (otherSymbols == '.') can be added to skip replacement if it isn't needed.

[lukaszlenart]

The if (Number.class.isInstance(value)) should have same conditions as in number converter

Not sure what you mean by that, here we are converting a number to a String, while in the NumberConverter we are converting a String to a number.

[aleksandr-m]

What point to convert number to string according to locale if it is cannot be converted back (e.g. on form submit).
Are we converting all instances of Number in number converter? If yes than it is ok.

[yasserzamani]

@aleksandr-m , it was to fix WW-3171. And I think locale has not any problem to converting back from string, according to this PR's NumberConverterTest.

I think it's ok and just needs to expand max fraction digits which this PR does.

[lukaszlenart]

All the converters are Locale-aware which means they will convert a String to a Number using user Locale and then they will be able convert the Number back to a String using the same Locale.

And as Yasser pointed out this is all about fraction digits ... what I like in your suggestion about using the .toString method is that it will resolve one problem which can be observed in the Select tag - when key is defined as a BigDecimal, current version of converter will add a .0 to it, with .toString there is no such problem.

[yasserzamani]

Łukasz, if you like to not add a .0, deleting format.setMinimumFractionDigits(1); does this. I did it in my first commit but saw some test fails then reverted it back and thought it's needed.

[lukaszlenart]

The .0 is not always needed, it would be good to compare it with tests before I had introduced the Locale aware converters.

[yasserzamani]

The .0 is not always needed

How previous version detects when needed? I saw previous StringConverter and seems only can converts arrays and dates and returns null elsewhere !

[lukaszlenart]

I meant check the tests :) The previous version was using .toString I think

[yasserzamani]

Łukasz, I reviewed tests. It seems the root cause is a very old 13 years old issue, WW-455. I think not adding .0 will not cause any trouble with that issue any more. Maybe the old WebWork was adding .0 which is not rational I think.

I think we can fix Select-3.txt contents and rest tests in orther and delete format.setMinimumFractionDigits(1), then if this raised any issue, that issue should be fixed itself not by adding not meaningful .0 :) wdyt?

[lukaszlenart]

This how it was before:

        // XW-409: If the input is a Number we should format it to a string using the choosen locale and use java's numberformatter
        if (Number.class.isAssignableFrom(toType)) {
            NumberFormat numFormat = NumberFormat.getInstance(getLocale(context));
            if (isIntegerType(toType)) {
                numFormat.setParseIntegerOnly(true);
            }
            numFormat.setGroupingUsed(true);
            numFormat.setMaximumFractionDigits(99); // to be sure we include all digits after decimal seperator, otherwise some of the fractions can be chopped

            String number = numFormat.format(value);
            if (number != null) {
                return number;
            }
        }

        return null; // no number

(XWorkBasicConverter)

[lukaszlenart]

I have no idea why I did use the setMinimumFractionDigits(1) :\

[lukaszlenart]

ouch! you are right, I have focused on BigDecimals and totally forgot about that, sorry :(

This makes me wonder why our tests didn't catch this.

[yasserzamani]

@aleksandr-m , this is a known java floating point behavior because these types use a binary mantissa, they cannot precisely represent many finite decimal numbers, such as 0.1, because these numbers have repeating binary representations.

So, IMHO, I don't see any bad thing to display 100234.398 in jsp because it's the real value of fff which java will use in any computation.

When precise computation is necessary, such as when performing currency calculations, user should not use floating-point types. Instead, an alternative representation that can completely represent the necessary values should be used, e.g. BigDecimal.

Are you sure .toString() always has your mentioned behavior? e.g. for 0.1f?

[aleksandr-m]

@yasserzamani Don't think about computation, think about presentation layer.

100234.398 in jsp because it's the real value of fff which java will use in any computation.

No. If you change formatter settings it will be 100234.3984375 or something else.

Are you sure .toString() always has your mentioned behavior? e.g. for 0.1f?

Why not? It is simple to string.

[yasserzamani]

Thanks; I saw Float.toString docs, you're right, it's more suitable for presentation layer as will generally produce the shortest decimal representation that can unambiguously identify the true value of the floating-point number! NumberFormat.format seems create string from the actual real value till max fraction digits.

Maybe we can .toString, count it's result fraction digits, setMaxFractionDigits to that then format?

[yasserzamani]

This makes me wonder why our tests didn't catch this.

Łukasz, I found some time to go deeper and saw that this happens when value cannot be represented exactly with a finite binary number. Failing test examples are added at end.

After all, contrary to my expectation, .toString has better behavior than Locale.NumberFormat.format ! It produces the shortest decimal representation that can unambiguously identify the true value of the floating-point number, but that, produces the true value as is !

Following tests fails:

    public void testDoubleToStringConversionPL() throws Exception {
        // given
        StringConverter converter = new StringConverter();
        Map<String, Object> context = new HashMap<>();
        context.put(ActionContext.LOCALE, new Locale("pl", "PL"));

        // when cannot be represented exactly with a finite binary number
        value = converter.convertValue(context, null, null, null, 0.1 + 0.1 + 0.1, null);

        // then produce the shortest decimal representation that can unambiguously identify the true value of the floating-point number
        assertEquals("0,3", value);
    }

    public void testFloatToStringConversionPL() throws Exception {
        // given
        StringConverter converter = new StringConverter();
        Map<String, Object> context = new HashMap<>();
        context.put(ActionContext.LOCALE, new Locale("pl", "PL"));

        // when cannot be represented exactly with a finite binary number
        value = converter.convertValue(context, null, null, null, 0.1f, null);

        // then produce the shortest decimal representation that can unambiguously identify the true value of the floating-point number
        assertEquals("0,1", value);
    }

[lukaszlenart]

I think @aleksandr-m is right and the 0.1+0.1+0.1 example fails but I would expect that is must fail:

junit.framework.AssertionFailedError: 
Expected :0,3
Actual   :0,30000000000000004

user would also be surprised if this wouldn't fail.

[yasserzamani]

You're right; 0.1+0.1+0.1 also fails with .toString. However, 0.1f does not.

[aleksandr-m]

The point is, if you print the final number into the console from the action and later show it in the JSP both should be the same (except decimal separator because we take numbers according the locale).

If you're doing some computation like 0.1+0.1+0.1 in the action and want to show it in JSP then it is expected that you need to somehow beautify the numbers.

[yasserzamani]

@aleksandr-m , @lukaszlenart , while decimal 0.1 cannot be represented exactly with a finite binary number, but this PR formats 0.1d (double) to 0,1 but formats 0.1f (float) to 0,10000000149011612. As for both the max fraction digits is Integer.MAX_VALUE and as both java float and double types follow IEEE 754, I cannot understand this different behavior.

wdyt? It seems we have the issue only with float data type.

[yasserzamani]

Eureka! I reviewed java.text.NumberFormat.format:254 :

        } else if (number instanceof Number) {
            return format(((Number)number).doubleValue(), toAppendTo, pos);
        } 

((Number)number).doubleValue() causes the issue. I fixed that in above commit.

[aleksandr-m]

@yasserzamani Looks like a hack. ;) Why not to use code provided by Lukasz #173 (comment) ?

[yasserzamani]

Yes I see :) please let me leave it as is until I discuss this issue with jdk team. java.text.NumberFormat.format should not have different behavior against 0.1f and 0.1d, I think. For 0.1d it produces 0,1 but for 0.1f produces 0,10000000149011612 while both double and float are IEEE 754 ❗️ I'm almost sure this should be a bug.

Łukasz code is ok but please let me keep this hack until jdk's fix as we do not have this issue with other Numbers just for float which is jdk's failure, I think :)

[lukaszlenart]

I don't like fixing JDK's bugs inside the framework, it's hard to track if the bug was fixed and we supposed to drop the workaround or even make it deprecated and allow users to switch to a new solution. If you decide keep the current solution, please add TODO or FIXME to document this workaround.

That being said, I would like to have this fixed and start preparing a new release.

[yasserzamani]

Yes I see, I just was in wait for a reference jdk url to being added to the TODO (I reported at http://bugreport.java.com/ and they wrote they will list this in their database or contact me for more info if was needed).

However, I added the TODO; I'll add a reference url from jdk when was ready :) thanks!

[lukaszlenart]

I have just one doubt: re-assigning to an input parameter

[yasserzamani]

:) you're right! I added a test in a commit which confirms that StringConverter does not change input parameter value.

[lukaszlenart]

I meant that this isn't a problem from compiler point-of-view as parameters are always local (compiler makes a copy of them), it's rather a bad style :)

[yasserzamani]

Oh :) I thought you are in doubt because we pass the value parameter by type Object.

Thanks for learning this to me! I researched and it seems there are two best practices ; using a defined local variable, or returning from method at that line. what do you recommend?

[lukaszlenart]

I would use a dedicated local variable in this case.

[yasserzamani]

Thanks! I think I fixed it in my another commit ; I'm sorry for these a lot of commits :/

[lukaszlenart]

@aleksandr-m I'm down with these changes and this PR can be merged IMO

[aleksandr-m]

@lukaszlenart Better than current state. +1 for merging.

[yasserzamani]

"a double value is able to hold this value precisely", they said after seven days in a comment and simply closed the issue 😮 The issue is visible on bugs.java.com at the following url JDK-8190928.

I replied back that if you believe a double value is able to hold 0.1d precisely, then please try:

System.out.println(new java.math.BigDecimal(0.1d));
System.out.println(0.1d + 0.1d + 0.1d);

The output is:

0.1000000000000000055511151231257827021181583404541015625
0.30000000000000004

IT IS NOT:

0.1
0.3

so a double value also is not able to hold 0.1d precisely, i think.

I hope they will reply. I know they're advanced experts but I cannot understand what is the wrong with my understanding :( I hope they will help.

[lukaszlenart]

The linke explains everything ... but you must have at least PhD to understand that fully ;-)
https://docs.oracle.com/cd/E19957-01/806-3568/ncg_goldberg.html

[yasserzamani]

yeah 😃 however all my current below evidences show this should be a bug:

• Why java.text.NumberFormat.format(Float.MIN_VALUE) does not represent Float.MIN_VALUE correctly? it contains wastes digits coming from float -> double conversion ❗️
• I searched 0.1 in above link and it does not distinguish between single or double precision for 0.1. It always says 0.1 cannot be represented exactly in binary in floating-point formats. It does not distinguish double for this rule.
• Anyway, if java has implemented double in such a way which can stores 0.1 exactly, so, why System.out.println(0.1d + 0.1d + 0.1d); does not print 0.3 in java.

[lukaszlenart]

LGTM 👍

[lukaszlenart]

The most discussed PR ever!

[yasserzamani]

yeah! I should thank Aleksandr and you for your time to review. In the end it became very better than my beginnings commits.

[gregh3269]

As a workaround, you can do to restore to original output: ie 1.0 rather than just 1

format.double={0,number,##0.0}
<s:textfield name="bean.field" value="%{getText('format.double',{bean.field})}" />

Cheers Greg