Closed tjquinno closed 2 years ago
@tjquinno - you make some good points. I think the question is how can it be solved? For this particular case, likely UserResource
could be added to the collection returned by getSingletons
.
More generally, support for multiple applications is a bit of a challenge. Requiring implementations to invoke application methods during annotation scanning to get the included list of providers introduces the possibility of side effects during scanning. Because of that, I wonder if introducing some new annotation would help with this. For example, a class that is used for multiple applications might be annotated like this:
@APIResource(applications = { Application1.class, Application2.class })
@Path("common")
public CommonResource {
...
}
Of course this requires the user to repeat their configuration, but due to the side effect issue, I'm not sure there is any other way to avoid that.
@MikeEdgar I completely agree that a totally clean solution to this might elude us.
First, the TCK should not force servers to create inconsistent (i.e., wrong) OpenAPI documents with respect to the endpoints they actually implement. The change in the test app's getSingletons
method you mentioned would resolve that problem. There might be tests in the TCK other than the one I mentioned which need the same kind of change.
A related question... I know balloting is in progress (or about to start) for 3.0 and that this set of spec releases is geared
mostly to the javax
-> jakarta
namespace change. That said, 3.0 already contains some TCK changes and it would be nice if 3.0 could start out on the right foot without asking servers to (continue to) publish flawed OpenAPI documents.
Longer-term, adding a new annotation might be the best approach for the reasons you described. The annotation you suggested asks the developer of the resource to know and declare all the applications the resource is associated with. That might not always be practical, esp. if a resource is reused in a variety of situations that cannot be anticipated.
Another approach would be for the developer of the application to declare via an annotation what resources it encompasses, something like a build-time analog of the runtime getSingletons
and getClasses
methods.
@APIResource(AirlinesResource.class)
@APIResource(AvailabilityResource.class)
@APIResource(BookingResource.class)
@APIResource(ReviewResource.class)
@APIResource(UserResource.class)
@ApplicationPath("/")
public class JAXRSApp extends Application {
...
}
and
@APIResource(PetResource.class)
@APIResource(PetStoreResource.class)
@APIResource(UserResource.class)
@ApplicationPath("/")
public class PetStoreApp extends Application {
...
}
(I'm less concerned at this point with the exact syntax and style of the annotation and more with conveying the key information in a logical way. For example, maybe @APIApplication(resources = {X.class, Y.class})
is an alternative.)
My main point now is that annotating the application class (rather than the resource class) might be more natural for developers, given that at least loosely speaking an Application
"contains" its resources (certainly as far as the app's path being a prefix for all the app's resources' paths).
In fact, the examples above are from the TCK tests themselves. UserResource
being common to both applications highlights that the author of a resource might not always be able to predict what apps will use it.
What do people think about a short-term fix in the 3.0 release?
As @MikeEdgar described, adding UserResource.class
to the return value from getSingletons
would address the immediate concern.
@Emily-Jiang - is there an opportunity to make an additional TCK change before 3.0?
@MikeEdgar I am afraid it is too late. It can be done after the release. We can do a service release straightaway after MP 5.0. Please don't push the changes until the final release is staged.
@tjquinno the service release 3.0.1 can be done immediately after MP 5.0 without any voting involved. it can be done on the same day even. You can use that TCK jar to certify. The other alternative is to treat this as a challenge and you can exclude this test from your TCK runs.
There are other tests which explicitly look for /user
paths (e.g. AirlinesAppTest.testOperationUserResource
), so I've opened #510 to add the UserResource
class to the set of singletons.
TL;DR
To pass this TCK test (and perhaps others), the server must generate an OpenAPI document that conflicts with the conventional behavior as described in the JAX-RS spec and as implemented by at least some MicroProfile-compliant servers.
Specifically, the test requires that the OpenAPI document declare endpoints that the server does not, in fact, expose. This is a problem in that the TCK requires servers to produce incorrect OpenAPI documents.
Details
In the TCK test in the issue title,
JAXRSApp$getSingletons
returns four JAX-RS resource class instances:AirlinesResource
AvailabilityResource
BookingResource
ReviewResource
The class does not override
getClasses
so the superclass implementation returns the empty set.The JAX-RS spec https://jakarta.ee/specifications/restful-ws/3.0/jakarta-restful-ws-spec-3.0.html#servlet describes how servlet implementations must handle this:
It continues (importantly):
Of course, MicroProfile neither refers to nor implements servlets, but it is very reasonable for our users to expect MP-compliant servers to honor the JAX-RS semantics described there. Two server implementations I checked (OpenLiberty and Helidon) do so and I suspect so do others.
In contrast, the subject TCK test as written requires 10
path
elements in the returned OpenAPI document. Two of the resources returned fromJAXRSApp$getSingletons
--ReviewResource
andAvailabilityResource
--are excluded by themp.openapi.scan.exclude.classes
config setting used in the test. For the document to contain 10path
elements it must include the endpoints not only fromAirlinesResource
andBookingResource
but also those fromUserResource
which is not represented in the return values fromJAXRSApp$getResources
orgetSingletons
. Including theUserResource
endpoints would be expected ifJAXRSApp
overrode neithergetClasses
norgetSingletons
but it does overridegetSingletons
.This test seems flawed in that:
There might be other tests with similar behavior.