[prototype]Upgrade Swagger 2 to OpenAPI 3 #916

jannyHou · 2018-01-26T19:08:22Z

connect to #753

Checklist

npm test passes on your machine
New tests added or existing tests modified to cover all changes
Code conforms with the style guide
Related API Documentation was updated
Affected artifact templates in packages/cli were updated
Affected example projects in packages/example-* were updated

Here is some quick notes for review this PR, file changes are huge, but the main change is in:

package openapi-v3
parser function in rest package rest/src/parser.ts

The other ones are just the api upgrade from v2-->v3 and dependency change in package.json.
Writing more details for migration doc but it would not block the review.

Migration guide

https://github.com/strongloop/loopback-next/pull/916/files#diff-c1ba763eefca769778124b8f1c5c6b3f

Package Rename

openapi-spec ---> openapi-spec-types
openapi-v2 ---> openapi-v3

New code

response spec
- openapi-spec-builder
server spec
- openapi-v3/controller-spec
- rest/src/utils/url-generator
- rest/test/*/routing.test
- rest/test/*/routing.table.test
schema spec
- everything in package openapi-v3
  - break down the old controller-spec into 6 files
  - rewrite @param
  - add @requestBody
new validator
- test-lab
build arguments
- rest/src/parser.ts

Discussion

name all interfaces with version as prefix, e.g. OAS3SchemaObject
merge openapi-spec-builder and openapi-v3?
and the questions in some code comments and migration.md file

jannyHou · 2018-01-30T22:53:51Z

packages/openapi-v3/test/unit/controller-spec/json-to-schema-object.test.ts

@@ -42,16 +42,6 @@ describe('jsonToSchemaObject', () => {
    propertyConversionTest(allOfDef, expectedAllOf);
  });

