-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
GraphQL - make sure the panache entities are updated in the index #19883
GraphQL - make sure the panache entities are updated in the index #19883
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM,
What about adding ITs with a Panache entity and a Reactive Panache entity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added one big question and one small one.
Also a test case would be nice.
...eployment/src/main/java/io/quarkus/smallrye/graphql/deployment/SmallRyeGraphQLProcessor.java
Outdated
Show resolved
Hide resolved
...eployment/src/main/java/io/quarkus/smallrye/graphql/deployment/SmallRyeGraphQLProcessor.java
Show resolved
Hide resolved
@cescoffier - I added an integration test |
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building c63e719
Full information is available in the Build summary check run. Failures⚙️ JVM Tests - JDK 16 #- Failing: integration-tests/vault-agroal
📦 integration-tests/vault-agroal✖
✖
✖
|
Signed-off-by: Phillip Kruger <[email protected]>
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building 366d49b
Full information is available in the Build summary check run. Failures⚙️ JVM Tests - JDK 11 #- Failing: integration-tests/hibernate-orm-tenancy/connection-resolver integration-tests/mongodb-panache
📦 integration-tests/hibernate-orm-tenancy/connection-resolver✖
✖
✖
📦 integration-tests/mongodb-panache✖
⚙️ JVM Tests - JDK 16 #- Failing: integration-tests/hibernate-orm-tenancy/connection-resolver integration-tests/mongodb-panache
📦 integration-tests/hibernate-orm-tenancy/connection-resolver✖
✖
✖
📦 integration-tests/mongodb-panache✖
⚙️ Native Tests - Data1 #- Failing: integration-tests/hibernate-orm-tenancy/connection-resolver
📦 integration-tests/hibernate-orm-tenancy/connection-resolver✖
✖
✖
|
Failing Jobs - Building 366d49b
Full information is available in the Build summary check run. Failures⚙️ JVM Tests - JDK 11 #- Failing: integration-tests/hibernate-orm-tenancy/connection-resolver integration-tests/mongodb-panache
📦 integration-tests/hibernate-orm-tenancy/connection-resolver✖
✖
✖
📦 integration-tests/mongodb-panache✖
⚙️ JVM Tests - JDK 16 #- Failing: integration-tests/hibernate-orm-tenancy/connection-resolver integration-tests/mongodb-panache
📦 integration-tests/hibernate-orm-tenancy/connection-resolver✖
✖
✖
📦 integration-tests/mongodb-panache✖
⚙️ Native Tests - Data1 #- Failing: integration-tests/hibernate-orm-tenancy/connection-resolver
📦 integration-tests/hibernate-orm-tenancy/connection-resolver✖
✖
✖
|
@gsmet - the test failures is unrelated, and running the ci again, still fails. Is it ok to merge ? |
@geoand - I think gsmet is on PTO, maybe you can help. The build failures is not related, and running the ci again does not fix it. Can I merge ? Or should I rebase ? |
I'm not :). I confirm the failures are unrelated and has been fixed. Also they did not cause anything to be skipped so it should be safe enough to merge this. I haven't looked at what it does, though :) |
Great ! Thanks @gsmet . This "updates" the Jandex index for GraphQL (only) so that the updated panache classes is in the index, and when used with GraphQL actually works. I was reviewed by @Ladicek and parts of this PR might move to Jandex, and then we will update this again. For how GraphQL use Jandex at the moment, this is safe, but might need more testing for other cases once it moved to Jandex. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@phillip-kruger after upgrading from 2.2.3.Final to 2.3.0.Final I keep getting the following stack trace while tests are trying to start in dev-mode (mvn quarkus:dev). Building and running outside dev-mode works just fine so I first spotted this during hot-reload and tests tried to start. Since 2.3.0.Final is not released yet I maybe should wait reporting this but maybe this is a known issue? Downgrading to 2.2.3.Final everything works fine. Looks related to the new classes added here (OverridabeIndex.java). I can try to create a small reproducer if needed.
java.lang.RuntimeException: java.lang.RuntimeException: io.quarkus.builder.BuildException: Build failure: Build failed due to errors
[error]: Build step io.quarkus.smallrye.graphql.deployment.SmallRyeGraphQLProcessor#buildExecutionService threw an exception: java.lang.NullPointerException: Cannot invoke "String.equals(Object)" because the return value of "org.jboss.jandex.MethodParameterInfo.name()" is null
at io.quarkus.smallrye.graphql.deployment.OverridableIndex$7.compare(OverridableIndex.java:160)
at io.quarkus.smallrye.graphql.deployment.OverridableIndex$7.compare(OverridableIndex.java:155)
at io.quarkus.smallrye.graphql.deployment.OverridableIndex$8.compare(OverridableIndex.java:199)
at io.quarkus.smallrye.graphql.deployment.OverridableIndex$8.compare(OverridableIndex.java:168)
at java.base/java.util.TreeMap.compare(TreeMap.java:1570)
at java.base/java.util.TreeMap.addEntryToEmptyMap(TreeMap.java:776)
at java.base/java.util.TreeMap.put(TreeMap.java:785)
at java.base/java.util.TreeMap.put(TreeMap.java:534)
at java.base/java.util.TreeSet.add(TreeSet.java:255)
at java.base/java.util.AbstractCollection.addAll(AbstractCollection.java:336)
at java.base/java.util.TreeSet.addAll(TreeSet.java:309)
at io.quarkus.smallrye.graphql.deployment.OverridableIndex.overrideCollection(OverridableIndex.java:239)
at io.quarkus.smallrye.graphql.deployment.OverridableIndex.getAnnotations(OverridableIndex.java:69)
at io.smallrye.graphql.schema.helper.SourceOperationHelper.scanAllSourceAnnotations(SourceOperationHelper.java:50)
at io.smallrye.graphql.schema.helper.SourceOperationHelper.(SourceOperationHelper.java:29)
at io.smallrye.graphql.schema.creator.type.AbstractCreator.addOperations(AbstractCreator.java:115)
at io.smallrye.graphql.schema.creator.type.AbstractCreator.create(AbstractCreator.java:76)
at io.smallrye.graphql.schema.creator.type.TypeCreator.create(TypeCreator.java:34)
at io.smallrye.graphql.schema.creator.type.AbstractCreator.create(AbstractCreator.java:32)
at io.smallrye.graphql.schema.SchemaBuilder.createAndAddToSchema(SchemaBuilder.java:219)
at io.smallrye.graphql.schema.SchemaBuilder.addTypesToSchema(SchemaBuilder.java:153)
at io.smallrye.graphql.schema.SchemaBuilder.generateSchema(SchemaBuilder.java:121)
at io.smallrye.graphql.schema.SchemaBuilder.build(SchemaBuilder.java:86)
at io.quarkus.smallrye.graphql.deployment.SmallRyeGraphQLProcessor.buildExecutionService(SmallRyeGraphQLProcessor.java:207)
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$2.execute(ExtensionLoader.java:820)
at io.quarkus.builder.BuildContext.run(BuildContext.java:277)
at org.jboss.threads.ContextHandler$1.runWith(ContextHandler.java:18)
at org.jboss.threads.EnhancedQueueExecutor$Task.run(EnhancedQueueExecutor.java:2449)
at org.jboss.threads.EnhancedQueueExecutor$ThreadBody.run(EnhancedQueueExecutor.java:1478)
at java.base/java.lang.Thread.run(Thread.java:833)
at org.jboss.threads.JBossThread.run(JBossThread.java:501)
Hi @enbohm - Do you have a small reproducer ? |
@phillip-kruger Only have production code but I'll try to create a reproducer today or tomorrow. Will keep you posted |
Great ! Thanks :) |
@phillip-kruger I can now reproduce this issue but it might be me bad; when specifying the maven-compiler-plugin v.3.8.1 in pom.xml it seems to be working (see below). Without explicit setting the version, which I hadn't, my default maven-compiler-plugin version was 3.1 which seems to cause the above stacktrace. So apparently I had a misconfiguration but not sure if it is documented at quarkus.io that this version is required? Anyway since it is working now I guess there is no point creating an issue for this. <plugin>
<artifactId>maven-compiler-plugin</artifactId>
<version>3.8.1</version>
</plugin> |
Great ! Thanks |
This PR updates the index used to build the GraphQL schema with the modified panache classes, so that panache and GraphQL can work together.
Signed-off-by:Phillip Kruger [email protected]