Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

org.json.junit.XMLTest.testIssue537CaseSensitiveHexEscapeFullFile fails #745

Closed
davejbur opened this issue Jun 12, 2023 · 10 comments
Closed

Comments

@davejbur
Copy link

davejbur commented Jun 12, 2023

I'm having problems compiling, as testIssue537CaseSensitiveHexEscapeFullFile fails. I'm using:

  • Windows 11 64bit
  • Netbeans 16
  • JDK1.8

The error I get is:

org.json.junit.XMLTest.testIssue537CaseSensitiveHexEscapeFullFile -- Time elapsed: 0.056 s <<< FAILURE!
org.junit.ComparisonFailure: values should be equal expected:<... sensitive CRP, IL-1[?])> but was:<... sensitive CRP, IL-1[╬▓])>
	at org.junit.Assert.assertEquals(Assert.java:117)
	at org.json.junit.Util.compareActualVsExpectedObjects(Util.java:93)
	at org.json.junit.Util.compareActualVsExpectedJsonObjects(Util.java:52)
	at org.json.junit.Util.compareActualVsExpectedObjects(Util.java:68)
	at org.json.junit.Util.compareActualVsExpectedJsonArrays(Util.java:33)
	at org.json.junit.Util.compareActualVsExpectedObjects(Util.java:74)
	at org.json.junit.Util.compareActualVsExpectedJsonObjects(Util.java:52)
	at org.json.junit.Util.compareActualVsExpectedObjects(Util.java:68)
	at org.json.junit.Util.compareActualVsExpectedJsonObjects(Util.java:52)
	at org.json.junit.XMLTest.testIssue537CaseSensitiveHexEscapeFullFile(XMLTest.java:923)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.junit.rules.ExternalResource$1.evaluate(ExternalResource.java:54)
	at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
	at org.junit.runners.BlockJUnit4ClassRunner$1.evaluate(BlockJUnit4ClassRunner.java:100)
	at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:103)
	at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:63)
	at org.junit.runners.ParentRunner$4.run(ParentRunner.java:331)
	at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:79)
	at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:329)
	at org.junit.runners.ParentRunner.access$100(ParentRunner.java:66)
	at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:293)
	at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
	at org.junit.runners.ParentRunner.run(ParentRunner.java:413)
	at org.apache.maven.surefire.junit4.JUnit4Provider.execute(JUnit4Provider.java:316)
	at org.apache.maven.surefire.junit4.JUnit4Provider.executeWithRerun(JUnit4Provider.java:240)
	at org.apache.maven.surefire.junit4.JUnit4Provider.executeTestSet(JUnit4Provider.java:214)
	at org.apache.maven.surefire.junit4.JUnit4Provider.invoke(JUnit4Provider.java:155)
	at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:385)
	at org.apache.maven.surefire.booter.ForkedBooter.execute(ForkedBooter.java:162)
	at org.apache.maven.surefire.booter.ForkedBooter.run(ForkedBooter.java:507)
	at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:495)

This is using the latest version of the code. I have also gone back to previous releases and tried compiling them, as follows:
20220924 - success, no issues
20230227 - fails, but at a different test

I'm just wondering whether it's something to do with the mod in JSONTokener.java line 60, where a recent change now explicitly uses UTF-8?

@stleary
Copy link
Owner

stleary commented Jun 13, 2023

Is anyone else seeing this error?

@davejbur
Copy link
Author

I tried commenting out , Charset.forName("UTF-8") but got the same problem, so it's not (directly) that change that's causing the problem.
I wonder if it's GitHub doing something silly on Windows again, just like the line-endings issue I had earlier? Anyone else using Windows?

@stleary
Copy link
Owner

stleary commented Jun 13, 2023

I just recreated the problem on Windows with Java 17.0.7.
Looks like something is broken, but don't have a root cause yet.
There are also a couple of new warnings that will need to be fixed, but these are not specific to Java 17:

JSON-java/src/main/java/org/json/ParserConfiguration.java:77: warning: no @param for <T>
    public <T extends ParserConfiguration> T withKeepStrings(final boolean newVal) {
                                             ^
JSON-java/src/main/java/org/json/ParserConfiguration.java:101: warning: no @param for <T>
    public <T extends ParserConfiguration> T withMaxNestingDepth(int maxNestingDepth) {

Also confirmed on Mac pro with Java 17.0.7, but seeing many warnings, and these new errors:

org.json.junit.JSONObjectTest > bigNumberOperations FAILED
    java.lang.ExceptionInInitializerError at JSONObjectTest.java:1425
        Caused by: java.lang.reflect.InaccessibleObjectException at JSONObjectTest.java:1425

org.json.junit.JSONObjectTest > jsonObjectByBean1 FAILED
    java.lang.NoClassDefFoundError at JSONObjectTest.java:600
        Caused by: java.lang.ExceptionInInitializerError at JSONObjectTest.java:1425

@davejbur
Copy link
Author

davejbur commented Jun 14, 2023

Just to be more specific, I'm using jdk8u362-b09.

I originally said, then deleted, but should probably have left in, that the test testIndentComplicatedJsonObjectWithArrayAndWithConfig also fails. It would appear that the expected response ends:

</response>
<error>null</error>]
>

but the actual response ends:

</response>
<error>null</error>
]
>

(note extra line break)
I find this test also fails when trying to compile the code used in the 20230227 release. The 20220924 release compiles with no problems.

@davejbur
Copy link
Author

The test files Issue537.json & Issue537.xml contain a symbol that, on my Windows 11 system, appears to display as a German "eszett" or Greek letter "beta".

However, when compiling the code & running the tests, the character is displayed as:
Expected: ? (query mark)
Actual: ╬▓ (two ascii graphics characters
image
)

What would we actually expect to see in this test? What do others on different OSs see?

@davejbur
Copy link
Author

I've now dug into this further.

JSONTokener#nextString(char quote) calls JSONTokener#next(), where line 185 returns a char:

this.previous = (char) c;

On Windows, the non-standard character in Issue537.json gets returned as ?. I've added a line outputting the int value of this char - it's 946.

I'm wondering whether #nextString shouldn't contain an additional case for chars greater than 255? It would of course need careful testing, since non-Windows systems apparently handle the whole test with no problems.

@davejbur
Copy link
Author

davejbur commented Jun 22, 2023

Lightbulb moment!
It's definitely an encoding issue. Greek beta is U+03B2 in UTF-8 (which is decimal 946), and is represented by the bytes CE B2. The test file contains the bytes CE B2 at that position - ascii codes 206 & 178. In straight ascii characters, that's the two I'm getting in my output (see graphic previous post). So my guess now is that the "expected" result is indeed the correct one, it's the "actual" one that's wrong. I'll have a look into that side tomorrow if I get a chance.

@davejbur
Copy link
Author

OK, some more information.

I went back to the 20220924 release (which compiles correctly on my system), and added the same logging (basic System.out.println`()` ) changes as I've done to the current XMLTest.javafile. (I also cut down theIssue537.json&Issue537.xml`` files to make the actual problem here stand out better).

With the 20220924 release, the test passes because both the expected and actual output is {"c":{"s":{"d":"╬▓"}}}.
(With the current version of the code, the expected output is reported as {"c":{"s":{"d":"?"}}} whilst the actual output is still {"c":{"s":{"d":"╬▓"}}}. (Note that, delving further in, the ? appears to be recognised further back in the code as U+03B2, decimal 946).

So two main questions:

(1) What has changed between the two releases that means the expected output has changed at this point?

(2) What is correct? I've assumed so far that the problem lies in the change in expected output, i.e. ? is incorrect. But what if that is actually correct, and the incorrect reading of the file is ╬▓? What I mean is, both expected & actual were incorrect (on Windows) back in release 20220924; now, the expected output has been corrected but the actual output is still wrong? Indeed, I would have thought that both expected & actual output should have been something more like {"c":{"s":{"d":"β"}}} anyway?

Hope that makes sense - and that this is of interest to someone!

@davejbur
Copy link
Author

Success!

In XMLTest.java, method testIssue537CaseSensitiveHexEscapeFullFile, the line:

                Reader xmlReader = new InputStreamReader(xmlStream);

needs to be changed to:

                Reader xmlReader = new InputStreamReader(xmlStream, Charset.forName("UTF-8"));

This makes the "actual" & "expected" results match ({"c":{"s":{"d":"?"}}} using my cutdown test file).

However, before I submit a PR, I'd like confirmation that this is the correct approach, as per my previous comment.

@stleary
Copy link
Owner

stleary commented Jun 23, 2023

@davejbur Thanks for the analysis and recommendation. I think it should be fine to update the XMLTest test case as you suggested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants