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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 5 commits into from Jun 25, 2019

Conversation

@bajtos
Copy link
Member

commented Jun 21, 2019

  • 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 馃憟

@bajtos bajtos requested review from jannyHou, samarpanB, nabdelgadir and strongloop/loopback-maintainers Jun 21, 2019

@bajtos bajtos requested a review from raymondfeng as a code owner Jun 21, 2019

@bajtos bajtos self-assigned this Jun 21, 2019

@bajtos bajtos force-pushed the feat/partial-model-schema branch from 7cef6f8 to 23f91c1 Jun 21, 2019

@bajtos bajtos referenced this pull request Jun 21, 2019

Closed

Emit schema with all model properties optional #2652

5 of 5 tasks complete
@b-admike
Copy link
Member

left a comment

Nice 馃挴. I have minor comments, otherwise LGTM.

Show resolved Hide resolved packages/repository-json-schema/src/build-schema.ts Outdated
Show resolved Hide resolved packages/repository-json-schema/src/build-schema.ts Outdated
@@ -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},

This comment has been minimized.

Copy link
@samarpanB

samarpanB Jun 21, 2019

Contributor

Any specific reason for removing this type check ?

This comment has been minimized.

Copy link
@bajtos

bajtos Jun 24, 2019

Author Member

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.

This comment has been minimized.

Copy link
@samarpanB

samarpanB Jun 24, 2019

Contributor

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.

Show resolved Hide resolved packages/repository-json-schema/src/build-schema.ts Outdated
Show resolved Hide resolved examples/todo-list/src/controllers/todo.controller.ts Outdated
Show resolved Hide resolved examples/todo-list/src/controllers/todo-list.controller.ts Outdated
Show resolved Hide resolved examples/todo-list/src/controllers/todo-list.controller.ts Outdated
Show resolved Hide resolved examples/todo-list/src/controllers/todo.controller.ts Outdated
/**
* TODO(semver-major) remove these constants in the next major version
* @deprecated
*/

This comment has been minimized.

Copy link
@nabdelgadir

nabdelgadir Jun 21, 2019

Contributor

Can I ask why they're being removed?

This comment has been minimized.

Copy link
@bajtos

bajtos Jun 24, 2019

Author Member

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.

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

This comment has been minimized.

Copy link
@raymondfeng

raymondfeng Jun 21, 2019

Member

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

This comment has been minimized.

Copy link
@bajtos

bajtos Jun 24, 2019

Author Member

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 bajtos force-pushed the feat/partial-model-schema branch from 23f91c1 to 59f0dbc Jun 24, 2019

@bajtos

This comment has been minimized.

Copy link
Member Author

commented Jun 24, 2019

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

@nabdelgadir

This comment has been minimized.

Copy link
Contributor

commented Jun 24, 2019

Should we perhaps use + instead of ;?

expect(key).to.equal('includeRelations+partial');

I like + more than ; personally

@nabdelgadir
Copy link
Contributor

left a comment

馃憤

@samarpanB
Copy link
Contributor

left a comment

Looks good to me 馃憤

bajtos added some commits Jun 20, 2019

feat(repository-json-schema): add an option to emit partial schema
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
Signed-off-by: Miroslav Bajto拧 <mbajtoss@gmail.com>
fix(example-express-composition): allow partial updates via PATCH
Signed-off-by: Miroslav Bajto拧 <mbajtoss@gmail.com>
feat(cli): modify Controller templates to allow partial updates via P鈥
鈥TCH

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

@bajtos bajtos force-pushed the feat/partial-model-schema branch from 59f0dbc to 28cdc08 Jun 25, 2019

@bajtos bajtos merged commit c7c6695 into master Jun 25, 2019

3 checks passed

clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@delete-merged-branch delete-merged-branch bot deleted the feat/partial-model-schema branch Jun 25, 2019

@bajtos bajtos added the OpenAPI label Jun 25, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.