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.

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

Cache JsonSerializer "per" ObjectMapper #643

Merged
merged 1 commit into from
Sep 14, 2021
Merged

Conversation

brenuart
Copy link
Collaborator

This PR adresses issue #642.

The primary objective of these changes is to maintain a separate cache of JsonProvider per ObjectMapper instance.

It also contains the following changes that are functional equivalent as the original implementation but delegate more of the work to Jackson itself:

  • do not create a new instance of SerializerProvider ourselves but instead use ObjectMapper#getSerializerProviderInstance introduced in Jackson 2.7
  • do not create a JsonSerializer for the class to serialize ourselves but instead delegate to Jackson SerializerProvider#findValueSerializer(Class) and therefore leverage its internal cache

- Maintain a separate cache of JsonProvider per ObjectMapper
- do not create a new instance of SerializerProvider ourselves but instead use ObjectMapper#getSerializerProviderInstance introduced in Jackson 2.7
- do not create a JsonSerializer for the object class ourselves but instead delegate to Jackson SerializerProvider and therefore leverage its internal cache

Addresses issue #642
@brenuart
Copy link
Collaborator Author

brenuart commented Sep 10, 2021

As mentioned in the Javadoc, I'm also "worried" about maintaining a static cache of JsonProvider... :-(
There is an easy alternative however that does not require any caching at all. It basically relies on Jackson's ability to convert from one type to another... The writeTo method could be reduced to the following:

public void writeTo(JsonGenerator generator) throws IOException {
    if (this.object != null) {
        try {
            ObjectMapper mapper = (ObjectMapper) generator.getCodec();
            Map<String, Object> converted = mapper.convertValue(this.object, Map.class);
            for (Map.Entry<String, Object> entry: converted.entrySet()) {
                generator.writeObjectField(entry.getKey(), entry.getValue());
            }
        } catch (IllegalArgumentException e) {
            // thrown when object is not unwrappable
        }
    }
}

@brenuart
Copy link
Collaborator Author

@philsttr The more I think about the alternative approach described above, the more I believe it is a very good alternative. Overhead seems to be limited to the intermediate map, without the burden of maintaining static caches... Seems to be a good tradeoff.

@philsttr
Copy link
Collaborator

philsttr commented Sep 14, 2021

Creating an intermediary map every time the marker is written seems like too much overhead to me (for both memory and performance).

Using mapper.convertValue and then writing each entry from a Map is not guaranteed to result in the same JSON as the current behavior since it does not take some annotations like @JsonSerialize into consideration. This will surprising to users expecting Jackson's behavior, with no clear way to revert to the old behavior.

The number of ObjectMapper instances will be small... one per CompositeJsonFormatter, of which there are very few per application.

Given all the above, a static cache seems better to me.

@brenuart brenuart merged commit 35de528 into main Sep 14, 2021
@brenuart brenuart deleted the gh642-objectFieldsAppender branch September 14, 2021 07:46
@philsttr philsttr added this to the 7.0 milestone Sep 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ObjectFieldsAppendingMarker may use wrong unwrapping JsonSerializer when multiple ObjectMapper are used
2 participants