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

Allow Jackson configuration through properties #18572

Closed
edeandrea opened this issue Jul 9, 2021 · 30 comments
Closed

Allow Jackson configuration through properties #18572

edeandrea opened this issue Jul 9, 2021 · 30 comments
Assignees
Labels
area/jackson Issues related to Jackson (JSON library) kind/enhancement New feature or request

Comments

@edeandrea
Copy link
Contributor

edeandrea commented Jul 9, 2021

Description

It would be nice to be able to configure the Jackson ObjectMapper through properties rather than having to create a class extending ObjectMapperCustomizer.

Rather than having to constantly map Quarkus-specific properties to Jackson properties, just allow free-form mapping within the properties that would "flow-through" to the underlying Jackson enum properties. Something like this:

Enum Property Values
com.fasterxml.jackson.databind.DeserializationFeature quarkus.jackson.deserialization.<feature_name> true, false
com.fasterxml.jackson.databind.SerializationFeature quarkus.jackson.serialization.<feature_name> true, false
com.fasterxml.jackson.databind.MapperFeature quarkus.jackson.mapper.<feature_name> true, false
com.fasterxml.jackson.core.JsonGenerator.Feature quarkus.jackson.generator.<feature_name> true, false
com.fasterxml.jackson.core.JsonParser.Feature quarkus.jackson.parser.<feature_name> true, false
com.fasterxml.jackson.annotation.JsonInclude.Include quarkus.jackson.default-property-inclusion always, non_null, non_absent, non_default, non_empty

This way, if/as new "things" are added to the enums, there isn't any work that needs to be done in the extension. As long as <feature_name> aligns with the enum constant, it would just map.

I would envision these properties would be fixed at build time and not overridable at runtime. We'd have to decide what to do in the case that an application provides an ObjectMapperCustomizer and there are properties set. I would think that we would union the properties and what is defined in the ObjectMapperCustomizer. We'd have to figure out "who wins" in the situation where a property is set to one value and the customizer sets a different value for the same enum property. Don't property values specified in application.properties/application.yml always "win" over whats in the code?

Just a side note, I did get this idea from Spring and the Spring documentation which allows for this. Its a very convenient feature to have that eliminates the need to write code.

@edeandrea edeandrea added the kind/enhancement New feature or request label Jul 9, 2021
@geoand
Copy link
Contributor

geoand commented Sep 30, 2021

I think this is a nice enhancement and we actually already do this for JsonInclude.Include.

@geoand geoand added area/jackson Issues related to Jackson (JSON library) and removed triage/needs-triage labels Sep 30, 2021
@quarkus-bot
Copy link

quarkus-bot bot commented Sep 30, 2021

/cc @gsmet

@geoand geoand self-assigned this Sep 30, 2021
@geoand
Copy link
Contributor

geoand commented Sep 30, 2021

I'll do this after #20468 is in

@geoand
Copy link
Contributor

geoand commented Oct 4, 2021

There is one thing I don't like however about the proposal: It is not conductive to auto-completion by IDEs because the feature names are simply strings.

@edeandrea
Copy link
Contributor Author

I think the downside though is anytime there is a new config property in Jackson then Quarkus needs to make a change and "catch up", whereas them just being Strings allows users to use new features as they become available without Quarkus needing to make any changes.

That being said I don't know the internal implementation, so it may not be a big deal.

@geoand
Copy link
Contributor

geoand commented Oct 4, 2021

I think the downside though is anytime there is a new config property in Jackson then Quarkus needs to make a change and "catch up", whereas them just being Strings allows users to use new features as they become available without Quarkus needing to make any changes.

Yeah, that's the price to pay for sure.

@edeandrea
Copy link
Contributor Author

Do you really want to have to remember to update Quarkus config/tests/etc every time Jackson makes an update? Thats the real question.

Me personally I would prefer convention where I don't have to touch framework code if a library that the framework uses makes a change. But that's me and I'm not implementing this :)

Just my $0.02.

@geoand
Copy link
Contributor

geoand commented Oct 4, 2021

We always have to think about developer joy though as well 😉

@edeandrea
Copy link
Contributor Author

As a developer I would have much joy if new things in Jackson just worked without me having to file a ticket to have some property implemented/recognized by Quarkus :D

@edeandrea
Copy link
Contributor Author

Hey @geoand gentle ping on this...thoughts?

I'm up for trying out a PR for this if you can point me to where I might get started?

@geoand
Copy link
Contributor

geoand commented Mar 8, 2022

This and this should be a good start.

@edeandrea
Copy link
Contributor Author

Thanks thats helpful!

Are there tests anywhere that test that the ObjectMapper that gets created has all the correct attributes?

@geoand
Copy link
Contributor

geoand commented Mar 9, 2022

@edeandrea
Copy link
Contributor Author

I'll throw a draft PR together shortly for your to take a look at

@geoand
Copy link
Contributor

geoand commented Mar 9, 2022

Cool :)

@edeandrea
Copy link
Contributor Author

Ahh seems I can't do this in JacksonBuildTimeConfig?

   /**
     * Enable/disable Jackson serialization ({@code com.fasterxml.jackson.databind.SerializationFeature}) features.
     */
    @ConfigItem
    public Map<SerializationFeature, Boolean> serialization = new EnumMap<>(SerializationFeature.class);

    /**
     * Enable/disable Jackson deserialization ({@code com.fasterxml.jackson.databind.DeserializationFeature}) features.
     */
    @ConfigItem
    public Map<DeserializationFeature, Boolean> deserialization = new EnumMap<>(DeserializationFeature.class);

    /**
     * Enable/disable Jackson general purpose mapping ({@code com.fasterxml.jackson.databind.MapperFeature}) features.
     */
    @ConfigItem
    public Map<MapperFeature, Boolean> mapper = new EnumMap<>(MapperFeature.class);

    /**
     * Enable/disable Jackson parser ({@code com.fasterxml.jackson.core.JsonParser.Feature}) features.
     */
    @ConfigItem
    public Map<JsonParser.Feature, Boolean> parser = new EnumMap<>(JsonParser.Feature.class);

    /**
     * Enable/disable Jackson generator ({@code com.fasterxml.jackson.core.JsonGenerator.Feature}) features.
     */
    @ConfigItem
    public Map<JsonGenerator.Feature, Boolean> generator = new EnumMap<>(JsonGenerator.Feature.class);

@geoand
Copy link
Contributor

geoand commented Mar 9, 2022

You mean have an enum as the key of the map? If so, then correct, you can't

@edeandrea
Copy link
Contributor Author

Yes thats what I mean. I would think this would be a common thing no? Couldn't we modify BuildTimeConfigurationReader.processValue where it does

if (keyClass != String.class) {
  throw reportError(field, "Map key types other than String are not yet supported");
}

?

@geoand
Copy link
Contributor

geoand commented Mar 9, 2022

Yes, it can be done, although no it's not common - no one has asked for it AFAIK in the 3 years Quarkus has been around

@edeandrea
Copy link
Contributor Author

take you what? an hour or so? :D

@geoand
Copy link
Contributor

geoand commented Mar 9, 2022

My guess is that it's a one liner 😜.

I'll have a look tomorrow

@edeandrea
Copy link
Contributor Author

I changed that if statement to

 if ((keyClass != String.class) && !keyClass.isEnum()) {
  throw reportError(field, "Map key types other than String and Enum are not yet supported");
  }

but no idea if that fixes it :) I'll play with it and see if my tests work :)

@geoand
Copy link
Contributor

geoand commented Mar 9, 2022

😎

@edeandrea
Copy link
Contributor Author

Unfortunately that in itself doesn't seem to do it. Seems that the config class (JacksonBuildTimeConfig in this case) is instantiated programmatically via reflection, so even if I set a default value on the Map field (like public Map<SerializationFeature, Boolean> serialization = new EnumMap<>(SerializationFeature.class);) it doesn't take

The BuildTimeConfigurationReader.readConfigGroup method blindly creates a TreeMap as a placeholder for values (see

if (member instanceof ClassDefinition.MapMember) {
// get these on the sweep-up
try {
field.set(instance, new TreeMap<>());
} catch (IllegalAccessException e) {
throw toError(e);
}
continue;
).

I'm not sure I understand enough of how the config parsing works to be able to take this on. Do you want me to create a separate ticket for this?

@geoand
Copy link
Contributor

geoand commented Mar 9, 2022

Sure yeah

@edeandrea
Copy link
Contributor Author

I created ##24214

@gsmet
Copy link
Member

gsmet commented Mar 9, 2022

It might just be me but I don't see how it is more practical than a customizer given you don't have any completion so you will somehow have to go in the doc to find the right name, whereas you can just use completion with a customizer.

It's a bit more code but it's more flexible and less brittle.

And bonus point: you know when a feature is deprecated at compile time.

I could see an interest in that if we exposed and documented the most useful configuration properties as we started to do it but a free form thing, I don't know.

@edeandrea
Copy link
Contributor Author

It might just be me but I don't see how it is more practical than a customizer given you don't have any completion so you will somehow have to go in the doc to find the right name, whereas you can just use completion with a customizer.

I'm not sure what you mean by customizer. Can you provide an example? I'm not at all familiar with this part of the codebase.

@geoand
Copy link
Contributor

geoand commented Mar 9, 2022

@gsmet means an ObjectMapperCustomizer.

@edeandrea
Copy link
Contributor Author

Yeah I was just in the middle of writing that ObjectMapperCustomizer is what I thought it was. Me personally I'd prefer to have less code and more config - being able to supply configuration once in my config and then leave it be. Just my opinion.

@edeandrea edeandrea closed this as not planned Won't fix, can't repro, duplicate, stale Jun 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/jackson Issues related to Jackson (JSON library) kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants