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

FISH-663 MicroProfile OpenAPI 2.0 (MP 4.0) #5065

Merged
merged 30 commits into from
Dec 23, 2020
Merged

FISH-663 MicroProfile OpenAPI 2.0 (MP 4.0) #5065

merged 30 commits into from
Dec 23, 2020

Conversation

MattGill98
Copy link
Contributor

@MattGill98 MattGill98 commented Dec 21, 2020

Description

This is an improvement, upgrading our implementation of MicroProfile OpenAPI to version 2.0

Changes include:

  • The @SchemaProperty annotation has been added to allow the properties for a schema to be defined inline
  • The @RequestBodySchema annotation has been added to provide a shorthand mechanism to specify the schema for a request body
  • The @APIResponseSchema annotation has been added to provide a shorthand mechanism to specify the schema for a response body
  • The mp.openapi.schema.* MicroProfile Config property has been added to allow the schema for a specific class to be specified. This property would typically be used in cases where the application developer does not have access to the source code of a class
  • Getter methods on model interfaces that return a list or map now return an immutable copy of that collection
  • Setter methods on model interfaces that take a list or a map as a parameter MUST not use the list/map instance directly

The TCK has been upgraded in various places, meaning that not all of the changes here are strictly due to new behaviour.

Important Information

HK2 has received a patch to now parse exception types when creating MethodModels. This was needed as exception mappers are now supported in OpenAPI (they may have been expected before, but not implemented).
Patched source: payara/patched-src-hk2#19
Patched artifact: payara/Payara_PatchedProjects#352

Testing

New tests

TCK Upgrade: payara/MicroProfile-TCK-Runners#132

Testing Performed

Ran the new TCK. OpenAPI is reasonably isolated, so there shouldn't bean affect on many other tests.

Notes for Reviewers

I've disabled a TCK test and raised an upstream issue for it. I don't believe it's intended, and it shouldn't affect or compatibility.

This PR also fixes #4955 (FISH-760)

Upgraded the dependencies and fixed compilation errors.

Signed-off-by: Matthew Gill <[email protected]>
All data returned from the API should be deterministically ordered, so
all HashMaps have been replaced with LinkedHashMap.

Signed-off-by: Matthew Gill <[email protected]>
For some reason, the upgrade is causing the ContentImpl instance to be
counted as not-empty, which is causing the current serialization
inclusion settings to include it. This breaks several classes and tests
that expect any items with $ref tags to be otherwise empty (but it
contains 'content: {}'). This commit changes the inclusion settings to
disable this behaviour.

Signed-off-by: Matthew Gill <[email protected]>
Some errors were thrown at runtime, so a new test has been added and
they have been patched.

Signed-off-by: Matthew Gill <[email protected]>
If a Schema was declared in the OpenAPIDefinition, the @Schema class
annotations would totally replace the initial definition. It is now
merged correctly.

Signed-off-by: Matthew Gill <[email protected]>
The TCK now checks that the get*() methods return immutable types.

Signed-off-by: Matthew Gill <[email protected]>
The set methods would allow you to set unserializable types or types
such as unmodifiableCollections which could cause issues.

Signed-off-by: Matthew Gill <[email protected]>
The node was printed, but not the context for the node. The errors now
print the OpenAPI document, with all the problems added as causes.

Signed-off-by: Matthew Gill <[email protected]>
If a property Schema had a ref, type=OBJECT was still added. A check has
now been added for this.

Signed-off-by: Matthew Gill <[email protected]>
The TCK requires that the collections be nullable, where it wasn't
previously possible. Technically only one collection is tested, but in
the name of consistency, this functionality has been replicated
everywhere.

The collection types have also been abstracted, to allow them to be
changed if required.

Signed-off-by: Matthew Gill <[email protected]>
Prevent scanning all Config API Properties multiple times. The methods
have been refactored to take less time to scan for OpenAPI properties.

Signed-off-by: Matthew Gill <[email protected]>
Config Properties that specify schema objects are now parsed and
included in the final object. Schema objects with parents also include
all the parent class properties.

Signed-off-by: Matthew Gill <[email protected]>
The extractAnnotations methods hadunused variables, which have been
removed. The ServerVariables weren't being added as they were being
added directly, then replaced with a blank map. This has been fixed, as
well as the UnmodifiableCollection modification issues caused by fixing
it.

