oas3/upgrade #1067

jannyHou · 2018-03-01T16:09:53Z

connect to: #753

Description

Upgrade the whole project to use openapi-v3

This is the 4rd PR of prototype PR: #916 (comment)

connect to issue: #753

Review

I would suggest review by each commit.

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

shimks

Looks 😎.
I think there are a couple of stuff that needs to be mentioned. Here's a list:

we need tests for the new parser logic (the one with requestBody indexing)
is todoSchema used anywhere else other than in the lines you've deleted? If it isn't, should we get rid of it or nah
have you checked for upgrading imports in the READMEs? If you did and haven't found any, ignore my comment.

bajtos · 2018-03-02T12:11:48Z

While I was reviewing the changes, I came across the issue of content type for YAML and OpenAPI Spec documents. While it's probably not relevant to the changes made in this pull request, I'd still like to post what I found for future use.

Ideally, /openapi.json should be returning application/json content type.

Eventually, when OpenAPI spec working group decides on the content type for OAI documents (see OAI/OpenAPI-Specification#110), we should switch to that one instead.

Their latest proposal (21 days old, see OAI/OpenAPI-Specification#110 (comment)):

application/vnd.oai.openapi (YAML variant)
application/vnd.oai.openapi+json (JSON only variant)

With an optional version parameter:

application/vnd.oai.openapi;version=3.0.0

As for /openapi.yaml endpoint, it looks like there is no official IANA-registered content type yet :( Looks like text/yaml may be the best option, even if it's not entirely spec-conforming. Alternatively, application/x-yaml and test/x-yaml are spec-conforming options we can use instead. See https://www.ietf.org/mail-archive/web/media-types/current/msg00693.html

bajtos

This is awesome 🎉

The last commit seems to contain formatting/linting fixes for previous commits, I assume you are going to squash the commits before landing.

Should we remove swagger (v2) related packages as part of this pull request too?

I have few (hopefully minor) comments to address, see below.

bajtos · 2018-03-02T11:49:51Z

packages/openapi-spec-builder/src/openapi-spec-builder.ts

+      content: {
+        // TODO(janny) will change it to a default value
+        // after we figure out the plan for content type
+        '*/*': {


IIRC, the intent of withStringResponse was to describe a response that returns text/plain, see https://github.com/strongloop/loopback-next/blob/ba13710378fd288550989135bc4447e26183b900/packages/rest/src/writer.ts#L47-L48

I am proposing to use text/plain instead of */*.

bajtos · 2018-03-02T11:51:44Z

packages/rest/src/parser.ts


+  let requestBodyIndex: number = -1;
+  if (hasArgumentsFromBody(operationSpec)) {
+    const i = (<RequestBodyObject>operationSpec.requestBody)[


Why is it necessary to cast operationSpec.requestBody to RequestBodyObject? I would expect the type definition for operation specs to already specify requestBody as being of type RequestBodyObject.

What am I missing?

requestBody could be RequestBodyObject or ReferenceObject, see: https://github.com/metadevpro/openapi3-ts/blob/master/src/model/OpenApi.ts#L99

I specify the type here to avoid resolving the ReferenceObject, I will add a comment here.

bajtos · 2018-03-02T11:54:40Z

packages/rest/src/parser.ts

+      args.push(body);
+      return;
+    }
+    for (const arg of paramArgs) {


Have you considered using Array.prototype.splice to insert the body parameter at the given index?

Something along the following lines:

paramArgs.splice(requestBodyIndex, 0, body); args = paramArgs;

bajtos · 2018-03-02T11:56:29Z

packages/rest/src/rest-server.ts

+    if (options.version !== '3.0.0') {
+      throw new Error(
+        'Swagger2 spec is not supported in rest server, ' +
+        'please upgrade it to OpenAPI3',


I think you need to disable routes /swagger.json and /swagger.yaml - these are the users of options.version = '2.0'. See https://github.com/strongloop/loopback-next/blob/ba13710378fd288550989135bc4447e26183b900/packages/rest/src/rest-server.ts#L48-L53

bajtos · 2018-03-02T12:06:06Z

packages/rest/test/integration/rest-server.integration.ts

      responses: {
        200: {
-          schema: {type: 'string'},
+          content: {'*/*': {schema: {type: 'string'}}},


Similarly here, the endpoint should specify text/plain as the response content type.

bajtos · 2018-03-02T12:06:10Z

packages/rest/test/integration/rest-server.integration.ts

      responses: {
        200: {
-          schema: {type: 'string'},
+          content: {'*/*': {schema: {type: 'string'}}},


bajtos · 2018-03-02T12:12:19Z

packages/openapi-v3/README.md

-                        description: 'The string result.',
-                        schema: { type: 'string' }
-                    }
-                },


Is it a valid controller spec when there are no responses described?

IMO, we should preserve the 200 response and describe it as returning text/plain content type.

This is probably a bug, I manually wrote the code to generate the spec from getControllerSpec().

According to the code in controller-spec

the responses should be {}.

we should preserve the 200 response and describe it as returning text/plain content type

Right now unless user provides responses spec in @operation/@api, we don't generate responses object for them....

We need a story/epic for adding responses leveraging controller function's return type.

I understand that it's a huge gap, and if you remember the two line of commented code I forgot to delete in the previous PR, I tried to add some simple discovery for the response schema, but stopped after realizing it's a big feature.

I am creating a story for it. Sorry I know this is a big gap but it's not possible to fit it in this PR or the upgrade story :(

Related story: #1086

bajtos · 2018-03-02T12:14:18Z

packages/example-log-extension/package.json

  "dependencies": {
    "@loopback/context": "^0.1.2",
    "@loopback/core": "^0.1.2",
+    "@loopback/openapi-v3": "^0.1.1",


Can we re-export decorators like get and param from @loopback/rest so that LoopBack consumers don't have to upgrade when we change the underlying spec (like from Swagger to OpenAPI)?

I am also thinking of re-exporting it or not doing that.

LoopBack consumers don't have to upgrade when we change the underlying spec (like from Swagger to OpenAPI)

Actually I would like to encourage them to change the importing module, like how they import @model and @property from @loopback/repository.
And we re-exported it before to avoid breaking change, while v2-->v3 upgrade is a breaking change anyway, so if we want to decouple controller decorators and rest package, this is a proper time.

Since this could be a user experience issue, I would like to hear more opinions from the team @strongloop/loopback-next :

Would you prefer to import controller decorators (e.g. @get, @post, @param, @requestBody) from @loopback/rest or @loopback/openapi-v3?

I think it's better to re-export the decorators from rest since it simplifies the user experience by abstracting away the underlying package.

As a user trying to create REST APIs, I want to be able to use the rest package to do so without having to understand what OpenAPI is / what it does, etc. I just want to be able to create the api.

+1 for what @virkt25 said 👍

cool @virkt25 thanks for the feedback :) Added the export code.

b-admike

Nice work! I've left some feedback

b-admike · 2018-03-02T15:53:32Z

packages/example-getting-started/src/controllers/todo.controller.ts

+  requestBody,
+} from '@loopback/openapi-v3';
 import {HttpErrors} from '@loopback/rest';
-import {TodoSchema, Todo} from '../models';


Can we get rid of TodoSchema definition if it is not used for anything else?

@b-admike ah, answered ^ in the last part of comment #1067 (comment)

b-admike · 2018-03-02T16:35:35Z

packages/rest/src/parser.ts

  pathParams: PathParameterValues,
  body?: MaybeBody,
 ): OperationArgs {
  const args: OperationArgs = [];


Guess we don't need args anymore since we have paramArgs defined below

args is for keeping all arguments, including parameters + requestBody
paramArgs is for keeping parameters only.

b-admike · 2018-03-02T16:44:49Z

packages/rest/test/integration/http-handler.integration.ts

-  });
-
-  context('with a body-parameter route', () => {
+  context('with a body request route', () => {


I presume there are tests coming here :)

don't trust the git compare :( the old test cases are updated, if you are talking about the acceptance tests, added them in commit 8e7570c :)

I think @b-admike might be referring to the formdata tests. I don't see them in the above commit. Does this have to do with how V3 handles the form data?

@virkt25 V3 doesn't have a concept of "form data", it always treats requestBody as a whole, you can describe requestBody by any complicated schema, but you could not specify an argument as part of a requestBody schema. So I refactored all the V2 "form data" + "request body" tests to be V3 "request body" tests.

b-admike · 2018-03-02T16:47:56Z

packages/testlab/package.json

    "sinon": "^4.1.2",
    "supertest": "^3.0.0",
-    "swagger-parser": "^4.0.1"
+    "swagger-parser": "^4.0.1",


Do we have any use for swagger-parser anymore?

good catch nope

jannyHou · 2018-03-02T20:16:50Z

@shimks

have you checked for upgrading imports in the READMEs?

Could you elaborate more about this? I know the document(loopback.io) need a bunch of updates for code example, but could not think of any READMEs needs update, besides openapi-v3 and openapi-v3-types(they are updated :) )

And for the tests, good point in term of the acceptance tests 👍 added in commit 70019b8

The index testing is covered in integration test

All those requestBody tests in openapi-v3 package guarantees it generates the correct spec, and tests in http-handler.integration.test.ts guarantees the handler process a good spec correctly.

is todoSchema used anywhere else other than in the lines you've deleted? If it isn't, should we get rid of it or nah

I think @b-admike also raise the same concern.

I have seen lots of discussions/PRs regarding @loopback/example-getting-started and bottom-up approach, and I have a feeling that todoSchema is generated for semi-top-down approach, and removing it from the example repo will turn the repo's essence from semi-top-down to entire bottom-up. So I hesitated.

jannyHou · 2018-03-02T22:19:13Z

@bajtos

The last commit seems to contain formatting/linting fixes for previous commits, I assume you are going to squash the commits before landing.

Yep I will squash all commits to one, broke them into smaller ones to make review easier.

Should we remove swagger (v2) related packages as part of this pull request too?

I prefer to leave them in this PR and I'll create anther PR doing:

rename openapi-spec-builder to openapi-v3-spec-builder
we can decide how to deal with the v2 packages.... Is it possible to archive a package in mono repo(searched and I guess not)?, even we drop them, I want to move them to an archived repository instead of moving them entirely.

jannyHou · 2018-03-02T22:40:43Z

packages/example-getting-started/docs/7-controller.md

@@ -0,0 +1,71 @@
+### Create your controller


OOPS accidentally added it when rebase, deleted it in fc6b3d1

bajtos · 2018-03-05T10:34:44Z

@jannyHou

we can decide how to deal with the v2 packages.... Is it possible to archive a package in mono repo(searched and I guess not)?, even we drop them, I want to move them to an archived repository instead of moving them entirely.

The packages will be always preserved in git history. People wishing to obtain the old code should check out and older SHA revision (commit). If you want to make the discovery process easier, then you can create a named tag (or a branch?) pointing to the latest commit before the code was deleted.

It would be nice to update the README of v2 packages to say that they are deprecated and no longer maintained, and publish a new version of each deprecated package to npmjs, so that e.g. this page shows the current status: https://www.npmjs.com/package/@loopback/openapi-v2

jannyHou · 2018-03-05T14:18:05Z

The packages will be always preserved in git history

👍

@bajtos I added a deprecation notice in this PR for openapi-v2 and openapi-spec.
I prefer to drop them in next PR to separate the code/doc clean from the v3 upgrade.

bajtos

I am suggesting a slightly different notice text:

This package has been deprecated in favor of OpenAPI Spec version 3.0.0, we are no longer maintaining it.

bajtos · 2018-03-05T14:50:53Z

packages/openapi-spec/README.md

@@ -1,3 +1,7 @@
+## DEPRECATION NOTICE
+
+This package is deprecated since we upgrade loopback-next to use openapi 3.0.0 starndard.


typo, should be standard.

bajtos · 2018-03-05T14:50:58Z

packages/openapi-v2/README.md

@@ -1,3 +1,7 @@
+## DEPRECATION NOTICE
+
+This package is deprecated since we upgrade loopback-next to use openapi 3.0.0 starndard.


typo, should be standard.

bajtos · 2018-03-05T14:52:59Z

packages/example-getting-started/docs/7-controller.md

@@ -1,71 +0,0 @@
-### Create your controller


Are you intentionally removing 7-controller.md file? Isn't this going to break Kevin's tutorial on loopback.io?

@bajtos my bad rebase added it in the first commit, so I removed it in this fix commit, I checked https://github.com/strongloop/loopback-next/tree/master/packages/example-getting-started/docs, that doc should be renamed/replaced by controller.md, but I will double check with @kjdelisle

bajtos · 2018-03-05T14:54:39Z

packages/rest/src/rest-server.ts

  '/openapi.json': {version: '3.0.0', format: 'json'},
  '/openapi.yaml': {version: '3.0.0', format: 'yaml'},
-  '/swagger.json': {version: '2.0', format: 'json'},
-  '/swagger.yaml': {version: '2.0', format: 'yaml'},


When cleaning up git history, please make sure that the commit message mentions the breaking change - removal of GET /swagger.{json,yaml} endpoints.

I will add list all breaking changes in the commit message ^

jannyHou · 2018-03-05T16:50:55Z

Added all the breaking changes in commit message: 6074525

And I will squash commits into one when merging

virkt25 · 2018-03-05T16:57:58Z

+1 for using @bajtos version of the commit message since it's more general. Just have a comment about form-data tests. Apart from that LGTM! 👍

virkt25 · 2018-03-05T19:19:59Z

Oops. I meant deprecation notice not commit message.

jannyHou · 2018-03-05T20:19:24Z

@virkt25 ah I see, good catch, I missed that message. Re-phrased it to Miroslav's version.

b-admike

🚢🇮🇹

* (rest) Remove GET /swagger.json and GET /swagger.yaml * (all packages) Upgrade packages from using @loopback/openapi-v2 to @loopback/openapi-v3 * (all pakcages) Upgrade packages from using @loopback/openapi-spec to @loopback/openapi-v3-types * (testlab) Replace Swagger validator by OAI3 validator provided by swagger2openapi

jannyHou requested review from bajtos, kjdelisle, raymondfeng and virkt25 as code owners March 1, 2018 16:09

jannyHou added the needs-priority label Mar 1, 2018

jannyHou force-pushed the oas3/upgrade branch from 417b028 to 8521f4f Compare March 1, 2018 17:33

jannyHou changed the title ~~[Just started, WIP] Oas3/upgrade~~ [Just started, WIP] oas3/upgrade Mar 1, 2018

jannyHou self-assigned this Mar 1, 2018

jannyHou force-pushed the oas3/upgrade branch from bc54ef5 to ce795bf Compare March 1, 2018 19:51

jannyHou changed the title ~~[Just started, WIP] oas3/upgrade~~ oas3/upgrade Mar 1, 2018

jannyHou force-pushed the oas3/upgrade branch 2 times, most recently from 744c855 to 5e3937a Compare March 1, 2018 20:27

shimks reviewed Mar 1, 2018

View reviewed changes

bajtos suggested changes Mar 2, 2018

View reviewed changes

b-admike reviewed Mar 2, 2018

View reviewed changes

jannyHou force-pushed the oas3/upgrade branch from 70019b8 to 8e7570c Compare March 2, 2018 21:36

jannyHou force-pushed the oas3/upgrade branch from 179e3f9 to 6bc5124 Compare March 2, 2018 22:37

jannyHou commented Mar 2, 2018

View reviewed changes

jannyHou force-pushed the oas3/upgrade branch 2 times, most recently from 1c72cf4 to 7b9a7fa Compare March 5, 2018 14:09

bajtos approved these changes Mar 5, 2018

View reviewed changes

bajtos reviewed Mar 5, 2018

View reviewed changes

shimks approved these changes Mar 5, 2018

View reviewed changes

jannyHou force-pushed the oas3/upgrade branch from 5397a0c to 1ce938b Compare March 5, 2018 16:44

jannyHou force-pushed the oas3/upgrade branch 2 times, most recently from fb72ac3 to 88e19bc Compare March 5, 2018 18:29

raymondfeng approved these changes Mar 5, 2018

View reviewed changes

virkt25 approved these changes Mar 5, 2018

View reviewed changes

b-admike approved these changes Mar 5, 2018

View reviewed changes

jannyHou force-pushed the oas3/upgrade branch from d7994ef to af65720 Compare March 5, 2018 20:39

jannyHou force-pushed the oas3/upgrade branch from af65720 to 72cb883 Compare March 5, 2018 20:47

jannyHou merged commit 71e5af1 into master Mar 5, 2018

jannyHou removed the needs-priority label Mar 5, 2018

jannyHou deleted the oas3/upgrade branch March 6, 2018 01:37

jannyHou mentioned this pull request Mar 6, 2018

update the content type for /openapi.json and /openapi.yaml #1089

Closed

		@@ -1,3 +1,7 @@
		## DEPRECATION NOTICE

		This package is deprecated since we upgrade loopback-next to use openapi 3.0.0 starndard.

oas3/upgrade #1067

oas3/upgrade #1067

Uh oh!

Conversation

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

Uh oh!

Description

Review

Next

Checklist

Uh oh!

shimks left a comment

Choose a reason for hiding this comment

Uh oh!

bajtos commented Mar 2, 2018

Uh oh!

bajtos left a comment

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

Uh oh!

jannyHou Mar 2, 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!

jannyHou Mar 5, 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!

b-admike left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jannyHou Mar 2, 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!

jannyHou Mar 2, 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!

jannyHou commented Mar 1, 2018 •

edited

Loading

jannyHou Mar 2, 2018 •

edited

Loading

jannyHou Mar 5, 2018 •

edited

Loading

jannyHou Mar 2, 2018 •

edited

Loading

jannyHou Mar 2, 2018 •

edited

Loading

jannyHou commented Mar 2, 2018 •

edited

Loading

jannyHou commented Mar 2, 2018 •

edited

Loading