tdunning / open-json

Open JSON - a truly open source JSON implementation
Apache License 2.0
18 stars 18 forks source link

Package interface is not compatible with original org.json.* #11

Closed mfursov closed 6 months ago

mfursov commented 7 years ago

My project uses both this package and Google Firebase. As the result I have this error:

java.lang.NoSuchMethodError: org.json.JSONStringer.object()Lorg/json/JSONWriter; at com.google.firebase.database.util.JsonMapper.serializeJsonValue(JsonMapper.java:56) at com.google.firebase.database.util.JsonMapper.serializeJsonValue(JsonMapper.java:45) at com.google.firebase.database.util.JsonMapper.serializeJson(JsonMapper.java:25) at com.google.firebase.database.util.GAuthToken.serializeToString(GAuthToken.java:53) at com.google.firebase.database.core.JvmAuthTokenProvider$1.onComplete(JvmAuthTokenProvider.java:40) at com.google.firebase.tasks.OnCompleteCompletionListener$1.run(OnCompleteCompletionListener.java:37)

Reason: Google libraries uses one library and Wicket uses another by the same name. These 2 libraries are in conflict.

I would suggest to have 100% compatible API with the latest version of the original package or change package name.

https://search.maven.org/#artifactdetails%7Ccom.google.firebase%7Cfirebase-server-sdk%7C3.0.3%7Cjar

tdunning commented 7 years ago

Can you suggest what semantics and API points you want?

There isn't an actual specification available and I don't want to compromise the clean-room status of open json.

Test cases would be very helpful.

On Tue, Feb 28, 2017 at 3:37 AM, Mikhail Fursov notifications@github.com wrote:

My project uses both this package and Google Firebase. As the result I have this error:

java.lang.NoSuchMethodError: org.json.JSONStringer.object() Lorg/json/JSONWriter; at com.google.firebase.database.util.JsonMapper. serializeJsonValue(JsonMapper.java:56) at com.google.firebase.database.util.JsonMapper. serializeJsonValue(JsonMapper.java:45) at com.google.firebase.database.util.JsonMapper.serializeJson( JsonMapper.java:25) at com.google.firebase.database.util.GAuthToken. serializeToString(GAuthToken.java:53) at com.google.firebase.database.core.JvmAuthTokenProvider$1.onComplete( JvmAuthTokenProvider.java:40) at com.google.firebase.tasks.OnCompleteCompletionListener$1.run( OnCompleteCompletionListener.java:37)

Reason: Google libraries uses one library and Wicket uses another by the same name. These 2 libraries are in conflict.

I would suggest to have 100% compatible API or do not use the same package name.

https://search.maven.org/#artifactdetails%7Ccom.google. firebase%7Cfirebase-server-sdk%7C3.0.3%7Cjar

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/tdunning/open-json/issues/11, or mute the thread https://github.com/notifications/unsubscribe-auth/AAPSegkmorxymoJm6tVdf6NLh1uz9XU8ks5rg5ZfgaJpZM4MN7fN .

martin-g commented 7 years ago

@tdunning I think the real problem is that OpenJson uses org.json as package name. This way it clashes badly with Douglas Crockford's JSON library. Some projects (like Wicket) bring OpenJson as dependency, others (like Google Firebase) bring the old JSON library. At the end the JVM picks the class (e.g. JSONObject) from the one that is earlier in the constructed classpath. IMO in the long run it would be safer if OpenJson changes its package name to org.openjson or something like that.

tdunning commented 7 years ago

Well, the point here is to completely replace the Crockford's library since he refuses to release it under an open source license.

As such, exactly emulating the API to the degree that could be discerned was necessary because Wicket had a deadline to remove the original org.json library. If you care about your code being open source and distributable, then you might want to consider dependency surgery to get Firebase to not use the problematic dependency.

If you can specify what functionality that would require, then I am happy to look at implementing it.

I am a little surprised that Firebase uses Crockford's non-open library, however, since Google did a clean-room re-implementation (that I used as a base) for Android.

