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 - Tests #3852

Merged
merged 32 commits into from
Apr 8, 2019

Conversation

jbee
Copy link
Contributor

@jbee jbee commented Mar 19, 2019

This PR adds more tests for the Open API module motivated by the defects reported earlier and found during update to Open API 1.1.

A particular focus area is the JSON rendering. Being annotation guided semi-automatic the output wasn't always intuitive and often incorrect. There were problems both with missing elements and elements that should never have been in the output.

To verify the JSON output from our model against the Open API spec (as outlined here https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.0.md ) I created a model for the spec structure (see NoteType) and a OpenApiValidator that can verify the structure of a Open API document given as JsonNode. As this is no trivial operation in itself the validator itself needed testing to verify its correctness. For that reason I added example JSON snippets from the spec which are expected to pass the verification. So none of the .json files needs serious review.

The likely correct structural Open API document validator is then used to validate documents created for existing and new tests.

A common base class OpenApiApplicationTest was extracted that bootstraps the applications with the extras of the class under test and verifies the structure of created document so that the individual tests only have to focus on verifying the contents of the document.
The same was done with OpenApiBuilderTest as a base for tests using the model fluent API to setup the basis of the document.

A larger change in the annotation processing was needed to handle annotations as if they are inherited from super-classes and interfaces. The util AnnotationInfo takes care of this aggregation for each type.

@jbee jbee added this to the 5.192 milestone Mar 19, 2019
@jbee jbee self-assigned this Mar 19, 2019
jbee added 2 commits March 19, 2019 18:09
… added test for callbacks, added components references to validator model
@MarkWareham MarkWareham added the PR: DO NOT MERGE Don't merge PR until further notice label Mar 21, 2019
@ApplicationPath("/test")
public class TestApplication extends Application {

@Override
public Set<Class<?>> getClasses() {
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: Now passed to the test setup to be more flexible and isolated per test.

@jbee
Copy link
Contributor Author

jbee commented Mar 28, 2019

jenkins test please

1 similar comment
@jbee
Copy link
Contributor Author

jbee commented Mar 28, 2019

jenkins test please

@jbee jbee requested review from MattGill98 and Pandrex247 March 28, 2019 10:20
for (Method m : annotation.annotationType().getDeclaredMethods()) {
if (m.getParameterCount() == 0) {
try {
Object value = m.invoke(annotation);
if (value != 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: Main point was to change the .cast(...) to (Type)value style of cast or instanceof where appropriate. Also values from annotation can never be null or a Collection type so I removed these checks. To be sure this is correct I added tests for this util method.

@jbee
Copy link
Contributor Author

jbee commented Apr 2, 2019

jenkins test please

* @param type any class, not null
* @return true, if the give type is a known type in this context, else false
*/
boolean isApplicationType(Class<?> type);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this to allow the ApplicationProcessor logic to make this check even though the information on this is within the OpenApiWalker that creates the ApiContext.

@@ -457,24 +438,9 @@ private void addParameter(AnnotatedElement element, ApiContext context,
if (context.getWorkingOperation() != null) {
context.getWorkingOperation().addParameter(newParameter);
} else {
if (element instanceof Field) {
Field field = Field.class.cast(element);
ApiContext apiContext;
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: Below logic moved to the walker so the walker now is the only one creating the context which is both cleaner and was needed to allow moving inner class processing to the walker what needed to delay generation of resource mapping which also moved to the walker.

/**
* Generates a map listing the location each resource class is mapped to.
*/
private static Map<String, Set<Class<?>>> generateResourceMapping(Set<Class<?>> classList) {
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: Moved to the walker as is.

}

@Override
public void accept(ApiVisitor visitor) {
// OpenAPI necessary annotations
processAnnotations(OpenAPIDefinition.class, visitor::visitOpenAPI, true);
processAnnotations(Schema.class, visitor::visitSchema, true);
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: Schema was processed twice which was needed previously but now works when keeping the later processing of Schema annotation.

|| annotationClass == CookieParam.class
|| annotationClass == PathParam.class
|| annotationClass == QueryParam.class) {
// NB. if fields are annotated as Param all methods have it
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: The field => method logic part was moved here from the application processor

/**
* Generates a map listing the location each resource class is mapped to.
*/
private static Map<String, Set<Class<?>>> generateResourceMapping(Set<Class<?>> classList) {
Copy link
Contributor Author

@jbee jbee Apr 3, 2019

Choose a reason for hiding this comment

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

FYI: Method moved here from ApplicationProcessor

@jbee
Copy link
Contributor Author

jbee commented Apr 3, 2019

jenkins test please

Copy link
Member

@Pandrex247 Pandrex247 left a comment

Choose a reason for hiding this comment

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

Haven't spotted anything, though there was a lot to look through!

@jbee
Copy link
Contributor Author

jbee commented Apr 3, 2019

@Pandrex247 I would have liked to split in multiple PRs but its hard to do that for fixes and confirming tests where most of the fixes only in union lead to meaningful and correct results one can test for. Therefore I felt it was still the best option to keep it all in one PR.

@smillidge smillidge requested a review from MattGill98 April 6, 2019 22:54
@MattGill98 MattGill98 merged commit 373eaff into payara:master Apr 8, 2019
@MattGill98
Copy link
Contributor

🎉 🎉 🎉

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.

6 participants