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

Use MP OpenAPI 1.2 and adapt to SmallRye OpenAPI 2.0.26 which supports it #3294

Merged
merged 6 commits into from
Aug 23, 2021
Merged

Use MP OpenAPI 1.2 and adapt to SmallRye OpenAPI 2.0.26 which supports it #3294

merged 6 commits into from
Aug 23, 2021

Conversation

tjquinno
Copy link
Member

@tjquinno tjquinno commented Aug 18, 2021

Resolves #2671

Adopting MP OpenAPI 1.2 is simple in concept. There were no API changes.

We have been layering on SmallRye's OpenAPI implementation 1.x, and SmallRye built against MP OpenAPI 1.2 only starting with 2.0.26. That means we needed to step up to that release also.

There were several API changes in the SmallRye implementation, but almost all are internal to our code.

Customer-visible changes

Backward-incompatible changes

  1. OpenAPI documents permit the use of $ref in input and output declarations to point to definitions elsewhere, rather than in-lining them (perhaps repeatedly). SmallRye used to support a config flag to disable that but in 2.0.26 that support is always on.

    Helidon exposed an OpenAPI config setting schema-references.enable for this. With this PR Helidon logs a warning if the user sets this to any value, announcing that references are always supported now.

    We expect it is extremely rare that users would have set this value, and even rarer that they would have set it to false. This should be a minimal-impact change.

    To preserve the behavior, we would have to add significant code in Helidon's OpenAPI implementation to disable $ref and the cost seems far higher than the benefit.

  2. An internal Helidon class OpenAPIConfigImpl implements a SmallRye OpenAPI interface. That interface has changed four method return types from Set<String> to Pattern and removed two methods. This Helidon class is public in helidon/openapi only so our code in helidon/microprofile/openapi can use it. In fact, it resides in the io.helidon.openapi.internal package (which, yes, goes against our coding conventions but turns out to be useful for situations such as this!) to reinforce that users should not use it. Users have no need to use this class directly, in fact.

We will document these changes in a release note with 2.4.0 but impact on real users is very unlikely.

New warnings

Earlier releases of Helidon silently accepted illegal OpenAPI documents which did not enclose HTTP status values (keys in the responses section` in quotes. The OpenAPI spec says they should be quoted.

With this PR, Helidon will continue to accept such documents but will log warnings about unquoted HTTP status values.

Changes in SmallRye -> Changes in Helidon

Here are the changes in SmallRye and, in parentheses, the Helidon files that changed as a result.

  1. Change in maven packaging - we now depend on the io.smallrye.openapi.core artifact/module (pom.xml, module-info.java).
  2. Moved Format class from OpenApiSerializer to its own class (OpenAPISupport).
  3. MergeUtil.merge method which combines two in-memory OpenAPI models now skips the openapi element in the input models, so we have to supply it ourselves explicitly in the resulting new document. (OpenAPISupport).
  4. OpenApiAnnotationScanner#scan changed its return type from OpenAPIImpl to OpenAPI (OpenAPISupport).
  5. SmallRye OpenAPIConfig supported a setting to control whether $ref references were followed or not, defaulted to true. This is not a standard setting from the MP OpenAPI spec. We let users assign this setting in previous Helidon releases, but because SmallRye no longer honors it we cannot either. It is very unlikely that users would have turned this off, but the PR preserves the now-obsolete methods and logs a warning if the user explicitly sets the value, either via the API directly (they should not be using the class anyway) or in config. (OpenAPIConfigImpl)
  6. SmallRye ReferenceImpl class changed field name from $ref to ref. (ExpandedTypeDescription, ImplTypeDescription, Serializer, new tests)
  7. SmallRye changed how they express class and package inclusions and exclusions or scanning. They used to be Set<String>, but now they are Patterns. Helidon never exposed this to users, except via an internal public class which users should not ever use. (OpenAPIConfigImpl, MPOpenAPIBuilder)

Signed-off-by: [email protected] [email protected]

@tjquinno tjquinno added this to the 2.4.0 milestone Aug 18, 2021
@tjquinno tjquinno self-assigned this Aug 18, 2021
@@ -37,7 +37,7 @@ paths:
type: integer
format: int32
responses:
200:
'200':
Copy link
Member

Choose a reason for hiding this comment

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

Is this required? This seems like quite a big backward incompatible change - if somebody uses numbers in current version, will this fail?

Copy link
Member Author

Choose a reason for hiding this comment

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

A document which excludes the quotes around the HTTP status codes for response is in violation of the OpenAPI spec: https://swagger.io/specification/#responses-object:

HTTP status code field: ...This field MUST be enclosed in quotation marks (for example, "200") for compatibility between JSON and YAML.

Our test file should never have omitted the quotation marks. That said, in the past we would accept this error in documents. I am looking at how complicated it would be to customize the parsing to continue to accept this type of error (and log warnings as we do so).

Copy link
Member Author

Choose a reason for hiding this comment

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

Flagging these with warnings while accepting them is relatively simple. Another push will arrive shortly.

tomas-langer
tomas-langer previously approved these changes Aug 19, 2021
Copy link
Member

@tomas-langer tomas-langer left a comment

Choose a reason for hiding this comment

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

LGTM

@barchetta barchetta added the dependencies Pull requests that update a dependency file label Aug 19, 2021
@barchetta barchetta mentioned this pull request Aug 19, 2021
23 tasks
@tjquinno
Copy link
Member Author

I am seeing intermittent failures in an OpenAPI MP test, both in the pipelines and locally. The symptom is that during BasicServerTest the server responds to a GET to the /openapi endpoint with a valid document, but it's not the correct one. It is the document expected for one of the other tests. There are no other servers running on my system other than those created by the tests. Still digging.

…set the 'openapi' element when merging documents (because SmallRye skips that during merge)
…annotation processing, add some logging, and use method reference instead of a lambda
…r reporting in TestUtil, and update pom.xml for testing changes
@tjquinno
Copy link
Member Author

The preceding commits took care of the intermittent test failures.

@tjquinno tjquinno requested a review from tomas-langer August 21, 2021 10:45
Copy link
Member

@tomas-langer tomas-langer left a comment

Choose a reason for hiding this comment

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

LGTM

@tjquinno tjquinno merged commit 8a8d411 into helidon-io:master Aug 23, 2021
@tjquinno tjquinno deleted the mpopenapi-1.2rc1 branch August 23, 2021 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update SmallRye OpenAPI to 2.0.x
3 participants