HHH-14198 - Expose CompositeUserTypes through JPA Metamodel

[jwgmeligmeyling]

Composite User Types work like regular Composite Types (like Embeddable) in HQL. However, because they cannot be represented in the JPA metamodel, libraries like GraphQL for JPA or Blaze-Persistence cannot fully utilize them. In order to make the composite property names available to these libraries, it would be nice to optionally expose these attributes as embedded attributes. This pull request aims to make that change and makes it configurable through a custom setting.

Composite User Types are a common solution for mapping composite interfaces. A common example is for example Money from the Java Money API (JSR-354), for which composite user types are implemented in Jadira.

I know Composite User Types are currently not consiered in Hibernate 6.x. See also this Zulip thread. I am not sure if Hibernate 6.x will even have multi column types, which I presume would be a requirement to even introduce Composite User types back at some point. Usually Embeddables are a much easier, suitable mechanism for composite user types. But Embeddables are not always a viable alternative, because Embeddables require the type to be subclassed (as an interface cannot be mapped, and the type may not solely comprise fields that can be mapped to a simple basic type). To deal with this exact problem, MonetaryAmounts are still mapped as composite user type. There also have been suggestions to the JPA Spec to consider AttributeConverters for Embeddables for pracitcally the same purpose (which I think is going to be a mess of an implementation). See: jakartaee/persistence#105

Anyways, regardless of whether this gets integrated in 5.x, I don't expect it to be integrated in 6.x unless we also reintroduce Composite User Types. I am willing to contribute Composite User Types for 6.x if people see benefit in it and think it can be done in the first place.

https://hibernate.atlassian.net/browse/HHH-14198

[NathanQingyangXu]

It seems we might need to swap the two parameter orders for assertEquals(). The issue only manifests itself when testing is broken, however.

[beikov]

After fixing this, I see nothing else blocking this PR. Any concerns @Sanne ?

[Sanne]

hi @jwgmeligmeyling , this seems very reasonable to me. I'm wondering, do you think there's substantial drawbacks that made you choose to not enable this by default?

Is it because treating composite types and embeddables should ideally be exposed differently?

thanks!

[beikov]

hi @jwgmeligmeyling , this seems very reasonable to me. I'm wondering, do you think there's substantial drawbacks that made you choose to not enable this by default?

Is it because treating composite types and embeddables should ideally be exposed differently?

thanks!

I guess backwards-compatibility is a major point here, but I agree that enabling this by default is a good idea.

[jwgmeligmeyling]

Hi @Sanne ,

This was indeed from a backwards compatibility point of view. Although I don't think many users will be affected by that change. To be really affected by that you'd have to be dependent on the getType() result for a singular attribute bound to a composite type, which is a highly suspicious end result anyway.

Furthermore, its probably debatable whether a composite type should be represented as an embeddable. In the Hibernate code, the relationship is actually the other way around: an embeddable is a composite. We have to see to what extent that is an implementation choice or actual architecture. I think Composite types are relatively older than JPA's embeddables, which is probably one of the reasons embeddables were built upon composites rather than the other way around.

Considering this feature is not yet available in 6.0 I think the structure still remains a bit unclear. From issues like jakartaee/persistence#105 it is however clear that there remains a wish for being able to map interfaces through any other means than Emeddables, and I think some way of composite types behaving as embeddable is a nice addition (whether we call it an composite or not).

So in conclusion:

Is it because treating composite types and embeddables should ideally be exposed differently?

I don't think it has to, the way the code is currently is set up suggests otherwise, although future implementations may do this differently.

[Sanne]

thanks the additional context. FWIW, I think it would be fine to include this patch either as-is, or without the configuration knob if you're fine with having this "always on".

I'm generally trying to reduce the configuration properties: I believe it would help for us take better reponsibility for features if they are "always on", rather than an obscure setup that only a few will know (and test).

[jwgmeligmeyling]

@beikov WDYT? Should we omit the configuration property?

[beikov]

Since we are changing attribute types from JPA BasicType to EmbeddableType I'd say it's worth allowing the old behavior for 5.x but for 6.x we could drop it IMO.

[jwgmeligmeyling]

Hmm it seems that if it becomes the default behaviour, then we do not yet agree whether it should be configurable.

@Sanne I am comfortable with removing the property personally.

Should I make that change?

[Sanne]

yes I'd keep it simple, and assuming that we won't backport this further back than 5.4 it's ok to make minor adjustments in behaviour - when we don't expect to break existing applications. (only 5.3 has very strict backwards compatibility requirements).

[jwgmeligmeyling]

I have rebased the PR. I've also tested it against my personal integration test suite and the one from Blaze-Persistence to catch any potential regressions. One issue that I've found has since been patched by fixing the if-condition. The setting is now always-on and not configurable.

[beikov]

Looks good to me!

[jwgmeligmeyling]

@Sanne I removed the configurability, @beikov re-reviewed my changes. Can you have another look?

Ignore the last push, I removed that commit since, and the head now points to the same commit again

[beikov]

After fixing this, I see nothing else blocking this PR. Any concerns @Sanne ?

[jwgmeligmeyling]

@beikov Fixed!

[Sanne]

nice! thanks. Will you need it in 5.4.x ?

[beikov]

This would for sure be an interesting feature for 5.4 as well, but I'm not sure if we should backport features.

[Sanne]

it's going to take a while before we release 5.5.x versions, so while I agree we shouldn't backport significant features - we can be flexible about minor things. Up to you..

[beikov]

If the 5.5.0-Beta1 release is not planned to be released any time soon, I'd be happy to see this in 5.4.

[jwgmeligmeyling]

I’d love to see it in 5.4 as well 😃

[odrotbohm]

Isn't this change effectively breaking the contract of Metamodel.managedType(…)? The Javadoc of that says:

Return the metamodel managed type representing the entity, mapped superclass, or embeddable class.

None of this is true for custom Hibernate user types, is it? I am asking because this change causes a lot of ripple effects in downstream code that uses the Metamodel API and expect it to behave as specified, i.e. the aforementioned methods only returning an instance of ManagedType if the type handed in really is an entity, mapped superclass or embeddable in the way, JPA defines it.

[beikov]

You can set the hibernate.jpa.metamodel.population config property to skipUnsupported to only expose the types that are real JPA types. Internally and also from HQL, CompositeUserType is very similar to an embeddable, hence the decision to do this.

[odrotbohm]

I am not convinced that arbitrarily reimagining a specified API and providing knobs one now explicitly has to set to actually make it work as specified is a proper way of implementing such an API. The change provided here literally breaks that specified API, i.e. all code that's been written against the originally specified semantics. There's not that much of that code, I admit, but we're getting littered with problems stemming from that change because some prominent JPA implementation decided something is "very similar". The spec doesn't say "… or similar".

I can only recommend to at least turn this into an opt-in feature, so that folks who enable it know that they're switching to non-standard API behavior, and thus have to live with the downstream consequences.

[beikov]

I am not convinced that arbitrarily reimagining a specified API and providing knobs one now explicitly has to set to actually make it work as specified is a proper way of implementing such an API. The change provided here literally breaks that specified API, i.e. all code that's been written against the originally specified semantics. There's not that much of that code, I admit, but we're getting littered with problems stemming from that change because some prominent JPA implementation decided something is "very similar". The spec doesn't say "… or similar".

We are not "reimagining" an API and what we return here didn't "change" from a JPA supported point of view. We returned "unsupported" types from the JPA metamodel API already before when you use certain Hibernate specific features like e.g. Hibernate Envers. And even before this change, you were able to set the hibernate.jpa.metamodel.population property to avoid seeing these types.

The spec doesn't say "… or similar".

The spec also doesn't say anything about custom user types, so should we remove support for that? ;)

I'm obviously kidding, but you see where this "spec doesn't say" leads to. If you want Hibernate to fully behave like the spec says, you have to enable the spec compliance setting, but believe me, that's probably not something you usually want as that has other consequences as well, which is why the hibernate.jpa.metamodel.population property exists.

I can only recommend to at least turn this into an opt-in feature, so that folks who enable it know that they're switching to non-standard API behavior, and thus have to live with the downstream consequences.

If you enable JPA compliance, this is an opt-in feature.

Not sure what to tell you, 1.5 years after this was merged. It wasn't an issue for anyone so far, so maybe you just had wrong expectations? Don't get me wrong, but as far as see this, you will simply have to change your code or configuration, just like other people have to adapt to changes that happen in the Spring ecosystem.

[odrotbohm]

The spec also doesn't say anything about custom user types, so should we remove support for that? ;) I'm obviously kidding, but you see where this "spec doesn't say" leads to.

It's pointless to discuss this way. The spec says: "returns elements that match X, Y, Z". It's allow-listing what is to be returned. It is of course not listing all elements that must not be returned as that set is unbounded. What's next? Returning entities from ….embeddable(…) because the spec didn't say that you must not return entities from that method? I mean, how can this be a serious argument?

Not sure what to tell you, 1.5 years after this was merged. It wasn't an issue for anyone so far, so maybe you just had wrong expectations? Don't get me wrong, but as far as see this, you will simply have to change your code or configuration, just like other people have to adapt to changes that happen in the Spring ecosystem.

I am not controlling the update cycles of the projects of our users. Some might just start to use the feature that triggers this misbehavior now.

If you enable JPA compliance, this is an opt-in feature.

So, you're saying that I need to explicitly enable spec compliance with a spec implementation? Interesting. Why didn't Hibernate expose this custom behavior via a custom method on its implementation class? Folks interested could then cast to that and invoke the method.

But thanks for considering the feedback.

[beikov]

What's next? Returning entities from ….embeddable(…) because the spec didn't say that you must not return entities from that method? I mean, how can this be a serious argument?

Let's stay serious here, a CompositeUserType is like a special kind of embeddable, but defined through a Hibernate specific API. In fact, in 6.0 we removed the CompositeUserType contract and introduced new SPIs that allow you to handle such cases completely through the embeddable mechanism.

I am not controlling the update cycles of the projects of our users. Some might just start to use the feature that triggers this misbehavior now.
So, you're saying that I need to explicitly enable spec compliance with a spec implementation? Interesting. Why didn't Hibernate expose this custom behavior via a custom method on its implementation class? Folks interested could then cast to that and invoke the method.

First and foremost, all implementations that implement the JPA spec usually have features that are "non-standard" which need to somehow be mapped to the JPA model or at least brought into harmony with it. I guess you wouldn't like it if we simply don't show you managed types just because they use a Hibernate specific feature, right? I hope you understand that there are multiple interests that we try to fulfill.

So, you're saying that I need to explicitly enable spec compliance with a spec implementation? Interesting.

JPA providers are free to require some sort of configuration for the TCK and Hibernate has had a JPA spec compliance setting for a very long time.

Why didn't Hibernate expose this custom behavior via a custom method on its implementation class? Folks interested could then cast to that and invoke the method.

We can of course clutter our APIs but we generally don't do that when we think that we are doing the right thing. Like I tried to explain, Hibernate returned "unsupported" types for a long time and it had a setting to disable that for a long time as well. So I don't think Hibernate is at fault here just because you didn't encounter a model yet that exposed this feature of Hibernate.

[sebersole]

Why are we still having this non-discussion?

[gavinking]

@odrotbohm Remind me which specification Spring Data implements? I'm having a senior moment and can't quite seem to recall...