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

Quarkus 3.4.x - Build failure: java.lang.Object is used as an embeddable but does not have an @Embeddable annotation #36421

Closed
jimbogithub opened this issue Oct 11, 2023 Discussed in #36368 · 16 comments · Fixed by #36713
Labels
area/hibernate-orm Hibernate ORM kind/bug Something isn't working
Milestone

Comments

@jimbogithub
Copy link

Discussed in #36368

Originally posted by jimbogithub October 10, 2023
I'm seeing this new build error with Quarkus 3.4.2 (was fine for 3.3.3 and prior):

java.lang.RuntimeException: io.quarkus.builder.BuildException: Build failure: Build failed due to errors
	[error]: Build step io.quarkus.hibernate.orm.deployment.HibernateOrmProcessor#defineJpaEntities threw an exception: java.lang.IllegalStateException: io.quarkus.builder.BuildException: Build failure: java.lang.Object is used as an embeddable but does not have an @Embeddable annotation.
	at io.quarkus.deployment.ExtensionLoader$3.execute(ExtensionLoader.java:867)
	at io.quarkus.builder.BuildContext.run(BuildContext.java:282)
	at org.jboss.threads.ContextHandler$1.runWith(ContextHandler.java:18)
	at org.jboss.threads.EnhancedQueueExecutor$Task.run(EnhancedQueueExecutor.java:2513)
	at org.jboss.threads.EnhancedQueueExecutor$ThreadBody.run(EnhancedQueueExecutor.java:1538)
	at java.base/java.lang.Thread.run(Thread.java:833)
	at org.jboss.threads.JBossThread.run(JBossThread.java:501)
Caused by: io.quarkus.builder.BuildException: Build failure: java.lang.Object is used as an embeddable but does not have an @Embeddable annotation.
	at io.quarkus.hibernate.orm.deployment.JpaJandexScavenger.collectEmbeddedType(JpaJandexScavenger.java:513)
	at io.quarkus.hibernate.orm.deployment.JpaJandexScavenger.enlistEmbeddedsAndElementCollections(JpaJandexScavenger.java:323)
	at io.quarkus.hibernate.orm.deployment.JpaJandexScavenger.discoverModelAndRegisterForReflection(JpaJandexScavenger.java:95)
	at io.quarkus.hibernate.orm.deployment.HibernateOrmProcessor.defineJpaEntities(HibernateOrmProcessor.java:409)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at io.quarkus.deployment.ExtensionLoader$3.execute(ExtensionLoader.java:858)

I can't find any explicit use of Object as an embedded type in our JPA models.
Is it possible that #35598 / #35822 has caused this?

Looks like the new @Embeddable checks are being polluted by abstract super-structure and causing false positives.

@jimbogithub
Copy link
Author

Reproducer: jpa.zip

Note the Identifiable interface implemented by MyEntity. However it is clear that Object can never be embedded.

@jimbogithub
Copy link
Author

@gsmet Is it possible to get this on someones radar. It's a showstopper for us. Suspect it's a simple fix and a reproducer is already attached. Thanks.

@quarkus-bot
Copy link

quarkus-bot bot commented Oct 26, 2023

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

@yrodiere yrodiere added the kind/bug Something isn't working label Oct 26, 2023
@yrodiere
Copy link
Member

Confirmed, affects 3.5.0 as well. I'm having a look.

@gsmet
Copy link
Member

gsmet commented Oct 26, 2023

Also, while at it could we improve the error message to give more context?

Because with java.lang.Object is used as an embeddable but does not have an @Embeddable annotation, it's hard to know where the problem is.

@yrodiere
Copy link
Member

yrodiere commented Oct 26, 2023

So it looks like Jandex is actually returning the method definition in the superclass instead of the one in the subclass... I'll try to work around that but it's weird.

Hey, @Ladicek is it normal that with the following model, when I do index.getAnnotations(DotName.createSimple("jakarta.persistence.EmbeddedId")), the returned AnnotationInstance targets the method from Identifiable instead of the method from MyEntity (and thus has the wrong return type)?

    @MappedSuperclass
    public static interface Identifiable {
        Object getId();
    }

    @Entity
    public static class MyEntity implements Identifiable {
        private MyEmbeddable id;

        @Override
        @EmbeddedId
        public MyEmbeddable getId() {
            return id;
        }

        public void setId(MyEmbeddable id) {
            this.id = id;
        }
    }

    @Embeddable
    public static class MyEmbeddable {
        private String text;

        protected MyEmbeddable() {
            // For Hibernate ORM only - it will change the property value through reflection
        }

        public MyEmbeddable(String text) {
            this.text = text;
        }

        public String getText() {
            return text;
        }
    }

@yrodiere
Copy link
Member

yrodiere commented Oct 26, 2023

Hey, @Ladicek is it normal that with the following model, when I do index.getAnnotations(DotName.createSimple("jakarta.persistence.EmbeddedId")), the returned AnnotationInstance targets the method from Identifiable instead of the method from MyEntity (and thus has the wrong return type)?

So actually the returned AnnotationInstance for MyEntity#getId is there and works fine, it's just that we have an extra AnnotationInstance for Identifiable#getId... which seems wrong since getId in Identifiable is not annotated with @EmbeddedId.

@yrodiere
Copy link
Member

Yeah so it really looks like a bug in Jandex, if I look into the content of the MethodInfo for Identifiable#getId it doesn't reflect (ha!) the actual code:

image
image
image

Weirdly, it looks like Identifiable#getId is considered as being defined in EntityWithEmbeddedIdAndOverriddenGetter... See the duplicate getId methods. I mean I suppose this can make sense depending how you look at it, but considering that Identifiable#getId is annotated with @EmbeddedId... not so much.

@Ladicek Please let me know whether I'm mistaken, whether there is a workaround, and whether I should submit a bug report with a reproducer to Jandex itself.

In the meantime, this can be reproduced and debugged by checking out this branch: https://github.com/yrodiere/quarkus/tree/i36421
Then running mvn clean install -pl extensions/hibernate-orm/deployment -Dtest=HibernateEntityEnhancerPresentEmbeddableTest -Dmaven.surefire.debug
Then connecting with an IDE with a breakpoint on io/quarkus/hibernate/orm/deployment/JpaJandexScavenger.java:513

@Ladicek
Copy link
Contributor

Ladicek commented Oct 26, 2023

I suppose one of the getId() methods is actually a bridge method. You should probably ignore those.

@Ladicek
Copy link
Contributor

Ladicek commented Oct 26, 2023

Looking at the code above,

    @MappedSuperclass
    public static interface Identifiable {
        Object getId();
    }

    @Entity
    public static class MyEntity implements Identifiable {
        private MyEmbeddable id;

        @Override
        @EmbeddedId
        public MyEmbeddable getId() {
            return id;
        }

        public void setId(MyEmbeddable id) {
            this.id = id;
        }
    }

    @Embeddable
    public static class MyEmbeddable {
        private String text;

        protected MyEmbeddable() {
            // For Hibernate ORM only - it will change the property value through reflection
        }

        public MyEmbeddable(String text) {
            this.text = text;
        }

        public String getText() {
            return text;
        }
    }

you will indeed see 2 getId() methods in the bytecode of MyEntity. One will have a return type of MyEmbeddable -- that's the one you want. The other will have a return type of Object and will be a bridge method (due to covariant change of return type, because MyEntity is a subtype of Identifiable). You can identify that by MethodInfo.isBridge() (or MethodInfo.isSynthetic() if you want to filter out all synthetic methods, not just bridges).

@yrodiere
Copy link
Member

you will indeed see 2 getId() methods in the bytecode of MyEntity. One will have a return type of MyEmbeddable -- that's the one you want. The other will have a return type of Object and will be a bridge method (due to covariant change of return type, because MyEntity is a subtype of Identifiable). You can identify that by MethodInfo.isBridge() (or MethodInfo.isSynthetic() if you want to filter out all synthetic methods, not just bridges).