On Tue, Feb 28, 2017 at 8:42 AM, Martin Grigorov notifications@github.com wrote:

@tdunning https://github.com/tdunning I think the real problem is that OpenJson uses org.json as package name. This way it clashes badly with Douglas Crockford's JSON library. Some projects (like Wicket) bring OpenJson as dependency, others (like Google Firebase) bring the old JSON library. At the end the JVM picks the class (e.g. JSONObject) from the one that is earlier in the constructed classpath. IMO in the long run it would be safer if OpenJson changes its package name to org.openjson or something like that.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/tdunning/open-json/issues/11#issuecomment-282977598, or mute the thread https://github.com/notifications/unsubscribe-auth/AAPSepeOXHXMUmRBPHTkU8WdFHyrGBpWks5rg93zgaJpZM4MN7fN .

mfursov commented 7 years ago

I support Martin that using unique package name is the best solution.

Reasons:

tdunning commented 7 years ago

That may well be, but the point of open JSON is to provide a temporary plug compatible replacement for projects like wicket or hadoop who need to get rid of the old library immediately.

Over the longer term, switching to Jackson is the right thing to do. Open JSON is not a long-term solution.

So, I repeat my limited offer. If you have a limited addition to the API, I am happy to help. Changing the package name, however, is off the table because it conflicts with the primary goal.

On Feb 28, 2017 6:41 AM, "Mikhail Fursov" notifications@github.com wrote:

I support Martin that using unique package name is the best solution.

Reasons:

-

The 'old' library has multiple releases. The latest is dated by 2016. For some people this project is alive: https://search.maven.org/# search%7Cgav%7C1%7Cg%3A%22org.codeartisans%22%20AND%20a%3A% 22org.json%22 https://search.maven.org/#search%7Cgav%7C1%7Cg%3A%22org.codeartisans%22%20AND%20a%3A%22org.json%22

There are 1000+ projects that relies on old code: https://mvnrepository.com/artifact/org.json/json https://mvnrepository.com/artifact/org.json/json Having any of them in classpath may cause runtime (not compile time) issues.

It is actually a big work to support all methods from 'org.json'. It also includes XML support :)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/tdunning/open-json/issues/11#issuecomment-283056647, or mute the thread https://github.com/notifications/unsubscribe-auth/AAPSek-1x1--jA8K2SF1wFYia0THgNVsks5rhDIBgaJpZM4MN7fN .

mfursov commented 7 years ago

If the primary goal is to get rid of old code why package name is so important? In Wicket, for example, the package name today is not org.json but org.apache.wicket.ajax.json.

Another example could be: when Sun included Xerces into JDK - they changed the package name for the same reason: to avoid conflicts.

About 'limited addition to API': you either 100% compatible or not. 1000+ projects in Maven depends on original features of json.org. Some of them from Google, some from Spring, some from Apache. Why to have this problem at all?

Missing any of class, method, constant, code behavior is a hidden bug that will reveal itself in runtime.

mfursov commented 7 years ago

Here is jardiff report. changes.html.zip

solomax commented 7 years ago

I would like to come up with an alternative solution: Maybe additional branch like 2.0 can be created and will have 2.X.X version and different package name So users willing to use simple fully open library can switch to 2.x.x once Others can use 1.x.x with old name as "short term" solution

WDYT?

mfursov commented 7 years ago

I would definitely like this project and contribute to it. I'm a happy user of 'JSONObject' library for many years. This library has a lot of benefits for me when compared to other solutions: it's simple, it has smaller jar size, it has no dependencies, it has better performance than Jackson (checked for parsing 4mb json files).

I do not like original https://github.com/stleary/JSON-java copy because it has no LinkedHashMap inside of JSONObject and the maintainer of the library refuses to accept any related fixes, however 3rd party datasets I depend upon rely on ordering in JSON to optimize data size. So Wicket impl was perfect for me and I would definitely contribute to another version that follows the same goals.

BTW, Hi Max :) Long time no see!

mfursov commented 7 years ago

just for fun: below are benchmark results for this package vs org.json: (benchmark origin: https://github.com/RichardHightower/json-parsers-benchmark)

Benchmark                                Mode Thr     Count  Sec         Mean 

i.g.j.s.OrgJsonBenchmark.actionLabel    thrpt   8         3    1   409515.806
i.g.j.s.OpenjsonBenchmark.actionLabel   thrpt   8         3    1   860833.050

i.g.j.s.OrgJsonBenchmark.citmCatalog    thrpt   8         3    1      280.911
i.g.j.s.OpenjsonBenchmark.citmCatalog   thrpt   8         3    1      508.311

i.g.j.s.OrgJsonBenchmark.medium         thrpt   8         3    1   176217.306
i.g.j.s.OpenjsonBenchmark.medium        thrpt   8         3    1   605602.511

i.g.j.s.OrgJsonBenchmark.menu           thrpt   8         3    1  1425624.972
i.g.j.s.OpenjsonBenchmark.menu          thrpt   8         3    1  2547549.411

i.g.j.s.OrgJsonBenchmark.sgml           thrpt   8         3    1   628004.661
i.g.j.s.OpenjsonBenchmark.sgml          thrpt   8         3    1  1447232.944

i.g.j.s.OrgJsonBenchmark.small          thrpt   8         3    1  8125230.083
i.g.j.s.OpenjsonBenchmark.small         thrpt   8         3    1 11046816.617

i.g.j.s.OrgJsonBenchmark.webxml         thrpt   8         3    1   106045.972
i.g.j.s.OpenjsonBenchmark.webxml        thrpt   8         3    1   337826.061

i.g.j.s.OrgJsonBenchmark.widget         thrpt   8         3    1   401139.761
i.g.j.s.OpenjsonBenchmark.widget        thrpt   8         3    1   911970.478
martin-g commented 7 years ago

Another candidate to switch to OpenJson: Apache Tapestry. I will cross link the issues so they are aware of our current findings.

https://issues.apache.org/jira/browse/TAP5-2575?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15897115#comment-15897115

solomax commented 7 years ago

@martin-g should I move wicketstuff projects to com.github.openjson? @mfursov I see lots of commits :) when are you planning first release?

mfursov commented 7 years ago

Max,

All these commits are tests only. I see no reasons for any new release until some real issues fixed.

Originally I thought the package should be 100% compatible with original 'org.json' and started implementing missed parts, but later removed all these changes: I understood and appreciated the way the package was minified. Really good job.

So, 'com.github.openjson' package is 100% the same as this package now.

I'm going to add missed tests, want to make github to understand the licence correctly (some external tools like 'versioneye' depends on it) - but all these are infrastructure related changes only, not API.

solomax commented 7 years ago

Why I'm asking: Wicket is now in Milestone period (8.0.0-M4), and all major changes are only allowed in this period So I guess it might worth to release this artifact with new coordinates, so wicket can receive updates after 8.0.0 will be finalized ...

mfursov commented 7 years ago

But it was released at the moment of fork: https://search.maven.org/#search%7Cga%7C1%7Copenjson

and is used by Wicket: https://github.com/apache/wicket/commit/f25687ade198b8e9dedf91d120c7f0d3ec0f1039

Did I miss something here?

solomax commented 7 years ago

