Fix non deterministic for LoadPlanTest.testJson()

[annhchen89]

What does this PR do?

This PR is fixing a nondeterministic test failure in LoadPlanTest.testJson() when running with Nondex.

Problem

Previously, the test used:

assertEquals(expected.replace("'", "\""), json);

This approach directly compared JSON strings. However, JSON object field order is nondeterministic, and NonDex randomizes iteration orders.
As a result, the test failed when field order in the generated JSON differed from the expected literal string, even though both represented the same structure.

Reproduce Test

Run:

mvn edu.illinois:nondex-maven-plugin:2.2.1:nondex -pl core -Dtest=org.apache.accumulo.core.data.LoadPlanTest#testJson

The Fix

Instead of comparing JSON strings directly, the test now parses both expected and actual JSON into JsonNode objects and asserts structural equality. This ensures the comparison is order-independent and validates actual JSON semantics, not textual layout.

[dlmarion]

@annhchen89 - Are #5961 and #5962 the only issues you found using NonDex in the Accumulo codebase? I looked at the NonDex Maven plugin details to see if it would fail the build if the plugin found any violations. I didn't find mention of failing the build, do you know if it will?

[annhchen89]

@dlmarion There is also #5947 and #5948, and Yes if there are non-deterministic ID Test found, then it will fail the build.

[dlmarion]

If LoadPlan implemented equals(), then we could use that to deserialize the json and compare the original and deserialized objects. It doesn't currently do that, so this is sufficient for now.

[kevinrr888]

Can these changes and the changes in any other PR related to violations from NonDex be put into a single PR?

I closed #5948 due to inactivity and it being in an unmergable state (see #5948 (comment)), but could include those changes in the new/updated singular PR.

[dlmarion]

@kevinrr888 - I was reviewing your #5948 (comment) and while I agree with your comment that the JSON is valid, I think the issue is that we are comparing JSON as Strings in the test code and the gson code for serialization order is non-deterministic unless you tell it what the order should be.

I think the better solution is to not compare Strings in the test, but deserialize the json back to an object and do object comparison. I'm thinking we should add NonDex to the build to identify these occurrences so we can fix them to do object comparison.

Thoughts?

[kevinrr888]

@dlmarion The issue is that the build fails with NonDex with something like:

    1) ObjectMapper mapper = new ObjectMapper();

    2) JsonNode node1 = mapper.readTree("{\"foo\":1,\"bar\":2}");
    3) JsonNode node2 = mapper.readTree("{\"bar\":2,\"foo\":1}");

    4) assertEquals(node1, node2);

at line 4. In this test, we are not comparing Strings but JsonNode objects which can be compared directly.

[dlmarion]

@dlmarion The issue is that the build fails with NonDex with something like:

    1) ObjectMapper mapper = new ObjectMapper();

    2) JsonNode node1 = mapper.readTree("{\"foo\":1,\"bar\":2}");
    3) JsonNode node2 = mapper.readTree("{\"bar\":2,\"foo\":1}");

    4) assertEquals(node1, node2);

at line 4. In this test, we are not comparing Strings but JsonNode objects which can be compared directly.

Line 4 fails because JsonNode.equals, specifically ObjectNode.equals, uses a LinkedHashMap for the children. Jackson's JsonNode is enforcing order at this point.

Edit: NonDex is pointing out that Gson serializes in non-deterministic order by default. So if we are comparing json strings in our test where one String is serialized using gson, then it could fail at a future point. I think the suggestion of using Jackson to perform the test as we are using a different json serialization framework in the test vs what we are using at runtime. I think the solution is to not compare json strings in the tests. Use Gson to deserialize the string back to an object and use equals().

[kevinrr888]

Line 4 fails because JsonNode.equals, specifically ObjectNode.equals, uses a LinkedHashMap for the children. Jackson's JsonNode is enforcing order at this point.

But order does not matter when comparing LinkedHashMap...

I certainly agree that we shouldn't be comparing JSON strings, but I am just pointing out that NonDex might not be the way to go about enforcing this in our build due to the above issue

Dismissing my approval. This fix gets the NonDex build plugin to pass, but I don't think using Jackson to test Gson serialized output is the right solution.

[kevinrr888]

More context:
Seems my simplified example to:

    1) ObjectMapper mapper = new ObjectMapper();

    2) JsonNode node1 = mapper.readTree("{\"foo\":1,\"bar\":2}");
    3) JsonNode node2 = mapper.readTree("{\"bar\":2,\"foo\":1}");

    4) assertEquals(node1, node2);

Does NOT actually fail the build, but the exact example (from #5948)

    1) ObjectMapper mapper = new ObjectMapper();
    
    2) JsonNode expectedJson = mapper.readTree(zKRootV2);
    3) JsonNode actualJson = mapper.readTree(new String(byteCapture.getValue(), UTF_8));

    4) assertEquals(expectedJson, actualJson);

does fail the build... I was trying to give a simple failure example but ended up removing the failure.
While I don't understand what's going on here, @dlmarion seems to, and is going to look into adding NonDex to the build

[dlmarion]

The NonDex plugin doesn't work on a full build, so we can't add it to the build. I'm going to put up a different PR to resolve the issue in LoadPlanTest

[dlmarion]

Closing in favor of #6008