INT-2831: Add Jackson 2 support #774

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
2 participants
Member

artembilan commented Mar 23, 2013

  • Introduce JsonSerializer strategy
  • Change ObjectToJsonTransformer and JsonToObjectTransformer to use JsonSerializer
  • Add JacksonMapperJsonSerializer and Jackson2MapperJsonSerializer implementations
  • Change the object-mapper attribute to the json-serializer
  • Introduce JacksonMapperJsonSerializerProvider factory to provide
    JacksonMapperJsonSerializer or Jackson2MapperJsonSerializer instances dependently of the classpath

JIRA: https://jira.springsource.org/browse/INT-2831

Member

artembilan commented Mar 23, 2013

So, I spent less time, than I was afraid ;-).
If it is OK, there should be done Migration Guide note.
What I see else:

  • Jackso 2 support should be added to the HttpRequestHandlingEndpointSupport, but I can do it now within my PR about Request mapping.
  • Does it make sense to have jackson dependency in the 'spring-integration-redis' module? Maybe just remove it?
Member

artembilan commented Apr 5, 2013

So, I've found more Jackson-aware components:

  • JsonInboundMessageMapper
  • JsonOutboundMessageMapper
  • ObjectToMapTransformer

Regarding last one I once suggested: https://jira.springsource.org/browse/INT-2341
Regarding *Message Mapper implementation: I can do it within this PR, but I want to be sure in my initial implementation. So, I'm waiting your feedback.

}
- if (element.hasAttribute(MessageHeaders.CONTENT_TYPE)){
+ if (element.hasAttribute("content-type")){
@garyrussell

garyrussell Apr 8, 2013

Member

Why change from the constant to a literal here?

@artembilan

artembilan Apr 9, 2013

Member

Hi, Gary!
It was a mix of concerns: we checked an attribute, but said header. Strange and confused. Why does parser have to know about MessageHeaders?
If you are interest we may add a constant to the IntegrationNamespaceUtils

@garyrussell

garyrussell Apr 9, 2013

Member

I see what you mean. No need for a constant, I think.

@@ -2119,16 +2119,16 @@
]]></xsd:documentation>
</xsd:annotation>
</xsd:attribute>
- <xsd:attribute name="object-mapper" use="optional">
+ <xsd:attribute name="json-serializer" use="optional">
@garyrussell

garyrussell Apr 8, 2013

Member

This seems overly-invasive. Jackson2 still has an ObjectMapper - we would just lose the tooling "type" assist.

While I agree the serializer abstraction is cleaner, I don't think we want to force everyone to have to change their configuration - especially if they decide to keep jackson 1.x.

Perhaps a middle ground would be to call te SI abstraction ObjectMapper?

@artembilan

artembilan Apr 9, 2013

Member

Do you mean to leave attribute as object-mapper? Agree.
From other side I've changed a bit my mind and thought that serializer means some narrow sphere of application, so I wanted to rename JsonSerializer to the JsonMapper (or ObjectMapper as you mentioned) to be more compatible with Jackson's one: it's not full serializer as similar abstraction in the Spring, it's not converter, because we use the last one to convert from/to Message.
And one more thought: how about to allows provide Jackson's ObjectMapper ref as it is now? At least before SI 3.1 and say that it is deprecated. In this case guys with Jackson 1.x don't have to change their configs at all.
I think I'll be able to do something in the parser to divide a level of responsibility.
WDYT?

@garyrussell

garyrussell Apr 9, 2013

Member

Yes, that makes sense; we could also remove the tooling assistance - and let any kind of class be provided (but the parser will insist on our abstraction or a jackson-specific object).

Finally, I don't know enough about Jackson2 and its compatibility with Jackson1. I agree we should default to Jackson2 if both are on the classpath, but I also think we might need the option to force the use of Jackson1 if both are available.

WDYT?

@artembilan

artembilan Apr 9, 2013

Member

Jackson2 and its compatibility with Jackson1

There is no any compatibility: they live in different packages, but they both provide similar JSON. I think we can create JSON from J1 and deserialize it in J2 and, of course, vise versa.

we might need the option to force the use of Jackson1 if both are available.

I don't think so. It can be achieved via simple config manipulation. Take a look into tests within this PR.
Eventually J1 will sink into oblivion and we will have to remove force attribute.

So,

  1. Will I rename our abstraction to the JsonMapper or ObjectMapper?
  2. Will it be OK that I say J1 reference is deprecated?
  3. Should we allow to provide reference to the J2? There will be a lot of parser logic and eventually someone would say that he wants to have a reference to gson not SI-strategy.
  4. Is it OK to go ahead and polish Json*MessageMapper? BTW should they support both Jacksons too?
  5. WDYT about ObjectToMapTransformer ?
@garyrussell

garyrussell Apr 9, 2013

Member

What I meant by compatibility was the content of the output - if someone today uses jackson1 is it possible the JSON output might be (subtly) different with jackson2 - such that it might cause problems with the consuming app? Perhaps a remote chance, but as long we have a way to force the use of J1 when both is on the classpath, we are ok.

  1. I would call the abstraction JsonObjectMapper and leave the attribute as object-mapper.
  2. Yes
  3. I don't think so - can be the new abstraction, or a reference to a J1 (for backwards compat - which will be derprecated and removed in 3.1).
  4. They should probably use the abstraction (in case we ever implement gson versions).
  5. Not sure why you mention that one here.
@artembilan

artembilan Apr 9, 2013

Member

Not sure why you mention that one here.

Here: SpringSource#774 (comment)
But I can to force it to J2 as you mentioned in the #763

@garyrussell

garyrussell Apr 9, 2013

Member

Sorry - I am still missing something #774 is this PR.

Not sure what OTMT has to do with jackson.

Re: #763; now that I see you have an abstraction, I think I'd use that rather than only supporting J2.

Member

artembilan commented Apr 13, 2013

Pushed.
Of course, JsonInboundMessageMapper blew my brain ;)

+ }
+ }
+
+}
@garyrussell

garyrussell Apr 15, 2013

Member

This seems highly convoluted just to provide a type-safe constructor argument for the transformers etc, allowing the object-mapper argument to be the existing jackson 1.x instance, or the new Abstraction.

Wouldn't it be much simpler to just add a second (deprecated) constructor to the transformers etc that takes an Object (for the 1.9 objectMapper) and Assert.isTrue(mapper.getClass().getName().equals("....codehaus....ObjectMapper") ??

artembilan added some commits Mar 23, 2013

INT-2831: Add Jackson 2 support
The main reason of this commit to make backward compatibility as much as possible.

JIRA: https://jira.springsource.org/browse/INT-2831
INT-2831: Polishing
* remove `JsonObjectMapperFactoryBean`
* add @Deprecated constructors to `ObjectToJsonTransformer` and `JsonToObjectTransformer`
Member

artembilan commented Apr 16, 2013

Thanks, Gary, that have guided on the right path.
So, rebased and pushed

Member

garyrussell commented Apr 29, 2013

Hi Artem,

LGTM; some minor polishing here (garyrussell/spring-integration@12f0715) mainly to eliminate compiler warnings and a bit of English polish.

I also added a few words in the JSON section of the transformers chapter and a what's new entry.

I will merge tomorrow unless you see some problems with my polish.

garyrussell added a commit that referenced this pull request Apr 30, 2013

Merge pull request #774 from artembilan/INT-2831
* INT-2831:
  INT-2831: Add Jackson 2 support
Member

garyrussell commented Apr 30, 2013

Merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment