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.

Sign up for GitHub

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

Parameter schema improvement & bug fix (2.0.x) #661

Merged
merged 2 commits into from
Feb 7, 2021
Merged

Parameter schema improvement & bug fix (2.0.x) #661

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a444d66
Improve handling of parameter refs and missing parameter key properties
MikeEdgar Feb 7, 2021
fa8607c
Fix scanning of schema-related parameter annotations
MikeEdgar Feb 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 58 additions & 0 deletions core/src/main/java/io/smallrye/openapi/api/models/media/SchemaImpl.java
View file
Open in desktop

Large diffs are not rendered by default.

6 changes: 4 additions & 2 deletions core/src/main/java/io/smallrye/openapi/runtime/scanner/ResourceParameters.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,10 @@
*/
public class ResourceParameters {

private static final Comparator<Parameter> PARAMETER_COMPARATOR = Comparator.comparing(Parameter::getIn)
.thenComparing(Parameter::getName);
public static final Comparator<Parameter> 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\\.-]*)\\}");

Expand Down
211 changes: 163 additions & 48 deletions core/src/main/java/io/smallrye/openapi/runtime/scanner/spi/AbstractParameterProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -156,39 +155,62 @@ 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
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;
}
}

Expand Down Expand Up @@ -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
Expand All @@ -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);

Expand Down Expand Up @@ -391,8 +413,7 @@ protected List<Parameter> 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;
Expand Down Expand Up @@ -499,17 +520,7 @@ private Parameter mapParameter(MethodInfo resourceMethod, ParameterContext conte
}

if (param.getSchema() != null) {
//TODO: Test BV annotations on all target types
BeanValidationScanner.applyConstraints(context.target,
param.getSchema(),
param.getName(),
(target, name) -> {
if (param.getRequired() == null) {
param.setRequired(Boolean.TRUE);
}
});

setDefaultValue(param.getSchema(), context.defaultValue);
augmentParamSchema(param, context);
}

if (param.getRequired() == null && TypeUtil.isOptional(context.targetType)) {
Expand All @@ -519,6 +530,61 @@ private Parameter mapParameter(MethodInfo resourceMethod, ParameterContext conte
return param;
}

/**
* Scan the AnnotationTarget associated with Parameter <code>param</code> for supported
* default value or Bean Validation annotations that may alter the schema. When
* found, update param's schema either directly or, when a <code>ref</code> is present, by
* creating a union of the local and reference schemas using <code>allOf</code>.
*
* @param param the Parameter containing a schema for update
* @param context ParameterContext associated with param
*/
void augmentParamSchema(Parameter param, ParameterContext context) {
Schema paramSchema = param.getSchema();
String ref = paramSchema.getRef();
Schema localSchema;

if (ref != null) {
Type paramType = TypeUtil.unwrapType(context.targetType);

/*
* Lookup the schema `type` from components (if available) or guess the type if
* the `ref` is not available.
*/
Schema refSchema = ModelUtil.getComponent(scannerContext.getOpenApi(), ref);

if (refSchema != null) {
localSchema = new SchemaImpl().type(refSchema.getType());
} else {
localSchema = new SchemaImpl().type(SchemaType
.valueOf(TypeUtil.getTypeAttributes(paramType).get(SchemaConstant.PROP_TYPE).toString().toUpperCase()));
}
} else {
localSchema = paramSchema;
}

int modCount = SchemaImpl.getModCount(localSchema);

BeanValidationScanner.applyConstraints(context.target, localSchema, param.getName(),
(target, name) -> {
if (param.getRequired() == null) {
param.setRequired(Boolean.TRUE);
}
});

setDefaultValue(localSchema, context.defaultValue);

if (localOnlySchemaModified(paramSchema, localSchema, modCount)) {
// Add new `allOf` schema, erasing `type` derived above from the local schema
Schema allOf = new SchemaImpl().addAllOf(paramSchema).addAllOf(localSchema.type(null));
param.setSchema(allOf);
}
}

boolean localOnlySchemaModified(Schema paramSchema, Schema localSchema, int originalModCount) {
return localSchema != paramSchema && originalModCount != -1 && SchemaImpl.getModCount(localSchema) > originalModCount;
}

/**
* Converts the collection of parameter annotations to properties set on the
* given schema.
Expand Down Expand Up @@ -587,7 +653,7 @@ private void setRequired(String name, Schema schema) {
}

private void setDefaultValue(Schema schema, Object defaultValue) {
if (schema.getDefaultValue() == null) {
if (schema.getDefaultValue() == null && defaultValue != null) {
schema.setDefaultValue(defaultValue);
}
}
Expand Down Expand Up @@ -661,10 +727,11 @@ protected void addEncoding(Map<String, Encoding> 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
Expand All @@ -680,7 +747,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;
}

Expand Down Expand Up @@ -791,7 +858,7 @@ protected abstract void readAnnotatedType(AnnotationInstance annotation, Annotat
* @param param the {@link Parameter}
* @return the param's style, the default style defined based on <code>in</code>, or null if <code>in</code> is not defined.
*/
protected Style styleOf(Parameter param) {
protected static Style styleOf(Parameter param) {
if (param.getStyle() != null) {
return param.getStyle();
}
Expand Down Expand Up @@ -1064,6 +1131,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
Expand Down Expand Up @@ -1162,49 +1244,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.
*
Expand Down
14 changes: 13 additions & 1 deletion core/src/main/java/io/smallrye/openapi/runtime/util/JandexUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,15 @@ public enum RefType {
RefType(String componentPath) {
this.componentPath = componentPath;
}

public static RefType fromComponentPath(String path) {
for (RefType ref : values()) {
if (ref.componentPath.equals(path)) {
return ref;
}
}
return null;
}
}

/**
Expand Down Expand Up @@ -510,7 +519,10 @@ public static Map<ClassInfo, Type> 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;
}

Expand Down
Loading