-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add Jackson JSON serialization for JdbcChannelMessageStore #10508
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
base: main
Are you sure you want to change the base?
Conversation
|
Hi, I'd like your input on two decisions 1. Column naming:
|
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.
Please, add your legal name to the sign-off of the commit.
You can change your Git client config on the matter.
...mework/integration/jdbc/store/channel/JacksonChannelMessageStorePreparedStatementSetter.java
Outdated
Show resolved
Hide resolved
...mework/integration/jdbc/store/channel/JacksonChannelMessageStorePreparedStatementSetter.java
Outdated
Show resolved
Hide resolved
...mework/integration/jdbc/store/channel/JacksonChannelMessageStorePreparedStatementSetter.java
Outdated
Show resolved
Hide resolved
...mework/integration/jdbc/store/channel/JacksonChannelMessageStorePreparedStatementSetter.java
Outdated
Show resolved
Hide resolved
.../springframework/integration/jdbc/store/channel/PostgresJacksonChannelMessageStoreTests.java
Outdated
Show resolved
Hide resolved
...n-jdbc/src/test/java/org/springframework/integration/jdbc/store/channel/TestMailMessage.java
Outdated
Show resolved
Hide resolved
src/reference/antora/modules/ROOT/pages/jdbc/message-store-json.adoc
Outdated
Show resolved
Hide resolved
src/reference/antora/modules/ROOT/pages/jdbc/message-store-json.adoc
Outdated
Show resolved
Hide resolved
|
Thanks for the feedback @artembilan! Two questions:
1. Column NamingYou mentioned Is it okay with a breaking change for better naming? Or keep 2. Type-Safe ReadingCurrent
|
|
I wonder if that could be kept as it is unfortunate that you are contributing this too late in the current 7.0 generation. |
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.
OK. I think we are on the right track, but still need dedicate more thinking on this.
I will aim this for the next 7.1.
Meanwhile I'll make a small (well, it is going to be big by the amount of code affected 😉 ) breaking change for the today's 7.0.0-RC1 - the last chance where we can do that.
The change would be from MESSAGE_BYTES to MESSAGE_CONTENT.
This will be a good foundation for your work over here.
Thank you!
|
Here you are: #10524! |
|
@artembilan Thanks for the detailed explanation and for taking on the naming change PR. I’m updating the PR per your review.
I understand your point about simplifying the schemas and documenting JSON serialization setup instead. What do you think about keeping a lightweight schema just for the tests (under |
That totally makes sense. |
38f850d to
c1d0b32
Compare
|
Thanks for feedback @artembilan! I’ve updated the code based on the review for c1d0b32. |
Thank you @yybmion ! But as I said before: we cannot accept a new code into the current Does that make sense to you? |
|
Thank you for the explanation @artembilan ! Yes, I completely understand the release schedule. Please let me know if there are any updates or changes needed before the merge. Thanks again for your guidance throughout the review process! |
...mework/integration/jdbc/store/channel/JacksonChannelMessageStorePreparedStatementSetter.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/springframework/integration/jdbc/store/channel/JacksonMessageRowMapper.java
Outdated
Show resolved
Hide resolved
...mework/integration/jdbc/store/channel/JacksonChannelMessageStorePreparedStatementSetter.java
Outdated
Show resolved
Hide resolved
...mework/integration/jdbc/store/channel/JacksonChannelMessageStorePreparedStatementSetter.java
Outdated
Show resolved
Hide resolved
...va/org/springframework/integration/jdbc/store/channel/H2JacksonChannelMessageStoreTests.java
Outdated
Show resolved
Hide resolved
src/reference/antora/modules/ROOT/pages/jdbc/message-store-json.adoc
Outdated
Show resolved
Hide resolved
|
Thanks for feedback @artembilan. Improved as requested in 1e0dfcb |
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.
Mostly this is great.
Just couple nit-picks and desired clean up.
Let me know if you can make until this end of the day when we are going to perform release.
Thanks
| * and Java classes. Custom payload types will require their package to be | ||
| * specified using {@link #JsonMessageRowMapper(String...)}. | ||
| */ | ||
| public JsonMessageRowMapper() { |
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.
I think we don't need this ctor.
And another one could be like String @Nullable ... trustedPackages
| * A {@link ChannelMessageStorePreparedStatementSetter} implementation that uses JSON | ||
| * serialization for {@link Message} objects instead of Java serialization. | ||
| * <p> | ||
| * This implementation stores the entire message (including headers and payload) as JSON, |
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.
We have to say By default, since there is no guarantee that provided JsonObjectMapper is always for Jackson.
| throw new SQLException( | ||
| "Failed to deserialize message from JSON at row " + rowNum + ". " | ||
| + "Ensure the JSON was created by JsonChannelMessageStorePreparedStatementSetter " | ||
| + "and contains proper @class type information.", |
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.
I don't think this is only a reason of an error.
Plus some other JSON serializers might not provide that @class information and can (de)serialize some other way.
|
|
||
| == JSON Structure | ||
|
|
||
| Messages are stored with the following JSON structure: |
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.
I think we need to emphasize here that this is a result of Jackson polymorphic serialization.
|
|
||
| The `MESSAGE_CONTENT` column must be changed to a text-based type that can store JSON. | ||
|
|
||
| === PostgreSQL |
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.
Can we consider to make these 3 samples as tabs?
As well as the following for the whole table schema.
|
Sure @artembilan, I’ll address the nits today before release. |
- Add JacksonChannelMessageStorePreparedStatementSetter for serialization - Add JacksonMessageRowMapper for deserialization with trusted package validation - Support PostgreSQL (JSONB), MySQL (JSON), and H2 (CLOB) databases - Add comprehensive test coverage and documentation Fixes: spring-projectsgh-9312 Signed-off-by: Yoobin Yoon <[email protected]>
|
Thanks @artembilan , Improved as requested in a432da5 |
| * | ||
| * @author Yoobin Yoon | ||
| */ | ||
| public record TestMailMessage( |
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.
@since 7.0 and not public.
I don't want test data classes to leak from one package to another.
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.
OK. I'm taking this locally for final review, clean up and merge.
Thank you again!
Thanks @artembilan! |
Fixes : #9312
JSON serialization as an alternative to Java serialization, enabling text-based message storage with type information preservation.
JacksonChannelMessageStorePreparedStatementSetterfor serializationJacksonMessageRowMapperfor deserialization