Allow JSON serialization customization in ORM

[marko-bekhta]

• add @JsonFormat/@XmlFormat annotations to apply to custom FormatMapper implementations for JSON/XML formatting (must also use @PersistenceUnitExtension)
• use a Quarkus-configured FormatMapping if Jackson/JSONB/Jaxb is present, and the user did not specify a format mapper

Closes: #32029

[quarkus-bot]

/cc @Sanne (hibernate-orm), @gsmet (hibernate-orm), @yrodiere (hibernate-orm)

[yrodiere]

This looks good, but I wonder why we're going through a configuration property when we could just use CDI? I.e. like we do for interceptors.

Also I'd advise against configuration properties that refer to class names, that'll put us in a difficult position if we ever end up allowing to point to a CDI bean through configuration properties. Hibernate Search supports that, but it requires some infrastructure that I'm not sure ORM provides at the moment (see https://docs.jboss.org/hibernate/stable/search/reference/en-US/html_single/#configuration-bean-reference-parsing).

[marko-bekhta]

but I wonder why we're going through a configuration property when we could just use CDI?

IDK 🤷🏻 🤷🏻‍♂️ 🤷🏻‍♀️ 😆 I'm probably just more used to config properties than to configuring things through beans 😃. So if we'd go with CDI.. then it would looks something like:

InjectableInstance<FormatMapper> formatMapper = PersistenceUnitUtil.singleExtensionInstanceForPersistenceUnit(
        FormatMapper.class, persistenceUnitName);
if (!formatMapper.isUnsatisfied()) {
    descriptor.getProperties().put(AvailableSettings.JSON_FORMAT_MAPPER, formatMapper.get());
}

and then the user would do:

@PersistenceUnitExtension("some-pu")
public class SomePuFormatMapper implements FormatMapper {
// ...
}

But since this FormatMapper is used for both JSON and XML formatters in ORM... should we create JsonFormatMapper and XmlFormatMapper and expect users to implement these so we'd know to which property to attach the formatter to? Or is there some other means to distinguish them ... (BTW, we should probably go ahead and add the same thing for XML while we are at it, no?)

[geoand]

This looks good, but I wonder why we're going through a configuration property when we could just use CDI? I.e. like we do for interceptors.

+1 for this - it also makes for a consistent user experience

Also I'd advise against configuration properties that refer to class names

Very very true

[yrodiere]

I added a comment below about something I didn't spot earlier.

But since this FormatMapper is used for both JSON and XML formatters in ORM... should we create JsonFormatMapper and XmlFormatMapper and expect users to implement these so we'd know to which property to attach the formatter to?

FormatMapper is an ORM interface, so I wouldn't extend it.

You may be able to introduce additional CDI qualifiers, such as @Json and @Xml, and pick the mapper based on that? And default to an object mapper without any of these qualifiers.

I wonder if there is already something like this for e.g. RestEasy, @geoand? I'd rather have these qualifiers defined in some serialization-related module than in the Hibernate ORM extension, if possible...

BTW, we should probably go ahead and add the same thing for XML while we are at it, no?

Probably, if it works, including in native mode.

[geoand]

You may be able to introduce additional CDI qualifiers, such as @JSON and @xml, and pick the mapper based on that? And default to an object mapper without any of these qualifiers.

We don't have such qualifiers anywhere AFAIK.

I wonder if there is already something like this for e.g. RestEasy, @geoand? I'd rather have these qualifiers defined in some serialization-related module than in the Hibernate ORM extension, if possible...

We had talked about for RESTEasy Reactive, but it never materialized (not least because there are other ways to do this in RESTEasy Reactive)

[yrodiere]

We had talked about for RESTEasy Reactive, but it never materialized (not least because there are other ways to do this in RESTEasy Reactive)

Ok, so now might be the time. There's no obvious place to put these annotations in, though... I can think of two approaches:

1. Put them in the core (e.g. core/runtime/src/main/java/io/quarkus/runtime/serialization) with rather generic names such as @Json and @Xml, so that they can be shared among multiple extension. Though for now only Hibernate ORM would use them.
2. Put them in the Hibernate ORM extension with more specific names such as @JsonFormat and @XmlFormat, to use them only with Hibernate ORM. Though we might need to go through a deprecation cycle if we add more generic annotations later.

I'd tend to favor 2 since there doesn't seem to be much momentum to add similar annotations for RestEasy. WDYT?

[geoand]

I agree with going for 2 for now - best to be cautious for starters and expand in the future if we see it makes sense.

[Sanne]

I'd also prefet option 2, as it might not be a good idea to dictate a format that necessarily applies to multiple layers: there might be some need to map the same object differently in the DB than in other external representations, as one would typically update one service at a time.

[yrodiere]

I'd also prefet option 2, as it might not be a good idea to dictate a format that necessarily applies to multiple layers: there might be some need to map the same object differently in the DB than in other external representations, as one would typically update one service at a time.

Sure, but having a common @Json/@Xml qualifier does not necessary lead to that problem. Object mappers would be annotated with other qualifiers on top of @Json/@Xml. For ORM we need @PersistenceUnitExtension anyway, those for Resteasy we'd have another, resteasy-specific qualifier. At least that's how I envisoned it.

Anyway, let's forget about a common @Json/@Xml, we're not there yet.

@marko-bekhta , to sum up the consensus seems to be that if you add support for XML FormatMappers, you should add @JsonFormat and @XmlFormat CDI qualifiers in the Hibernate ORM extension to distinguish between JSON and XML ObjectMappers. Those qualifiers would be understood by the Hibernate ORM extension only.

[marko-bekhta]

This will need the next release of ORM to make options.applyJsonFormatMapper and options.applyXmlFormatMapper available. So I've converted this PR to a draft. But otherwise, it works.

[github-actions]

🙈 The PR is closed and the preview is expired.

[yrodiere]

This will need the next release of ORM to make options.applyJsonFormatMapper and options.applyXmlFormatMapper available. So I've converted this PR to a draft. But otherwise, it works.

Rebasing and marking as ready for review since we're now using a version of ORM that exposes these two options.

[yrodiere]

LGTM, thanks.

@geoand maybe you'll want to have a look at the changes to the JSON extensions?

[geoand]

Lovely!

[quarkus-bot]

Failing Jobs - Building 47ecd0a

Status	Name	Step	Failures	Logs	Raw logs	Build scan
✔️	JVM Tests - JDK 11					🔍
✔️	JVM Tests - JDK 17					🔍
✖	JVM Tests - JDK 21	Build	Failures	Logs	Raw logs	🔍

You can also consult the Develocity build scans.

Failures

⚙️ JVM Tests - JDK 21 #

- Failing: integration-tests/container-image/maven-invoker-way 

📦 integration-tests/container-image/maven-invoker-way

✖ Failed to execute goal org.apache.maven.plugins:maven-invoker-plugin:3.6.0:run (integration-tests) on project quarkus-integration-test-container-image-invoker: 1 build failed. See console output above for details.

[yrodiere]

Build failure is unrelated:

2023-11-22T13:06:49.5498841Z [INFO] [ERROR] Caused by: java.util.concurrent.ExecutionException: com.google.cloud.tools.jib.http.ResponseException: 500 Internal Server Error
2023-11-22T13:06:49.5505141Z [INFO] [ERROR] GET https://registry.access.redhat.com/v2/ubi8/openjdk-11-runtime/blobs/sha256:9c3f7aec4e3ecbaf95a7953bd9dd6ba23d3e79fc54441e70f3d090e374f59c79
2023-11-22T13:06:49.5509538Z [INFO] [ERROR] <html>
2023-11-22T13:06:49.5512257Z [INFO] [ERROR]   <head>
2023-11-22T13:06:49.5513371Z [INFO] [ERROR]     <title>Internal Server Error</title>
2023-11-22T13:06:49.5515480Z [INFO] [ERROR]   </head>
2023-11-22T13:06:49.5516506Z [INFO] [ERROR]   <body>
2023-11-22T13:06:49.5517809Z [INFO] [ERROR]     <h1><p>Internal Server Error</p></h1>
2023-11-22T13:06:49.5518705Z [INFO] [ERROR]     
2023-11-22T13:06:49.5519665Z [INFO] [ERROR]   </body>
2023-11-22T13:06:49.5520554Z [INFO] [ERROR] </html>

[yrodiere]

@marko-bekhta Merged, thanks!