-
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.
build.gradle
Outdated
jmsApiVersion = '3.1.0' | ||
jpaApiVersion = '3.2.0' | ||
jrubyVersion = '10.0.2.0' | ||
jsonassertVersion = '1.5.1' |
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.
Any real justification to manage an extra dependency?
* </ul> | ||
* <p> | ||
* <b>Note:</b> Subpackages are automatically included without wildcards. | ||
* |
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.
No blank lines in method Javadocs, please.
* | ||
* @param trustedPackages the additional packages to trust for deserialization | ||
*/ | ||
public JacksonChannelMessageStorePreparedStatementSetter(String... trustedPackages) { |
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.
The security is not consulted on the serialization, therefore this option is redundant here.
* schemas from {@code schema-*-json.sql} files. | ||
* | ||
* @author Yoobin Yoon | ||
* @since 7.0 |
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.
Blank line after @author
list.
Just to emphasize who exactly did the change.
* | ||
* @param trustedPackages the additional packages to trust for deserialization | ||
*/ | ||
public JacksonChannelMessageStorePreparedStatementSetter(String... trustedPackages) { |
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 is something what is not used on serialization. Therefore this option is redundant.
@ContextConfiguration | ||
public class PostgresJacksonChannelMessageStoreTests extends AbstractJacksonChannelMessageStoreTests { | ||
|
||
private static final PostgreSQLContainer<?> POSTGRES_CONTAINER = |
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.
Why doesn't PostgresContainerTest
API work for this test scope?
Therefore, might be the case that this class has to go that postgres
package in our tests.
* | ||
* @author Yoobin Yoon | ||
*/ | ||
public class TestMailMessage implements Serializable { |
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.
If we talk about JSON, then Serializable
is something redundant.
I also wonder why this class is not a Java record
.
See xref:jdbc/lock-registry.adoc[] for more information. | ||
|
||
The `JdbcChannelMessageStore` now supports built-in Jackson JSON serialization as an alternative to Java serialization, enabling human-readable message storage. | ||
New components `JacksonChannelMessageStorePreparedStatementSetter` and `JacksonMessageRowMapper` provide this functionality with JSON-specific database schemas for PostgreSQL, MySQL, and H2. |
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 it has any specific value to mention those DBs.
I believe that any CLOB-supporting DB is OK to have a JSON as its value.
So, your statement about JSON-specific database schemas
does not reflect reality somehow...
Maybe you meant those custom schema files?
Then, OK: since we talk about dropping them in favor of docs, then this sentence would be changed as well.
= JDBC Channel Message Store JSON Serialization | ||
|
||
Version 7.0 introduced Jackson JSON serialization support for `JdbcChannelMessageStore`. | ||
By default, Spring Integration uses Java serialization to store messages in the database as binary BLOB data, which is not human-readable. |
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.
The "human-readable" statement makes me think twice.
Is it really a goal to let DBA to read the content of messages?
In most cases the payload
and headers
contain sensitive business data.
So, expose such an info DB directly might not be very secure.
May we at least don't emphasize somehow that it is better than byte[]
?
From my point of view it is even dangerous.
I know that we can encode data in DBs, but let's at least don't mention that it is human-readable
!
|
||
Version 7.0 introduced Jackson JSON serialization support for `JdbcChannelMessageStore`. | ||
By default, Spring Integration uses Java serialization to store messages in the database as binary BLOB data, which is not human-readable. | ||
The new JSON serialization support allows messages to be stored as readable JSON, making debugging and troubleshooting significantly easier. |
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, and this sentence also drops any data protection rules away.
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. |
- 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 <yunyubin54@gmail.com>
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! |
Fixes : #9312
JSON serialization as an alternative to Java serialization, enabling text-based message storage with type information preservation.
JacksonChannelMessageStorePreparedStatementSetter
for serializationJacksonMessageRowMapper
for deserialization