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

Add support for OpenAPI 3 #181

Draft
wants to merge 15 commits into
base: master
Choose a base branch
from
Draft

Add support for OpenAPI 3 #181

wants to merge 15 commits into from

Conversation

tpburch
Copy link
Contributor

@tpburch tpburch commented May 19, 2020

Addresses #146

When completed, this PR will add support for OpenAPI 3. This necessarily a very invasive change to the codebase. Due to this, I wanted to get a draft of the partial changes out for initial feedback on the approach I am taking.

The major changes in this initial draft are:

  • I created an abstraction to separate the swagger/openapi logic from the Hapi logic. ApiDto exposes an interface that is used to register the Hapi components. The Dto uses the strategy pattern to map either a swagger or openapi document into the interface.

  • I did some significant refactoring of index.js, mostly to help me understand what all needed to be done. For the most part, this file was just rearranged, and can be put back if desired.

  • routes.js was rewritten to consume ApiDto. Part of the existing logic here was moved into the new mappers, while the Hapi-facing logic was all kept in routes.js.

  • Unit testing: most of the existing unit tests have been updated to run against both an oas2 and oas3 schema. This involved branching /test/fixtures/defs into "oas2" and "oas3" directories, where the "oas2" directory are the pre-existing versions and the "oas3" directory contains a converted version of the same schema. Each test is then run once for each schema file. While this makes the unit tests a little more involved, I thought the benefit of ensuring compatibility across the board was worth it. With this pattern established, it will be easier to ensure future changes are compatible with both schema versions.

Next steps:

  • Some more work needs to be done on unifying the structure of parameters across schema versions. The goal is to simplify the logic of what we send to enjoi, what modifications we have to make to the resulting joi schema, and how parameters are handled in preRequest coercion.

  • Parameter serialization: the schema support for this is pretty different between oas2 and oas3. The cases currently unit tested work, but others aren't supported yet. Some more testing needs to be added for other serialization types.

  • File uploads: this is another area where oas2 & oas3 are fairly different.

As this is a draft PR - I am very interested in any comments, concerns, gotchas, etc.

Tim Burch added 5 commits June 14, 2020 11:41
Each of the fixtures/defs have been branched to oas2 and oas3 versions.
Rather than using the "official" petstore OpenAPI 3 samples, I converted
the existing ones to ensure they are the same functionally.

Most of the unit tests have been updated to run both oas2 and oas3
versions to ensure all existing functionality works for both versions.

There are several issues that still need to be solved:

- oas3 servers: Right now, I am assuming there will be one server and
it's value will essentially be "basePath". I'd like to change this to
handle a full server declaration, probably using URL to parse it and get
to the correct basePath.

- oas3 file parameters: These are pretty different in oas3 and oas2. I
haven't looked to deeply at this yet - still a TODO.

- parameter serialization: In oas3, "collectionFormat" is used. Is oas3,
    this is split between "style" and "explode". Existing unit test
    cases are passing, but that is mostly a happy coincidence. Better
    support needs to be added.

- parameter schemas/templates: I'd like to find a better way to unify
oas2 and oas3 parameter structures so we can avoid a bunch of "if"
checks everywhere. I was initially trying to flatten oas3 parameters
to be more like oas2. I am now thinking that it might be better to do
the opposite and separate "parameter" metadata and the parameter
"schema".
@tlivings
Copy link
Contributor

There are now 2 PRs for this (#183 and this one).

Can we make sure we converge on one?

@tpburch
Copy link
Contributor Author

tpburch commented Jun 27, 2020

@tlivings @sprootshift Agreed - we should probably determine an overall direction. This PR is definitely a bigger overall design change, which may not be the direction you want to go on this. My goal is to centralize the logic differences needed to handle different schema versions.

I think the approach in #183 is equally valid. It will likely result in fewer overall changes, but the specifics of both OAS2 and OAS3 schemas will be more spread out throughout the library.

It looks like #183 is handling the schema version differences in regards to the interaction with enjoi. There are a few other areas where OAS2 & OAS3 differ that need to be handled, assuming the goal is feature parity for schema versions.

Which way would you like to go with this?

@tlivings
Copy link
Contributor

Can @tpburch and @sprootshift come to an alignment on the change?

Thanks.

@sprootshift
Copy link
Contributor

I think these are somewhat independent PRs. I agree with @tpburch re "#183 likely result in fewer overall changes, but the specifics of both OAS2 and OAS3 schemas will be more spread out throughout the library". I consider #183 a feature expansion and #181 a refactoring to isolate OpenAPI 2 and 3 schema differences and hide behind a common interface. Although some isolation is already provided by swagger-parser.

@tpburch
Copy link
Contributor Author

tpburch commented Jul 6, 2020

With the recent announcement that Hapi will be sunset, my use case for these changes has basically evaporated. Based on that, it probably makes sense to go ahead with #183 for the overall functionality.

@sprootshift - Having said that, feel free to use any or all of the changes in this PR if you feel like it will help and/or add additional functionality.

@tlivings
Copy link
Contributor

tlivings commented Jul 6, 2020

Hapi will continue to be supported for 2 years minimum and is unlikely to be going away in the near term.

@mahnunchik
Copy link

Is this project dead?

@tlivings
Copy link
Contributor

tlivings commented Dec 3, 2020

No, but I’m the only maintainer left from the team and don’t have a lot of time to focus on it these days.

I appreciate and try to keep up with PRs as they come but ideally a couple of additional maintainers can start helping out.

Copy link
Contributor

@dcharbonnier dcharbonnier left a comment

Choose a reason for hiding this comment

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

@tlivings why don't you create an open-api-3 branch or a future branch so we can get something merged and make progress from there? I would probably use this package if we manage to get it stable enough and this would help to make progress

package-lock=true
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can just remove this file, this is the default. Never seen a project with a .npmrc file

Copy link
Contributor

Choose a reason for hiding this comment

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

I have create a new branch next to land changes in.

In the case of this particular .npmrc file, I think it can be removed. Checking one in can be useful at times.

@dcharbonnier
Copy link
Contributor

I guess we can close this ont @tlivings

@dcharbonnier dcharbonnier mentioned this pull request Dec 6, 2021
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.

5 participants