jdaugherty commented on PR #16275: URL: https://github.com/apache/grails-core/pull/16275#issuecomment-5874072558
Re-reviewed at `4065ea5372d6b46916e165db83246e5eb2f4c100`, including the five OpenAPI fixes since my last review at `6195462424`. The exact scalar cases from that review are fixed. I also agree with the revised resource-action design: keeping the conventional defaults for an override that only adds annotations is reasonable, the way to withdraw paging is documented and tested, and `Location` now follows the inherited implementation. I would not reopen that design question. I found three narrower annotation combinations that still produce incorrect documents, and reproduced them through the generator and its JSON serializer. Could we work through these before merging? The complete reproducer is included below so we can use the same cases. **1. [P2] An annotation-nullable reference still cannot describe the values it permits.** [GrailsModelConverter.groovy:699-709](https://github.com/apache/grails-core/blob/4065ea5372d6b46916e165db83246e5eb2f4c100/grails-openapi/src/main/groovy/org/grails/openapi/GrailsModelConverter.groovy#L699-L709) wraps a nullable reference only when the constraint permits null. With a `Validateable` declaring no constraints: ```groovy class ReviewContainer implements Validateable { @Schema(nullable = true) ReviewAddress address } ``` `address` now correctly stays out of `required`, but its written 3.1 schema is: ```yaml type: "null" $ref: "#/components/schemas/ReviewAddress" ``` Those requirements intersect rather than form a union: neither an address object nor null satisfies both. The written 3.0 property is just the bare `$ref`, with no nullability. This is a remaining gap in annotation handling, not a claim that the new required-property fix introduced it. Could annotation-declared nullability also take the reference-handling path, preserving the referenced object as well as null? **2. [P2] Correcting a 3.1 property's type leaves numeric annotation values as strings.** [GrailsModelConverter.groovy:584-598](https://github.com/apache/grails-core/blob/4065ea5372d6b46916e165db83246e5eb2f4c100/grails-openapi/src/main/groovy/org/grails/openapi/GrailsModelConverter.groovy#L584-L598) corrects `types` after swagger-core has processed the annotation. This property: ```groovy @Schema(type = 'integer', format = 'int32', allowableValues = ['1', '2'], defaultValue = '1') Integer level ``` is written in 3.1 as: ```json {"type":"integer","format":"int32","default":"1","enum":["1","2"]} ``` No integer satisfies that enum. The same test passes in 3.0 with numeric `enum: [1, 2]` and `default: 1`. This starts with swagger-core's handling, but updating only the type leaves the resulting schema inconsistent. Could the workaround preserve correctly typed annotation values too, or resolve the property with its declared type honored before those values are processed? **3. [P2] Replacing derived success responses can remove an explicitly declared controller response.** [ActionAnnotations.groovy:128-151](https://github.com/apache/grails-core/blob/4065ea5372d6b46916e165db83246e5eb2f4c100/grails-openapi/src/main/groovy/org/grails/openapi/ActionAnnotations.groovy#L128-L151) records the derived codes before applying controller annotations, and [the final removal](https://github.com/apache/grails-core/blob/4065ea5372d6b46916e165db83246e5eb2f4c100/grails-openapi/src/main/groovy/org/grails/openapi/ActionAnnotations.groovy#L163-L175) still treats those codes as only derived. A controller declaring `@ApiResponse(responseCode = '200', description = 'Completed synchronously')` and an action declaring `202` therefore produces only `202`. The explicit controller `200` is deleted. This reproduces with both a direct `@ApiResponse` and `@Operation(responses = ...)` on the action. A controller-declared `203` would survive instead, so preservation depends on whether the code happens to match the conventional default. The guide says controller responses are added to each action. Could controller-declared codes be excluded from the derived-response cleanup? **Verification** On this head, before adding the reproducer: ```shell ./gradlew :grails-openapi:check :grails-test-examples-openapi:integrationTest :grails-test-examples-openapi-rest-api:integrationTest -PmaxTestParallel=2 --max-workers=4 --console=plain ``` - `grails-openapi`: 283 unit tests and 9 CLI tests passed; main and CLI CodeNarc checks passed. - The two example applications: 39 and 9 integration tests passed. - This was scoped verification, not a fresh run of the entire repository's suites. The additional spec below has six cases: five fail, confirming the three issues above; the numeric OpenAPI 3.0 control passes. It uses the existing `OpenApiFixture` and checks the serialized document rather than only the in-memory Swagger model. Reproducer path: `grails-openapi/src/test/groovy/grails/openapi/ReviewRegressionSpec.groovy`. ```shell ./gradlew :grails-openapi:test --tests grails.openapi.ReviewRegressionSpec -PmaxTestParallel=1 --max-workers=4 --console=plain ``` <details> <summary>Full ReviewRegressionSpec.groovy reproducer</summary> ```groovy /* * Licensed to the Apache Software Foundation (ASF) under one * or more contributor license agreements. See the NOTICE file * distributed with this work for additional information * regarding copyright ownership. The ASF licenses this file * to you under the Apache License, Version 2.0 (the * "License"); you may not use this file except in compliance * with the License. You may obtain a copy of the License at * * https://www.apache.org/licenses/LICENSE-2.0 * * Unless required by applicable law or agreed to in writing, * software distributed under the License is distributed on an * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY * KIND, either express or implied. See the License for the * specific language governing permissions and limitations * under the License. */ package grails.openapi import groovy.json.JsonOutput import groovy.json.JsonSlurper import io.swagger.v3.oas.annotations.Operation import io.swagger.v3.oas.annotations.media.Content import io.swagger.v3.oas.annotations.media.Schema import io.swagger.v3.oas.annotations.responses.ApiResponse import grails.artefact.Artefact import grails.validation.Validateable import spock.lang.Specification import spock.lang.Unroll class ReviewRegressionSpec extends Specification { @Unroll void 'numeric annotation values match their declared type in #version'() { when: Map document = written(version) Map level = document.components.schemas.ReviewNumeric.get('properties').level then: assert level.enum == [1, 2] : JsonOutput.toJson(level) assert level.default == 1 : JsonOutput.toJson(level) level.type == 'integer' where: version << ['openapi_3_0', 'openapi_3_1'] } @Unroll void 'a nullable reference annotation permits null and the referenced object in #version'() { when: Map document = written(version) Map container = document.components.schemas.ReviewContainer Map address = container.get('properties').address then: !(container.required ?: []).contains('address') version == 'openapi_3_0' ? address.nullable == true && address.allOf == [['$ref': '#/components/schemas/ReviewAddress']] : address.oneOf == [['$ref': '#/components/schemas/ReviewAddress'], [type: 'null']] where: version << ['openapi_3_0', 'openapi_3_1'] } @Unroll void 'an action success response preserves an explicitly declared controller success at #path'() { when: Map responses = written('openapi_3_1').paths[path].post.responses then: responses.keySet() == ['200', '202'] as Set responses['200'].description == 'Completed synchronously' where: path << ['/review/direct', '/review/operation'] } private static Map written(String version) { def document = OpenApiFixture.document(['springdoc.api-docs.version': version], [ReviewSchemaController, ReviewDispatchController], []) { '/review/numeric'(controller: 'reviewSchema', action: 'numeric') '/review/container'(controller: 'reviewSchema', action: 'container') post '/review/direct'(controller: 'reviewDispatch', action: 'direct') post '/review/operation'(controller: 'reviewDispatch', action: 'operation') } (Map) new JsonSlurper().parseText(GrailsOpenApiGenerator.serialize(document, 'json')) } } class ReviewNumeric { @Schema(type = 'integer', format = 'int32', allowableValues = ['1', '2'], defaultValue = '1') Integer level } class ReviewContainer implements Validateable { @Schema(nullable = true) ReviewAddress address } class ReviewAddress { String street } @Artefact('Controller') class ReviewSchemaController { @ApiResponse(responseCode = '200', content = @Content(schema = @Schema(implementation = ReviewNumeric))) def numeric() { } @ApiResponse(responseCode = '200', content = @Content(schema = @Schema(implementation = ReviewContainer))) def container() { } } @Artefact('Controller') @ApiResponse(responseCode = '200', description = 'Completed synchronously') class ReviewDispatchController { @ApiResponse(responseCode = '202', description = 'Queued') def direct() { } @Operation(responses = [@ApiResponse(responseCode = '202', description = 'Queued')]) def operation() { } } ``` </details> -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
