-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
INT-1807 Add Mechanism For Headers with TCP #763
Conversation
Hi, Gary! |
} | ||
|
||
public <P> Message<P> toMessage(Object object) { | ||
Assert.isInstanceOf(Map.class, object, "This converter expects a Map"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add generic type to the MessageConverter
? Or it can take one type on toMessage
, but produce another one from fromMessage
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right - we can take any payload type in fromMessage; the resulting payload type here depends on what the deserializer puts in the Map's 'payload' element. So we have to leave it unspecified.
Regarding your general comment - yes there is some overlap between converters and transformers, but this wouldn't be the first place for that. The Also, a |
Rebased, pushed. |
* @since 3.0 | ||
* | ||
*/ | ||
public class MapJsonSerializer implements Serializer<Map<?, ?>>, Deserializer<Map<?, ?>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gary, regarding this implementation.
It fully depends from Jackson 1.
However we are planning to support both #774
So, maybe we push MapJsonSerializer
feature to future after merging my PR?
My point - to make dependency as less as posible, otherwise we will should provide MapJsonSerializer
for Jackson 2.
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes; I agree - I will try to get your PR reviewed/merged today or Monday.
And, I think I can just make this serializer J2 (no need for both, I think - if someone really, really wants a 1.x version, they can make their own).
|
||
public void afterPropertiesSet() throws Exception { | ||
this.objectMapper.configure(Feature.AUTO_CLOSE_SOURCE, false); | ||
this.objectMapper.configure(JsonGenerator.Feature.AUTO_CLOSE_TARGET, false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gery, let's get back to this class after merging JsonObjectMapper
abstraction.
I dwell on these two lines because I didn't introduce any configuration hook and I didn't know how to abstract it in case auto-detecation of Jackson lib at CLASSPATH.
Do you have any idea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh; you are right; let me think a while.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.objectMapper.configure(JsonGenerator.Feature.AUTO_CLOSE_TARGET, false);
Does not make sense: it applies for local ByteArrayOutputStream
in the serialize
below.
From other side, Gary, comes to mind a solution with some generic JsonObjectMapperConfiguration
, which will be mapped to concrate configuration based on JsonObjectMapper
implementation. E.g. GSON doesn't support similar featuers, but it has GsonBuilder
. In the end it may look like thankless work to try provide hooks for all existing configuration features in different JSON mapping engines...
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we get the package tangle code merged, I'll re-visit this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Artem; since this is brand new, I am inclined to support only Jackson2 out of the box (wire up a JsonObjectMapper
with a properly configured Jackson2 mapper), but allow the user to inject his own JsonObjectMapper
if he wishes to use Jackson1 or some other implementation.
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What you say is right. But my general concern here that you are using feature
this.objectMapper.configure(Feature.AUTO_CLOSE_SOURCE, false);
I think I'll play today with JsonObjectMapper
here, before you wake up. 😄
Gary, take a look, please, into my polishing: |
TCP streams have no standard message structure. Therefore, the TCP implementation previously only transferred the message payload. If someone wanted to convey header information, they would have to write their own wrapper and/or use Java serialization for the entire message. This change provides a strategy to allow users to determine which headers are transferred, and how. A MessageConvertingMessageMapper is now provided that invokes any MessageConverter. A MapMessageConverter is provided that converts the payload, and selected heades to a Map with two entries ("payload") and ("headers"). A MapJsonSerializer is provided that converts a Map to/from JSON. Jackson can't delimit multiple objects in a stream so another serializer is required to encode/decode structure. A ByteArrayLfSerializer is used by default, inserting a linefeed between JSON objects. The combination of these elements now allows header information to be transferred over TCP. Of course, users can implment their own (de)serializer to format the bits on the wire exactly as needed by their application. INT-1807 Polishing Add a test that uses a Map MessageConverter with a Java (de)serializer. INT-1807: Polishing INT-1807: Rebased and polished Change `MapJsonSerializer` to use `JsonObjectMapper` abstraction Doc Polishing
Thanks Artem; I applied your commit on top and polished the docs; I realize now that those Jackson settings were no longer required after I added the second serializer (LF) to delineate between json objects. Now that the objectmapper is no longer acting on the socket's inputstream it doesn't matter that it closes the stream (it is simply closing a byte array stream that is already at the end. Merging... |
TCP streams have no standard message structure. Therefore, the
TCP implementation previously only transferred the message
payload.
If someone wanted to convey header information, they would have
to write their own wrapper and/or use Java serialization for
the entire message.
This change provides a strategy to allow users to determine
which headers are transferred, and how.
A MessageConvertingMessageMapper is now provided that invokes
any MessageConverter. A MapMessageConverter is provided that
converts the payload, and selected heades to a Map with two
entries ("payload") and ("headers").
A MapJsonSerializer is provided that converts a Map to/from
JSON. Jackson can't delimit multiple objects in a stream
so another serializer is required to encode/decode structure.
A ByteArrayLfSerializer is used by default, inserting a
linefeed between JSON objects.
The combination of these elements now allows header
information to be transferred over TCP. Of course, users
can implment their own (de)serializer to format the
bits on the wire exactly as needed by their application.
INT-1807 Polishing
Add a test that uses a Map MessageConverter with a
Java (de)serializer.