From a444d66e9b4f0f1f82a2b7d71c014a1eec46f299 Mon Sep 17 00:00:00 2001 From: Michael Edgar Date: Sun, 7 Feb 2021 09:17:24 -0500 Subject: [PATCH] Improve handling of parameter refs and missing parameter key properties --- .../runtime/scanner/ResourceParameters.java | 6 +- .../spi/AbstractParameterProcessor.java | 142 +++++++++++++----- .../openapi/runtime/util/JandexUtil.java | 5 +- .../runtime/scanner/IndexScannerTestBase.java | 2 + .../jaxrs/JaxRsParameterProcessor.java | 11 +- .../runtime/scanner/ParameterScanTests.java | 44 +++++- .../params.parameter-ref-property.json | 92 ++++++++++++ .../spring/SpringParameterProcessor.java | 11 +- .../vertx/VertxParameterProcessor.java | 10 +- 9 files changed, 256 insertions(+), 67 deletions(-) create mode 100644 extension-jaxrs/src/test/resources/io/smallrye/openapi/runtime/scanner/params.parameter-ref-property.json diff --git a/core/src/main/java/io/smallrye/openapi/runtime/scanner/ResourceParameters.java b/core/src/main/java/io/smallrye/openapi/runtime/scanner/ResourceParameters.java index b97a70d90..6ba98ecbf 100644 --- a/core/src/main/java/io/smallrye/openapi/runtime/scanner/ResourceParameters.java +++ b/core/src/main/java/io/smallrye/openapi/runtime/scanner/ResourceParameters.java @@ -24,8 +24,10 @@ */ public class ResourceParameters { - private static final Comparator PARAMETER_COMPARATOR = Comparator.comparing(Parameter::getIn) - .thenComparing(Parameter::getName); + public static final Comparator PARAMETER_COMPARATOR = Comparator + .comparing(Parameter::getRef, Comparator.nullsFirst(Comparator.naturalOrder())) + .thenComparing(Parameter::getIn, Comparator.nullsLast(Comparator.naturalOrder())) + .thenComparing(Parameter::getName, Comparator.nullsLast(Comparator.naturalOrder())); static final Pattern TEMPLATE_PARAM_PATTERN = Pattern.compile("\\{(\\w[\\w\\.-]*)\\}"); diff --git a/core/src/main/java/io/smallrye/openapi/runtime/scanner/spi/AbstractParameterProcessor.java b/core/src/main/java/io/smallrye/openapi/runtime/scanner/spi/AbstractParameterProcessor.java index 5cbf6d786..92e6589b1 100644 --- a/core/src/main/java/io/smallrye/openapi/runtime/scanner/spi/AbstractParameterProcessor.java +++ b/core/src/main/java/io/smallrye/openapi/runtime/scanner/spi/AbstractParameterProcessor.java @@ -10,7 +10,6 @@ import java.util.Arrays; import java.util.Collection; import java.util.Collections; -import java.util.Comparator; import java.util.HashMap; import java.util.HashSet; import java.util.LinkedHashMap; @@ -156,17 +155,27 @@ protected static class ParameterContextKey { final String name; final In location; final Style style; + final String ref; public ParameterContextKey(String name, In location, Style style) { this.name = name; this.location = location; this.style = style; + this.ref = null; + } + + public ParameterContextKey(Parameter oaiParam) { + this.name = oaiParam.getName(); + this.location = oaiParam.getIn(); + this.style = styleOf(oaiParam); + this.ref = oaiParam.getRef(); } public ParameterContextKey(ParameterContext context) { this.name = context.name; this.location = context.location; this.style = context.style; + this.ref = context.oaiParam != null ? context.oaiParam.getRef() : null; } @Override @@ -174,21 +183,34 @@ public boolean equals(Object obj) { if (obj instanceof ParameterContextKey) { ParameterContextKey other = (ParameterContextKey) obj; + if (isNull() && other.isNull()) { + return this == other; + } + + if (ref != null) { + return ref.equals(other.ref); + } + return Objects.equals(this.name, other.name) && Objects.equals(this.location, other.location) && Objects.equals(this.style, other.style); } + return false; } @Override public int hashCode() { - return Objects.hash(name, location, style); + return isNull() ? super.hashCode() : Objects.hash(name, location, style, ref); } @Override public String toString() { - return "name: " + name + "; in: " + location; + return "name: " + name + "; in: " + location + "; style: " + style + "; ref: " + ref; + } + + public boolean isNull() { + return name == null && location == null && style == null && ref == null; } } @@ -232,7 +254,7 @@ protected void processOperationParameters(MethodInfo resourceMethod, ResourcePar // Phase II - Read method argument @Parameter and framework's annotations resourceMethod.annotations() .stream() - .filter(a -> !a.target().equals(resourceMethod)) + .filter(a -> !JandexUtil.equals(a.target(), resourceMethod)) .forEach(annotation -> { /* * This condition exists to support @Parameters wrapper annotation @@ -248,7 +270,7 @@ protected void processOperationParameters(MethodInfo resourceMethod, ResourcePar // Phase III - Read method @Parameter(s) annotations resourceMethod.annotations() .stream() - .filter(a -> a.target().equals(resourceMethod)) + .filter(a -> JandexUtil.equals(a.target(), resourceMethod)) .filter(a -> openApiParameterAnnotations.contains(a.name())) .forEach(this::readParameterAnnotation); @@ -391,8 +413,7 @@ protected List getParameters(MethodInfo resourceMethod) { .stream() .map(context -> this.mapParameter(resourceMethod, context)) .filter(Objects::nonNull) - .sorted(Comparator.comparing(Parameter::getIn) - .thenComparing(Parameter::getName)) + .sorted(ResourceParameters.PARAMETER_COMPARATOR) .collect(Collectors.toList()); return parameters.isEmpty() ? null : parameters; @@ -661,10 +682,11 @@ protected void addEncoding(Map encodings, String paramName, An * @see org.eclipse.microprofile.openapi.annotations.parameters.Parameter#in() */ protected boolean isIgnoredParameter(Parameter parameter, AnnotationTarget resourceMethod) { - String paramName = parameter.getName(); - In paramIn = parameter.getIn(); + final String ref = parameter.getRef(); + final String paramName = parameter.getName(); + final In paramIn = parameter.getIn(); - if (paramIn == null) { + if (paramIn == null && ref == null) { /* * Per @Parameter JavaDoc, ignored when empty string (i.e., unspecified). * This may occur when @Parameter is specified without a matching framework @@ -680,7 +702,7 @@ protected boolean isIgnoredParameter(Parameter parameter, AnnotationTarget resou /* * Name is REQUIRED unless it is a reference. */ - if ((paramName == null || paramName.trim().isEmpty()) && parameter.getRef() == null) { + if ((paramName == null || paramName.trim().isEmpty()) && ref == null) { return true; } @@ -791,7 +813,7 @@ protected abstract void readAnnotatedType(AnnotationInstance annotation, Annotat * @param param the {@link Parameter} * @return the param's style, the default style defined based on in, or null if in is not defined. */ - protected Style styleOf(Parameter param) { + protected static Style styleOf(Parameter param) { if (param.getStyle() != null) { return param.getStyle(); } @@ -1064,6 +1086,21 @@ protected static Type getType(AnnotationTarget target) { return type; } + protected boolean isReadableParameterAnnotation(DotName name) { + return ParameterConstant.DOTNAME_PARAMETER.equals(name) && readerFunction != null; + } + + protected void readParameterAnnotation(AnnotationInstance annotation, boolean overriddenParametersOnly) { + Parameter oaiParam = readerFunction.apply(annotation); + + readParameter(new ParameterContextKey(oaiParam), + oaiParam, + null, + null, + annotation.target(), + overriddenParametersOnly); + } + /** * Merges MP-OAI {@link Parameter}s and framework-specific parameters for the same {@link In} and name, * and {@link Style}. When overriddenParametersOnly is true, new parameters not already known @@ -1162,49 +1199,82 @@ ParameterContext getParameterContext(ParameterContextKey key, AnnotationTarget t context = params .values() .stream() + // Only attempt to match parameters without `ref` specified + .filter(c -> c.oaiParam == null || c.oaiParam.getRef() == null) .filter(c -> haveSameAnnotatedTarget(c, target, key.name)) + .filter(c -> attributesCompatible(c.location, key.location)) + .filter(c -> attributesCompatible(c.style, key.style)) .findFirst() - .orElse(null); - } - - if (context == null) { - /* - * Allow a match on just the name and style if one of the Parameter.In values - * is not specified - */ - context = params.values().stream().filter(c -> { - if (c.location == null || key.location == null) { - return Objects.equals(c.name, key.name) && Objects.equals(c.style, key.style); - } - return false; - }).findFirst().orElse(null); - + .orElseGet(() -> params.values() + .stream() + .filter(c -> nameAndStyleMatch(c, key)) + .findFirst() + .orElse(null)); } return context; } boolean haveSameAnnotatedTarget(ParameterContext context, AnnotationTarget target, String name) { - /* - * Consider names to match if one is unspecified or they are equal. - */ - boolean nameMatches = (context.name == null || name == null || Objects.equals(context.name, name)); + boolean sameTarget = false; if (JandexUtil.equals(target, context.target)) { + if (target.kind() != Kind.METHOD) { + // Both annotations are set on the same method argument or field + sameTarget = true; + } else { + /* + * Method targets: consider names to match if *one* is unspecified or they are equal. + */ + if (name != null) { + sameTarget = context.name == null || Objects.equals(name, context.name); + } else if (context.name != null) { + sameTarget = true; + } + } + } else if (JandexUtil.equals(targetMethod(target), targetMethod(context.target))) { /* - * The name must match for annotations on a method because it is - * ambiguous which parameters is being referenced. + * One annotation is on the method and the other is on a parameter. Require that + * the names are both given and match. */ - return nameMatches || target.kind() != Kind.METHOD; + sameTarget = name != null && name.equals(context.name); } - if (nameMatches && target.kind() == Kind.METHOD && context.target.kind() == Kind.METHOD_PARAMETER) { - return context.target.asMethodParameter().method().equals(target); + return sameTarget; + } + + boolean attributesCompatible(Object a1, Object a2) { + return a1 == null || a2 == null || a1.equals(a2); + } + + boolean nameAndStyleMatch(ParameterContext context, ParameterContextKey key) { + /* + * Allow a match on just the name and style if one of the Parameter.In values + * is not specified + */ + if ((context.location == null || key.location == null) && key.name != null && key.style != null) { + return key.name.equals(context.name) && key.style.equals(context.style); } return false; } + /** + * Obtain the MethodInfo associated with the annotation target. + * + * @param target annotated item. Only method and method parameter targets. + * @return the MethodInfo associated with the target, or null if target is not a method or parameter. + */ + static MethodInfo targetMethod(AnnotationTarget target) { + if (target.kind() == Kind.METHOD) { + return target.asMethod(); + } + if (target.kind() == Kind.METHOD_PARAMETER) { + return target.asMethodParameter().method(); + } + return null; + } + /** * Scans for class level parameters on the given class argument and its ancestors. * diff --git a/core/src/main/java/io/smallrye/openapi/runtime/util/JandexUtil.java b/core/src/main/java/io/smallrye/openapi/runtime/util/JandexUtil.java index 5bbab7622..bdab55957 100644 --- a/core/src/main/java/io/smallrye/openapi/runtime/util/JandexUtil.java +++ b/core/src/main/java/io/smallrye/openapi/runtime/util/JandexUtil.java @@ -510,7 +510,10 @@ public static Map inheritanceChain(IndexView index, ClassInfo k } public static boolean equals(AnnotationTarget t1, AnnotationTarget t2) { - if (t1.kind() != t2.kind()) { + if (t1 == t2) { + return true; + } + if (t1 == null || t2 == null || t1.kind() != t2.kind()) { return false; } diff --git a/core/src/test/java/io/smallrye/openapi/runtime/scanner/IndexScannerTestBase.java b/core/src/test/java/io/smallrye/openapi/runtime/scanner/IndexScannerTestBase.java index 7287158ba..f418778ef 100644 --- a/core/src/test/java/io/smallrye/openapi/runtime/scanner/IndexScannerTestBase.java +++ b/core/src/test/java/io/smallrye/openapi/runtime/scanner/IndexScannerTestBase.java @@ -29,6 +29,7 @@ import io.smallrye.openapi.api.OpenApiConfigImpl; import io.smallrye.openapi.api.models.ComponentsImpl; import io.smallrye.openapi.api.models.OpenAPIImpl; +import io.smallrye.openapi.runtime.io.CurrentScannerInfo; import io.smallrye.openapi.runtime.io.Format; import io.smallrye.openapi.runtime.io.OpenApiSerializer; @@ -39,6 +40,7 @@ public class IndexScannerTestBase { @After public void removeSchemaRegistry() { SchemaRegistry.remove(); + CurrentScannerInfo.remove(); } protected static String pathOf(Class clazz) { diff --git a/extension-jaxrs/src/main/java/io/smallrye/openapi/jaxrs/JaxRsParameterProcessor.java b/extension-jaxrs/src/main/java/io/smallrye/openapi/jaxrs/JaxRsParameterProcessor.java index 4b1294897..5cdc46d48 100644 --- a/extension-jaxrs/src/main/java/io/smallrye/openapi/jaxrs/JaxRsParameterProcessor.java +++ b/extension-jaxrs/src/main/java/io/smallrye/openapi/jaxrs/JaxRsParameterProcessor.java @@ -158,15 +158,8 @@ protected void readAnnotatedType(AnnotationInstance annotation, AnnotationInstan boolean overriddenParametersOnly) { DotName name = annotation.name(); - if (ParameterConstant.DOTNAME_PARAMETER.equals(name) && readerFunction != null) { - Parameter oaiParam = readerFunction.apply(annotation); - - readParameter(new ParameterContextKey(oaiParam.getName(), oaiParam.getIn(), styleOf(oaiParam)), - oaiParam, - null, - null, - annotation.target(), - overriddenParametersOnly); + if (isReadableParameterAnnotation(name)) { + readParameterAnnotation(annotation, overriddenParametersOnly); } else { FrameworkParameter frameworkParam = JaxRsParameter.forName(name); diff --git a/extension-jaxrs/src/test/java/io/smallrye/openapi/runtime/scanner/ParameterScanTests.java b/extension-jaxrs/src/test/java/io/smallrye/openapi/runtime/scanner/ParameterScanTests.java index e4e47c3f9..d83dacccd 100644 --- a/extension-jaxrs/src/test/java/io/smallrye/openapi/runtime/scanner/ParameterScanTests.java +++ b/extension-jaxrs/src/test/java/io/smallrye/openapi/runtime/scanner/ParameterScanTests.java @@ -4,7 +4,6 @@ import java.io.ByteArrayOutputStream; import java.io.IOException; import java.io.InputStream; -import java.util.HashMap; import java.util.Optional; import java.util.OptionalDouble; import java.util.OptionalLong; @@ -31,17 +30,21 @@ import javax.ws.rs.PathParam; import javax.ws.rs.Produces; import javax.ws.rs.QueryParam; +import javax.ws.rs.core.Application; import javax.ws.rs.core.Context; import javax.ws.rs.core.MediaType; import javax.ws.rs.core.PathSegment; import javax.ws.rs.core.Request; +import org.eclipse.microprofile.openapi.annotations.Components; import org.eclipse.microprofile.openapi.annotations.ExternalDocumentation; +import org.eclipse.microprofile.openapi.annotations.OpenAPIDefinition; import org.eclipse.microprofile.openapi.annotations.Operation; import org.eclipse.microprofile.openapi.annotations.enums.ParameterIn; import org.eclipse.microprofile.openapi.annotations.enums.ParameterStyle; import org.eclipse.microprofile.openapi.annotations.enums.SchemaType; import org.eclipse.microprofile.openapi.annotations.extensions.Extension; +import org.eclipse.microprofile.openapi.annotations.info.Info; import org.eclipse.microprofile.openapi.annotations.media.Content; import org.eclipse.microprofile.openapi.annotations.media.Encoding; import org.eclipse.microprofile.openapi.annotations.media.Schema; @@ -71,7 +74,7 @@ public class ParameterScanTests extends IndexScannerTestBase { private static void test(String expectedResource, Class... classes) throws IOException, JSONException { Index index = indexOf(classes); - OpenApiAnnotationScanner scanner = new OpenApiAnnotationScanner(dynamicConfig(new HashMap()), index); + OpenApiAnnotationScanner scanner = new OpenApiAnnotationScanner(emptyConfig(), index); OpenAPI result = scanner.scan(); printToConsole(result); assertJsonEquals(expectedResource, result); @@ -261,6 +264,11 @@ public void testSerializedIndexParameterAnnotations() throws IOException, JSONEx assertJsonEquals("params.serialized-annotation-index.json", result); } + @Test + public void testParameterRefOnly() throws IOException, JSONException { + test("params.parameter-ref-property.json", ParameterRefTestApplication.class, ParameterRefTestResource.class); + } + /***************** Test models and resources below. ***********************/ public static class Widget { @@ -973,4 +981,36 @@ public static class GreetingMessage { String message; } } + + @OpenAPIDefinition(info = @Info(title = "title", version = "1"), components = @Components(parameters = { + @Parameter(name = "queryParam1", in = ParameterIn.QUERY), + @Parameter(name = "pathParam2", in = ParameterIn.PATH, description = "`pathParam2` with info in components") })) + static class ParameterRefTestApplication extends Application { + } + + @Path("/{pathParam1}/{pathParam2}") + static class ParameterRefTestResource { + @GET + @Path("one") + @Parameter(ref = "queryParam1") + String exampleEndpoint1(@PathParam("pathParam1") String pathParam1, + @PathParam("pathParam2") String pathParam2) { + return null; + } + + @GET + @Path("/two") + @Parameter(name = "pathParam1", style = ParameterStyle.SIMPLE) + @Parameter(ref = "pathParam2") + // `name` on `queryParam1` ref ignored + @Parameter(ref = "queryParam1", name = "queryParamOne") + @Parameter(in = ParameterIn.COOKIE, description = "Ignored: missing key attributes") + @Parameter(in = ParameterIn.DEFAULT, description = "Ignored: missing key attributes") + @Parameter(in = ParameterIn.HEADER, description = "Ignored: missing key attributes") + @Parameter(in = ParameterIn.PATH, description = "Ignored: missing key attributes") + String exampleEndpoint2(@PathParam("pathParam1") String pathParam1, + @Parameter(hidden = true) @PathParam("pathParam2") String pathParam2) { + return null; + } + } } diff --git a/extension-jaxrs/src/test/resources/io/smallrye/openapi/runtime/scanner/params.parameter-ref-property.json b/extension-jaxrs/src/test/resources/io/smallrye/openapi/runtime/scanner/params.parameter-ref-property.json new file mode 100644 index 000000000..d06be7169 --- /dev/null +++ b/extension-jaxrs/src/test/resources/io/smallrye/openapi/runtime/scanner/params.parameter-ref-property.json @@ -0,0 +1,92 @@ +{ + "openapi": "3.0.3", + "info": { + "title": "title", + "version": "1" + }, + "paths": { + "/{pathParam1}/{pathParam2}/one": { + "get": { + "parameters": [ + { + "name": "pathParam1", + "in": "path", + "required": true, + "schema": { + "type": "string" + } + }, + { + "name": "pathParam2", + "in": "path", + "required": true, + "schema": { + "type": "string" + } + }, + { + "$ref": "#/components/parameters/queryParam1" + } + ], + "responses": { + "200": { + "description": "OK", + "content": { + "*/*": { + "schema": { + "type": "string" + } + } + } + } + } + } + }, + "/{pathParam1}/{pathParam2}/two": { + "get": { + "parameters": [ + { + "name": "pathParam1", + "in": "path", + "style": "simple", + "required": true, + "schema": { + "type": "string" + } + }, + { + "$ref": "#/components/parameters/pathParam2" + }, + { + "$ref": "#/components/parameters/queryParam1" + } + ], + "responses": { + "200": { + "description": "OK", + "content": { + "*/*": { + "schema": { + "type": "string" + } + } + } + } + } + } + } + }, + "components": { + "parameters": { + "queryParam1": { + "name": "queryParam1", + "in": "query" + }, + "pathParam2": { + "name": "pathParam2", + "in": "path", + "description": "`pathParam2` with info in components" + } + } + } +} diff --git a/extension-spring/src/main/java/io/smallrye/openapi/spring/SpringParameterProcessor.java b/extension-spring/src/main/java/io/smallrye/openapi/spring/SpringParameterProcessor.java index e9bc9d971..81296925f 100644 --- a/extension-spring/src/main/java/io/smallrye/openapi/spring/SpringParameterProcessor.java +++ b/extension-spring/src/main/java/io/smallrye/openapi/spring/SpringParameterProcessor.java @@ -89,15 +89,8 @@ protected void readAnnotatedType(AnnotationInstance annotation, AnnotationInstan boolean overriddenParametersOnly) { DotName name = annotation.name(); - if (ParameterConstant.DOTNAME_PARAMETER.equals(name) && readerFunction != null) { - Parameter oaiParam = readerFunction.apply(annotation); - - readParameter(new ParameterContextKey(oaiParam.getName(), oaiParam.getIn(), styleOf(oaiParam)), - oaiParam, - null, - null, - annotation.target(), - overriddenParametersOnly); + if (isReadableParameterAnnotation(name)) { + readParameterAnnotation(annotation, overriddenParametersOnly); } else { FrameworkParameter frameworkParam = SpringParameter.forName(name); diff --git a/extension-vertx/src/main/java/io/smallrye/openapi/vertx/VertxParameterProcessor.java b/extension-vertx/src/main/java/io/smallrye/openapi/vertx/VertxParameterProcessor.java index 61939b03d..560ee8c51 100644 --- a/extension-vertx/src/main/java/io/smallrye/openapi/vertx/VertxParameterProcessor.java +++ b/extension-vertx/src/main/java/io/smallrye/openapi/vertx/VertxParameterProcessor.java @@ -91,14 +91,8 @@ protected void readAnnotatedType(AnnotationInstance annotation, AnnotationInstan DotName name = annotation.name(); - if (ParameterConstant.DOTNAME_PARAMETER.equals(name) && readerFunction != null) { - Parameter oaiParam = readerFunction.apply(annotation); - readParameter(new ParameterContextKey(oaiParam.getName(), oaiParam.getIn(), styleOf(oaiParam)), - oaiParam, - null, - null, /* defaultValue */ - annotation.target(), - overriddenParametersOnly); + if (isReadableParameterAnnotation(name)) { + readParameterAnnotation(annotation, overriddenParametersOnly); } else if (VertxConstants.PARAM.equals(name) && annotation.value() != null) { String parameterName = annotation.value().asString(); String path = null;