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

TCK error - OASConfigExcludeClassesTest$testExcludedClasses requires incorrect OpenAPI document to pass #498

Closed
tjquinno opened this issue Nov 4, 2021 · 7 comments · Fixed by #510
Assignees
Labels

Comments

@tjquinno
Copy link

tjquinno commented Nov 4, 2021

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:

if both Application.getClasses and Application.getSingletons return an empty collection then all root resource classes and providers packaged in the web application MUST be included

It continues (importantly):

If either getClasses or getSingletons returns a non-empty collection then only those classes or singletons returned MUST be included in the published JAX-RS application.

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 from JAXRSApp$getSingletons--ReviewResource and AvailabilityResource--are excluded by the mp.openapi.scan.exclude.classes config setting used in the test. For the document to contain 10 path elements it must include the endpoints not only from AirlinesResource and BookingResource but also those from UserResource which is not represented in the return values from JAXRSApp$getResources or getSingletons. Including the UserResource endpoints would be expected if JAXRSApp overrode neither getClasses nor getSingletons but it does override getSingletons.

This test seems flawed in that:

  1. It requires the server to produce an OpenAPI document which conflicts with the behavior described in the JAX-RS spec.
  2. For servers which follow the semantics as described in the JAX-RS spec, to pass the TCK they must, in some cases, produce OpenAPI documents which conflict with the endpoints they actually expose.

There might be other tests with similar behavior.

@MikeEdgar
Copy link
Member

@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.

@tjquinno
Copy link
Author

tjquinno commented Nov 7, 2021

@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.

@tjquinno
Copy link
Author

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.

@MikeEdgar
Copy link
Member

@Emily-Jiang - is there an opportunity to make an additional TCK change before 3.0?

@Emily-Jiang
Copy link
Member

@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.

@Emily-Jiang
Copy link
Member

@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.

@MikeEdgar MikeEdgar added this to the MP OpenAPI 3.1 milestone Feb 6, 2022
@Azquelt Azquelt self-assigned this Feb 22, 2022
@Azquelt
Copy link
Member

Azquelt commented Feb 22, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants