Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

org.json should not be used - Update unirest library to new version #3703

Closed
koppor opened this issue Feb 6, 2018 · 22 comments
Closed

org.json should not be used - Update unirest library to new version #3703

koppor opened this issue Feb 6, 2018 · 22 comments
Labels
good first issue An issue intended for project-newcomers. Varies in difficulty. type: code-quality Issues related to code or architecture decisions

Comments

@koppor
Copy link
Member

koppor commented Feb 6, 2018

org.json is non-free software: https://wiki.debian.org/qa.debian.org/jsonevil

Jackson is the way to go.

@koppor koppor added the type: code-quality Issues related to code or architecture decisions label Feb 6, 2018
@Siedlerchr
Copy link
Member

I would prefer gson, I use that in share latex and we already have guava as deps

@stefan-kolb
Copy link
Member

Is it this library?! https://github.com/stleary/JSON-java We could make a PR there, if you want to.

@koppor
Copy link
Member Author

koppor commented Feb 7, 2018

grafik

@koppor
Copy link
Member Author

koppor commented Feb 7, 2018

@koppor
Copy link
Member Author

koppor commented Feb 10, 2018

https://github.com/openjson/openjson seems to be most modern drop-in alternative.

@Siedlerchr
Copy link
Member

The reason I prefer gson over the standard json is because it directly supports generics and allows to have a clear hierarchy (all classes are dervied from JsonElement - such kind of structure does not exist in org.json/openjson) and you can always chain methods, e.g. getAsJsonArray().get(0).getAsInt()
Furthermore it is already java 9 compatible,

@koppor
Copy link
Member Author

koppor commented Feb 10, 2018

@Siedlerchr Feel free to rewrite the code of JabRef to use gson instead of org.json. 😇

@Siedlerchr
Copy link
Member

The dependency comes from UniRest. http://unirest.io/java.html
I think we decided on the jabcon to remove that dependency and stick with jersey, the official reference implementation.
https://jersey.github.io/
So I will convert the code to use jersey + gson

@koppor
Copy link
Member Author

koppor commented Feb 11, 2018

Yeah - this is the way to go!

Side story: I tried to replace the dependency by openjson during the gradle build. However, they changed the package name to com.github.openjson. Replacing it by 'com.vaadin.external.google:android-json:0.0.20131108.vaadin1' leads to an additional exception JSONException, which needs to be handled by our code somehow.

@Siedlerchr
Copy link
Member

Siedlerchr commented Feb 11, 2018

I currently try to use it with gson. It works in general, but is has the disadvantage against the openJson implementation that it does not have these pseudo-optional methods:
OpenJson can return pseudo optionals//defaults e.g. if a field is not present it can return an empty string by default.

@tobiasdiez
Copy link
Member

So the only reason why we have to rewrite everything JSON and unirest-related is because of a single stupid line in a license file? Can we not just ignore this as we have done so far?

@koppor
Copy link
Member Author

koppor commented Feb 11, 2018

Yes, because we want to keep JabRef distributed in Linux distributions. Otherwise, JabRef is not open source and free software.

This license is considered non-free by Debian, GNU Project, Fedora, Google, Red Hat legal, Free Software Foundation

Source: https://wiki.debian.org/qa.debian.org/jsonevil

+1 for devcall.

@jonasstein
Copy link

Which binary of jabref introduced the non free library first? We have to adjust the license settings in distributions. Please update the wrong LICENSE file according to the real license until it is fixed.
Please inform distributions next time you recognize that your license information was wrong.

@Siedlerchr
Copy link
Member

@jonasstein It was an external dependency. See above.
I see there is now https://github.com/OpenUnirest/unirest-java
It also uses org.json, but it seems to be under active developmend. so they could replace it with openjson I hope.

@jonasstein
Copy link

jonasstein commented Sep 21, 2018

@Siedlerchr Excuse me, if I misunderstand your reply. As far I could see you provide a jabref JAR which includes the non free org.json library.

@koppor
Copy link
Member Author

koppor commented Aug 25, 2019

Refs Kong/unirest-java#176.

@koppor
Copy link
Member Author

koppor commented Aug 25, 2019

OK HTTP seems to be the recent "cool" http client: https://square.github.io/okhttp/

Other alternatives: https://github.com/eclipse/microprofile-rest-client#implementations

Indepenent of what we do, we need anADR

@darkdragon-001
Copy link

darkdragon-001 commented Sep 23, 2019

@koppor Kong/unirest-java#176 is resolved and allows use of gson instead of org.json.

@Siedlerchr Siedlerchr added good first issue An issue intended for project-newcomers. Varies in difficulty. hacktoberfest Hacktoberfest tag. See https://hacktoberfest.digitalocean.com/faq/ for details. labels Oct 11, 2019
@Siedlerchr Siedlerchr changed the title org.json should not be used org.json should not be used - Update unit rest library to new version Oct 11, 2019
@Siedlerchr
Copy link
Member

As unirest now has removed org.json it should be safe to upgrade to the newest version.
Note that the library name has changed.

@Siedlerchr Siedlerchr changed the title org.json should not be used - Update unit rest library to new version org.json should not be used - Update unirest library to new version Oct 11, 2019
@koppor
Copy link
Member Author

koppor commented Oct 12, 2019

@Siedlerchr Could you provide more details (from which name to which name) or just replace the dependency and make a PR? 😇

@koppor koppor removed the hacktoberfest Hacktoberfest tag. See https://hacktoberfest.digitalocean.com/faq/ for details. label Oct 12, 2019
@Siedlerchr Siedlerchr assigned Siedlerchr and unassigned Siedlerchr Oct 12, 2019
@Siedlerchr
Copy link
Member

I will try to look into it, if no one else is interested. Could be a good first issue.

See the Upgrade guide: https://github.com/Kong/unirest-java/blob/master/UPGRADE_GUIDE.md
New namespace is now:

<dependency>
    <groupId>com.konghq</groupId>
    <artifactId>unirest-java</artifactId>
    <version>3.1.00</version>
</dependency>

@matthiasgeiger matthiasgeiger mentioned this issue Oct 24, 2019
6 tasks
JoHaHu pushed a commit to JoHaHu/jabref that referenced this issue Oct 25, 2019
JoHaHu pushed a commit to JoHaHu/jabref that referenced this issue Oct 25, 2019
JoHaHu pushed a commit to JoHaHu/jabref that referenced this issue Oct 28, 2019
JoHaHu pushed a commit to JoHaHu/jabref that referenced this issue Oct 28, 2019
JoHaHu pushed a commit to JoHaHu/jabref that referenced this issue Oct 28, 2019
@Siedlerchr
Copy link
Member

Thanks to @JoHaHu this issue is now resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue An issue intended for project-newcomers. Varies in difficulty. type: code-quality Issues related to code or architecture decisions
Projects
None yet
Development

No branches or pull requests

6 participants