It seems I missed all the changes, sorry for the noise :( (was on mountain skiing session for a while) OK, going to check wicketstuff and perform necessary changes Thanks for the pointers

klopfdreh commented 7 years ago

Hi,

I am a bit confused. Where does the release now comes from:

<dependency>
   <groupId>com.github.openjson</groupId>
   <artifactId>openjson</artifactId>
   <version>1.0.0</version>
   <type>jar</type>
</dependency>

I didn't see any changes in this repository. May I missed something.

klopfdreh commented 7 years ago

Ah now I see its this fork:

https://github.com/openjson/openjson

I would suggest to release one branch that is able to exchange the json.org lib as @tdunning mentioned and one branch that uses a new package names. This could be released separately and you could decide which to use:

<dependency>
   <groupId>com.github.openjson</groupId>
   <artifactId>openjson-json-org-replacement</artifactId>
   <version>1.0.0</version>
   <type>jar</type>
</dependency>
<dependency>
   <groupId>com.github.openjson</groupId>
   <artifactId>openjson</artifactId>
   <version>1.0.0</version>
   <type>jar</type>
</dependency>
martin-g commented 7 years ago

@klopfdreh I don't understand your suggestion. Please give more details!

tdunning commented 7 years ago

I think that the suggestion is to release two versions of the library, one with good package hygiene and one with package name collision.

On Wed, Mar 22, 2017 at 11:33 PM, Martin Grigorov notifications@github.com wrote:

@klopfdreh https://github.com/klopfdreh I don't understand your suggestion. Please give more details!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/tdunning/open-json/issues/11#issuecomment-288629647, or mute the thread https://github.com/notifications/unsubscribe-auth/AAPSeqEFhnUJxkuFhykLcEItUJZoPSknks5rohJSgaJpZM4MN7fN .

klopfdreh commented 7 years ago

Exactly.

slankas commented 6 years ago

This library's naming causes issues in larger projects with multiple dependencies and sub-dependencies. It is not a 100% compatible version with the "org.json" as maintained now at https://github.com/stleary (there are several issues I've noticed - especially with automatic conversion of collections into json objects and arrays along with XML conversion capabilities)

While I'm speaking from hindsight, it was a significant mistake to duplicate the org.json package name. Replacing package names within a Java project is a very straightforward task with little cost. Projects who wanted to use this implementation should have been forced to incur that cost. Instead, I had to hunt down a problem because the Tika implementation group decided to use this library.

solomax commented 6 years ago

Hello @slankas, this was temporary solution, finally package name was changed: https://github.com/openjson/openjson I can contact Apache Tika and propose this little refactoring as a PR :)

tdunning commented 6 years ago

Look, you can say whatever you want about significant mistakes. But you weren't there. You didn't make a library to solve an excellent problem. In fact, of you are cataloging errors, I would count NOT implementing anything when it was needed as a larger mistake.

Also, this is open source. You don't like it, fix it. To want a different path, fork it. Please.

But don't go second guessing someone who took action when action was needed. Remember also, this library was a stopgap measure intended as an emergency way to buy time. To should be switching to a real library for json.

On Oct 26, 2017 4:08 PM, "slankas" notifications@github.com wrote:

This library's naming causes issues in larger projects with multiple dependencies and sub-dependencies. It is not a 100% compatible version with the "org.json" as maintained now at https://github.com/stleary (there are several issues I've noticed - especially with automatic conversion of collections into json objects and arrays along with XML conversion capabilities)

While I'm speaking from hindsight, it was a significant mistake to duplicate the org.json package name. Replacing package names within a Java project is a very straightforward task with little cost. Projects who wanted to use this implementation should have been forced to incur that cost. Instead, I had to hunt down a problem because the Tika implementation group decided to use this library.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/tdunning/open-json/issues/11#issuecomment-339826108, or mute the thread https://github.com/notifications/unsubscribe-auth/AAPSeiiQJBQ9p3ME9kgWmGsbeHh-EhAjks5swRDcgaJpZM4MN7fN .

slankas commented 6 years ago

@solomax Thank you. The movement to the openjson package name and avoiding the conflicts was definitely needed. I still disagree with maintaining the org.json duplication - that will continue to raise conflicts. Unfortunately, there are 10 libraries within Maven that have picked up this particular implementation. (https://mvnrepository.com/artifact/com.tdunning/json/usages). I don't think it appropriate for you to contact the Tika team. To @tdunning 's point, if I don't like, I should fix it.

@tdunning I stand by my criticism. The continued use of the org.json package name will continue to cause conflicts. My solution would be to shut this project down. Direct others to the new package name. I will make a suggestion(through an issue and will provided text in a fork) to @stleary to add commentary on his readme for those looking for an open-source version that avoid's Crockford's licensing issue to use the https://github.com/openjson/openjson implementation. I do appreciate the work you've put int this project. Performing a clean-room implementation of an in existing API is not insignificant.

solomax commented 6 years ago

@mfursov, @tdunning, all :)

