Internal refactor of JsonDiff and add some tests.

[LouizFC]

While I was reading this I had a brief insight. With a custom tailored diff algorithm, maybe we could fix both https://github.com/flipkart-incubator/zjsonpatch/issues/18 and https://github.com/flipkart-incubator/zjsonpatch/issues/20.

I propose the following refactors on JsonDiff:

1. Create better tests for JsonDiff to avoid things like https://github.com/flipkart-incubator/zjsonpatch/issues/61.
2. Use a explicit stack (ArrayDeque) for it's recursive functions, to avoid StackOverflow exceptions.
3. Maybe refactor JsonDiff to have configurable instances instead of being only a "util"/"static" class, having one internal "default" instance.

More on 3: Refactor JsonDiff to have the following schema:

public class JsonDiff {

    /* current static methods */

    /**
     *  separate the "compactDiffs" and "introduceCopyOperation" to their own classes.
     *  These compactor's would be executed sequentially.
     */
    private List<Compactor> compactors = new ArrayList<>();

    private JsonEquivalence equivalence;

    private EnumSet<DiffFlags> flags;

    /* diff logic */

    /* fluent setter for flags. */
}

/**
 * Maybe use a enum that implement this interface, so we can have
 * safe "singletons" for the default Compactors ("move", "copy" and "replace")
 */
interface Compactor {
    /**
     *  maybe other methods that don't require a list as param,
     * so we don't need to create unnecessary objects.
     */
    Diff compact(List<Diff> diffs);

}

/**
 * use custom "equivalences" for future features and solving current issue.
 */
interface JsonEquivalence{
    boolean equals(JsonNode first, JsonNode second);
}

Also, use Linear Space Myers and/or Hirschberg LCS algorithms as a base to improve perfomance and memory usage.

Of course, that would take a fair amount of time, so I am opening this issue for informative porpouses and to collect some opnions before I dive into it.

Edit: Just for clarification, most of these changes would be internal api. Maybe in future we could release it as public

[holograph]

I’m theoretically all for it, except the added complexity had better be worth it; to be frank in our use cases performance was never a serious issue or consideration, but perhaps others have different requirements? On Fri, 19 Jan 2018 at 13:52 Luiz Felipe da Cruz Oliveira < notifications@github.com> wrote:

While I was reading this https://news.ycombinator.com/item?id=13983085 I had a brief insight. With a custom tailored diff algorithm, maybe we could fix both #18 https://github.com/flipkart-incubator/zjsonpatch/issues/18 and #20 https://github.com/flipkart-incubator/zjsonpatch/issues/20.

I propose the following refactors on JsonDiff:

1. Create better tests for JsonDiff to avoid things like #61 https://github.com/flipkart-incubator/zjsonpatch/issues/61.
2. Use a explicit stack (ArrayDeque) for it's recursive functions, to avoid StackOverflow exceptions.
3. Maybe refactor JsonDiff to have configurable instances instead of being only a "util"/"static" class, having one internal "default" instance.

More on 3: Refactor JsonDiff to have the following schema:

public class JsonDiff {

/* current static methods */

/**     *  separate the "compactDiffs" and "introduceCopyOperation" to their own classes.     *  These compactor's would be executed sequentially.     */
private List<Compactor> compactors = new ArrayList<>();

private JsonEquivalence equivalence;

private EnumSet<DiffFlags> flags;

/* diff logic */

/* fluent setter for flags. */

} /* Maybe use a enum that implement this interface, so we can have safe "singletons" for the default Compactors ("move", "copy" and "replace") /interface Compactor { /* maybe other methods that don't require a list as param, so we don't need to create unnecessary objects. / Diff compact(List diffs);

} /* use custom "equivalences" for future features and solving current issue. */interface JsonEquivalence{ boolean equals(JsonNode first, JsonNode second); }

Also, use Linear Space Myers and/or Hirschberg LCS algorithms as a base to improve perfomance and memory usage.

Of course, that would take a fair amount of time, so I am opening this issue for informative porpouses and to collect some opnions before I dive into it.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/flipkart-incubator/zjsonpatch/issues/63, or mute the thread https://github.com/notifications/unsubscribe-auth/ABEC4CqhGo48rPcy4xBjZ9QUjamMfMMCks5tMIHjgaJpZM4RkXk1 .

[LouizFC]

@holograph Thanks for your reply. Perfomance tweaks are more a of a habit of mine. As you said, the added complexity just for perfomance sake may not pay off in the end.

But in general I want to add more complexity for the sake of flexibility.

I want to (try to) make bug fixing and feature creation easier and I think that the better way to do this is separating responsabilities and delegating them to other more "change friendly" classes, making "core changes" only necessary on critical changes.

[LouizFC]

Some updates on this issue:

• Since Jackson 2.6, JsonNode has a method called equals(Comparator<JsonNode>comparator, JsonNode other). This method returns true if the given Comparator returns 0 (according to Comparator contract/documentation, 0 means equality) on its "Comparator#compare" method. So the numeric equivalence can be easily solved using that, without any backward incompatible changes.

• As I was rewritting the JsonDiff class, I felt like it was getting very out of the context of this repo. I will drop most of the proposed changes from this issue. I have written another library from scratch for a more general approach on root based Tree diff. I plan to release it on github when it get's more mature and production ready, when I do this I will link it here.

I plan to begin working on the numeric equivalence issue this weekend.

[LouizFC]

Closing this issue, see #73 for more info