Remove "default" formatter mappers and let ORM build them itself

[marko-bekhta]

fixes #47588 (in a way 😄)

• there is some ongoing work in Hibernate ORM that will preconfigure the Jackson ObjectMapper in a specific way if the Oracle DB is in use. This will probably be part of 7.1 or so, and once it's out, we'd need to adapt and "copy" that logic here
• this has been causing other problems in the past
• we don't want to let the global mapper configuration affect the db serialisation/deserialisation

so with that ^ let's just drop the "default" mappers and let Hibernate ORM do its thing and configure it in a way it wants by default. If users want more control over it they could (and should) use the @io.quarkus.hibernate.orm.JsonFormat + @io.quarkus.hibernate.orm.PersistenceUnitExtension : https://quarkus.io/guides/hibernate-orm#json_xml_serialization_deserialization

As this is a breaking change ... for migration purposes... we'd suggest that users define a FormatMapper like:

@JsonFormat
@PersistenceUnitExtension
public class OrmJsonFormatMapper implements FormatMapper {

    private final JacksonJsonFormatMapper mapper;

    @Inject
    public OrmJsonFormatMapper(ObjectMapper objectMapper) {
        this.mapper = new JacksonJsonFormatMapper(objectMapper);
    }

    @Override
    public <T> T fromString(CharSequence charSequence, JavaType<T> javaType, WrapperOptions wrapperOptions) {
        return mapper.fromString(charSequence, javaType, wrapperOptions);
    }

    @Override
    public <T> String toString(T value, JavaType<T> javaType, WrapperOptions wrapperOptions) {
        return mapper.toString(value, javaType, wrapperOptions);
    }
}

to get the same behaviour as right now, and hint them to reconsider using the injected object mapper, but built one themselves for persistence purposes ?

I'm opening this as a draft to discuss this approach. If we agree on it, I will prepare and test examples for each mapper type and write the migration note. 😃

[quarkus-bot]

/cc @gsmet (hibernate-orm)

[marko-bekhta]

also pinging @yrodiere and @lucamolteni 🙂

[yrodiere]

IIUC custom formats are still supported through CDI and @JsonFormat etc., we just don't use the (generally REST-oriented) objectmappers/formatters by default, and instead rely on ORM's?

This seems reasonable to me.

I have two concerns:

1. Regarding migration, do you think there is any way that switching from the default ObjectMapper (or similar) to the one built by Hibernate ORM could lead to silent data loss? Errors are fine (ish) if there's a migration path, but silently dropping data would be a huge problem.
2. Do we have native tests for these features? Because depending when Hibernate ORM builds the "default formatters", the reflection involved could lead to quite a few problems.

[marko-bekhta]

IIUC custom formats are still supported through CDI and @jsonformat etc., we just don't use the (generally REST-oriented) objectmappers/formatters by default, and instead rely on ORM's?

yes 👍🏻

Regarding migration, do you think there is any way that switching from the default ObjectMapper (or similar) to the one built by Hibernate ORM could lead to silent data loss? Errors are fine (ish) if there's a migration path, but silently dropping data would be a huge problem.

well the ORM's one is a pretty straightforward config (e.g. new ObjectMapper().findAndRegisterModules()). So it all depends on what users have done with their Quarkus' ObjectMapper customization... if it's just some fancy date/time format, then it should fail with exception, but if it's something more, like a custom serializer for types, then there could be a data loss...

So my thinking here is that for the migration guide, we suggest this:

    @Inject
    public OrmJsonFormatMapper(ObjectMapper objectMapper) {
        this.mapper = new JacksonJsonFormatMapper(objectMapper);
    }

so that the app keeps doing what it was doing, (inject the Quarkus' object mapper (or Jsonb or anything else) into this formatter to get them up and running and as a second step we'd suggest that they get rid of the injection and create a clean mapper + configure it the way they need it for the DB operations + do any data migration as necessary.

now ... yes, if someone misses this in the migration guide... I suppose we could try creating these beans from our side if user haven't provided their custom one (that's actually somewhat where we are right now), and log some warnings telling them "hey this will break in the next version, you should change it", and then actually make it fail in the next one? But then we'd force everyone to create some format mappers... IDK that would be safer (for data) but more annoying for the users..
I think one of the problems here ... that no matter what we do, any existing app that customized the mapper will need to have a custom format mapper bean, or do the "data migration" to convert their jsons in the db to the "new default" format...

Do we have native tests for these features? Because depending when Hibernate ORM builds the "default formatters", the reflection involved could lead to quite a few problems.

yes, in the Postgresql module ... here's one for the "custom formatter":

quarkus/integration-tests/jpa-postgresql/src/main/java/io/quarkus/it/jpa/postgresql/otherpu/EntityWithJsonOtherPU.java

Lines 23 to 24 in 47ecd0a

	@JdbcTypeCode(SqlTypes.JSON)
	ToBeSerializedWithDateTime json;

