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

Disable Jackson's FAIL_ON_UNKNOWN_PROPERTIES by default #13361

Merged
merged 2 commits into from
Nov 23, 2020

Conversation

gsmet
Copy link
Member

@gsmet gsmet commented Nov 18, 2020

This is what was discussed on the mailing list.

Commits should be reviewed separately as the first one is a cleanup commit, renaming a file.

/cc @maxandersen @loicmathieu @geoand

Copy link
Contributor

@loicmathieu loicmathieu left a comment

Choose a reason for hiding this comment

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

We should document that the default Jackson ObjectMapper is customized by default.
Is it the only customization that we wants by default (I usually always disabled date as timestamp as we better off with ISO dates for interoperability).

I think we must provide more configuration options for the more customized features of Jackson. Or maybe we can have "generic configuration" where a developer can use :
quarkus.jakson.feature.<feature-name>=value?

@gsmet
Copy link
Member Author

gsmet commented Nov 18, 2020

So to be honest, I'm not for having all the features in the config.

People who want to customize Jackson can do it with a customizer, that's practical enough.

So for me it's really about:

  • providing more sane defaults if we think they make sense
  • providing an easy way to go back to Jackson's default behavior

I will write the docs once we agree on the idea.

@loicmathieu
Copy link
Contributor

@gsmet OK for me then.

providing more sane defaults if we think they make sense

I think serializing dates as ISO (as JSON-B) is a better default than timestamp.
So I'll disabled the features WRITE_DATES_AS_TIMESTAMP by default and add a config item for it.

Keep in mind that it will be a breaking change for the frameworks that currently uses the default ObjectMapper (AWS Lambda, Funqy, GCP Functions, Azure Functions at least).

@geoand
Copy link
Contributor

geoand commented Nov 18, 2020

+1 for this.

But we do need to communicate that this is a breaking change

@gsmet
Copy link
Member Author

gsmet commented Nov 19, 2020

I'm not exactly sure about the timestamp change format. @geoand WDYT?

@geoand
Copy link
Contributor

geoand commented Nov 19, 2020

I like the idea, +1

@gsmet
Copy link
Member Author

gsmet commented Nov 19, 2020

@geoand so I tried to make the config BUILD_TIME_AND_RUNTIME_FIXED and thought the bean would then be initialized at a proper time.

It somehow doesn't work: no injection errors but the injected bean does not have the configured value.

I ended up using the usual Support pattern but I think it should be doable to inject BUILD_TIME_AND_RUNTIME_FIXED and that this bean should be initialized earlier to avoid the issue we have with a purely runtime config.

As for the timestamp, I'll let others do the work. I have no idea what it does and doesn't really have time to experiment.

Now that the infrastructure is here, it should be easy enough.

If nobody takes it, I'll take care of it sometime next week.

Now let's see if CI prefers this version.

@maxandersen
Copy link
Member

Fail on properties is good. But timestamp seems like it risk breaking many or ?

@geoand
Copy link
Contributor

geoand commented Nov 19, 2020

I think it's a reasonable default but indeed it's another breaking change.

We can certainly leave it out for now

ObjectMapper objectMapper = new ObjectMapper();
if (!jacksonConfigSupport.isFailOnUnknownProperties()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea to have this first, as it allows users to override the behavior with their own customizers if need be

@@ -186,6 +186,11 @@ It will allow to narrow down the number of JAX-RS providers (which can be seen a

==== Jackson

In Quarkus, Jackson is configured to ignore the unknown properties (by disabling the `DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES` feature).
Copy link
Contributor

Choose a reason for hiding this comment

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

You might want to add a note that this only works for the ObjectMapper configured by Quarkus and which they obtain via CDI (or use implicitly via RESTEasy etc).

I know it should be evident, but I am sure it might come as a surprise to some users.

Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

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

👍

Also provide a way to go back to the previous behavior.
@gsmet
Copy link
Member Author

gsmet commented Nov 23, 2020

You're right, it's not obvious, I added some precisions.

@gsmet gsmet merged commit cd992fb into quarkusio:master Nov 23, 2020
@ghost ghost added this to the 1.11 - master milestone Nov 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants