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

Initial OpenAPI v3 support #177

Merged
merged 39 commits into from Feb 11, 2019

Conversation

2 participants
@blast-hardcheese
Copy link
Collaborator

blast-hardcheese commented Jan 26, 2019

Resolves #54

Continuing work from @stanislav-chetvertkov on supporting the OpenAPI v3 specification.

  • Compiles with some shims, reflection, and casting
  • Tests all pass
  • Reducing reliance on casting
  • Adding dense OpenAPI v3 specification test

No API changes are expected to the consumer API, meaning that while OpenAPI 3.x specifications are able to be used to generate clients and servers, full and correct adherence to the specification is not a goal of this PR.

Check the OpenAPI 3.x milestone for full support.

@blast-hardcheese blast-hardcheese added this to the OpenAPI 3.x milestone Feb 2, 2019

@blast-hardcheese blast-hardcheese changed the title OpenAPI v3 support Initial OpenAPI v3 support Feb 2, 2019

@blast-hardcheese blast-hardcheese force-pushed the blast-hardcheese:openapi-v3 branch 4 times, most recently from 6677b5e to 711065f Feb 3, 2019

@blast-hardcheese blast-hardcheese force-pushed the blast-hardcheese:openapi-v3 branch 7 times, most recently from 9774602 to 300ee4c Feb 3, 2019

@@ -43,6 +43,8 @@ class Issue166 extends FunSuite with Matchers with SwaggerSpecRunner {
test("Handle generation of models") {
val opts = new ParseOptions()
opts.setResolve(true)
opts.setResolveFully(true)
opts.setResolveCombinators(true)

This comment has been minimized.

This comment has been minimized.

@blast-hardcheese

blast-hardcheese Feb 4, 2019

Author Collaborator

I couldn't find any documentation either, but from my experience, it walks the model after parsing and links $refs -- at least for now, this is advantageous, as the heuristic of get$ref.split("/").last will fail when $ref: #/structures/api1/Pet and $ref: #/structures/api2/Pet. There needs to be a better way to disambiguate.

@@ -140,7 +140,7 @@ object ScalaParameter {
resolved <- SwaggerUtil.ResolvedType.resolve[L, F](meta, protocolElems)
SwaggerUtil.Resolved(paramType, _, baseDefaultValue) = resolved

required = parameter.getRequired()
required = Option[java.lang.Boolean](parameter.getRequired()).fold(false)(identity)

This comment has been minimized.

@stanislav-chetvertkov

stanislav-chetvertkov Feb 4, 2019

Collaborator

I still prefer having extension methods defined for swagger model entities to reduce boilerplate for such occasions

This comment has been minimized.

@blast-hardcheese

blast-hardcheese Feb 4, 2019

Author Collaborator

I'm in favor as well. My goal after this is to push all OpenAPI knowledge to its own algebra, so future changes can be done from the outside without exposing so much knowledge (Option, exceptions, $ref, tree-walking) into the core generators. I didn't know this was absolutely necessary before starting this work, but now I'm leaning very heavily in favor of that strategy. It's too much noise right now.

@stanislav-chetvertkov
Copy link
Collaborator

stanislav-chetvertkov left a comment

Thanks for splitting it into such small commits, was really easy to follow

@blast-hardcheese blast-hardcheese force-pushed the blast-hardcheese:openapi-v3 branch from 300ee4c to 4293262 Feb 11, 2019

@blast-hardcheese blast-hardcheese force-pushed the blast-hardcheese:openapi-v3 branch from 4293262 to dacc72d Feb 11, 2019

@blast-hardcheese

This comment has been minimized.

Copy link
Collaborator Author

blast-hardcheese commented Feb 11, 2019

  • Rebased off master
  • Ensured that the source diffs of the sample project are identical between master and this branch

There are some known omissions from this initial attempt at OpenAPI v3. While I'm sure there are others, this is the initial list:

#/components/requestBodies are not considered at all

This will require a complete overhaul of how Content-Types are handled. Instead of hacking things together, I'd much rather have a more clear strategy of supporting different content types natively in generated clients and servers. This may coincide with work to unify identical definitions between different runs as well.

Multiple Content-Types for individual routes

Currently, to the best of our abilities, the new structure is shimmed into the old structure. This is likely not going to work for anything but the most trivial of OpenAPI v3 schemas. v2 should continue to work unchanged, as the v2 -> v3 converter seems to do a good job maintaining compatibility.

@blast-hardcheese blast-hardcheese merged commit 2a8a6ef into twilio:master Feb 11, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@blast-hardcheese blast-hardcheese deleted the blast-hardcheese:openapi-v3 branch Feb 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment