-
Notifications
You must be signed in to change notification settings - Fork 81
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
TCK error - OASConfigExcludeClassesTest$testExcludedClasses requires incorrect OpenAPI document to pass #498
Comments
@tjquinno - you make some good points. I think the question is how can it be solved? For this particular case, likely 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 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 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 @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 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 In fact, the examples above are from the TCK tests themselves. |
What do people think about a short-term fix in the 3.0 release? As @MikeEdgar described, adding |
@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 |
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.
The text was updated successfully, but these errors were encountered: