Skip to content

Allow configuring custom NullValueSerializer #2905

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

Allow configuring custom NullValueSerializer #2905

wants to merge 2 commits into from

Conversation

AnneMayor
Copy link
Contributor

  • You have read the Spring Data contribution guidelines.
  • You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
  • You submit test cases (unit or integration tests) that back your changes.
  • You added yourself as author in the headers of the classes you touched. Amend the date range in the Apache license header if needed. For new types, add the license header (copy from another file and set the current year only).

resolved #2878

@pivotal-cla
Copy link

@AnneMayor Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label May 11, 2024
@pivotal-cla
Copy link

@AnneMayor Thank you for signing the Contributor License Agreement!

AnneMayor
AnneMayor commented May 11, 2024
View reviewed changes
...a/org/springframework/data/redis/serializer/GenericJackson2JsonRedisSerializerUnitTests.java
@@ -420,7 +423,7 @@ void deserializesJavaTimeFrimBytes() {
}

@Test // GH-2601
public void internalObjectMapperCustomization() {
Copy link
Contributor Author

@AnneMayor AnneMayor May 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just removed according to sonarlint rules. It looks like an unnecessary keyword.

AnneMayor
AnneMayor commented May 11, 2024
View reviewed changes
...a/org/springframework/data/redis/serializer/GenericJackson2JsonRedisSerializerUnitTests.java
@@ -438,14 +441,34 @@ public void internalObjectMapperCustomization() {
}

@Test // GH-2601
public void configureWithNullConsumerThrowsIllegalArgumentException() {
Copy link
Contributor Author

@AnneMayor AnneMayor May 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just removed according to sonarlint rules. It looks like an unnecessary keyword.

@AnneMayor
Copy link
Contributor Author

I'm looking for another great solution and trying to apply it.

@AnneMayor AnneMayor force-pushed the custom-null-value-serializer-for-generic-jackson-2-json-redis-serializer branch from 28dee8e to cba9492 Compare May 12, 2024 11:14
@AnneMayor
Copy link
Contributor Author

Please, review it. Thanks :)

@AnneMayor AnneMayor changed the title feat: add setter method to define custom NullValueSerializer feat: add custom NullValueSerializer initializer May 12, 2024
@AnneMayor AnneMayor force-pushed the custom-null-value-serializer-for-generic-jackson-2-json-redis-serializer branch 6 times, most recently from 048b21a to 4f5a47a Compare May 12, 2024 12:38
@AnneMayor AnneMayor force-pushed the custom-null-value-serializer-for-generic-jackson-2-json-redis-serializer branch from 4f5a47a to 58ef24b Compare May 12, 2024 12:50
@mp911de mp911de self-assigned this May 13, 2024
@mp911de
Copy link
Member

mp911de commented May 13, 2024

Thanks for looking into this enhancement. Taking a step back from the actual thing we want to solve, we have a growing number of constructors (6 constructors already). Adding another one is weakening the design. I would suggest creating a builder that accepts ObjectMapper, JacksonObjectReader, JacksonObjectWriter, and classPropertyTypeName for typing information. Also, we should add registerNullValueSerializer (boolean) for more flexibility.

That way, we can hide customization in the builder and allow further configuration options without stretching the number of constructors.

Let me know whether this makes sense.

@mp911de mp911de added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels May 13, 2024
@AnneMayor
Copy link
Contributor Author

@mp911de
Thank you for the detailed review! I think that's a great idea too. I am gonna create a new Builder class as you suggested within this weekend:) Thanks 👍

This was referenced May 13, 2024
Closed
Closed
@AnneMayor
Copy link
Contributor Author

@mp911de
I created Builder class. Please, review it :D

@AnneMayor AnneMayor force-pushed the custom-null-value-serializer-for-generic-jackson-2-json-redis-serializer branch from c23e1b0 to 98d4062 Compare May 20, 2024 14:10
@AnneMayor AnneMayor force-pushed the custom-null-value-serializer-for-generic-jackson-2-json-redis-serializer branch from 98d4062 to 675a1ab Compare May 20, 2024 14:12
AnneMayor
AnneMayor commented May 20, 2024
View reviewed changes
...a/org/springframework/data/redis/serializer/GenericJackson2JsonRedisSerializerUnitTests.java
@@ -432,20 +436,63 @@ public void internalObjectMapperCustomization() {

assertThat(serializer.configure(configurer)).isSameAs(serializer);

verify(mockObjectMapper, times(1)).registerModule(eq(mockModule));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just removed unnecessary command.

@mp911de mp911de changed the title feat: add custom NullValueSerializer initializer Allow configuring custom NullValueSerializer May 21, 2024
mp911de pushed a commit that referenced this pull request May 21, 2024
@AnneMayor @mp911de
Allow configuring custom NullValueSerializer.
33326aa 
Closes #2878
Original pull request: #2905
mp911de added a commit that referenced this pull request May 21, 2024
@mp911de
Polishing.
8357c7d 
Revise builder. Accept builder components in builder methods instead of the builder factory method. Enforce valid parameters instead of lenient, potentially null parameters.

Introduce configuration means to control default typing. Extend tests.

See #2878
Original pull request: #2905
mp911de pushed a commit that referenced this pull request May 21, 2024
@AnneMayor @mp911de
Allow configuring custom NullValueSerializer.
6b56cfb 
Closes #2878
Original pull request: #2905
mp911de added a commit that referenced this pull request May 21, 2024
@mp911de
Polishing.
fb140e3 
Revise builder. Accept builder components in builder methods instead of the builder factory method. Enforce valid parameters instead of lenient, potentially null parameters.

Introduce configuration means to control default typing. Extend tests.

See #2878
Original pull request: #2905
@mp911de
Copy link
Member

mp911de commented May 21, 2024

Thank you for your contribution. That's merged, polished, and backported now. You might be interested in the polishing commit 8357c7d that refines the builder API to accept parameter-by-parameter instead of requiring all elements upfront to ease with usage experience. Let me know if you have any questions.

@mp911de mp911de closed this May 21, 2024
@mp911de mp911de added this to the 3.3.1 (2024.0.1) milestone May 21, 2024
@injae-kim
Copy link

injae-kim commented May 21, 2024 •
edited
Loading

Polishing commit 8357c7d is really awsome 👍

Thank you for kind&rapid review! (I'm friend of @AnneMayor and helping her to contribute on open source 😃)

@AnneMayor
Copy link
Contributor Author

AnneMayor commented May 21, 2024 •
edited
Loading

@mp911de ,
Thank you so much for your kind and rapid review 😊
I am very happy to contribute this open source.
I am going to look into your polishing commit and learn from it.
Again, thanks a lot 👍
@injae-kim he is a great supporter to help me to contribute.
Without him, I might be unable to merge the PR so fast.
Thank you for your kind support, @injae-kim 👍 You're the best!

@AnneMayor AnneMayor mentioned this pull request May 23, 2024
Closed
@injae-kim injae-kim mentioned this pull request Jul 5, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@mp911de mp911de

Labels
type: enhancement A general enhancement
Projects
None yet
Milestone
3.3.1 (2024.0.1)
Development

Successfully merging this pull request may close these issues.

Support custom NullValueSerializer in GenericJackson2JsonRedisSerializer
5 participants
@AnneMayor @pivotal-cla @mp911de @injae-kim @spring-projects-issues