eclipse / microprofile-open-api

Microprofile open api
Apache License 2.0
131 stars 81 forks source link

Extensions propagation in APIResponse(s) #556

Closed aubi closed 11 months ago

aubi commented 1 year ago

TCK tests requires, that annotation Extension is propagated down to APIResponses.APIResponse.extensions, but there is no reason for this.

TCK contains this piece of code in tck/src/main/java/org/eclipse/microprofile/openapi/apps/petstore/resource/PetStoreResource.java:

    @GET
    @Path("/inventory")
    @Produces({"application/json", "application/xml"})
    @APIResponses(extensions = @Extension(name = "x-responses-ext", value = "test-responses-ext"), value = {
            @APIResponse(responseCode = "200", description = "successful operation",
                         extensions = @Extension(name = "x-response-ext", value = "test-response-ext")),
            @APIResponse(responseCode = "500", description = "server error", extensions = {}),
            @APIResponse(responseCode = "503", description = "service not available",
                         content = @Content(extensions = @Extension(name = "x-notavailable-ext", value = "true"))),
    })
    @Operation(summary = "Returns pet inventories by status",
               description = "Returns a map of status codes to quantities")
    @Extension(name = "x-operation-ext", value = "test-operation-ext")
    public java.util.Map<String, Integer> getInventory() {

One of the tests checks, if the annotation @Extension(name = "x-operation-ext", value = "test-operation-ext") was propagated down to the "503" response in PetStoreAppTest.java:

vr.body(opPath + ".responses.'503'", hasEntry(equalTo(X_OPERATION_EXT), equalTo(TEST_OPERATION_EXT)));

This seems strange to me -- why @Extension on method level should be propagated to @APIResponses?

I can imagine, that x-responses-ext, defined on APIResponses is propagated to all responses, which don't specify otherwise (only 503 in this case), but in fact I didn't see any support for this behavior in the OpenAPI specification; the javadoc warns, that this usage can lead to wrong behavior, but IMHO doesn't say anything about propagation.

MikeEdgar commented 1 year ago

Hi @aubi . I think the idea is that when using @Extension on a language element like a method, it is ambiguous which API elements it should be applied to. Your point about extensions on @APIResponses seems valid. Perhaps it's worth discussing whether that being present should block the propagation of the method-level @Extension to the nested @APIResponses.

Also note that support for using @Extension outside of another annotation may be removed [1] due to the inherent ambiguity.

[1] https://github.com/eclipse/microprofile-open-api/blob/8c8adaad12a7dd4cdd0bd12bb3967e60d62db98f/api/src/main/java/org/eclipse/microprofile/openapi/annotations/extensions/Extension.java#L27-L39

aubi commented 1 year ago

Should I create a PR, which will remove this one test?

MikeEdgar commented 1 year ago

I would lean toward having the extensions property on @APIResponses apply to the 503 response rather than the @Extension from the method in this case. However, I do still think it's valid for the value to be propagated as long as @Extension is allowed on language elements and is not embedded in another annotation.

@Azquelt , what is your thinking on this one?

Azquelt commented 1 year ago

The extensions property on @APIResponses should apply to the Responses object itself - which is correct in that test. It should not apply to any of the actual responses.

Personally, I would have expected @Extension on the method level to apply only to the operation and not to any of the responses.

I think the test is wrong here. As far as I can see, the spec is very vague and does not explicitly state what an @Extension on a method should apply to. While it might be valid to cascade it down so that it applies to all responses, the spec doesn't seem to require that and so the TCK shouldn't test it.

Azquelt commented 1 year ago

We discussed this a little on the call today and we couldn't actually come up with a use case for propagating the extension down to any child entries.

@MikeEdgar Are you aware of any use cases where you would want to cascade an extension declaration from an operation down to its children like this?

Extensions are user defined, either they're purely cosmetic or there must be some tooling which is looking for this extension for it to have any effect. That being the case, if some sort of cascading behaviour was needed, I think it would make more sense for that tooling to look up the tree to see if a parent element declared an extension, rather than for us to automatically apply an extension declaration to all children.

MikeEdgar commented 1 year ago

Sorry for the delay. I think the only case for cascading an extension is to support the "legacy" @Extension annotation that was only allowed on a language element and not nested in other annotations. That's not what the TCK is testing with the 503 response and I agree the test is not correct in that regard.