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

PAYARA-3466 Open API 1.1 #3827

Merged
merged 10 commits into from
Mar 19, 2019
Merged

Conversation

jbee
Copy link
Contributor

@jbee jbee commented Mar 12, 2019

Summary:

  • Add generics to Extensible hierarchy (added in 1.1)
  • Implemented new methods (mostly explicit type-safe add/remove/contains)
  • Replaced usage of deprecated methods with type-safe replacements added in 1.1
  • Removed methods now implemented as default methods in the API interfaces
  • Added processing of @Parameters (missed both by TCK and us in 1.0)
  • Added processing of @Extensions (missed both by TCK and us in 1.0)
  • Added null check to addX methods (I think 1.1 addec this semantic or 1.0 TCK missed to check)
  • Added copy constructor for Map model types for their "copy getter" (see API javadoc)
  • Extracted a common base type for Extensible Maps (share impl.)
  • Some more specific local fixes for minor deviations from the spec since 1.0 that TCK 1.1 now found
  • Fixed some serialisation issues (present since 1.0)

@jbee jbee added this to the 5.192 milestone Mar 12, 2019
@jbee jbee self-assigned this Mar 12, 2019
@@ -98,4 +129,34 @@ public static Object convertExtensionValue(String value) {
return value;
}

@Override
public String toString() {
Copy link
Contributor Author

@jbee jbee Mar 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: Added this to ease debugging by providing a helpful toString for the model.

ExampleImpl.merge(example, to.getMediaType(typeName).getExamples(), override);
}
if (!from.example().isEmpty()) {
to.getMediaType(typeName).setExample(from.example());
Copy link
Contributor Author

@jbee jbee Mar 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: example is a new property in 1.1


@Override
public Scopes addScope(String name, String item) {
this.put(name, item);
this.put(name, item); // this DOES accept null!
Copy link
Contributor Author

@jbee jbee Mar 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: This seems inconsistent but this add method explicitly allows for null while usually the add methods do not allow for null or should ignore it.

@Override
public SecurityRequirement addScheme(String name, String item) {
this.put(name, Arrays.asList(item));
this.put(name, item == null ? new ArrayList<>() : Arrays.asList(item));
Copy link
Contributor Author

@jbee jbee Mar 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: null should become empty list

return this;
}

@Override
public SecurityRequirement addScheme(String name, List<String> item) {
this.put(name, item);
this.put(name, item == null ? new ArrayList<>() : item);
Copy link
Contributor Author

@jbee jbee Mar 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: null should become empty list

return variables;
return variables instanceof ServerVariables || variables == null
? (ServerVariables) variables
: new ServerVariablesImpl(variables);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: ServerVariables got "half-removed". It is deprecated and should be removed in the future. In some places it already was replaced with a Map<String, SeverVariable> what causes changes here.

public static Boolean mergeProperty(boolean current, boolean offer, boolean override) {
return mergeProperty(Boolean.valueOf(current), Boolean.valueOf(offer), override);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: I added this overloaded variants to avoid boxing/unboxing warnings. The module is almost free of warnings and it would be nice to get there.

try (DataInputStream in = new DataInputStream(stream)) {
// Reading the binary data
in.readFully(buff);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: fixed potential resource leak.

if (element instanceof java.lang.reflect.Parameter) {
visitSchemaParameter(schema, (java.lang.reflect.Parameter) element, context);
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: extracted the cases into one method each to make this more readable.

String name = extension.name();
if (name != null && !name.isEmpty() && value != null
&& !value.isEmpty()) {
Object parsedValue = ExtensibleImpl.convertExtensionValue(value, extension.parseValue());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: here we were missing applying conversion via convertExtensionValue.

}
}
}
matchedParam = findOperationParameterFor(parameter, (Method) element, context);
Copy link
Contributor Author

@jbee jbee Mar 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: extracted the cases to make it more readable. Also the loop in the cases should stop when a match is found. This is simpler done with a return in a separate method.

.removeIf(x -> ModelUtils.getParameterType(x) != In.valueOf(in.name()));
}
if (matchingMethodParameters.isEmpty()) {
return null;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: This is a fix I added. 1.0 TCK simply had no case that would otherwise fail the next line where we always get the first entry of the list. But some cases do not have a matching parameter.

@jbee
Copy link
Contributor Author

jbee commented Mar 12, 2019

jenkins test please

// Get all parameters with the same name
List<java.lang.reflect.Parameter> matchingMethodParameters = Arrays
.asList(annotated.getParameters()).stream()
.filter(x -> name.equals(ModelUtils.getParameterName(x)))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: swapped the equals because ModelUtils.getParameterName(x) can be null causeing NPE while name is from an annotation and cannot be null.

Copy link
Contributor

@MattGill98 MattGill98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally good, just a couple of comments from me. As an aside, I'm glad someone has shared my pain of maintaining masses of model classes! 😆

@jbee
Copy link
Contributor Author

jbee commented Mar 13, 2019

jenkins test please

* {@link ExtensibleTreeMap#extensions} map to the output object unless the value represents a {@link Reference} in
* which case only the {@link Reference#getRef()} is added.
*/
static class ExtensibleTreeMapSerializer extends JsonSerializer<ExtensibleTreeMap<?,?>> {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: This fixes two defects in the JSON/YAML serialisation present since 1.0 where properties on Map extending types would be ignored as they are handled by the map serializer. Thereby extensions would not be added and in case of a reference an empty object was rendered instead of the reference.

@jbee
Copy link
Contributor Author

jbee commented Mar 18, 2019

jenkins test please

@Pandrex247 Pandrex247 merged commit ef8e36d into payara:master Mar 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants