-
Notifications
You must be signed in to change notification settings - Fork 3k
Allow for static JAX-RS resource methods #46908
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
base: main
Are you sure you want to change the base?
Allow for static JAX-RS resource methods #46908
Conversation
This comment has been minimized.
This comment has been minimized.
🎊 PR Preview a721231 has been successfully built and deployed to https://quarkus-pr-main-46908-preview.surge.sh/version/main/guides/
|
Interesting idea! |
This comment has been minimized.
This comment has been minimized.
7feff3c
to
d31bed1
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
2630a49
to
3cb5822
Compare
This comment has been minimized.
This comment has been minimized.
3cb5822
to
55018ad
Compare
This comment has been minimized.
This comment has been minimized.
I have no idea why the new test is failing, it passes for me locally, and testing out a sample app also works. |
98bdb85
to
2fa493c
Compare
This comment has been minimized.
This comment has been minimized.
2fa493c
to
534aa34
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
7fc5870
to
38af752
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@stuartwdouglas I'm off today so I'll give it a shot first thing tomorrow |
Lemme try. This is a nice idea, for sure. |
The test passes locally here :-/ |
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.
The code looks good though, let's check what the test logs say…
Quarkus also extends the JAX-RS spec to allow you to use static methods. If the static method is on a class annotated with | ||
`@Path`, it will be treated the same as a non-static endpoint. If there is no `@Path` annotation on the class then the path | ||
is assumed to be a root path. This is especially useful for Kotlin as it allows you to define endpoints without creating a class. | ||
|
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'm not sure if we should add a Kotlin code sample here, but it sounds like it would be a good place to add one.
String path = scannedResourcePaths.get(classInfo.name()); | ||
ResourceClass clazz = new ResourceClass(); | ||
if ((classInfo.enclosingClass() != null) && !Modifier.isStatic(classInfo.flags())) { | ||
if ((classInfo.enclosingClass() != null) && !Modifier.isStatic(classInfo.flags()) && path != null) { |
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.
Why would the presence of a path
change the outcome of this test?
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.
It's the difference between a static method on a class that is already considered a resource, vs allowing static methods on classes that are otherwise not considered JAX-RS resource (but instead locatable subresources).
It's the |
|
Let's see what CI says with my change. Note that I'm detecting this feature bcause there's a static method annotated with public class FooNoClassPathStaticMethod {
@GET
public static String hello() {
return "noclass";
}
} As in, no I did not add support for that. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Didn't help :( |
continue; | ||
} | ||
URITemplate classTemplate = new URITemplate(clazz.getPath(), true); | ||
List<List<ResourceClass>> rcMap = List.of(locatableResourceClasses, resourceClasses); |
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.
This logic is what picks up the static methods on locatable subresources. Locally it seems to work, both when I run the test and if I create an example app. I'm not sure what could be different about CI that causes it to fail.
8b8ce6e
to
a0aa662
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
a0aa662
to
172c52d
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
172c52d
to
90221df
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This is really useful in kotlin as it allows you to define methods that are not inside a class.
90221df
to
d40f170
Compare
Status for workflow
|
Status for workflow
|
Status | Name | Step | Failures | Logs | Raw logs | Build scan |
---|---|---|---|---|---|---|
✖ | JVM Tests - JDK 17 | Build |
Failures | Logs | Raw logs | 🔍 |
✖ | JVM Tests - JDK 21 | Build |
Failures | Logs | Raw logs | 🔍 |
✖ | JVM Tests - JDK 17 Windows | Build |
Failures | Logs | Raw logs | 🔍 |
Full information is available in the Build summary check run.
You can consult the Develocity build scans.
Failures
⚙️ JVM Tests - JDK 17 #
- Failing: extensions/resteasy-reactive/rest/deployment
! Skipped: extensions/avro/deployment extensions/grpc/deployment extensions/hibernate-reactive/deployment and 62 more
📦 extensions/resteasy-reactive/rest/deployment
✖ io.quarkus.resteasy.reactive.server.test.StaticMethodTest.testStaticMethod
line 38
- History - More details - Source on GitHub
java.lang.AssertionError:
1 expectation failed.
Expected status code <200> but was <404>.
at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:77)
at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:500)
⚙️ JVM Tests - JDK 21 #
- Failing: extensions/resteasy-reactive/rest/deployment
! Skipped: extensions/avro/deployment extensions/grpc/deployment extensions/hibernate-reactive/deployment and 62 more
📦 extensions/resteasy-reactive/rest/deployment
✖ io.quarkus.resteasy.reactive.server.test.StaticMethodTest.testStaticMethod
line 38
- History - More details - Source on GitHub
java.lang.AssertionError:
1 expectation failed.
Expected status code <200> but was <404>.
at java.base/jdk.internal.reflect.DirectConstructorHandleAccessor.newInstance(DirectConstructorHandleAccessor.java:62)
at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:502)
at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:486)
at org.codehaus.groovy.reflection.CachedConstructor.invoke(CachedConstructor.java:73)
⚙️ JVM Tests - JDK 17 Windows #
- Failing: extensions/resteasy-reactive/rest/deployment
! Skipped: extensions/avro/deployment extensions/grpc/deployment extensions/hibernate-reactive/deployment and 62 more
📦 extensions/resteasy-reactive/rest/deployment
✖ io.quarkus.resteasy.reactive.server.test.StaticMethodTest.testStaticMethod
line 38
- History - More details - Source on GitHub
java.lang.AssertionError:
1 expectation failed.
Expected status code <200> but was <404>.
at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:77)
at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:500)
Flaky tests - Develocity
⚙️ JVM Integration Tests - JDK 21
📦 integration-tests/opentelemetry-grpc-only
✖ io.quarkus.it.opentelemetry.grpc.HelloGrpcClientTest.testHello
- History
java.lang.RuntimeException: Failed to start quarkus
-java.lang.RuntimeException
java.lang.RuntimeException: java.lang.RuntimeException: Failed to start quarkus
at io.quarkus.test.junit.QuarkusTestExtension.throwBootFailureException(QuarkusTestExtension.java:693)
at io.quarkus.test.junit.QuarkusTestExtension.interceptTestClassConstructor(QuarkusTestExtension.java:791)
at java.base/java.util.Optional.orElseGet(Optional.java:364)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
Caused by: java.lang.RuntimeException: Failed to start quarkus
at io.quarkus.runner.ApplicationImpl.doStart(Unknown Source)
It sucks that this has been blocked so long, but I'm not sure what more I can do to figure out why this fails in CI :-/ |
FWIW, I can reproduce the problem locally. There is an initial problem that because of @Path("hello")
public static class StaticMethod {
@GET
public static String hello() {
return "hello";
}
} then the new feature still doesn't seem to work, but this time because there is no matcher, see screenshot: |
This is really useful in kotlin as it allows you to define methods that are not inside a class.
e.g. this allows you to write the Kotlin quickstart as: