flipkart-incubator / zjsonpatch

This is an implementation of RFC 6902 JSON Patch written in Java
Apache License 2.0
527 stars 153 forks source link

Interest to remove/migrate apache and guava dependencies #59

Closed LouizFC closed 6 years ago

LouizFC commented 6 years ago

while reading the source code, I realized that most of the methods used from apache and guava libraries could be ported to the kotlin std lib or written by hand. I think that the library would benefit much from the shrink in dependencies, since guava is almost 2.5MB

So I ask if there is interest in migrating? I can make a commit for this after the holidays. My only worries are about java 6 compability (in kotlin std lib case) and perfomance.

The main difficult of the port would be the apache LCS algorithm that JsonDiff uses.

vishwakarma commented 6 years ago

I agree, removal of apache and guava dependency will make this library more slim and useable. It would be more preferred if this library remains compatible with all java versions. All corresponding replacements of guava and apache usage can be replaced easily ( More than 1000 test cases to verify that ). I can pick this task since i am already aware of the codebase, but let me know the otherwise, i would be available for any help needed?

Thanks

LouizFC commented 6 years ago

@vishwakarma Thank you very much for your support. Because english is not my primary language I had some difficult understanding you last question, I am sorry.

for what I understanded: I will commit the changes in the source, and after that, you will change the tests?

another question: would you prefer to move all the new methods to a "InternalUtils.java" or declare each method inside the class that uses it?

vishwakarma commented 6 years ago

Hi @LouizFC No problem :) I have raised a PR #60 , which contains removal of apache commons dependency (Wrote own LCS). Please review it and kindly make further changes in the same branch for removal of guava dependency. I have a large test suite ( 16M json docs & updates ) apart from one checked-in with this library, I will use it to validate all our changes.

LouizFC commented 6 years ago

Thank you @vishwakarma I am doing the fixes now. I have two questions yet:

List<String> paths = Splitter.on('/').splitToList(path.toString().replaceAll("\"", ""));
return Lists.newArrayList(Iterables.transform(paths, DECODE_PATH_FUNCTION));

There in the replaceAll("\"", ""), it is removing all the quotes from the JsonNode#toString method. Is this intended? if I have the following jsons:

{"\"a\"": 1}

and

{"\"b\"": 1}

then apply this:

JsonDiff.asJson(jsonA, jsonB)

I end up with the following patch:

[{"op":"move","from":"/\"a\"","path":"/\"b\""}]

and if I apply the patch like this:

JsonPatch.apply(patch, jsonA)

I end up with the following json:

{"\"a\"":1,"\\b\\": null}

should I fix this too or it is intended to?

vishwakarma commented 6 years ago

Sure @LouizFC , Please go ahead and regarding "replaceAll("\"", "")", I will check this and get back to you

LouizFC commented 6 years ago

Seeing JsonNode implementation, it creates a TextNode for holding string values. It stores the value without quotes, you can retrieve this value using "asText". Maybe that can help.

vishwakarma commented 6 years ago

I will try out "asText"