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

Spike: simplify requestBody annotation with schema options #2654

Closed
4 of 6 tasks
bajtos opened this issue Mar 28, 2019 · 1 comment
Closed
4 of 6 tasks

Spike: simplify requestBody annotation with schema options #2654

bajtos opened this issue Mar 28, 2019 · 1 comment
Assignees
Labels
developer-experience Issues affecting ease of use and overall experience of LB users OpenAPI spike

Comments

@bajtos
Copy link
Member

bajtos commented Mar 28, 2019

This is a follow-up for the discussion started in #2082, #2152 and the related issues.

In #2631, we are introducing a new function getModelSchemaRef that can be used to obtain OpenAPI schema for a model in a configurable way. Example use:

class TodoListController {
  // ...

  @patch('/todo-lists/{id}', {
    responses: {
      // left out for brevity
    },
  })
  async updateById(
    @param.path.number('id') id: number,
    @requestBody({
      content: {
        'application/json': {
          schema: getModelSchemaRef(TodoList, {partial: true}),
          /***** ^^^ THIS IS IMPORTANT - OPENAPI SCHEMA ^^^ ****/
       },
     },
    })
    obj: Partial<TodoList>,
    /***** ^^^ THIS IS IMPORTANT - TYPESCRIPT TYPE ^^^ ****/
  ): Promise<void> {
    await this.todoListRepository.updateById(id, obj);
  }
}

Eventually, we plan to implement the following schema generator options:

The current approach is very verbose. Compare the example above with a controller method that uses default options:

class TodoController {
  // ...

  @put('/todos/{id}', {
    responses: {
       // left out for brevity
    },
  })
  async replaceTodo(
    @param.path.number('id') id: number,
    @requestBody() todo: Todo,
  ): Promise<void> {
    await this.todoRepo.replaceById(id, todo);
  }
}

In this spike, we should research different options for simplifying request-body definitions requiring custom schema options.

Few examples to get started:

  • In Partial update (PATCH) over REST #1722 (comment), @raymondfeng proposed:

    {
      schema: {
        'x-ts-type': Order,
        'x-ts-type-options': {
          partial: true,
          excludes: []
          }
      }

    This addresses only how to provide schema options when using x-ts-type extension. It does not address the problem of too complex spec required by @requestBody decorator.

  • In Partial update (PATCH) over REST #1722 (comment), @bajtos proposed a different solution:

    class TodoController {
        // ...
    
        // `id` property is not allowed for `create`:
        @post('/')
        createTodo(@requestBody(Todo, {exclude: ['id']}) todo: ExcludeKeys<Todo, 'id'>) {}
    
        // all properties are allowed, all are optional
        @patch('/')
        updateTodo(@requestBody(Todo, {partial: true}) todo: Partial<Todo>) {}
    }

Acceptance criteria

@bajtos bajtos added spike 2019Q2 developer-experience Issues affecting ease of use and overall experience of LB users OpenAPI labels Mar 28, 2019
@bajtos bajtos changed the title Spike: simplify requestBody annotation with JsonSchemaOptions Spike: simplify requestBody annotation with schema options Mar 28, 2019
@bajtos bajtos added the p2 label Apr 11, 2019
@dhmlau dhmlau added 2019Q3 and removed 2019Q2 labels Jun 11, 2019
@agnes512 agnes512 added this to the July 2019 milestone milestone Jun 27, 2019
@jannyHou jannyHou self-assigned this Jul 8, 2019
@jannyHou
Copy link
Contributor

jannyHou commented Jul 11, 2019

Update:

According to requestBody spec, different content types could have different schemas, this is why taking a single type and its options like

@post('/') createTodo(@requestBody(Todo, {exclude: ['id']}) todo: ExcludeKeys<Todo, 'id'>) {}

could not handle more complicated cases, even though it's the concisest signature.

The other proposal

{
  schema: {
    'x-ts-type': Order,
    'x-ts-type-options': {
      partial: true,
      excludes: []
      }
  }

looks more declarative than calling getModelSchemaRef(TodoList, {partial: true}), directly, but

It does not address the problem of too complex spec required by @requestbody decorator.

is still valid.

So I am proposing create some sugar apis like @requestBody().addContent('media-content-type-name', ModelCtor, options) to simplify the UX. And will try implement it in code.

@jannyHou
Copy link
Contributor

jannyHou commented Aug 1, 2019

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 OpenAPI spike
Projects
None yet
Development

No branches or pull requests

4 participants