long int as key makes NumberFormatException

[SakuraSa]

config/config/src/main/java/com/typesafe/config/impl/SimpleConfigObject.java

Line 452 in eeb9582

return Integer.compare(Integer.parseInt(a), Integer.parseInt(b));

when key is all digits, and bigger than Integer.MAX
this line can throw NumberFormatException

maybe use BigDecimal instead of Integer when dealing with all-digits long string key.

new BigDecimal(a).compareTo(new BigDecimal(b))

[havocp]

Thanks for debugging this, that makes sense. I'm a little tempted to try to figure out how to avoid creating those BigDecimal objects in the common case (common case is a few integers from 0-5 or so), but it would definitely make the code more complex to do that, so I dunno if it really matters.

Would also be good to add a test for this scenario.

[AngryGami]

RenderComparator inner class could be implemented without need for Integer.parseInt or BigDecimal, like this:

static final private class RenderComparator implements java.util.Comparator<String>, Serializable {
    private static final long serialVersionUID = 1L;
    // This is supposed to sort numbers before strings,
    // and sort the numbers numerically. The point is
    // to make objects which are really list-like
    // (numeric indices) appear in order.
    @Override
    public int compare(String a, String b) {

        int aLen = a.length(), bLen = b.length(), k = 0;
        int aFirstNotZeroIndex = -1, bFirstNotZeroIndex = -1;
        boolean aAllDigits = aLen != 0, bAllDigits = bLen != 0;
        int strCompareResult = 0, numCompareResult = 0;
        // must iterate to longest string to ensure all characters are digits
        int lim = Math.max(aLen, bLen);
        while (k < lim) {
            char ac = 0, bc = 0;
            if (k < aLen) {
                ac = a.charAt(k);
                aAllDigits = aAllDigits && Character.isDigit(ac);
                if (aAllDigits && aFirstNotZeroIndex == -1) {
                    aFirstNotZeroIndex = Character.getNumericValue(ac) == 0 ? aFirstNotZeroIndex : k;
                }
            }
            if (k < bLen) {
                bc = b.charAt(k);
                bAllDigits = bAllDigits && Character.isDigit(bc);
                if (bAllDigits && bFirstNotZeroIndex == -1) {
                    bFirstNotZeroIndex =  Character.getNumericValue(bc) == 0 ? bFirstNotZeroIndex : k;
                }
            }
            if (ac != bc && strCompareResult == 0) {
                strCompareResult = ac - bc;
            }
            if (!aAllDigits && !bAllDigits && strCompareResult != 0) {
                // stop iteration since here is known we dealing with non-numbers
                break;
            }
            if (aFirstNotZeroIndex != -1 && bFirstNotZeroIndex != -1 && aFirstNotZeroIndex == bFirstNotZeroIndex && numCompareResult == 0) {
                numCompareResult = Character.getNumericValue(ac) - Character.getNumericValue(bc);
            }
            k++;
        }
        if (!aAllDigits && !bAllDigits) {
            // both compared values are non-numbers - use string comparison rule
            return strCompareResult == 0 ? (aLen - bLen) : strCompareResult;
        }
        if (!aAllDigits || !bAllDigits) {
            // one of the compared values is number so it go first (i.e. it is "smaller")
            return aAllDigits ? -1 : 1;
        }
        // calculate length of number exluding leading zeros
        int aNumLength = aLen - Math.max(0, aFirstNotZeroIndex);
        int bNumLength = bLen - Math.max(0, bFirstNotZeroIndex);

        return aNumLength != bNumLength ? (aNumLength - bNumLength) : numCompareResult;
    }
}

[havocp]

I think this may be fixed on master - there was a PR that fixed it, anyway, even if not merged

[AngryGami]

No it is not fixed on master:
https://github.com/lightbend/config/blob/master/config/src/main/java/com/typesafe/config/impl/SimpleConfigObject.java#L452

[havocp]

it's fixed in this PR which isn't merged yet I guess #557

[AngryGami]

Yes, I see that now. If you still believe that using BigInteger might be an overkill here code above could be directly used in DefaultComparator.

[axaluss]

This is still not fixed?