Understood, will do. I confirm this is a bridge method. Thanks a lot!

Is it expected that the bridge method is considered annotated with @EmbeddedId? Or, at least, that bridge methods are returned by index.getAnnotations? I wonder how many uses of getAnnotations in Quarkus should ignore bridge methods, but don't... I found only two uses of MethodInfo#isBridge() in the whole codebase, which sounds very low...

@Ladicek
Copy link
Contributor

Ladicek commented Oct 26, 2023

Also, if you're interested why the bridge method has the annotation, that's what javac does. Here's a simple test I made:

$ cat A.java 
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;

@Retention(RetentionPolicy.RUNTIME)
@interface MyAnn {
}

interface Identifiable {
    Object getId();
}

class MyId {
}

class MyEntity implements Identifiable {
    @MyAnn
    @Override
    public MyId getId() { return null; }
}

public class A {
}

$ javac A.java
$ javap -v -p MyEntity.class
  ...
  public MyId getId();
    descriptor: ()LMyId;
    flags: (0x0001) ACC_PUBLIC
    Code:
      stack=1, locals=1, args_size=1
         0: aconst_null
         1: areturn
      LineNumberTable:
        line 18: 0
    RuntimeVisibleAnnotations:
      0: #13()
        MyAnn

  public java.lang.Object getId();
    descriptor: ()Ljava/lang/Object;
    flags: (0x1041) ACC_PUBLIC, ACC_BRIDGE, ACC_SYNTHETIC
    Code:
      stack=1, locals=1, args_size=1
         0: aload_0
         1: invokevirtual #2                  // Method getId:()LMyId;
         4: areturn
      LineNumberTable:
        line 15: 0
    RuntimeVisibleAnnotations:
      0: #13()
        MyAnn
  ...

You see that even the bridge method has the annotation. Jandex is just being faithful here.

@Ladicek
Copy link
Contributor

Ladicek commented Oct 26, 2023

Is it expected that the bridge method is considered annotated with @EmbeddedId?

Yes, see above.

Or, at least, that bridge methods are returned by index.getAnnotations?

IndexView.getAnnotations() returns all occurences of the annotation, so I'd say yes.

I wonder how many uses of getAnnotations in Quarkus should ignore bridge methods, but don't... I found only two uses of MethodInfo#isBridge() in the whole codebase, which sounds very low...

That is a great question. It is my opinion that Jandex, being a faithful representation of the bytecode, makes for a poor language model, due to issues like this (which are often ignored, because things work most of the time). On and off, I was thinking if we should introduce an actual language model in Jandex that would shield users from these details. We don't have anything like that yet, I'm afraid.

@yrodiere
Copy link
Member

On and off, I was thinking if we should introduce an actual language model in Jandex that would shield users from these details. We don't have anything like that yet, I'm afraid.

Agreed. FWIW @sebersole is currently working on a "domain model" layer (well, multiple layers, because of me 😅) that would sit between Jandex and Hibernate ORM. It's probably not exactly what you have in mind, since it's rather "javabean" oriented (with a concept of attributes like in JPA), but it's close. And would be a library separate from Hibernate ORM, at least for the part we're interested in here.

@Ladicek
Copy link
Contributor

Ladicek commented Oct 26, 2023

Understood. On my side, I was thinking I would add an implementation of the CDI Language Model (which is in fact independent of CDI) as a separate module of Jandex, and it would also include the annotation transformation layer. That should be good enough for a vast majority of users in Quarkus, but Jandex would still be available. In case anyone is interested, the API is in https://github.com/jakartaee/cdi/tree/main/lang-model/src/main/java/jakarta/enterprise/lang/model

@yrodiere
Copy link
Member

Ignoring bride methods does fix the problem, thanks @Ladicek ! I wouldn't have thought of this.

Created #36713

@quarkus-bot quarkus-bot bot added this to the 3.6 - main milestone Oct 26, 2023
@gsmet gsmet modified the milestones: 3.6 - main, 3.5.1 Oct 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/hibernate-orm Hibernate ORM kind/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants