Skip to content
This repository was archived by the owner on May 31, 2022. It is now read-only.

Jackson2 and Jackson1 support#59

Closed
bclozel wants to merge 9 commits into
spring-attic:masterfrom
bclozel:jackson2
Closed

Jackson2 and Jackson1 support#59
bclozel wants to merge 9 commits into
spring-attic:masterfrom
bclozel:jackson2

Conversation

@bclozel

@bclozel bclozel commented Nov 23, 2012

Copy link
Copy Markdown
Contributor

Like discussed in PR #58 - we can also add support for both Jackson1 and Jackson2 (like spring-hateoas).

This PR is far from perfect (I'm very happy about code duplication and adding annotations, but the package name change in Jackson made me do it!).

Should I copy/paste tests Jackson1 tests for Jackson2 ? Or do you have another testing strategy in mind?

@bclozel

bclozel commented Nov 23, 2012

Copy link
Copy Markdown
Contributor Author

By the way, I signed the contributor's agreement this morning and can provide you the confirmation number

@bclozel

bclozel commented Nov 23, 2012

Copy link
Copy Markdown
Contributor Author

Ok. It looks like spring-hateoas had to write separate abstract classes for jackson1 and jackson2, each one creating its own objectmapper.
Added tests to reflect that.

@dsyer

dsyer commented Nov 23, 2012

Copy link
Copy Markdown
Contributor

Great, thanks. Do you think we should make one of the Jackson dependencies non-optional (otherwise it won't work out of the box)? Any other ideas?

I'll wait and see if @Hons (or anyone else) can come up with a less invasive strategy (externalizing the config), but given that Spring HATEOAS hasn't gone that route I'm inclined to believe it can't be done. If not I see that as a weakness of Jackson, although to be fair it's really a weakness of Java annotations.

@bclozel

bclozel commented Nov 23, 2012

Copy link
Copy Markdown
Contributor Author

I don't know. It looks like spring-framework made the choice to declare both dependencies as optional.

@dsyer

dsyer commented Nov 23, 2012

Copy link
Copy Markdown
Contributor

It's different for Spring because Jackson is clearly not a mandatory dependency. For OAuth it is for anyone who expects to get JSON output from the endpoints. Actually that's he same for HATEOAS and they have chosen to make both optional, so I'll try and find out why. I think I prefer to leave Jackson 1.9 as non-optional (at least for 1.0.1) otherwise users that don't happen to care will break their apps when they upgrade.

@odrotbohm

Copy link
Copy Markdown

We decided to have both dependencies optional for three reasons:

  1. Spring HATEOAS might be used with XML only which works out of the box with the JAXB implementation available in the JDK. So if you're not interested in JSON we don't pull an additional dependency.
  2. If one is using JSON, the developer will have to add a Jackson library himself anyway and we seamlessly integrate with both. Infrastructure setup like detecting the version and setting up an appropriate ObjectMapper is transparently handeled by Spring MVC.
  3. As the entire dependency coordinates changed for version 2.0 we cannot depend on 1.9 and assume to pick up 2.0 in case a user has accidentally declared a higher version. Thus an explicit exclude would be needed on the user side.

@dsyer

dsyer commented Nov 23, 2012

Copy link
Copy Markdown
Contributor

Thanks @olivergierke for the clarification. I think OAuth might be a bit different because while we do support XML rendering it is not required by the spec, whereas JSON is. My inclination therefore is to leave Jackson 1.9 as a default (no optional) dependency, which it is already in OAuth 1.0.0, therefore requiring users to explicitly exclude it if they want to use Jackson 2.0 (or only XML).

@Hons

Hons commented Nov 23, 2012

Copy link
Copy Markdown
Contributor

👍

1 similar comment
@Laures

Laures commented Nov 23, 2012

Copy link
Copy Markdown
Contributor

👍

@dsyer

dsyer commented Nov 23, 2012

Copy link
Copy Markdown
Contributor

OK, thanks for the help with this. I squashed your commits, so discard your branch and reset to master.

@dsyer dsyer closed this Nov 23, 2012
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

5 participants