Check that embedded property types are marked as @Embeddable

[marko-bekhta]

Fixes #35598

[quarkus-bot]

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

[Sanne]

Great, thanks @marko-bekhta !

[Sanne]

I've added some backport labels - technically older ORM versions didn't require this but it would be better to already nudge at least the 3.2 users towards fixing their mapping.

[quarkus-bot]

✔️ 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.

[yrodiere]

Depending on how well things work without the annotation, you might want to hold off on the back port, because I know some people just don't have control over the embedded classes (e.g. because they are generated or taken from a library).

Unless you have plans to make things work without the annotation in a future version?

[Sanne]

Depending on how well things work without the annotation, you might want to hold off on the back port, because I know some people just don't have control over the embedded classes (e.g. because they are generated or taken from a library).

It's certainly having pros and const. But surely you're not suggesting that they'll never be able to upgrade?

My concern is that there might be an urgent need for an ORM upgrade to be backported at any point int time, e.g. security reasons, and when people need to apply a security patch they are in a rush - best to nudge them to have their model future-proof already when there is no time pressure.

Perhaps we'd need a backport of this which merely logs a warning about the need.

[Sanne]

I've removed the backport-3.2? label for now - let's talk about it with @gsmet .

[yrodiere]

But surely you're not suggesting that they'll never be able to upgrade?

More or less, yes. Or rather, I'm suggesting they wouldn't have a migration path right now.

My concern is that there might be an urgent need for an ORM upgrade to be backported at any point int time, e.g. security reasons, and when people need to apply a security patch they are in a rush - best to nudge them to have their model future-proof already when there is no time pressure.

I understand the concept of micro upgrades, Sanne :⁠-⁠) But it's incompatible with breaking changes in micros such as this one.

As I said it all depends how well embaddables without the annotation used to work. If they didn't at all, it's fine. If they did in some situations, ORM and by extension Quarkus are introducing a breaking change, so it's not something we should backport lightly.

Perhaps we'd need a backport of this which merely logs a warning about the need.

Yes that seems like a more conservative approach. Or a hard failure that can be disabled through a configuration flag.

[marko-bekhta]

Or a hard failure that can be disabled through a configuration flag.

Let me know which option you prefer, and I'll make a patch for a different branch. 😃

[Sanne]

Ok, let's backport a warning please.
And sure I wasn't advocating to backport a breaking change - but let's bear in mind that this mapping wasn't going to be working correctly, so there's the hope that this wouldn't have "broken" any existing application.

[marko-bekhta]

Hmm from what I see 3.2 branch is using ORM 6.2.7.Final , which already results in this cryptic error message:

HibernateEntityEnhancerMissingEmbeddableAnnotationTest$EntityWithEmbedded$EmbeddableMissingAnnotation' can't be cast to CompositeTracker

	at org.hibernate.engine.internal.ManagedTypeHelper.asCompositeTracker(ManagedTypeHelper.java:337)
	at io.quarkus.hibernate.orm.enhancer.HibernateEntityEnhancerMissingEmbeddableAnnotationTest$EntityWithEmbedded$EmbeddableWithAnnotation.$$_hibernate_write_embeddableMissingAnnotation(HibernateEntityEnhancerMissingEmbeddableAnnotationTest.java)
	at io.quarkus.hibernate.orm.enhancer.HibernateEntityEnhancerMissingEmbeddableAnnotationTest$EntityWithEmbedded$EmbeddableWithAnnotation.setEmbeddableMissingAnnotation(HibernateEntityEnhancerMissingEmbeddableAnnotationTest.java:142)
	at io.quarkus.hibernate.orm.enhancer.HibernateEntityEnhancerMissingEmbeddableAnnotationTest.lambda$test$1(HibernateEntityEnhancerMissingEmbeddableAnnotationTest.java:52)

when the annotation is missing... so it seems it is safe to backport the build exception to 3.2. As for the warning, we could add it to the version that would actually allow the missing annotation...

[yrodiere]

which already results in this cryptic error message [...] when the annotation is missing... so it seems it is safe to backport the build exception to 3.2. As for the warning, we could add it to the version that would actually allow the missing annotation...

+1 makes sense

[lostiniceland]

Hmm from what I see 3.2 branch is using ORM 6.2.7.Final , which already results in this cryptic error message:

HibernateEntityEnhancerMissingEmbeddableAnnotationTest$EntityWithEmbedded$EmbeddableMissingAnnotation' can't be cast to CompositeTracker

	at org.hibernate.engine.internal.ManagedTypeHelper.asCompositeTracker(ManagedTypeHelper.java:337)
	at io.quarkus.hibernate.orm.enhancer.HibernateEntityEnhancerMissingEmbeddableAnnotationTest$EntityWithEmbedded$EmbeddableWithAnnotation.$$_hibernate_write_embeddableMissingAnnotation(HibernateEntityEnhancerMissingEmbeddableAnnotationTest.java)
	at io.quarkus.hibernate.orm.enhancer.HibernateEntityEnhancerMissingEmbeddableAnnotationTest$EntityWithEmbedded$EmbeddableWithAnnotation.setEmbeddableMissingAnnotation(HibernateEntityEnhancerMissingEmbeddableAnnotationTest.java:142)
	at io.quarkus.hibernate.orm.enhancer.HibernateEntityEnhancerMissingEmbeddableAnnotationTest.lambda$test$1(HibernateEntityEnhancerMissingEmbeddableAnnotationTest.java:52)

when the annotation is missing... so it seems it is safe to backport the build exception to 3.2. As for the warning, we could add it to the version that would actually allow the missing annotation...

Just to chime in here. The cryptic message appeared for us when moving from 2 to 3, and it took a couple of hours to figure out what was going on. So backporting this in the 3.x line will actually help 3.x users.

And thanks @marko-bekhta