Skip to content
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

Add a way to configure DeserializerCache Jackson uses #4101

Merged
merged 65 commits into from
Sep 5, 2023
Merged

Add a way to configure DeserializerCache Jackson uses #4101

merged 65 commits into from
Sep 5, 2023

Conversation

JooHyukKim
Copy link
Member

@JooHyukKim JooHyukKim commented Aug 28, 2023 •
edited
Loading

part of #2502

@JooHyukKim JooHyukKim mentioned this pull request Aug 28, 2023
Closed
@cowtowncoder
Copy link
Member

I like this @JooHyukKim , great start! Added some notes on details, nothing major.

One other thing: somehow I thought master (3.0) had more work done here, so just in case might be worth a peek just in case (I may misremember this).

I think this should work. Maybe we can get it in 2.16, although right now I am bit overloaded on reviews.

@JooHyukKim
Copy link
Member Author

Thank you for the review! 🙏🏼 Comments came in earlier than I thought :) and I will work on them later after work.

although right now I am bit overloaded on reviews.

Right, no time pressure here 👍🏻

@JooHyukKim JooHyukKim changed the title [DRAFT] Allow Cache Configuration Allow Cache Configuration for DeserializerCache via CacheProvider Aug 29, 2023
@JooHyukKim
Copy link
Member Author

We can also split this PR into a few smaller ones :

  • One for creating CacheProvider class.
  • MapperBuilder declaration (as per my comment).
  • Then each PRs per retrofitting DeserializerCache and others.

... and of course a tracker-issue for tracking those PRs.

@JooHyukKim JooHyukKim requested a review from pjfanning August 29, 2023 23:48
cowtowncoder added a commit that referenced this pull request Sep 4, 2023
@cowtowncoder
Merging minor piece of #4101
e4d183f
cowtowncoder
cowtowncoder reviewed Sep 4, 2023
View reviewed changes
src/test/java/com/fasterxml/jackson/databind/cfg/CacheProviderTest.java Outdated
.cacheProvider(new CustomCacheProvider())
.build();

assertTrue(invoked);
Copy link
Member

Choose a reason for hiding this comment

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

I think it'd be better to make CustomCacheProvider itself stateful, possibly returning a shared instance of test SimpleTestCache and storing state in there -- relying on static in test class is bit tightly coupled.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. Will do👍🏻

Copy link
Member Author

Choose a reason for hiding this comment

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

done!

cowtowncoder
cowtowncoder reviewed Sep 5, 2023
View reviewed changes
src/test/java/com/fasterxml/jackson/databind/cfg/CacheProviderTest.java Outdated

@Override
public JsonDeserializer<Object> putIfAbsent(JavaType key, JsonDeserializer<Object> value) {
invoked = true;
Copy link
Member

Choose a reason for hiding this comment

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

Is it intended that all calls set invoked to true?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, Just to check that the instance is actually used. Let me change named to invokedAtLeastOnce

Copy link
Member Author

Choose a reason for hiding this comment

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

done also

cowtowncoder added a commit that referenced this pull request Sep 5, 2023
@cowtowncoder
Pre-merge piece of #4101
c0f01a5
@cowtowncoder cowtowncoder merged commit 341f8d3 into FasterXML:2.16 Sep 5, 2023
@cowtowncoder
Copy link
Member

@JooHyukKim Finally the first, trickiest part done -- will try to get merged into master now. I hope same pattern can be used for serializer-cache functionality.

@JooHyukKim
Copy link
Member Author

I hope same pattern can be used for serializer-cache functionality.

True, true. That's what we all want 😆

@cowtowncoder Thank you for the final(and finer) touches 👍🏻 . Later today, I will try to link commits and PRs back to #2502 in more structured way, as list of done’s and TODO’s.

@cowtowncoder
Copy link
Member

@JooHyukKim Thank you again for contributing this & tweaking things. I was able to merge it successfully into master (which actually was easier to wire up).

@JooHyukKim JooHyukKim deleted the 2502-poc branch September 5, 2023 23:27
This was referenced Sep 11, 2023
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cowtowncoder cowtowncoder cowtowncoder approved these changes

@pjfanning pjfanning Awaiting requested review from pjfanning

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@JooHyukKim @cowtowncoder @pjfanning