feat: adding openapi-v3 #1046

jannyHou · 2018-02-23T20:58:38Z

connect to: #753

Description

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

connect to issue: #753

Review

the first commit just copy paste openapi-v2 package for comparison, please review code change from the 2nd commit
commit test: add tests contains all the test files(30) update
other files update are separated into each commit, usually one or a few file's change

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~~

jannyHou · 2018-02-23T23:16:12Z

packages/openapi-v3/src/parameter-decorator.ts

+};

+/**
+ * Shortcut parameter decorators


The git compare is messy for the shorcuts. Suggest look at the file directly :)

jannyHou · 2018-02-24T21:23:43Z

packages/openapi-v3/src/parameter-decorator.ts

    }
+
+    ParameterDecoratorFactory.createDecorator<ParameterObject>(
+      OAI3.PARAMETERS_KEY,


^ fixed in commit "feat: add tests":
OAI3 ---> OAI3Keys

jannyHou · 2018-02-24T21:40:15Z

packages/openapi-v3/src/parameter-decorator.ts

     * Define a parameter of "integer" type that's read from the query string.
     * Usage: @param.query.string('paramName')
-     * 
+     *


The commit for lint is squashed into "test: add tests" :(

jannyHou · 2018-02-24T21:51:27Z

...es/openapi-v3/test/unit/controller-spec/controller-decorators/param-decorators/param.test.ts

-      ]);
-    });
-
-    it('can define multiple parameters by arguments', () => {


moves(and modified) to integration test:
integration/operation-spec.test.ts

jannyHou · 2018-02-24T21:59:42Z

...es/openapi-v3/test/unit/controller-spec/controller-decorators/param-decorators/param.test.ts

-      ]);
-    });
-    // tslint:disable-next-line:max-line-length
-    it('throws an error if @param is used at both method and parameter level', () => {


removed due to deprecating method level decorator.

jannyHou · 2018-02-24T22:09:58Z

...es/openapi-v3/test/unit/controller-spec/controller-decorators/param-decorators/param.test.ts

-      expect(actualSpec.paths['/greet']['get']).to.eql(expectedSpec);
-    });
-
-    it('infers simple body parameter type', () => {


replaced by new tests in request-body.test.ts

jannyHou · 2018-02-24T22:12:11Z

...es/openapi-v3/test/unit/controller-spec/controller-decorators/param-decorators/param.test.ts

-      expect(actualSpec.paths['/greet']['get']).to.eql(expectedSpec);
-    });
-
-    it('infers complex body parameter type', () => {


replaced by new tests in request-body.test.ts

jannyHou · 2018-02-24T22:12:24Z

...es/openapi-v3/test/unit/controller-spec/controller-decorators/param-decorators/param.test.ts

      expect(actualSpec.paths['/greet']['get']).to.eql(expectedSpec);
    });
-
-    it('infers complex body parameter schema into the controller spec', () => {


replaced by new tests in request-body.test.ts

jannyHou · 2018-02-24T22:36:26Z

...es/openapi-v3/test/unit/controller-spec/controller-decorators/param-decorators/param.test.ts

-      });
-    });
-
-    it('does not produce nested definitions', () => {


moved to test/integration/controller-spec.test.ts

b-admike

+1 on Miroslav's comments, but LGTM otherwise :)

shimks · 2018-02-27T17:39:23Z

packages/openapi-v3/src/parameter-decorator.ts

+  SchemaObject,
+} from '@loopback/openapi-v3-types';
+import {MetadataInspector, ParameterDecoratorFactory} from '@loopback/context';
+import {getSchemaForParam} from './';


Import should come from the file itself (generate-schema)

shimks · 2018-02-27T17:44:06Z

packages/openapi-v3/src/parameter-decorator.ts

  ) {
    paramSpec = paramSpec || {};
    // Get the design time method parameter metadata
    const methodSig = MetadataInspector.getDesignTypeForMethod(target, member);


I think this is a remnant of having method level @param decorator. Can we rework the code here so that we don't need leverage getDesignTypeForMethod here?

@shimks interestingly, I had the same thought, but I am afraid that's the only inspector method we can use, and it makes perfect sense.
If you look into all inspector methods in metadata/src/inspector.ts, this is the only function that returns design type information of a method, including its parameter types, return types(we can use it for generating response object in the future).

ah, but is ResponseObject something we want to generate from @param or @requestBody in the future? I guess that's up for discussion way down the road, but I guess it's ok to leave it in for now since it's not a huge issue anyway.

shimks · 2018-02-27T17:50:03Z

packages/openapi-v3/src/parameter-decorator.ts

+  return function(target: Object, member: string | symbol, index: number) {
+    paramSpec = paramSpec || {};
+    // Get the design time method parameter metadata
+    const methodSig = MetadataInspector.getDesignTypeForMethod(target, member);


Copying because github ignored my comment:
I think this is a remnant of having method level @param decorator. Can we rework the code here so that we don't need leverage getDesignTypeForMethod here?

see comment #1046 (comment)

shimks · 2018-02-27T17:55:22Z

packages/openapi-v3/src/generate-schema.ts

+ */
+export function getSchemaForParam(
+  type: Function,
+  schema: SchemaObject,


Can this be optional?

nope, see the method that calls it in parameter-decorator.ts:

paramSpec.schema = getSchemaForParam(paramType, paramSpec.schema || {});

that could be handled within the function itself can't it? with something like this:

export function getSchemaForparam( type: Function, schema?: SchemaObject, ) { schema = schema || {}; // .. rest of the code here }

or

export function getSchemaForParam( type: Function, schema: SchemaObject = {} ) {}

we wouldn't need to hand in an empty object every time we call the function

Good point!

shimks · 2018-02-27T17:56:52Z

packages/openapi-v3/src/api-decorator.ts

+// This file is licensed under the MIT License.
+// License text available at https://opensource.org/licenses/MIT
+
+import {ControllerSpec} from './';


import from the file itself please

Any reason or is it a documented convention to export it from the file itself?
For the ones exported in src/index.ts, I am used to import from root level

Importing from index.ts can cause a circular dependency for some reason

oh! that's important. changing all of them

shimks · 2018-02-27T17:57:35Z

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

+} from '@loopback/openapi-v3-types';
+import {getJsonSchema} from '@loopback/repository-json-schema';
+import {OAI3Keys} from './keys';
+import {jsonToSchemaObject} from './';


import from the file itself please

shimks · 2018-02-27T18:08:25Z

packages/openapi-v3/src/request-body-decorator.ts

+  ReferenceObject,
+} from '@loopback/openapi-v3-types';
+import {MetadataInspector, ParameterDecoratorFactory} from '@loopback/context';
+import {getSchemaForRequestBody} from './';


import from the file itself please

Done 👍
I delete exporting generate-schema.ts in index.ts, the functions in it are only for internal use

shimks · 2018-02-27T18:13:35Z

packages/openapi-v3/src/request-body-decorator.ts

+      requestBodySpec.content = {'application/json': {}};
+
+    // Get the design time method parameter metadata
+    const methodSig = MetadataInspector.getDesignTypeForMethod(target, member);


Same as in parameter decorator, can we replace this with getParameterMetadata or something that works like that?

see comment #1046 (comment)

shimks · 2018-02-27T18:22:46Z

packages/openapi-v3/test/integration/operation-spec.test.ts

+    class MyController {
+      @post('/users/{location}')
+      async createUser(
+        @param.query.string('type') type: string,


As a side note, do we ever use the string given as an argument (type in this case) in any of our codebase anymore? Is it possible for us to get rid of it?

@shimks sorry could you elaborate more about this? not sure I understand...:speak_no_evil:

Eh, I think I'm overloading the scope of this task, ignore my last comment :p

@shimks Did you mean to say @param.query('type') is good enough as we can infer string from the TS type?

jannyHou · 2018-02-27T20:12:19Z

@shimks thanks for looking into the long PR! feedbacks applied in 3cf42d1, and answered other comments

jannyHou · 2018-02-27T20:25:45Z

And @bajtos 's feedbacks applied in 7b05087

shimks

LGTM. It might be out of scope, but what do you think about allowing @requestBody to take in a string instead of an object to specify the content-type?

jannyHou · 2018-02-28T12:07:47Z

@shimks

but what do you think about allowing @RequestBody to take in a string instead of an object to specify the content-type?

Good point, this goes back to my old question: what if people specify different schemas(even slightly different) for multiple content-types? While I agree with you that the object style looks redundant when shares the same schema among multiple content-types:

@requestBody({'application/json': {}, 'application/text': {}})

So I think a better way is creating a shortcut, e.g.

// not a valid code, just a concept shows the type for each parameter
@requestBody.multipleContents(
  contents: string[],
  schemaSpec:? SchemaObject , 
  otherProperties: {description?: string, required: boolean})

shimks · 2018-02-28T15:21:34Z

I took a quick look at content-type headers and it seems like a request can set only one content-type in the header. For reference: https://greenbytes.de/tech/webdav/rfc2616.html#rfc.section.14.17. I remember you asking @raymondfeng about this, what was his response? Setting multiple content-types is fine in OpenAPI so nevermind.

I'm going to list out the cases that requestBody may need:

single content-type
- schema automatically generated with application/json
  - with or without optional properties
- schema automatically generated with explicitly set content-type
  - with or without optional properties
- schema set by user

Now as to the cases with multiple possible content-types, I think we just leave that out of the task and create another issue for it since we're already heavily overloaded on this one anyways. If users really want to provide multiple possibly content-types for the body, they should handle it themselves anyway (imo, unless this is commonly done; it's a discussion in the other potential issue).

As for the second case I've listed out (custom content-type), I think we should just allow users to provide a string of the content-type if you agree to forget about automatic schema generations for multiple content-types in this story.

shimks

Couple more comments 🔨

shimks · 2018-02-28T15:47:39Z

...t/unit/controller-spec/controller-decorators/request-body-decorators/primative-types.test.ts

+      assertRequestBodySpec({type: 'object'}, MyController);
+    });
+
+    it('infers array', () => {


Show we force the users to use @requestBody.array for the cases with array?

I also want to ask the same question with @param.array

This is not a valid use case ^

Show we force the users to use @requestBody.array for the cases with array?

Yes!

@shimks I am summarizing some features/improve we want for @requestBody, sorry I don't want to keep adding features in this PR since it blocks the 4th PR(upgrade the whole project), after land this one people can work on the improvement and upgrade in parallel.

shimks · 2018-02-28T16:08:20Z

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

+        @post('/greeting')
+        greet(
+          @requestBody.array(
+            {type: 'string'},


We should allow the parameters to take in primitive wrapper functions. This is so that users can write @requestBody.array(String, optSpec) instead of @requestBody.array({type: 'string'}, optSpec). We can leverage getSchemaForParam to do this

@shimks I didn't implement shortcut functions for @requestBody.array and also @param.array is because I have different plans for them, and I think it takes discussion time, created an issue for it:

#1064

shimks · 2018-02-28T16:10:21Z

packages/openapi-v3/src/parameter-decorator.ts

+  export const array = function(
+    name: string,
+    source: ParameterLocation,
+    itemSpec: SchemaObject | ReferenceObject,


This should accept a string ('number') or a function (Number). Make sure to fix the jsdoc so that it reflects the change if you want to go through with it.

^ the code is correct, jsdoc is out-of-date

shimks · 2018-02-28T16:11:24Z

packages/openapi-v3/src/parameter-decorator.ts

+   * ```ts
+   * export class MyController {
+   *   @get('/greet')
+   *   greet(@param.array('names', 'query', 'string') names: string[]): string {


Arguments are wrong here at the moment

that is the old document ^ I am changing it to reflect the current code

jannyHou · 2018-02-28T22:33:49Z

@shimks Thank you for the thoughtful feedbacks! Really appreciate them. And I totally agree with you that we should simplify some use cases, summarize them in issue #1064.

Sorry I could not address them in this PR since 1. IMO how to simplify the use cases still needs discussion 2. this PR blocks the whole upgrade, and those improvements can be made in parallel with the upgrade PR.

Let's keep discussion in PR#1064 and create a new PR to improve it.

jannyHou requested review from bajtos, kjdelisle and raymondfeng as code owners February 23, 2018 20:58

jannyHou added the needs-priority label Feb 23, 2018

jannyHou force-pushed the oas3/openapi-v3 branch 3 times, most recently from 44e1d86 to 4b4dc5c Compare February 23, 2018 23:12

jannyHou commented Feb 23, 2018

View reviewed changes

jannyHou force-pushed the oas3/openapi-v3 branch 10 times, most recently from fb494d9 to 5de8b9b Compare February 24, 2018 21:08

jannyHou commented Feb 24, 2018

View reviewed changes

jannyHou force-pushed the oas3/openapi-v3 branch from 5de8b9b to b9e5c23 Compare February 24, 2018 21:37

jannyHou commented Feb 24, 2018

View reviewed changes

jannyHou force-pushed the oas3/openapi-v3 branch 2 times, most recently from d55758d to b111a2c Compare February 24, 2018 21:55

jannyHou commented Feb 24, 2018

View reviewed changes

jannyHou force-pushed the oas3/openapi-v3 branch from b111a2c to bfa6537 Compare February 24, 2018 22:35

jannyHou commented Feb 24, 2018

View reviewed changes

b-admike approved these changes Feb 27, 2018

View reviewed changes

shimks reviewed Feb 27, 2018

View reviewed changes

jannyHou force-pushed the oas3/openapi-v3 branch from 5858055 to 4e5607d Compare February 27, 2018 18:18

shimks reviewed Feb 27, 2018

View reviewed changes

jannyHou force-pushed the oas3/openapi-v3 branch from 4e5607d to 3cf42d1 Compare February 27, 2018 18:37

kjdelisle force-pushed the master branch from c8af3f6 to eb06201 Compare February 27, 2018 19:24

jannyHou force-pushed the oas3/openapi-v3 branch from 3cf42d1 to 290a8b6 Compare February 27, 2018 20:31

jannyHou changed the title ~~feat: adding openapi-v3~~ [WIP]feat: adding openapi-v3 Feb 27, 2018

jannyHou changed the title ~~[WIP]feat: adding openapi-v3~~ feat: adding openapi-v3 Feb 27, 2018

shimks approved these changes Feb 27, 2018

View reviewed changes

jannyHou force-pushed the oas3/openapi-v3 branch from 7b32ad3 to fb79382 Compare February 28, 2018 12:11

shimks reviewed Feb 28, 2018

View reviewed changes

jannyHou force-pushed the oas3/openapi-v3 branch from 011f84c to e87c670 Compare February 28, 2018 22:24

jannyHou force-pushed the oas3/openapi-v3 branch from f53e919 to af60c07 Compare February 28, 2018 22:51

feat: add openapi-v3

0739ffc

jannyHou force-pushed the oas3/openapi-v3 branch from af60c07 to 0739ffc Compare March 1, 2018 03:32

jannyHou merged commit 45a4bdf into master Mar 1, 2018

jannyHou removed the needs-priority label Mar 1, 2018

jannyHou deleted the oas3/openapi-v3 branch March 1, 2018 15:20

feat: adding openapi-v3 #1046

feat: adding openapi-v3 #1046

Uh oh!

Conversation

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

Uh oh!

Description

Review

Checklist

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 Feb 24, 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!

jannyHou Feb 24, 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!

b-admike 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!

jannyHou Feb 27, 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!

shimks Feb 27, 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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

jannyHou commented Feb 23, 2018 •

edited

Loading

jannyHou Feb 24, 2018 •

edited

Loading

jannyHou Feb 24, 2018 •

edited

Loading

jannyHou Feb 27, 2018 •

edited

Loading

shimks Feb 27, 2018 •

edited

Loading

shimks Feb 27, 2018 •

edited

Loading

shimks commented Feb 28, 2018 •

edited

Loading

shimks Feb 28, 2018 •

edited

Loading