Skip to content

Consider replacing org.json in spring-boot-configuration-processor due to licence #5929

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

Closed
jgoldhammer opened this issue May 12, 2016 · 16 comments
Assignees
Labels
type: enhancement A general enhancement
Milestone

Comments

@jgoldhammer
Copy link

Using Spring Boot 1.3.4, we recognized that spring boot is using the json library.

Details here:
https://wiki.debian.org/qa.debian.org/jsonevil

Dependency graph:
--- org.springframework.boot:spring-boot-configuration-processor:1.3.4.RELEASE
+--- org.json:json:20140107
--- org.springframework:spring-core:4.2.4.RELEASE (*)

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label May 12, 2016
@philwebb philwebb changed the title Please replace non-free (!) org.json library with jackson in spring-boot-configuration-processor Consider replacing org.json in spring-boot-configuration-processor due to licence May 12, 2016
@philwebb
Copy link
Member

That's a very annoying clause in the license. We intentionally chose something lightweight for the configuration processor since it's a compiler plugin. Perhaps json-simple might be an option.

You can always remove spring-boot-configuration-processor if it's causing a big legal issue for you (although you won't get generated meta-data).

@philwebb philwebb added status: ideal-for-contribution An issue that a contributor can help us with for: team-attention An issue we'd like other members of the team to review and removed status: waiting-for-triage An issue we've not yet triaged labels May 12, 2016
@jgoldhammer
Copy link
Author

Just started hacking on it and I hit some challenges:

  • org-json uses reflection to write deep objects as json
  • simple-son does not use reflection to write complex objects- it just uses toString which does not work for ItemMetadata class...

What do you think? Is rewriting the tests useful?

2016-05-12_22-44-34

@snicoll
Copy link
Member

snicoll commented May 13, 2016

That's what the debian community states. Here is what the Apache foundation states

@philwebb
Copy link
Member

@snicoll Interesting, thanks!

@jgoldhammer have you actually hit a legal issue here?

@jgoldhammer
Copy link
Author

jgoldhammer commented May 14, 2016

No, we are just in preparation phase for an audit of our application and
just scanned the licences of the libraries we are using. I wanna make sure
that we have no findings in terms of licences....

What do you think, Phil?

Thanks,
Jens
Phil Webb [email protected] schrieb am Fr., 13. Mai 2016 um 22:41:

@snicoll https://github.com/snicoll Interesting, thanks!

@jgoldhammer https://github.com/jgoldhammer have you actually hit a
legal issue here?


You are receiving this because you were mentioned.

Reply to this email directly or view it on GitHub
#5929 (comment)

@philwebb
Copy link
Member

Unless someone wants to step-up and migrate the code I hope that the Apache legal statement will be enough for most people. Don't forget that the configuration processor is only used at compile time and isn't usually distributed with your jar.

@philwebb philwebb added status: on-hold We can't start working on this issue yet and removed for: team-attention An issue we'd like other members of the team to review labels May 23, 2016
@Davio
Copy link

Davio commented Nov 13, 2016

The statement by Apache was updated:
CAN APACHE PRODUCTS INCLUDE WORKS LICENSED UNDER THE JSON LICENSE?
No. As of 2016-11-03 this has been moved to the 'Category X' license list. Prior to this, use of the JSON Java library was allowed. See Debian's page for a list of alternatives.

@philwebb philwebb added the for: team-attention An issue we'd like other members of the team to review label Nov 14, 2016
kartoffelsup pushed a commit to kartoffelsup/spring-boot that referenced this issue Nov 15, 2016
@kartoffelsup
Copy link

I've started working on this -> kartoffelsup@c11f6d6

Replaced org.json in spring-boot-configuration-metadata. If you like it, I will gladly do the same for spring-boot-configuration-processor

@philwebb
Copy link
Member

@kartoffelsup Yes please!

@kartoffelsup
Copy link

There is indeed an issue with the writing of JSON with json-simple. Though if I'm not mistaken, the reflection serialization is only used/needed within the tests(?). I've added GSON to the test dependencies and used that to write the JSON for the tests and they run through fine. But I'm not sure if this is something we want to do. @philwebb @wilkinsona what do you think?
kartoffelsup@a0407da

@wilkinsona
Copy link
Member

wilkinsona commented Nov 16, 2016

I think a test only dependency on something more sophisticated is fine. I have a preference for using Jackson over GSON, though. Purely because Jackson is the default JSON library that's used elsewhere in the codebase.

kartoffelsup pushed a commit to kartoffelsup/spring-boot that referenced this issue Nov 16, 2016
kartoffelsup pushed a commit to kartoffelsup/spring-boot that referenced this issue Nov 16, 2016
kartoffelsup pushed a commit to kartoffelsup/spring-boot that referenced this issue Nov 16, 2016
@kartoffelsup
Copy link

I guess it's not as simple as I had hoped. :(
I was happy as the tests ran through but it turns out there are just a lot of test cases missing. For example, having a char []as default value will result in the following JSON:

      "defaultValue": [
        w,
        o,
        r,
        d
      ],

which is invalid. This is due to the fact that '#toString' gets called if json-simple does not 'recognize' (have an instanceof check inside the writer for) the type.

@philwebb philwebb removed the for: team-attention An issue we'd like other members of the team to review label Nov 16, 2016
@philwebb
Copy link
Member

philwebb commented Jan 3, 2017

This might be an option skyscreamer/JSONassert@2f3576c

@philwebb philwebb removed the status: ideal-for-contribution An issue that a contributor can help us with label Jan 5, 2017
@philwebb philwebb removed the status: on-hold We can't start working on this issue yet label Jan 5, 2017
@philwebb philwebb self-assigned this Jan 5, 2017
@philwebb philwebb added this to the 1.5.0 RC1 milestone Jan 5, 2017
@philwebb philwebb added in-progress type: enhancement A general enhancement labels Jan 5, 2017
@snicoll
Copy link
Member

snicoll commented Jan 5, 2017

Some public methods are now throwing Exception following to that change

@snicoll snicoll reopened this Jan 5, 2017
@snicoll
Copy link
Member

snicoll commented Jan 5, 2017

@philwebb those throws Exception on public methods bug me. I see that the move to the new JSON api makes it so that JSONException is now a checked exception. Can we please restore a state of affair where a we wrap that checked exception into a "runtime" JSONException as before. If we don't want to do that, can we please throw IOException, JSONException rather than a raw Exception?

@snicoll
Copy link
Member

snicoll commented Jan 5, 2017

OK, ignore me. I realized one of my tool was using a class from the configuration processor directly. Not really something that we should call public API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

7 participants