New flags introduces non compatible changes to JsonDiff.asJson

[PeterBackman]

Hi this is related to Issue #51. As of Issue #58 a new optional feature was introduced adding a new flag: OMIT_ORIGINAL_VALUE_ON_REPLACE. This made the API break for public static JsonNode asJson(final JsonNode source, final JsonNode target, EnumSet<DiffFlags> flags) since I get different output with unchanged input. I think this method should be treated with the same vigilance as the method without flags. Now I must go through all changes to see if the logic has changed between versions and add corresponding flags to the call to the method.

I propose that flags in the future are added with relation to the default behaviour of the library. For #58 it should have been ADD_ORIGINAL_VALUE_ON_REPLACE instead of OMIT_. I think it is much easier to understand. In this way both public methods can be kept compatible when new flags are added.

I hope you can see the benefit of this change of principles?

[holograph]

Agree wholeheartedly. I was thinking along those lines when I saw the merge, but you managed to articulate this much more clearly than I would 👍

…

On Thu, 28 Dec 2017 at 12:42 Peter Bäckman ***@***.***> wrote: Hi this is related to Issue #51 <#51>. As of Issue #58 <#58> a new optional feature was introduced adding a new flag: OMIT_ORIGINAL_VALUE_ON_REPLACE. This made the API break for public static JsonNode asJson(final JsonNode source, final JsonNode target, EnumSet<DiffFlags> flags) since I get different output with unchanged input. I think this method should be treated with the same vigilance as the method without flags. Now I must go through all changes to see if the logic has changed between versions and add corresponding flags to the call to the method. I propose that flags in the future are added with relation to the default behaviour of the library. For #58 <#58> it should have been ADD_ORIGINAL_VALUE_ON_REPLACE instead of OMIT_. I think it is much easier to understand. In this way both public methods can be kept compatible when new flags are added. I hope you can see the benefit of this change of principles? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#61>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABEC4Nf6K01lNPyemHz51U2S164tFmgYks5tE3CBgaJpZM4RONeC> .

[LouizFC]

I vote for the following approach:

1. Mark all public methods that require flags in the parameters as deprecated (JsonDiff#asJson, JsonPatch#apply and JsonPatch#applyInPlace) and rollback it to pre-Add original value information for the REPLACE information. #58
2. Make JsonPatch and JsonDiff instantiable, the current public static methods that don't requires flags can use a default instance internally.
3. Make something similar to Jackson's ObjectMapper#configure method, a set of fluent methods that you can use to customize your instance.

Edit: Also, maybe create a new Enum called JsonPatchFeatures and JsonDiffFeatures with the suggested names.

[LouizFC]

@PeterBackman, could you please provide a snippet of the issue? I will take some time later to fix it

[PeterBackman]

source = {"a":1}
target = {"a":2}

Sample code used in both versions:

EnumSet<DiffFlags> patchFlags = EnumSet.of(
  DiffFlags.OMIT_VALUE_ON_REMOVE,
  DiffFlags.OMIT_MOVE_OPERATION,
  DiffFlags.OMIT_COPY_OPERATION);
JsonNode result = JsonDiff.asJson(source, target, patchFlags);

JsonPatch 0.3.5:
result = [{"op":"replace","path":"/a","value":2}]

JsonPatch 0.4.0:
result = [{"op":"replace","fromValue":1,"path":"/a","value":2}]

[LouizFC]

Let me ask: Since compatibility is already "broke", should I rename the OMIT_. to ADD_. and change the behavior of the flag? Or like I said earlier, deprecate the method and follow with my approach?

[PeterBackman]

I looked at the commits and saw that the flag was introduced in 0.3.10 which is quite recent so it makes sense to change that flag. From my perspective the patch looks good.

Long term I think a rewrite of the interface is good. Using the methods with the flags parameter are non-intuitive. Perhaps something for next year 😃

[vishwakarma]

Thanks @PeterBackman @LouizFC