Signed-off-by: Matthew Gill <[email protected]>
OpenAPI should now scan and handle exception mappers. content annotation
values with only one @content value now set the content value
immediately, as they assume they're the only one.

Signed-off-by: Matthew Gill <[email protected]>
When parsing the @parameters annotation, the information wouldn' be
retained as the method looks for the result of @PathParam etc, which
hasn't been parsed as the method annotations are found first.

The Schema implementation handling is done earlier, to allow annotation
values to override the implementation class information.

Signed-off-by: Matthew Gill <[email protected]>
Schemas created as part of a @callback annotation didn't parse the
implementation properly. The implementation logic is now performed as
part of the initial creation, and also uses the ARRAY type properly.

Signed-off-by: Matthew Gill <[email protected]>
The TCK passes JSON in uppercase to the format query parameter to test
it. The field isnow case insensitive.

Signed-off-by: Matthew Gill <[email protected]>
Implemented annotation parsing for the new annotation.

Signed-off-by: Matthew Gill <[email protected]>
Properties marked as 'required' weren't added correctly to their parent
schemas.

Signed-off-by: Matthew Gill <[email protected]>
@Schema annotations are now parsed correctly when attached
to accessor methods. @XmlRootElement annotations are also now allowed to
denote Schema classes.

Signed-off-by: Matthew Gill <[email protected]>
@MattGill98 MattGill98 self-assigned this Dec 21, 2020
@MattGill98 MattGill98 removed the request for review from jGauravGupta December 21, 2020 16:28
Enums were including HK2 data in the enumeration object, rather than the
enum value name. This commit tests and fixes this behaviour.

Signed-off-by: Matthew Gill <[email protected]>
@jGauravGupta jGauravGupta self-requested a review December 22, 2020 09:24
@Pandrex247
Copy link
Member

[ERROR] /C:/Users/AndrewPielage/Git/Payara/appserver/payara-appserver-modules/microprofile/openapi/src/main/java/fish/payara/microprofile/openapi/impl/processor/ApplicationProcessor.java:[1151,43] cannot find symbol
[ERROR]   symbol:   method getExceptionTypes()
[ERROR]   location: variable method of type org.glassfish.hk2.classmodel.reflect.MethodModel

Sad panda

@MattGill98
Copy link
Contributor Author

MattGill98 commented Dec 22, 2020

I've just realised, this comes with an HK2 patch!

Upgraded HK2 to support exception type parsing

Signed-off-by: Matthew Gill <[email protected]>
Indices in paths should be accessed by using square brackets. The syntax
without this is ambiguous ans interferes with fetching response codes
e.g. 500 as this will be interpreted as an index in the array.

Signed-off-by: Matthew Gill <[email protected]>
When encountered in the wrong order, exception mappers weren't
respected. @APIResponseSchema also broke on exception mappers. This
commit fixes this basic behaviour.

Signed-off-by: Matthew Gill <[email protected]>
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 gone through the code with a fine-toothed comb, but passes the OpenAPI TCK and seems to work.

Copy link
Contributor

@MarkWareham MarkWareham left a comment

Choose a reason for hiding this comment

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

Partial review as Andrew got there before me.

No required changes, but some cleanup suggestions. There's a lot of superfluous null checking going on.

The server add function had reasonably high complexity, which has been
reduced. ExtensibleImpl had been missed in the mass collection fix, so
this has been kept consistent with the rest of the collection methods.

Signed-off-by: Matthew Gill <[email protected]>
ContentImpl merge function was being used in creation of Contents, which
causes the code to be entangled and difficult to debug. There was no
reason to merge them, so they've been replaced with simpler add
functions.

Signed-off-by: Matthew Gill <[email protected]>
@MattGill98
Copy link
Contributor Author

jenkins test please

@MattGill98 MattGill98 merged commit 181dc1c into payara:master Dec 23, 2020
JamesHillyard pushed a commit to JamesHillyard/Payara that referenced this pull request Sep 17, 2021
FISH-663 MicroProfile OpenAPI 2.0 (MP 4.0)
JamesHillyard pushed a commit to JamesHillyard/Payara that referenced this pull request Sep 17, 2021
FISH-663 MicroProfile OpenAPI 2.0 (MP 4.0)
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.

Payara Micro 5.2020.5: OpenAPI document creation failed / FISH-760
4 participants