Add a way to configure TypeFactory Jackson uses
#4115
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ | |
| import com.fasterxml.jackson.databind.util.LRUMap; | ||
| import com.fasterxml.jackson.databind.util.LookupCache; | ||
| import com.fasterxml.jackson.databind.util.TypeKey; | ||
| import java.util.List; | ||
| import org.junit.Test; | ||
|
|
||
| import java.util.HashMap; | ||
|
|
@@ -90,6 +91,11 @@ public LookupCache<JavaType, JsonDeserializer<Object>> forDeserializerCache(Dese | |
| return _cache; | ||
| } | ||
|
|
||
| @Override | ||
| public LookupCache<Object, JavaType> forTypeFactory() { | ||
| return new LRUMap<>(16, 64); | ||
| } | ||
|
|
||
| @Override | ||
| public LookupCache<TypeKey, JsonSerializer<Object>> forSerializerCache(SerializationConfig config) { | ||
| return new LRUMap<>(8, 64); | ||
|
|
@@ -110,6 +116,10 @@ public LookupCache<JavaType, JsonDeserializer<Object>> forDeserializerCache(Dese | |
| } | ||
|
|
||
| @Override | ||
| public LookupCache<Object, JavaType> forTypeFactory() { | ||
| return new LRUMap<>(16, 64); | ||
| } | ||
|
|
||
| public LookupCache<TypeKey, JsonSerializer<Object>> forSerializerCache(SerializationConfig config) { | ||
| return _cache; | ||
| } | ||
|
|
@@ -128,6 +138,40 @@ public JsonSerializer<Object> put(TypeKey key, JsonSerializer<Object> value) { | |
| return super.put(key, value); | ||
| } | ||
| } | ||
|
|
||
| static class CustomTypeFactoryCacheProvider implements CacheProvider { | ||
|
|
||
| final CustomTestTypeFactoryCache _cache = new CustomTestTypeFactoryCache(); | ||
|
|
||
| @Override | ||
| public LookupCache<JavaType, JsonDeserializer<Object>> forDeserializerCache(DeserializationConfig config) { | ||
| return new LRUMap<>(16, 64); | ||
| } | ||
|
|
||
| @Override | ||
| public LookupCache<Object, JavaType> forTypeFactory() { | ||
| return _cache; | ||
| } | ||
|
|
||
| public LookupCache<TypeKey, JsonSerializer<Object>> forSerializerCache(SerializationConfig config) { | ||
| return new LRUMap<>(16, 64); | ||
| } | ||
| } | ||
|
|
||
| static class CustomTestTypeFactoryCache extends LRUMap<Object,JavaType> { | ||
|
|
||
| public boolean _isInvoked = false; | ||
|
|
||
| public CustomTestTypeFactoryCache() { | ||
| super(8, 16); | ||
| } | ||
|
|
||
| @Override | ||
| public JavaType putIfAbsent(Object key, JavaType value) { | ||
| _isInvoked = true; | ||
| return super.putIfAbsent(key, value); | ||
| } | ||
| } | ||
|
|
||
| /* | ||
| /********************************************************************** | ||
|
|
@@ -186,7 +230,7 @@ public void testDefaultCacheProviderSharesCache() throws Exception | |
| .cacheProvider(cacheProvider) | ||
| .build(); | ||
|
|
||
| // Act | ||
| // 3. Add two different types to each mapper cache | ||
| mapper1.readValue("{\"point\":24}", RandomBean.class); | ||
| mapper2.readValue("{\"height\":24}", AnotherBean.class); | ||
|
|
@@ -205,10 +249,12 @@ public void testBuilderValueValidation() throws Exception | |
| DefaultCacheProvider.builder() | ||
| .maxDeserializerCacheSize(0) | ||
| .maxSerializerCacheSize(0) | ||
| .maxTypeFactoryCacheSize(0) | ||
|
There was a problem hiding this comment. One thing I wonder: I think that most cache implementations impose a minimum size, so Then again, it is now possibly to add custom cache instances too which can be "no-op" (do nothing) so maybe this doesn't matter. I also haven't tested out if backing There was a problem hiding this comment.
Caffeine, on the other hand, batches evictions, so the zero maximum size constraint can be temporarily violated. In addition, Cache2k, Android LruCache, and Ehcache all throw IllegalArgumentException when trying to create a cache with a maximum size of zero. There was a problem hiding this comment. Thank you @iProdigy . Sounds like we are good then. So 0 size for 2.14+ Jackson implementation will not retain anything in cache, and while there is write overhead that should be negligible. It'd be nice to test this invariant, but no further work should be necessary it sounds. There was a problem hiding this comment. Hmmm, I am trying to come up with JavaDoc like... * Note that setting value of {@code ...} to zero will not .... unlike some Cache providers.....but idk what to exactly say 🥲 any help? There was a problem hiding this comment. Added JavaDoc and tagged you there. |
||
| .build(); | ||
| DefaultCacheProvider.builder() | ||
| .maxDeserializerCacheSize(Integer.MAX_VALUE) | ||
| .maxSerializerCacheSize(Integer.MAX_VALUE) | ||
| .maxTypeFactoryCacheSize(Integer.MAX_VALUE) | ||
| .build(); | ||
|
|
||
| // fail cases | ||
|
|
@@ -224,6 +270,12 @@ public void testBuilderValueValidation() throws Exception | |
| } catch (IllegalArgumentException e) { | ||
| assertTrue(e.getMessage().contains("Cannot set maxSerializerCacheSize to a negative value")); | ||
| } | ||
| try { | ||
| DefaultCacheProvider.builder().maxTypeFactoryCacheSize(-1); | ||
| fail("Should not reach here"); | ||
| } catch (IllegalArgumentException e) { | ||
| assertTrue(e.getMessage().contains("Cannot set maxTypeFactoryCacheSize to a negative value")); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -258,4 +310,19 @@ private void _verifySerializeSuccess(CacheProvider cacheProvider) throws Excepti | |
| assertEquals("{\"slide\":123}", | ||
| mapper.writeValueAsString(new SerBean())); | ||
| } | ||
|
|
||
| /** | ||
| * Sanity test for serialization with {@link CacheProvider#forTypeFactory()} | ||
| */ | ||
| @Test | ||
| public void sanityCheckTypeFactoryCacheSize() throws Exception | ||
| { | ||
| // custom | ||
| CustomTypeFactoryCacheProvider customProvider = new CustomTypeFactoryCacheProvider(); | ||
| ObjectMapper mapper = JsonMapper.builder() | ||
| .cacheProvider(customProvider) | ||
| .build(); | ||
| mapper.getTypeFactory().constructParametricType(List.class, HashMap.class); | ||
| assertTrue(customProvider._cache._isInvoked); | ||
| } | ||
| } | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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.
Added JavaDoc as per comment #4115 (comment). WDYT?
/cc @iProdigy @cowtowncoder
Uh oh!
There was an error while loading. Please reload this page.
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 would rephrase... For DefaultCacheProvider, zero max size effectively does disable caching since entries are not retained in the underlying LRUMap. Yes, it's technically true that a write & removal occurs under the hood, but users don't care about this detail (other than for optimizing performance, as a special no-op implementation could've been used instead)
Perhaps:
Specifying a maximum size of zero prevents values from being retained in the cacheThere 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.
True, they don't need to. Thank you for the suggestion! 🙏🏼🙏🏼 Changed accordingly