Emit schema with all model properties optional + allow partial updates via PATCH #3199

bajtos · 2019-06-21T07:22:05Z

Allow Controller methods implementing PATCH operation to describe their input data as "a model with all properties optional". To emit schema with optional model properties, we are adding a new schema-generation option called partial.
Modify CLI templates to use the new partial option to describe request body parameter.
Modify example applications to leverage this new option too.
Update code snippets in our docs for consistency.

See #2652, #1722 and #1179

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

👉 Check out how to submit a PR 👈

b-admike

Nice 💯. I have minor comments, otherwise LGTM.

packages/repository-json-schema/src/build-schema.ts

samarpanB · 2019-06-21T14:54:30Z

packages/repository-json-schema/src/keys.ts

@@ -15,6 +19,6 @@ export const enum MODEL_TYPE_KEYS {
 * Metadata key used to set or retrieve repository JSON Schema
 */
 export const JSON_SCHEMA_KEY = MetadataAccessor.create<
-  {[options in MODEL_TYPE_KEYS]: JSONSchema},


Any specific reason for removing this type check ?

In the future, we will be adding more schema options like exclude (see #2653). Because options can be combined, e.g. partial with exclude, the number of possible combinations (and thus cache) keys is growing fast. I find it impractical to enumerate all valid keys in a TypeScript enum type. We will need 4 entries now, 8 entries when we implement exclude, 16 entries when we introduce another option, etc.

That's why I switched the indexer type from options in MODLE_TYPE_KEYS to key: string.

Makes sense. But on the hindsight, I think we should find some other key representation than just strings concatenation. That will resolve the actual issue as well as keep the type checks. It can be done in future. Not required in this PR.

packages/repository-json-schema/src/build-schema.ts

examples/todo-list/src/controllers/todo.controller.ts

examples/todo-list/src/controllers/todo-list.controller.ts

examples/todo-list/src/controllers/todo.controller.ts

nabdelgadir · 2019-06-21T18:44:29Z

packages/repository-json-schema/src/keys.ts

+/**
+ * TODO(semver-major) remove these constants in the next major version
+ * @deprecated
+ */


Can I ask why they're being removed?

Of course you can! 😄

Cross-posting my comment from the discussion thread #3199 (comment):

In the future, we will be adding more schema options like exclude (see #2653). Because options can be combined, e.g. partial with exclude, the number of possible combinations (and thus cache) keys is growing fast. I find it impractical to enumerate all valid keys in a TypeScript enum type. We will need 4 entries now, 8 entries when we implement exclude, 16 entries when we introduce another option, etc.

That's why I switched the indexer type from options in MODLE_TYPE_KEYS to key: string.

I am going to improve this comment to tell developers to use buildModelCacheKey instead.

raymondfeng · 2019-06-21T19:28:13Z

packages/repository-json-schema/src/build-schema.ts

+   * Set this flag to mark all model properties as optional. This is typically
+   * used to describe request body of PATCH endpoints.
+   */
+  partial?: boolean;


In the future, we can support more options such as exclude.

Definitely, see e.g. #2653:

Allow Controller methods implementing CREATE operation to describe their input data as "a model without id and _rev properties ". This story requires #2629 and #2631 to be implemented first.

To emit schema with certain properties excluded, we should add a new schema-generation option called exclude & accepting a list of property names to remove from the schema. To allow TypeScript compiler to verify that exclude items are valid property names, we should leverage TypeScript generics and keyof keyword.

bajtos · 2019-06-24T08:12:53Z

@b-admike @nabdelgadir @raymondfeng @samarpanB Thank you for the review. I believe I have addressed your comments in 6e16780 and 59f0dbc. LGTY now?

nabdelgadir · 2019-06-24T18:32:49Z

Should we perhaps use + instead of ;?
expect(key).to.equal('includeRelations+partial');

I like + more than ; personally

nabdelgadir

👍

samarpanB

Looks good to me 👍

Add a new option `partial: boolean` allowing callers of `getJsonSchema` and related helpers to request a model schema with all properties marked as optional. Signed-off-by: Miroslav Bajtoš <mbajtoss@gmail.com>

Signed-off-by: Miroslav Bajtoš <mbajtoss@gmail.com>

…ATCH Signed-off-by: Miroslav Bajtoš <mbajtoss@gmail.com>

bajtos added Examples feature CLI REST Issues related to @loopback/rest package and REST transport in general labels Jun 21, 2019

bajtos requested review from jannyHou, samarpanB, nabdelgadir and a team June 21, 2019 07:22

bajtos requested a review from raymondfeng as a code owner June 21, 2019 07:22

bajtos self-assigned this Jun 21, 2019

bajtos force-pushed the feat/partial-model-schema branch from 7cef6f8 to 23f91c1 Compare June 21, 2019 07:22

bajtos mentioned this pull request Jun 21, 2019

Emit schema with all model properties optional #2652

Closed

5 tasks

b-admike reviewed Jun 21, 2019

View reviewed changes

packages/repository-json-schema/src/build-schema.ts Outdated Show resolved Hide resolved

packages/repository-json-schema/src/build-schema.ts Outdated Show resolved Hide resolved

samarpanB reviewed Jun 21, 2019

View reviewed changes

nabdelgadir reviewed Jun 21, 2019

View reviewed changes

raymondfeng reviewed Jun 21, 2019

View reviewed changes

bajtos force-pushed the feat/partial-model-schema branch from 23f91c1 to 59f0dbc Compare June 24, 2019 08:11

bajtos requested review from raymondfeng, b-admike, nabdelgadir and samarpanB June 24, 2019 08:13

nabdelgadir approved these changes Jun 24, 2019

View reviewed changes

samarpanB approved these changes Jun 24, 2019

View reviewed changes

bajtos added 5 commits June 25, 2019 10:06

feat(repository-json-schema): add an option to emit partial schema

4cf1090

Add a new option `partial: boolean` allowing callers of `getJsonSchema` and related helpers to request a model schema with all properties marked as optional. Signed-off-by: Miroslav Bajtoš <mbajtoss@gmail.com>

fix(example-todo): allow partial updates via PATCH

9e19e54

Signed-off-by: Miroslav Bajtoš <mbajtoss@gmail.com>

fix(example-express-composition): allow partial updates via PATCH

df1133b

Signed-off-by: Miroslav Bajtoš <mbajtoss@gmail.com>

fix(example-todo-list): allow partial updates via PATCH

cb6f03a

feat(cli): modify Controller templates to allow partial updates via P…

28cdc08

…ATCH Signed-off-by: Miroslav Bajtoš <mbajtoss@gmail.com>

bajtos force-pushed the feat/partial-model-schema branch from 59f0dbc to 28cdc08 Compare June 25, 2019 08:07

bajtos merged commit c7c6695 into master Jun 25, 2019

delete-merged-branch bot deleted the feat/partial-model-schema branch June 25, 2019 08:34

bajtos added the OpenAPI label Jun 25, 2019

bajtos mentioned this pull request Jun 25, 2019

refactor(repository-json-schema): unify code building schema title and cache key #3238

Merged

7 tasks

nabdelgadir mentioned this pull request Jun 26, 2019

fix(docs): remove note about not supporting partial updates #3251

Merged

7 tasks

This was referenced Jun 27, 2019

Syntactic sugar for requestBody() with custom schema options #3257

Closed

Partial update (PATCH) over REST #1722

Closed

dhmlau mentioned this pull request Jul 11, 2019

Connector does not support empty update loopbackio/loopback-connector-postgresql#339

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Emit schema with all model properties optional + allow partial updates via PATCH #3199

Emit schema with all model properties optional + allow partial updates via PATCH #3199

bajtos commented Jun 21, 2019

b-admike left a comment

samarpanB Jun 21, 2019

bajtos Jun 24, 2019

samarpanB Jun 24, 2019 •

edited

nabdelgadir Jun 21, 2019

bajtos Jun 24, 2019

raymondfeng Jun 21, 2019

bajtos Jun 24, 2019

bajtos commented Jun 24, 2019

nabdelgadir commented Jun 24, 2019

nabdelgadir left a comment

samarpanB left a comment

Emit schema with all model properties optional + allow partial updates via PATCH #3199

Emit schema with all model properties optional + allow partial updates via PATCH #3199

Conversation

bajtos commented Jun 21, 2019

Checklist

b-admike left a comment

Choose a reason for hiding this comment

samarpanB Jun 21, 2019

Choose a reason for hiding this comment

bajtos Jun 24, 2019

Choose a reason for hiding this comment

samarpanB Jun 24, 2019 • edited

Choose a reason for hiding this comment

nabdelgadir Jun 21, 2019

Choose a reason for hiding this comment

bajtos Jun 24, 2019

Choose a reason for hiding this comment

raymondfeng Jun 21, 2019

Choose a reason for hiding this comment

bajtos Jun 24, 2019

Choose a reason for hiding this comment

bajtos commented Jun 24, 2019

nabdelgadir commented Jun 24, 2019

nabdelgadir left a comment

Choose a reason for hiding this comment

samarpanB left a comment

Choose a reason for hiding this comment

samarpanB Jun 24, 2019 •

edited