and then the one using the defaults:

quarkus/integration-tests/jpa-postgresql/src/main/java/io/quarkus/it/jpa/postgresql/EntityWithJson.java

Lines 22 to 23 in 47ecd0a

	@JdbcTypeCode(SqlTypes.JSON)
	ToBeSerializedWithDateTime json;

[yrodiere]

so that the app keeps doing what it was doing, (inject the Quarkus' object mapper (or Jsonb or anything else) into this formatter to get them up and running and as a second step we'd suggest that they get rid of the injection and create a clean mapper + configure it the way they need it for the DB operations + do any data migration as necessary.

now ... yes, if someone misses this in the migration guide... I suppose we could try creating these beans from our side if user haven't provided their custom one (that's actually somewhat where we are right now), and log some warnings telling them "hey this will break in the next version, you should change it", and then actually make it fail in the next one? But then we'd force everyone to create some format mappers... IDK that would be safer (for data) but more annoying for the users.. I think one of the problems here ... that no matter what we do, any existing app that customized the mapper will need to have a custom format mapper bean, or do the "data migration" to convert their jsons in the db to the "new default" format...

I'd say the focus here should be on detecting problematic cases with zero false negatives and as few false positives as possible.

From what I understand, there can be a problem if all of the following are true:

1. The app has a custom ObjectMapper (or other, for non-Jackson serialization).
2. The app does not set a custom @JsonFormat/@XmlFormat for at least one persistence unit.
3. The app actually uses JSON/XML formatting for that same persistence unit. I think we can detect uses of JsonJavaType in metadata?

If we can detect all that, I think we're in a pretty narrow case, and it would be fine to warn, then error out in a future version, like you mentioned.

Maybe put this behavior behind a setting (quarkus.hibernate-orm.mapping.format.global), with three possible values warn (legacy behavior + warning if conditions above)/fail (error if conditions above)/ignore (ignore global object mapper, future default). That way we start with warn, move to fail in the next version, and at any time, people who checked everything is good can simply set it to ignore?

@gsmet would that make sense?

[github-actions]

🎊 PR Preview 3e365d6 has been successfully built and deployed to https://quarkus-pr-main-48177-preview.surge.sh/version/main/guides/

• Images of blog posts older than 3 months are not available.
• Newsletters older than 3 months are not available.

[yrodiere]

LGTM. Let's merge once you've removed the deprecation?

Also, could you please set a reminder for yourself to switch the default to fail after 3.25 gets branched off, on July 16th? I personally won't be around but should be back on July 21st...

[marko-bekhta]

Also, could you please set a reminder for yourself to switch the default to fail after 3.25 gets branched off, on July 16th?

done 👍🏻 😃

[yrodiere]

Alright, thanks. Then let's merge after the build succeeds.
The migration guide entry will need to go there: https://github.com/quarkusio/quarkus/wiki/Migration-Guide-3.25 :)

[quarkus-bot]

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 2f7b184.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

[quarkus-bot]

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 2f7b184.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

---

Flaky tests - Develocity

⚙️ JVM Tests - JDK 17

📦 extensions/smallrye-reactive-messaging-kafka/deployment

✖ io.quarkus.smallrye.reactivemessaging.kafka.deployment.testing.KafkaDevServicesContinuousTestingWorkingAppPropsTestCase.testContinuousTestingScenario3 - History

• io.quarkus.builder.BuildException: Build failure: Build failed due to errors [error]: Build step io.quarkus.redis.deployment.client.DevServicesRedisProcessor\#startRedisContainers threw an exception: java.lang.RuntimeException: org.testcontainers.containers.ContainerLaunchException: Container startup failed for image docker.io/redis:7 at io.quarkus.redis.deployment.client.DevServicesRedisProcessor.startRedisContainers(DevServicesRedisProcessor.java:131) at java.base/java.lang.invoke.MethodHandle.invokeWithArguments(MethodHandle.java:732) at io.quarkus.deployment.ExtensionLoader$3.execute(ExtensionLoader.java:856) - java.lang.RuntimeException

java.lang.RuntimeException: 
io.quarkus.builder.BuildException: Build failure: Build failed due to errors
	[error]: Build step io.quarkus.redis.deployment.client.DevServicesRedisProcessor#startRedisContainers threw an exception: java.lang.RuntimeException: org.testcontainers.containers.ContainerLaunchException: Container startup failed for image docker.io/redis:7
	at io.quarkus.redis.deployment.client.DevServicesRedisProcessor.startRedisContainers(DevServicesRedisProcessor.java:131)
	at java.base/java.lang.invoke.MethodHandle.invokeWithArguments(MethodHandle.java:732)
	at io.quarkus.deployment.ExtensionLoader$3.execute(ExtensionLoader.java:856)
	at io.quarkus.builder.BuildContext.run(BuildContext.java:255)
	at org.jboss.threads.ContextHandler$1.runWith(ContextHandler.java:18)