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

In io.quarkus.jackson.ObjectMapperCustomizer, use JsonMapper.Builder instead of ObjectMapper #24256

Closed
wants to merge 1 commit into from

Conversation

chonton
Copy link
Contributor

@chonton chonton commented Mar 10, 2022

Closes #24019

@chonton
Copy link
Contributor Author

chonton commented Mar 10, 2022

Probably merge conflicts with #23995. (Are you ready to merge that?)

@quarkus-bot
Copy link

quarkus-bot bot commented Mar 11, 2022

Failing Jobs - Building 2ec31af

Status Name Step Failures Logs Raw logs
Devtools Tests - JDK 11 Build Failures Logs Raw logs
Devtools Tests - JDK 17 Build Failures Logs Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ Devtools Tests - JDK 11 #

- Failing: integration-tests/devtools 

📦 integration-tests/devtools

io.quarkus.platform.catalog.RegistrySnapshotCatalogCompatibilityTest.testRegistrySnapshotPlatformCatalog line 30 - More details - Source on GitHub

java.lang.IllegalStateException: Failed to locate the dev tools config file (.quarkus/config.yaml)
	at io.quarkus.devtools.testing.registry.client.TestRegistryClient.<init>(TestRegistryClient.java:33)
	at io.quarkus.devtools.testing.registry.client.TestRegistryClientFactory.buildRegistryClient(TestRegistryClientFactory.java:32)

⚙️ Devtools Tests - JDK 17 #

- Failing: integration-tests/devtools 

📦 integration-tests/devtools

io.quarkus.platform.catalog.RegistrySnapshotCatalogCompatibilityTest.testRegistrySnapshotPlatformCatalog line 30 - More details - Source on GitHub

java.lang.IllegalStateException: Failed to locate the dev tools config file (.quarkus/config.yaml)
	at io.quarkus.devtools.testing.registry.client.TestRegistryClient.<init>(TestRegistryClient.java:33)
	at io.quarkus.devtools.testing.registry.client.TestRegistryClientFactory.buildRegistryClient(TestRegistryClientFactory.java:32)

Copy link
Contributor

@gastaldi gastaldi left a comment

Choose a reason for hiding this comment

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

We can't break compatibility because injecting ObjectMapper is used a lot.

I suggest maintaining the same behavior by deprecating it and introduce the new enhancement

@gastaldi
Copy link
Contributor

Also how would that work for non-json content?

@gsmet
Copy link
Member

gsmet commented Mar 15, 2022

Yeah, this is a good start but we need to think about the proper strategy to make that change as we don't want to break all the applications around.

@chonton
Copy link
Contributor Author

chonton commented Mar 21, 2022

These changes should be backwards compatible. JsonMapper is an ObjectMapper. There is backwards compatibility for invoking the deprecated ObjectMapperCustomizer(s) provided by non-quarkus managed code.

@gastaldi, what do you mean by non-json content? Is there current or future support for any other text mappers such as YamlMapper? Support for any binary mappers such as AvroMapper?

@gastaldi
Copy link
Contributor

@chonton I don't think we do that now, but an ObjectMapperCustomizer instance could work on any ObjectMapper implementation (eg. YAMLMapper).

@chonton
Copy link
Contributor Author

chonton commented Mar 21, 2022

When XxxMapper is introduced, is it better to use a single Customizer interface, or specific XxxCustomizer per XxxMapper?

@gastaldi
Copy link
Contributor

When XxxMapper is introduced, is it better to use a single Customizer interface, or specific XxxCustomizer per XxxMapper?

IMHO a single Customizer interface would be easier, as it does not require any code changes when a new ObjectMapper implementation is introduced

@gsmet gsmet added triage/on-ice Frozen until external concerns are resolved triage/needs-rebase This PR needs to be rebased first because it has merge conflicts labels Feb 2, 2023
@chonton chonton closed this by deleting the head repository May 11, 2023
@quarkus-bot quarkus-bot bot added the triage/invalid This doesn't seem right label May 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/amazon-lambda area/funqy area/jackson Issues related to Jackson (JSON library) area/rest area/vertx triage/invalid This doesn't seem right triage/needs-rebase This PR needs to be rebased first because it has merge conflicts triage/on-ice Frozen until external concerns are resolved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

In io.quarkus.jackson.ObjectMapperCustomizer, use JsonMapper instead of ObjectMapper
3 participants