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

Pluggable JSON serialization #344

Merged
merged 1 commit into from
Jul 11, 2018
Merged

Pluggable JSON serialization #344

merged 1 commit into from
Jul 11, 2018

Conversation

lhazlewood
Copy link
Contributor

@lhazlewood lhazlewood commented Jul 9, 2018

Initial pluggable JSON (de)serialization support with Jackson and org.json as the first implementations. Jackson dependency is still the default, but this will no longer be necessary when #348 is resolved.

#335 Cannot be resolved until changelog and docs are updated.

@lhazlewood
Copy link
Contributor Author

cc @RyanBard and @azagniotov

@lhazlewood lhazlewood force-pushed the issue-335-custom-json branch 2 times, most recently from ae7e77f to ed1c347 Compare July 9, 2018 21:40
pom.xml Outdated
<dependency>
<groupId>com.fasterxml.jackson.core</groupId>
<artifactId>jackson-databind</artifactId>
<version>${jackson.version}</version>
<scope>compile</scope>
<optional>true</optional>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does potentially break peoples' builds (if they weren't explicitly depending on jackson and were getting it transitively from jjwt); think a mention in the CHANGELOG.md makes sense?

Copy link
Contributor Author

@lhazlewood lhazlewood Jul 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, most definitely still need to update the changelog before merging this PR.

The other solution that could very well happen for this PR is to break JJWT into multiple modules per #335's description and keep a jjwt artifact that is nothing more than a declaration of dependencies on jjwt-api, jjwt-impl and jjwt-jackson (for example).

This guarantees that those depending on the jjwt artifact still experience the same behavior.

That said, we don't have to do this and a changelog entry would be fine because JJWT's actual public API wont have backwards-incompatible changes. However the dependency-chain technique could be a nice-to-have that I'm sure would simplify things for people (and us answering questions on StackOverflow ;) ).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be how I would do it (slf4j style rather than spring-boot style).

I mean, if I didn't have a unit test exercising the jjwt code in the project I tested with, it wouldn't have even broken the build. It just would have been a runtime failure.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You also start to run into really weird errors when the versions of classes available don't match up.

Jackson is rough about that. If you accidentally mix different versions of the jackson libraries (usually due to complex maven hierarchies), you end up with runtime errors when it's actually used. In a past project, the dependencies were complex and 2.2 or something was mixed w/ 2.7 or so causing problems. We had unit tests that should have flushed this problem out, however, the test dependencies were different than the runtime dependencies, so you didn't find out until exercising the code in the app container.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I've experienced similar problems with Jackson - not fun. There's no question that the SLF4J-style is more deterministic, but people do love the spring-boot style.... Hrm...

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 6459d4c on issue-335-custom-json into bae78f0 on master.

…d org.json as the first implementations, with Jackson being the default. Added tests to retain 100% code coverage.
@lhazlewood lhazlewood force-pushed the issue-335-custom-json branch from 6459d4c to 8afca0d Compare July 11, 2018 20:37
@lhazlewood lhazlewood merged commit 2917260 into master Jul 11, 2018
@lhazlewood lhazlewood deleted the issue-335-custom-json branch July 11, 2018 21:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants