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

Partial update (PATCH) over REST #1722

Closed
bajtos opened this issue Sep 18, 2018 · 8 comments
Closed

Partial update (PATCH) over REST #1722

bajtos opened this issue Sep 18, 2018 · 8 comments
Assignees
Labels
blocked bug REST Issues related to @loopback/rest package and REST transport in general

Comments

@bajtos
Copy link
Member

bajtos commented Sep 18, 2018

Description / Steps to reproduce / Feature proposal

Consider the following acceptance tests for Todo controller in our example apps.

it('updates the todo by ID ', async () => {
  await client
    .patch(`/todos/${persistedTodo.id}`)
    .send({isComplete: true})
    .expect(204);
  const result = await todoRepo.findById(persistedTodo.id);
  const expected = Object.assign(persistedTodo.toJSON(), {isComplete: true};
  expect(result.toJSON()).to.eql(expected);
});

it('returns 404 when updating a todo that does not exist', () => {
  return client
    .patch('/todos/99999')
    .send({isComplete: true})
    .expect(404);
});

Current Behavior

Both tests fail because the PATCH request fails with 422 validation error. The request body is rejected because it's not a valid Todo instance (missing required title property, etc.)

Expected Behavior

The PATCH operation behaves like a real patch, accepting a subset of Todo properties to update. Both tests pass.

Related issues:

See Reporting Issues for more tips on writing good issues

@bajtos bajtos added bug post-GA REST Issues related to @loopback/rest package and REST transport in general labels Sep 18, 2018
@raymondfeng
Copy link
Contributor

I'm proposing to introduce an @schema decorator or extend existing decorators so that a schema can be something like:

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

@bajtos
Copy link
Member Author

bajtos commented Sep 20, 2018

I'm proposing to introduce an @Schema decorator or extend existing decorators so that a schema can be something like.

The discussion about excluding/including certain properties from parameter schema has already started in #1179, let's keep it in a single place there. I'll cross-post your comment @raymondfeng.

IMO, this issue should be focused on leveraging #1179 in our example projects and CLI controller templates.

Alternatively, we can close is as a duplicate of #1179.

@bajtos
Copy link
Member Author

bajtos commented Sep 20, 2018

On the second thought, maybe this issue is slightly different from #1179.

In #1179, we want to allow create operation to exclude certain model properties from the payload, for example id.

In this issue, we want to allow all model properties in the payload, but make them all optional.

Ideally, I'd like us to fine a mechanism (decorator API) that can support both use cases in a consistent way.

Building on top of what has been proposed in #1179 (comment):

// `id` property is not allowed for `create`:
@post('/')
createTodo(@requestBody(Todo, {propertyBlacklist: ['id']}) todo: Partial<Todo>) {}

// all properties are allowed, all are optional
@patch('/')
updateTodo(@requestBody(Todo, {partial: true}) todo: Partial<Todo>) {}

Internally, @requestBody can use x-ts-type-options proposed by @raymondfeng as an implementation detail.

@bajtos
Copy link
Member Author

bajtos commented Mar 11, 2019

In #2152 (comment), I mention a solution for defining types representing model data with a black-list applied.

  • A type used for creating new instances, this type typically excludes properties set by the database (e.g. id, _rev, etc.)

    // black-list based approach
    type NewTodo = Pick<Todo,  Exclude<keyof Product, 'id'>>;
    // white-list base approach
    type BumpCounterAndUpdated = Pick<MyModel, 'counter' | 'updated'>
  • A type used for performing a patch (partial update) operation, this type has all properties optional and probably should exclude properties controlled by the database

    type TodoPatch = Partial<NewTodo>;

Now that we know how to deal with types, we need to figure out how to generate OpenAPI schema for these different objects.

@b-admike
Copy link
Contributor

b-admike commented May 8, 2019

hey @bajtos @raymondfeng, I was trying to compile an acceptance criteria for this issue, but noticed that it's pretty similar to #2652. Can we close it in favour of the other or do you think they're different? If so, maybe we can chat to come up with an acceptance criteria for this issue and get it groomed.

@bajtos
Copy link
Member Author

bajtos commented May 9, 2019

Let's put this issue on hold until #2652 is done. Quoting from the acceptance criteria from that issue:

Review #1722 and #1179, either close them as fixed or post a comment explaining what's remaining to be done.

@bajtos
Copy link
Member Author

bajtos commented Jun 27, 2019

Most of the requested functionality was implemented by #3199. The remaining part is how to simplify @requestBody annotations, I opened a new issue to deal with that: #3257

Closing this one as done.

@odykyi
Copy link
Contributor

odykyi commented Sep 2, 2020

user: Partial < User > - works for me

 @patch('/users/{id}', {
    responses: {
      '204': {
        description: 'User PATCH success',
      },
    },
  })
  async updateById(
    @param.path.number('id') id: number,
    @requestBody({
      content: {
        'application/json': {
          schema: getModelSchemaRef(User, {partial: true}),
        },
      },
    })
    user: Partial<User>,
  ): Promise<void> {
    await this.userRepository.updateById(id, user);
  }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked bug REST Issues related to @loopback/rest package and REST transport in general
Projects
None yet
Development

No branches or pull requests

5 participants