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
bajtos opened this issue Mar 28, 2019 · 1 comment

Comments

@bajtos
Copy link
Member

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

  • Read through prior discussions: #1722, #1179, #2125, etc.
  • Research different options for improving the current situation
  • Open a spike pull request showing a PoC of the proposed implementation
  • In the PR, include a /_SPIKE_.md document explaining what options you considered, what were pros and cons, how and why did you pick the winner. Include a list of follow-up stories needed to implement the proposed solution.
  • Discuss the propsal with the team and get their approval
  • Create follow-up stories.

@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 referenced this issue Jun 3, 2019
24 of 37 tasks complete

@dhmlau dhmlau added 2019Q3 p1 and removed 2019Q2 p2 labels Jun 11, 2019

@dhmlau dhmlau referenced this issue Jun 25, 2019
18 of 30 tasks complete

@agnes512 agnes512 added this to the July 2019 milestone milestone Jun 27, 2019

@jannyHou jannyHou self-assigned this Jul 8, 2019

@jannyHou

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

The PoC of new proposal is created in draft PR #3466

Follow up stories see:

@jannyHou jannyHou closed this Aug 1, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.