lukas-krecan / JsonUnit

Compare JSON in your Unit Tests
Apache License 2.0
871 stars 114 forks source link

Unexpected behavior Diff.create(...) when using deep scan JSON paths (..) to be ignored #769

Closed pandoras-toolbox closed 1 month ago

pandoras-toolbox commented 1 month ago

When I use Diff.create(...) with deep scan JSON paths to be ignored it only works as expected if I call the internal method InternalJsonPathUtils.resolveJsonPaths(...). That took me quite a time to figure it out.

See the example below. I would have expected that I do not need to call this method. How could I know that I need to do that?

Perhaps you can remove the need to call that internal method? Or at least throw an exception if someone uses Diff.create(...) with a deep scan path to be ignored?

package com.swisssign.at.core;

import net.javacrumbs.jsonunit.core.Configuration;
import net.javacrumbs.jsonunit.core.internal.Diff;
import net.javacrumbs.jsonunit.jsonpath.InternalJsonPathUtils;

public final class Example {

    private static final String EXPECTED = "{ \"name\": \"John\", \"age\": 30, \"city\": \"New York\" }";
    private static final String ACTUAL = "{ \"name\": \"John\", \"age\": 31, \"city\": \"Los Angeles\" }";

    // Does not work as expected:
    private static Diff getDiff1() {
        return Diff.create(EXPECTED, ACTUAL, "", "",
            Configuration.empty().whenIgnoringPaths("$..age"));
    }

    // Works as expected:
    private static Diff getDiff2() {
        return Diff.create(EXPECTED, ACTUAL, "", "",
            InternalJsonPathUtils.resolveJsonPaths(ACTUAL,
                Configuration.empty().whenIgnoringPaths("$..age")));
    }

    public static void main(String[] args) {
        System.out.println("Diff 1:");
        System.out.println(getDiff1().differences());
        System.out.println("Diff 2:");
        System.out.println(getDiff2().differences());
    }

}
lukas-krecan commented 1 month ago

Hi, the Diff class is internal to the library and is not meant to be used directly as described in the Javadoc and in the package name. I can't stop you from using it, but you are on your own

pandoras-toolbox commented 1 month ago

I know that it is internal, and a red flag. But I have not found any other good way to solve it.

Don't you think that your library does not behave well in that case? It does not work correctly when using a deep scan JSON paths to ignore something. Or do you disagree, if yes, why?

I was not eager to use that internal method. Is there a way to use deep scan JSON paths to ingore something without having to use that internal method?

lukas-krecan commented 1 month ago

Can you please describe what you are trying to achieve? All the functionality provided by the Diff class should be exposed through the public APIs, and if it's not, let's fix that.

pandoras-toolbox commented 1 month ago

I want to compare two large JSONs, actual vs. expected for an automated E2E test.

They should be identical ignoring two attributes, timeCreated and timeModified which are be deeply nested in the JSON. There are many of them in the JSON tree, thus we want to exclude them with the best what JSON path has to offer, deep scans: $..timeCreated and $..timeModified.

But that does not work without using that internal method obviously.

Why don't we use assertThatJson(actual).isEqualTo(expected) you might ask? Because the built in error message is very incomplete for us. For instance the value of the attribute name might be different in the actual JSON, but it does not show the JSON path in the error message, and the name attribute appears in many places in the JSON, and it also does not say the type of difference (missing, extra or value difference).

The method net.javacrumbs.jsonunit.core.internal.Diff#getDifferences unfortunately returns a string and not a list of elements of type Difference, which would allow us to build a string to our own needs. So I had to use a DifferenceListener to achieve that, see below. But that is a lesser issue for me.

The main issue is that I had to use the internal method InternalJsonPathUtils.resolveJsonPaths(...) to make it work with deep scans because I do not want to provide a list of dozens of JSON pathes to be ignored and maintain them when I could simply use $..timeCreated and $..timeModified.

With this information, do you have any suggestions for me, or ideas how you could update JsonUnit so that I can achieve my goal without using an internal method, and perhaps also not needing to use a DifferenceListener?

var differences = JsonDiff.getDifferences(actualPolicyJson, expectedPolicyJson, Configuration.empty()
    .withOptions(Option.IGNORING_ARRAY_ORDER)
    .whenIgnoringPaths("$..timeCreated", "$..timeModified"));
assertThat(differences)
    .as("differences when comparing actual to expected")
    .isEmpty();
public static List<String> getDifferences(String actual, String expected, Configuration configuration) {
    List<String> differences = new ArrayList<>();
    var recordingDifferenceListener = new RecordingDifferenceListener();
    var diff = Diff.create(expected, actual, "", "",
        // If the configuration object would be used as it is then the ignored paths
        // would not work correctly regarding deep scans (for example $..timeStamp).
        // See https://github.com/lukas-krecan/JsonUnit/issues/769
        InternalJsonPathUtils.resolveJsonPaths(actual, configuration)
            .withDifferenceListener(recordingDifferenceListener));
    // Trigger comparison, otherwise the listener is never used:
    diff.similar();
    for (var difference : recordingDifferenceListener.getDifferenceList()) {
        switch (difference.getType()) {
            case DIFFERENT -> differences.add("Different value at JSON path '$.%s', expected '%s' but was '%s'"
                .formatted(difference.getActualPath(), difference.getExpected(), difference.getActual()));
            case MISSING -> differences.add("Missing JSON path '$.%s'"
                .formatted(difference.getExpectedPath()));
            case EXTRA -> differences.add("Extra JSON path '$.%s'"
                .formatted(difference.getActualPath()));
            default -> throw new IllegalStateException("Unsupported type: " + difference.getType());
        }
    }
    return differences;
}
pandoras-toolbox commented 1 month ago

I think I dismissed the assertThatJson(...) method too soon because it seemed that it does not tell where a difference occured. If it is a non-nested attribute then it simply references it as something like "node xyz". So I thought that it is not using a JSON path. But if it is deeper nested then it writes something like "node abc.yzy".

There is another problem with the way I implemented it related to arrays, if two (or more perhaps) values are different in it then the JSON path is wrong and it says it is not different but that something is missing or extra.

I think in the end I can use assertThatJson(...) simply for the equals comparison.

The very latest Chat GPT pointed me in the wrong direction, I should not use it anymore I guess.

lukas-krecan commented 1 month ago

You can try to use DifferenceListener for better difference handling. (until today it was undocumented)