-  it('converts definitions', () => {


The OpenAPI3 schemaObject does not have definitions under it.

shimks · 2018-01-31T20:16:53Z

packages/openapi-v3/src/controller-spec.ts

@@ -270,12 +274,6 @@ export function jsonToSchemaObject(jsonDef: JsonDefinition): SchemaObject {
        result.allOf = _.map(json.allOf, item => jsonToSchemaObject(item));
        break;
      }
-      case 'definitions': {


Please preserve the conversion here as we've discussed

Talked with @shimks , the broken scenario will be:

@model class Foo { @property bar: Bar; @property baz: Baz; }; @model class Bar { @property name: string }; @model class Baz { @property name: string }; class FooController { @get('/foo') find(@param(<a_param_spec>) foo: Foo): void { } }

Addressed it in my PR.

shimks · 2018-01-31T20:18:06Z

packages/openapi-v3/src/controller-spec.ts

        if (openapiSchema.definitions) {
          for (const key in openapiSchema.definitions) {
-            spec.definitions[key] = openapiSchema.definitions[key];


Make sure with the newest conversion introduced, that nested components/schemas don't exist. May need to fiddle around with the test to make sure this is the case.

jannyHou · 2018-02-01T20:08:08Z

shimks · 2018-02-06T15:59:00Z

@jannyHou I noticed that #181 covers adding in tests for openapi-v3. So no need to add it here (unless you consider that task to be a part of this one)

jannyHou · 2018-02-06T16:14:22Z

@shimks thanks for reminding :) nope I didn't plan to add tests for openapi-v3-spec.

jannyHou · 2018-02-14T16:33:11Z

packages/rest/src/parser.ts

+    ];
+    requestBodyIndex = i ? i : 0;
+  }
+  if (


I am refactoring the requestBody insert function to make the logic neater then these.

raymondfeng · 2018-02-14T16:56:02Z

package.json

    "test": "node packages/build/bin/run-nyc npm run mocha",
-    "mocha": "node packages/build/bin/run-mocha \"packages/*/DIST/test/**/*.js\" \"packages/cli/test/*.js\"",
+    "mocha":
+      "node packages/build/bin/run-mocha \"packages/*/DIST/test/**/*.js\" \"packages/cli/test/*.js\"",


The formatting changes for package.json should be reverted. Make sure you IDE won't format it automatically.

jannyHou · 2018-02-14T18:50:12Z

packages/rest/src/rest-server.ts

+        port: opts.port || 3000,
+        // hardcoded now, about allowing configured `basePath`,
+        // see issue https://github.com/strongloop/loopback-next/issues/914
+        // is it '/' or '/api'?


The explorer is loaded fine, so I think '/' is good.

jannyHou · 2018-02-14T18:56:23Z

...-v3/test/unit/controller-spec/controller-decorators/request-body-decorators/shortcut.test.ts

+      greetWithArray(@requestBody() name: string[]) {}
+      @post('/greetingWithObject')
+      greetWitObejct(@requestBody() name: object) {}
+      // @post('/greetingWithInteger')


shortcut hasn't implemented yet

bajtos

@jannyHou great work!

I took a quick and relatively high-level look at your proposed changes. See my comments below, but please don't address them yet!

In order to speed up review of this huge pull request, and to avoid too many merge conflicts, I am proposing to treat this pull request as a ~~spike~~ prototype demonstrating the direction we want to take (see https://gist.github.com/bajtos/58eb9a14ad563a3e7a349ad6a4d0f965#major-changes-and-large-patches) and start making the actual changes in a series of smaller follow-up pull requests.

PullRequest1 - OAIv3 types. Start by adding a new package openapi-spec-types. This package will provide types needed to support OpenAPISpec v3 only (no Swagger please). Keep the old openapi-spec package around with no modifications, so that all existing code keeps working. On the second though, I think openapi-v3-types would be a better package name?
PullRequest2 - OAIv3 decorators. Copy openapi-v2 to openapi-v3 and upgrade it to OAI v3. The first commit should be just a verbatim copy of all openapi-v2 files to openapi-v3, the follow-up commits should make the necessary changes. This way we can review what stays same and what is changing compared to v2. Keep openapi-v2 around with no modifications, this way all existing consumers will stay functional.
PullRequest3 - Upgrade REST from Swagger to OAIv3. The final integration of the new code added by your previous two pull requests and the existing REST layer.
PullRequest4 - Remove everything Swagger related. Packages like openapi-v2 and openapi-spec can be deleted now.

It may be possible to split some of the steps above into even smaller chunks. Please do so if you can find such a meaningful split.

A comment related to this big pull request:

This diff entry looks suspicious to me:

packages/openapi-v2/CHANGELOG.md → packages/openapi-v3/CHANGELOG.md`

I think the new module should start with an empty CHANGELOG?

bajtos · 2018-02-15T11:46:16Z

packages/openapi-spec-types/README.md

+
+* easy to extract common suger interfaces
+* less packages to maintain, clear layout to manage different versions
+* if we only support one version, export types from one package, no need to update each dependant


I believe we have already agreed to support only a single version of OpenAPI spec?

IMO, we should have a single repo describing only the current (latest) version of OpenAPI spec.

When a new, backwards-incompatible version of OAI is released, then we will release a new semver-major version of this package.

IMO, we should have a single repo describing only the current (latest) version of OpenAPI spec.

When a new, backwards-incompatible version of OAI is released, then we will release a new semver-major version of this package.

Let me take that back. Considering the effort needed to upgrade between OAI versions, I think it's better to have a new package for each major revision of OAI spec, because it makes it easier for us to split the work into independent chunks.

bajtos · 2018-02-15T11:52:40Z

packages/openapi-spec-types/openai-3-migration.md

+Discussion:
+
+If people decorate an argument with no input as `@requestBody() foo: Foo`, 
+I make `application/json` as the default content, is it ok?


Yes please.

As a user building JSON/REST APIs, I'd prefer to define request-body parameters as follows:

async createTodo( @requestBody(Todo) todo: Todo, ) { }

In my example, @requestBody(Todo) performs two steps under the hood:

call repository-json-schema to convert Todo model definition to JSON Schema (see feat(json-schema): add modules to convert TS models to JSON Schema #860 by @shimks)

convert this "shortcut" definition into a valid request body spec, e.g.
{ content: { 'application/json': { schema: TodoSchema } } }

However, considering how large this patch already is, let's leave this syntactic sugar out of scope of this initial pull request (but in the scope of the OAIv3 user story!)

bajtos · 2018-02-15T11:54:38Z

packages/openapi-spec-types/openai-3-migration.md

+
+## Parameter Serialization
+
+* Complex Serialization in Form Data


Could you please add more details, e.g. a sample HTTP request? I think this can (and should) be left out of the initial pull request, possibly out of the MVP scope too.

@bajtos I think Raymond is working on serialization in #941.

Oops, these are some notes I wrote when doing investigation, sorry for causing confusion! should remove them.

bajtos · 2018-02-15T11:55:27Z

packages/openapi-spec-types/openai-3-migration.md

+## Parameter Serialization
+
+* Complex Serialization in Form Data
+* Serialization in parameter


I am afraid I have no idea what does this mean, could you please add more details?

bajtos · 2018-02-15T11:57:29Z

packages/openapi-spec-types/openai-3-migration.md

+## additional properties
+
+Dictionaries, HashMaps and Associative Arrays
+https://swagger.io/docs/specification/data-models/dictionaries/


Similarly here. Is this something already done and missing migration docs, or is it something that needs implementation? I am proposing to leave it out of the initial pull request, possible out of the MVP scope.

bajtos · 2018-02-15T12:28:55Z

packages/testlab/src/validate-api-spec.ts

-import * as SwaggerParser from 'swagger-parser';
-import {OpenApiSpec} from '@loopback/openapi-spec';
+import {OpenApiSpec} from '@loopback/openapi-spec-types';
+const validator = require('swagger2openapi/validate.js');


Is the TypeScript compiler able to infer the type of validator, or is it falling back to default any? It would be great to have an explicit type specified for this function, even if it means we have to write it ourselves.

Actually, I care most about the type of promisifiedValidator, so if we are going to write the type def ourselves, then it's enough to describe promisifiedValidator.

@bajtos hmm, could you elaborate more about

Is the TypeScript compiler able to infer the type of validator

?
Would Function the expected type or you also expect the TypeScript compiler to know its input&return types?

Would Function the expected type or you also expect the TypeScript compiler to know its input&return types?

Yes, I'd like the typescript compiler to know the type of input arguments and the type of the returned value.

bajtos · 2018-02-15T12:29:35Z

packages/testlab/src/validate-api-spec.ts

+const validator = require('swagger2openapi/validate.js');
+import * as util from 'util';
+const promisify = util.promisify;
+const promisifiedValidator = promisify(validator.validate);


I find this naming convention rather unusual. Functions and methods should start with a verb.

I am proposing to use the name promisifyAsync instead.

I am proposing to use the name promisifyAsync instead.

Uh oh, I meant validateAsync.

bajtos · 2018-02-15T12:30:11Z

packages/testlab/src/validate-api-spec.ts

-  // TODO(bajtos) contribute these improvements to swagger-parser
-  if (!spec.swagger) {
+  const opts = {};
+  if (!spec.openapi) {


Are these explicit checks still required? I added them only because swagger-parser was reporting unhelpful errors in these edge cases.

just copy paste them from the old swagger check :-p I thought they are pre-check to prevent the complicated schema validation afterwards.

honestly...I haven't got time to check the edge cases missing in v3 validator. I will do a quick spike in the subsequent PR.

bajtos · 2018-02-15T12:33:03Z

packages/testlab/src/validate-api-spec.ts

+  try {
+    await promisifiedValidator(spec, opts);
+  } catch (err) {
+    throw new Error(err);


I find this suspicious. Are you wrapping err in a new Error object because it's a plain string? If that's the case, then you should add a check to avoid wrapping Error instances (e.g. TypeError) and losing the original stack trace.

} catch (err) { if (err instanceof Error) { throw err; } else { throw new Error(err); } }

Eventually, we should fix the upstream module swagger2openapi to correctly throw Error instances instead of string error messages.

Hopefully all instances of this should have been fixed in swagger2openapi some time ago, but just a heads-up that in the upcoming v4.0.0 release, swagger2openapi will declare, export and throw a S2OError class so you will be able to identify intentionally thrown errors from any other problems.

Thank you @MikeRalphson for chiming in. We have moved away from swagger2openapi and use oas-validator instead, see https://github.com/Mermade/oas-kit/tree/master/packages/oas-validator and https://www.npmjs.com/package/oas-validator

Unfortunately, it seems that we are still rewrapping errors in a new Error object :(

https://github.com/strongloop/loopback-next/blob/d2b79f6224db47b3d3a738eb3aa4c4276afb49b9/packages/testlab/src/validate-api-spec.ts#L15-L19

bajtos · 2018-02-15T12:38:44Z

...i-v3/test/unit/controller-spec/controller-decorators/param-decorators/param.shortcut.test.ts

+    expect(params[7].schema.type).to.eql('string');
+    expect(params[7].schema.format).to.eql('date-time');
+    expect(params[8].schema.type).to.eql('string');
+    expect(params[8].schema.format).to.eql('password');


Ditto, one logical assertion per test please.

bajtos · 2018-02-15T14:50:42Z

@jannyHou

PullRequest2 - OAIv3 decorators. Copy openapi-v2 to openapi-v3 and upgrade it to OAI v3. The first commit should be just a verbatim copy of all openapi-v2 files to openapi-v3, the follow-up commits should make the necessary changes. This way we can review what stays same and what is changing compared to v2. Keep openapi-v2 around with no modifications, this way all existing consumers will stay functional.

On the second thought, it may be better to split this work in two pull requests: the first one copying openapi-v2 to openapi-v3 and perhaps changing module name in all places like package.json and license headers, the second PR making the actual upgrade. That way we can review all changes in the second pull request and still see only the relevant upgrade.

The downside of my proposed approach is that git will no longer preserve history of openapi-v2 changes made in the past in the commit history of openapi-v3. (In your current proposal, the history is preserved across rename.)

Are there any other downsides to consider? Please do speak up if my proposed splitting is not practical!

jannyHou · 2018-02-15T15:00:27Z

@bajtos

The downside of my proposed approach is that git will no longer preserve history of openapi-v2 changes made in the past in the commit history of openapi-v3

This is also my concern, while ideally....there shouldn't be any new features going into openapi-v2 in the short term, I can manually check fixes as a workaround.

jannyHou · 2018-02-15T15:33:17Z

I am proposing to treat this pull request as a spike prototype demonstrating the direction we want to take (see https://gist.github.com/bajtos/58eb9a14ad563a3e7a349ad6a4d0f965#major-changes-and-large-patches) and start making the actual changes in a series of smaller follow-up pull requests.

I was talking with Raymond about the same thing yesterday 😐 this PR is more like a PoC, the tricky thing for OpenAPI upgrade is, any partial upgrade of the spec cannot check in until all the specs are upgraded to generate a complete 3.0.0 spec file.

@jannyHou Would it be possible to split the PR into the following:

TS models for OpenAPI v3

REST decorators for OpenAPI v3

Replacing the current v2 with v3

Janny Hou [12:25 PM]
thank you for looking into the huge change! That sounds a reasonable plan to me, I can create two new packages for v3:

types

decorators
They will live with v2 packages in a short term, then I have another PR to upgrade existing code from v2-->v3
and drop all v2 packages

He is suggesting a similar way to break it down. This is also good for us to discuss a proper name for new packages(or old package if we hold v2 and v3 in one package but different folders)
I will start with the PR for new types package first.

jannyHou · 2018-02-15T16:24:50Z

@bajtos Thank you for the review effort and all the feedbacks!
Based on our conversations, I would break down the PR into 4 ones:

types(PR merged)

create a new repository openapi-v3-types for new OpenAPI 3.0.0 Specification interfaces/types
the discussion regarding spec types may point to the code in this PR
~~update all related packages's dependencies to include "openapi-v3-types": "^4.0.0-alpha.1"~~
- add them in this PR to reduce the unrelated dependency changes in PRs afterwards
- those package having "openapi-spec" as dependency need to be updated
- the new package's name & should it be merged with v2 type package is still in discussion

break down `openapi-v2/src/controller-spec.ts`(PR merged)

this PR breaks down controller-spec.ts into 6 smaller ones according to their functionalities. It will be hard to do code compare between one old controller-spec.ts and 6 new ones
break it into the following files:
- controller.spec.ts
- generate-schema.ts
- json-to-schema.ts
- keys.ts
- parameter-decorator.ts

new operation decorators

create a new package openapi-v3(a proper name to be discussed)
in the first commit, copy paste all files from openapi-v2 for compare
copy paste the new src and test files in this PR to it
update related packages having "openapi-v2": "4.0.0-alpha.?" to include "openapi-v3": "alpha.1"

update the whole project from v2-v3

server spec
- may be helpful(haven't investigated): serverless-openapi-documentation
responses spec
rest/src/parser.ts
rest/src/rest-server.ts
@param.body --> @requestBody
@param.formData --> @requestBody
drop v2 if needed.

raymondfeng · 2018-03-07T20:12:36Z

@jannyHou Are we good to close this PR now?

jannyHou · 2018-03-07T22:55:54Z

All the related PRs are landed 🚢 I am closing this prototype PR, thanks again for all the review and suggestions!

raymondfeng · 2018-03-07T23:17:45Z

Great efforts!

jannyHou requested review from bajtos, kjdelisle and raymondfeng as code owners January 26, 2018 19:08

jannyHou added the needs-priority label Jan 26, 2018

jannyHou force-pushed the upgrade/v2-v3-copy branch from a9656fe to a8c0182 Compare January 26, 2018 19:32

jannyHou mentioned this pull request Jan 26, 2018

Better basePath support #914

Closed

jannyHou changed the title ~~[WIP, DO NOT REVIEW]Upgrade/v2 v3 copy~~ [WIP, DO NOT REVIEW]Upgrade/v2--->v3 Jan 27, 2018

This was referenced Jan 28, 2018

feat(rest): allow basePath in RestServer configuration #920

Closed

[DO NOT REVIEW, WIP]Upgrade/v2 v3 #845

Closed

jannyHou force-pushed the upgrade/v2-v3-copy branch 2 times, most recently from 69ee943 to dcd7808 Compare January 30, 2018 21:22

jannyHou mentioned this pull request Jan 30, 2018

Response code for POST method should be 201 not 200 #788

Closed

3 tasks

jannyHou force-pushed the upgrade/v2-v3-copy branch 3 times, most recently from 3e6faea to af3729b Compare January 30, 2018 22:42

jannyHou commented Jan 30, 2018

View reviewed changes

jannyHou force-pushed the upgrade/v2-v3-copy branch from af3729b to 1952aef Compare January 30, 2018 23:12

shimks reviewed Jan 31, 2018

View reviewed changes

jannyHou changed the title ~~[WIP, DO NOT REVIEW]Upgrade/v2--->v3~~ [WIP]Upgrade/v2--->v3 Feb 1, 2018

jannyHou commented Feb 14, 2018

View reviewed changes

raymondfeng reviewed Feb 14, 2018

View reviewed changes

jannyHou commented Feb 14, 2018

View reviewed changes

jannyHou force-pushed the upgrade/v2-v3-copy branch from 8b06e2e to d15def6 Compare February 14, 2018 19:54

jannyHou changed the title ~~[WIP]Upgrade/v2--->v3~~ Upgrade Swagger 2 to OpenAPI 3 Feb 14, 2018

jannyHou force-pushed the upgrade/v2-v3-copy branch 2 times, most recently from a9850d2 to e41f0b5 Compare February 14, 2018 20:30

jannyHou force-pushed the upgrade/v2-v3-copy branch 3 times, most recently from acb95cd to f810b7a Compare February 14, 2018 20:49

feat: upgrade types

5e6ee27

jannyHou force-pushed the upgrade/v2-v3-copy branch from f810b7a to 5e6ee27 Compare February 14, 2018 21:27

bajtos suggested changes Feb 15, 2018

View reviewed changes

bajtos assigned jannyHou Feb 15, 2018

jannyHou mentioned this pull request Feb 15, 2018

OpenAPI 3 feature: Support parameter location "in: cookie" #997

Closed

jannyHou mentioned this pull request Feb 15, 2018

feat: add openapi v3 types #999

Merged

6 tasks

jannyHou changed the title ~~Upgrade Swagger 2 to OpenAPI 3~~ [prototype]Upgrade Swagger 2 to OpenAPI 3 Feb 15, 2018

This was referenced Feb 20, 2018

refactor: break down controller-spec #1020

Merged

[OpenAPI] Switch to OpenAPI v3 for spec and routing #753

Closed

feat: adding openapi-v3 #1046

Merged

jannyHou mentioned this pull request Mar 1, 2018

oas3/upgrade #1067

Merged

6 tasks

jannyHou closed this Mar 7, 2018

jannyHou removed the needs-priority label Mar 7, 2018

jannyHou deleted the upgrade/v2-v3-copy branch May 17, 2018 21:17


		## Parameter Serialization

		* Complex Serialization in Form Data

[prototype]Upgrade Swagger 2 to OpenAPI 3 #916

[prototype]Upgrade Swagger 2 to OpenAPI 3 #916

Uh oh!

Conversation

jannyHou commented Jan 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Migration guide

Package Rename

New code

Discussion

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jannyHou Jan 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jannyHou commented Feb 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shimks commented Feb 6, 2018

Uh oh!

jannyHou commented Feb 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos Feb 15, 2018 • edited by jannyHou Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jannyHou Feb 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jannyHou commented Jan 26, 2018 •

edited

Loading

jannyHou Jan 31, 2018 •

edited

Loading

jannyHou commented Feb 1, 2018 •

edited

Loading

bajtos left a comment •

edited

Loading

bajtos Feb 15, 2018 •

edited by jannyHou

Loading

jannyHou Feb 15, 2018 •

edited

Loading

jannyHou commented Feb 15, 2018 •

edited

Loading

jannyHou commented Feb 15, 2018 •

edited

Loading

break down `openapi-v2/src/controller-spec.ts`(PR merged)