what can be done here, we can release new version of this library with following pom modification:

<distributionManagement>
    <relocation>
        <groupId>com.github.openjson</groupId>
        <artifactId>openjson</artifactId>
        <version>1.0.8</version>
    </relocation>
</distributionManagement>

as it is done here http://central.maven.org/maven2/xml-apis/xml-apis/2.0.2/xml-apis-2.0.2.pom This will result: http://mvnrepository.com/artifact/xml-apis/xml-apis/2.0.2

I guess this way all users will be notified they need to move to the correct version

WDYT?

tdunning commented 6 years ago

maxim,

I don't think I quite understand the specifics of what you are suggestion and who you are asking.

Who is "we" in "we can release".

Are you suggesting that the github.com/openjson project release a new version? Or that I release a new version?

Or are you suggesting that "this library" that should be released is xml-apis and you are talking about making a release of that with a new dependency?

I am happy to help. Just a bit too confused to do so.

On Thu, Oct 26, 2017 at 8:15 PM, Maxim Solodovnik notifications@github.com wrote:

@mfursov https://github.com/mfursov, @tdunning https://github.com/tdunning, all :)

what can be done here, we can release new version of this library with following pom modification:

com.github.openjson openjson 1.0.8

as it is done here http://central.maven.org/maven2/xml-apis/xml-apis/2.0. 2/xml-apis-2.0.2.pom This will result: http://mvnrepository.com/artifact/xml-apis/xml-apis/2. 0.2

I guess this way all users will be notified they need to move to the correct version

WDYT?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/tdunning/open-json/issues/11#issuecomment-339861171, or mute the thread https://github.com/notifications/unsubscribe-auth/AAPSevDkgzWwSRidrYNaT9Uc0z3X0T5Xks5swUq0gaJpZM4MN7fN .

solomax commented 6 years ago

Sorry for not being clear, English is not my native language :(

According to @slankas post users of com.tdunning:json library might not be aware of existence of com.github.openjson:openjson library. Also I believe com.github.openjson:openjson should be drop-in replacement for com.tdunning:json.

Maybe this need to be written to the

  1. README.md
  2. pom.xml relocation section

Since changes in pom.xml will be visible only after release I propose to release new version of com.tdunning:json with relocation coordinates

xml-apis was just an example of how this will look for the end user of the library :) I'm using "we" in "we can release" since I have contributed to this library and feel involved :))

tdunning commented 6 years ago

Inline

On Oct 27, 2017 8:18 PM, "Maxim Solodovnik" notifications@github.com wrote:

Sorry for not being clear, English is not my native language :(

I would not have known. Many a native speaker is less clear.

According to @slankas https://github.com/slankas post users of com.tdunning:json library might not be aware of existence of com.github.openjson:openjson library.

Good point. They should be told.

Also I believe com.github.openjson:openjson should be drop-in replacement for com.tdunning:json.

I don't think this can happen. They have changed the package names at least. Also they are making changes going forward to make the alternative better as they see it.

Maybe this need to be written to the

  1. README.md

For sure. Pull requests welcome.

  1. pom.xml relocation section

Hmm. Seems difficult because of packages at least. Surely redirection should be at the option of the downstream users.

...

I'm using "we" in "we can release" since I have contributed to this library and feel involved :))

And welcome. Thank you for that!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/tdunning/open-json/issues/11#issuecomment-340135599, or mute the thread https://github.com/notifications/unsubscribe-auth/AAPSeqw3qE9ZQmtCBEtvrXGXp2DVcYxCks5swp0XgaJpZM4MN7fN .