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

fix(rest): allow @patch update obj to be a Partial #1894

Closed

Conversation

joeytwiddle
Copy link
Contributor

This changes the type of the obj argument in @patch endpoints from Model to Partial<Model>.

Since the route checking has become more strict, requests to patch only some fields of a document are now being rejected, because the validation expects to see all required fields in the patch.

In our codebase, we have made the type of the update object Partial<Model> to allow partial updates, and that has been working well.

This is only a quick fix. I believe you may settle on a more comprehensive solution later, as discussed in #1722

  • I used Partial<Model> because I thought that was more appropriate than DeepPartial<Model> or DataObject<Model> but I could be wrong.

If this PR is not a step in the desired direction, then please feel free to close it.

Checklist

  • npm test passes on your machine
    I'm afraid the develop branch is currently failing for me with: Conflicting definitions for 'express-serve-static-core'
  • 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

@bajtos
Copy link
Member

bajtos commented Oct 22, 2018

@joeytwiddle thank you for opening this pull request. The proposed changes are going in the right direction, but I am don't think we have the required underlying block already in place.

-    @requestBody() data: Todo,
+    @requestBody() data: Partial<Todo>,

The way how @requestBody() currently works: it's using the so-called design:type metadata produced by TypeScript to learn about the type of the argument value (a Todo constructor). When this design:type is a model, then the decorator emits a JSON/OpenAPI schema describing model properties as the accepted value.

When you change the type from Todo to Partial<Todo>, this metadata is lost and @requestBody() decorator sees only a generic object type. As a result, the OpenAPI spec describing application's REST API is describing the endpoint as accepting an empty object. At least that's what I remember - could you please check the actual behavior yourself?

If my description above still holds, then we need to find a way how to explicitly tell requestBody decorator what type is the parameter accepting, before we can change the CRUD Controller template used by our CLI. See the discussion in #1722 for implementation proposals.

Would you like to take a look at this underlying feature first?

Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

☝️

@bajtos bajtos added Examples feature CLI OpenAPI Schema REST Issues related to @loopback/rest package and REST transport in general community contribution labels Oct 22, 2018
@ambrt
Copy link

ambrt commented Dec 26, 2018

-    @requestBody() data: Todo,
+    @requestBody() data: Partial<Todo>

As a result, the OpenAPI spec describing application's REST API is describing the endpoint as accepting an empty object. At least that's what I remember - could you please check the actual behavior yourself?

I can confirm - Partial<Todo> describes endpoint as accepting empty object.

@bajtos
Copy link
Member

bajtos commented Jan 7, 2019

I am closing this pull request as abandoned, feel free to reopen if you get time to work on this again.

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

Successfully merging this pull request may close these issues.

None yet

3 participants