make sure locale independent data is not upper/lowercased incorrectly…

[johnjaylward]

…. See #315

[johnjaylward]

Just for reference, Locale.ROOT was added in java6, which is why it was not in the original implementation. So this meets our current build requirement of JDK6.

[stleary]

Sounds reasonable. I thought there was a Character.toLowerCase() in XMLTokener, is that not an issue? Unit tests will be needed as well.

[johnjaylward]

I didn't see a locale option for the Character.toLowerCase() calls. Only for String.

[johnjaylward]

@stleary maybe in the XML tokener instead of calling toLowerCase for each character, we call it with the toString on https://github.com/stleary/JSON-java/blob/master/XMLTokener.java#L139

like this:

    public Object nextEntity(char ampersand) throws JSONException {
        StringBuilder sb = new StringBuilder();
        for (;;) {
            char c = next();
            if (Character.isLetterOrDigit(c) || c == '#') {
                sb.append(c);
            } else if (c == ';') {
                break;
            } else {
                throw syntaxError("Missing ';' in XML entity: &" + sb);
            }
        }
        String string = sb.toString().toLowerCase(Locale.ROOT);
        Object object = entity.get(string);
        return object != null ? object : ampersand + string + ";";
}

[johnjaylward]

Actually, for the nextEntity call, the locale should probably set to Locale.EN since all entities are either numeric or latin1 encoded.

• < = < or <

[stleary]

I can add unit tests tonight, unless you will be making further changes to the code.

[johnjaylward]

I won't make any more changes unless you think we need to modify the XMLTokenizer code. Thanks for taking care of the tests.

[stleary]

Glad we have unit tests! Looks like JSONPointer needs more work, but not yet sure where.

public class MyLocaleBean {
    private final String üx = "Tlocaleüx";
    public String getÜx() {  return üx;    }
}

MyLocaleBean myLocaleBean = new MyLocaleBean();
JSONObject jsonObject = new JSONObject(myLocaleBean);
String str1 = (String)jsonObject.get("üx");
String str2 = (String)jsonObject.query("/getÜx");
System.out.println("str1: " + str1 + " str2: " + str2);

str1: Tlocale?x
str2: null

[stleary]

On second thought:

String str1 = (String)jsonObject.get("üx");
String str2 = (String)jsonObject.query("/üx");

str1: Tlocale?x str2: Tlocale?x

[johnjaylward]

I think it's OK for the values. I'm not too clear on how the locale effects string output. Maybe test the output in Locale.EN as well and see what you get? On Feb 11, 2017 01:32, "Sean Leary" <notifications@github.com> wrote: On second thought: String str1 = (String)jsonObject.get("üx"); String str2 = (String)jsonObject.query("/üx"); str1: Tlocale?x str2: Tlocale?x — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#317 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAXa16FPSMXmyEP_91sVqrFIJCIB-7vRks5rbVYBgaJpZM4L9ezk> .

[stleary]

Turned out I was just using the wrong query string, fixed now. But this code has to be saved as UTF, so I am putting the tests into a different file. Don't want someone saving JSONObjectTest as ASCII and having the tests fail.

[stleary]

The following tests are successful without using the code in this pull request.

public class MyLocaleBean {
    private final String üx = "Tlocaleüx";
    private final String ü = "Tlocaleü";
    public String getÜx() {        return üx;    }
    public String getÜ() {        return ü;    }
}
        assertTrue("expected Tlocaleüx",
                "Tlocaleüx".equals(jsonObject.getString("üx")));
        assertTrue("expected Tlocalü",
                "Tlocaleü".equals(jsonObject.getString("ü")));
        assertTrue("expected Tlocaleüx",
                "Tlocaleüx".equals((String)(jsonObject.query("/üx"))));
        assertTrue("expected Tlocalü",
                "Tlocaleü".equals((String)(jsonObject.query("/ü"))));

To run the test I had to add this to build.gradle:

tasks.withType(JavaCompile) {
     // this subproject requires -parameters option
     options.compilerArgs << '-parameters'
     options.encoding = 'UTF-8'
}

My compiler version is: 1.8.0_05

Any suggestions?

[johnjaylward]

Try using the same example as the submitter. Specifically the Id with the switch from EN to TR.

…

On Feb 11, 2017 18:35, "Sean Leary" ***@***.***> wrote: The following tests are successful *without* using the code in this pull request. public class MyLocaleBean { private final String üx = "Tlocaleüx"; private final String ü = "Tlocaleü"; public String getÜx() { return üx; } public String getÜ() { return ü; } } assertTrue("expected Tlocaleüx", "Tlocaleüx".equals(jsonObject.getString("üx"))); assertTrue("expected Tlocalü", "Tlocaleü".equals(jsonObject.getString("ü"))); assertTrue("expected Tlocaleüx", "Tlocaleüx".equals((String)(jsonObject.query("/üx")))); assertTrue("expected Tlocalü", "Tlocaleü".equals((String)(jsonObject.query("/ü")))); To run the test I had to add this to build.gradle: tasks.withType(JavaCompile) { // this subproject requires -parameters option options.compilerArgs << '-parameters' options.encoding = 'UTF-8' } My compiler version is: 1.8.0_05 Any suggestions? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#317 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAXa10DkV9t_4qnDL3r0UOkPqLuAEIf7ks5rbkXEgaJpZM4L9ezk> .

[stleary]

What problem does this code solve?
JSONObject keys obtained from parsing beans should be invariant regardless of locale. This change enforces Locale.ROOT rules for lowercasing of the first letter of the key in get...() or is...() methods.

Risks
Low risk. This is likely a one-off that most developers are unlikely to see.

Changes to the API?
The first char of the key name is lowercased in a locale-invariant way when parsing beans.

Will this require a new release?
This change can be rolled into the next release.

Should the documentation be updated?
Not necessary at this time.

Does it break the unit tests?
No, but a unit test was added that will cause breakage until this change is committed. See stleary/JSON-Java-unit-test#64

Was any code refactored in this commit?
No, just the requested change was made.

Review status
ACCEPT. Starting 3 day comment window with this post.