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

feat: adds several convenience decorators #4406

Closed
wants to merge 5 commits into from

Conversation

mschnee
Copy link
Contributor

@mschnee mschnee commented Jan 10, 2020

Warning

This PR is incomplete- I am issuing it to request a preliminary round of feedback.
There are a bunch of concepts involved here that I don't entirely grok- such as the specific implementations of the DecoratorFactories, and a couple logical decisions that may be incorrect due to my amateur understanding of the OpenAPI specification.

Implements #4300

This branch adds the following openapi convenience decorators

  • @deprecated() to a class or method
  • @tags(tagNames: string []) to a class or method
  • @response(code: number, spec: ModelOrSpec, options?: {contentType: string, description: string}) to methods only

This branch adds the following features:

  • creates a MethodMultiDecoratorFactory, allowing multiple uses of the
    same decorator per method, see the @response() decorator.
  • adds support for the schema keywords 'allOf', 'anyOf', 'oneOf', and
    'not' in resolving schemas with the 'x-ts-type' extension
  • adds a get-dist-file script to packages/build as a utility to make it easier to
    debug a unit test file directly in vscode, see Debug Current Test File" in .vscode/launch.json`

Examples

The following TypeScript code:

@model()
class SuccessModel extends Model {
  constructor(err: Partial<SuccessModel>) {
    super(err);
  }
  @property({default: 'ok'})
  message: string;
}

@model()
class FooError extends Model {
  constructor(err: Partial<FooError>) {
    super(err);
  }
  @property({default: 'foo'})
  foo: string;
}

@model()
class BarError extends Model {
  constructor(err: Partial<BarError>) {
    super(err);
  }
  @property({default: 'bar'})
  bar: string;
}

class MyController {
  @get('/greet', {
    responses: {
      200: {
        description: 'Unknown',
        content: {
          'application/json': {schema: FIRST_SCHEMA},
        },
      },
    },
  })
  @response(200, {
        type: 'string',
        format: 'base64',
      }, {contentType: 'application/pdf'})
  @response(200, SuccessModel, {contentType: 'application/jsonc'})
  @response(404, [FooError, BarError], {contentType: 'application/jsonc'})
  @tags(['Foo', 'Bar'])
  @deprecated()
  greet() {
    return new SuccessModel({message: 'Hello, world!'});
  }
}

The following typec
Will result in the following json:

{
  "paths": {
    "/greet": {
      "get": {
        "responses": {
          "200": {
            "description": "Standard response for successful HTTP requests.",
            "content": {
              "application/json": {
                "schema": {
                  "type": "object",
                  "properties": {
                    "x": {
                      "type": "int",
                      "default": 1
                    },
                    "y": {
                      "type": "string",
                      "default": "2"
                    }
                  }
                }
              },
              "application/jsonc": {
                "schema": {
                  "$ref": "#/components/schemas/SuccessModel"
                }
              },
              "application/pdf": {
                "schema": {
                  "type": "string",
                  "format": "base64"
                }
              }
            }
          },
          "404": {
            "description": "The requested resource could not be found but may be available in the future. Subsequent requests by the client are permissible.",
            "content": {
              "application/jsonc": {
                "schema": {
                  "anyOf": [
                    {
                      "$ref": "#/components/schemas/FooError"
                    },
                    {
                      "$ref": "#/components/schemas/BarError"
                    }
                  ]
                }
              }
            }
          }
        },
        "deprecated": true,
        "tags": [
          "Foo",
          "Bar"
        ],
        "x-operation-name": "greet",
        "x-controller-name": "MyController",
        "operationId": "MyController.greet"
      }
    }
  },
  "components": {
    "schemas": {
      "SuccessModel": {
        "title": "SuccessModel",
        "properties": {
          "message": {
            "type": "string"
          }
        },
        "additionalProperties": false
      },
      "FooError": {
        "title": "FooError",
        "properties": {
          "foo": {
            "type": "string"
          }
        },
        "additionalProperties": false
      },
      "BarError": {
        "title": "BarError",
        "properties": {
          "bar": {
            "type": "string"
          }
        },
        "additionalProperties": false
      }
    }
  }
}

This PR is missing the following

  • The method-level @description() decorator
  • Updated API documentation
  • Updated user documentation
  • Updated examples
  • Integration tests to complement the unit tests.
    • Full openapi specification validation with the new decorators in complex arrangement.

This PR does NOT implement the following

  • After invoking a controller method decorated with @response(), use the response metadata to hint at the intended response code based on the returned object, or the thrown error, e.g. when a method is decorated as@response(401, SomeAuthError), then returning or catching SomeAuthError should hint that the response code should be a 401 instead of a 500, without requiring the developer to explicitly set the response code by injecting the Response object.
    • There are fair number of edge cases involved, like what happens when a Model is present in multiple status codes?
    • Can this behavior be made optional with configurable Send/Reject providers?
    • Where exactly does this behavior live?

This PR has the following missing features or bugs:

  • The content-map reducer function reduceSpecContent - does not check if the schema already exists from an existing @op or @api definition. It should check if there is an existing anyOf/allOf/oneOf keyword. What happens next is kind of a question: if there is no keyword block, the existing content should be added into an anyOf, if there is an existing anyOf block, it should be used. If there's an existing allOf or oneOf block... what happens next is kind of undefined.
  • Remove examples from the @response() decorator options.
  • Probably millions of typos.

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated
  • Rebase from upsteam

This branch adds the following openapi convenience decorators
- @deprecated
- @tags
- @response

This branch adds the following features:
- creates a `MethodMultiDecoratorFactory`, allowing multiple uses of the
same decorator per method.
- adds support for the schema keywords 'allOf', 'anyOf', 'oneOf', and
'not' in resolving schemas with the 'x-ts-type' extension
- adds a `get-dist-file` to build as a utility to make it easier to
debug a unit test file directly in vscode.
@mschnee
Copy link
Contributor Author

mschnee commented Jan 10, 2020

npm ERR! cipm can only install packages when your package.json and package-lock.json or npm-shrinkwrap.json are in sync. Please update your lock file with `npm install` before continuing.

Looks like some stuff left over from a bad lerna add command.

@achrinza achrinza added community-contribution feature OpenAPI developer-experience Issues affecting ease of use and overall experience of LB users labels Jan 12, 2020
@raymondfeng
Copy link
Contributor

@mschnee Thank you for the contribution. To make the review process easier, I suggest that we break down this PR into smaller ones and land them incrementally. For example, the 1st PR can be the one to add MethodMultiDecoratorFactory.

@mschnee
Copy link
Contributor Author

mschnee commented Jan 13, 2020

@raymondfeng Sure, I can get that started today.

@dhmlau
Copy link
Member

dhmlau commented Jan 14, 2020

@mschnee, thanks for breaking up the PRs into smaller ones. In order to avoid confusion and to avoid people reviewing this one, is it ok if you close this PR? Thanks!

@mschnee
Copy link
Contributor Author

mschnee commented Jan 15, 2020

Done! Thanks for all the fast feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
developer-experience Issues affecting ease of use and overall experience of LB users feature OpenAPI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants