feat: simplify filter and where usage for constraint, schema, and OpenAPI mapping #4745

raymondfeng · 2020-02-25T22:07:54Z

Fixes #1721, #3336, and #1749

Add support for "include" and "fields" to findById (REST API) #1721: add type and schema for filter excluding where for findById
Filter on findById is ignoring the where clause #3336: add a test for findById with filter.fields
Sugar decorators for Filter and Where parameters #1749: add sugar decorators for filter and where

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 👈

dougal83

Looks good. 👍

raymondfeng · 2020-02-26T00:22:09Z

@dougal83 I added one more feature. PTAL.

packages/openapi-v3/src/decorators/parameter.decorator.ts

nabdelgadir · 2020-02-27T04:21:57Z

@raymondfeng I'm a bit confused about why we're excluding where in findById calls. I understand from #3336 that it's being ignored, but do we want to ignore it rather than allow restrictions through it on findById calls (e.g. returning undefined if it doesn't match the restriction)?

(also there's a typo in the second commit message, excldue -> exclude)

raymondfeng · 2020-02-27T04:25:26Z

I'm a bit confused about why we're excluding where in findById calls. I understand from #3336 that it's being ignored, but do we want to ignore it rather than allow restrictions through it on findById calls (e.g. returning undefined if it doesn't match the restriction)?

The rationale behind findById is to look up a record by its primary key (id). No other search criteria should be used. If a client wants to search beyond id, use find or findOne instead.

nabdelgadir

The rationale behind findById is to look up a record by its primary key (id). No other search criteria should be used. If a client wants to search beyond id, use find or findOne instead.

Ah, I see. Thank you for the explanation 👍

derdeka · 2020-02-27T08:32:22Z

The rationale behind findById is to look up a record by its primary key (id). No other search criteria should be used. If a client wants to search beyond id, use find or findOne instead.

@raymondfeng
Thanks for explanation. I did not know this before.
To clarify this for developers, maybe we can add this rationale here?:
https://github.com/strongloop/loopback-next/blob/2e52f355f95cee82b5401bfa2bcbbf6ccc5a91d2/packages/repository/src/repositories/repository.ts#L173-L181

achrinza

Other than @derdeka's comments, everything else LGTM 👍

packages/repository/src/repositories/repository.ts

bajtos

Personally, I think it's a better design to allow callers of getFilterSchemaFor and @param.filter to provide {excludeWhere: true} when they want to remove where from the schema, I find it more extensible and future-proof.

Having said that, it's something we can implement in addition to what's proposed in this pull request. I see there have been already view approvals, I am ok to go with the crowd.

packages/cli/snapshots/integration/generators/controller.integration.snapshots.js

packages/openapi-v3/src/decorators/parameter.decorator.ts

packages/repository/src/query.ts

dougal83 · 2020-02-27T18:36:16Z

Personally, I think it's a better design to allow callers of getFilterSchemaFor and @param.filter to provide {excludeWhere: true} when they want to remove where from the schema, I find it more extensible and future-proof.

@bajtos +1 If given a preference I would prefer to use this approach that you've given. While either way will work it is neater to use an option.

raymondfeng · 2020-02-27T18:48:25Z

@dougal83 The PR has been refined to address the review comments. PTAL.

dougal83 · 2020-02-27T19:58:38Z

packages/cli/snapshots/integration/generators/relation.belongs-to.integration.snapshots.js

-exports[
-  `lb4 relation checks generated source class repository  answers {"relationType":"belongsTo","sourceModel":"Order","destinationModel":"Customer"} generates Order repository file with different inputs 1`
-] = `
+exports[`lb4 relation checks generated source class repository  answers {"relationType":"belongsTo","sourceModel":"Order","destinationModel":"Customer"} generates Order repository file with different inputs 1`] = `


nitpick: double space after repository?

Sorry, I'll move this commit to a separate PR. The snapshot is generated. If we need to fix the prompt, we should fix cli.

jannyHou

👍 Nice to have the shortcut!

bajtos

Awesome, I find the new implementation so much simpler and also more flexible 💪

I have few comments on details that could be improved, see below.

packages/openapi-v3/src/decorators/parameter.decorator.ts

packages/repository-json-schema/src/__tests__/unit/filter-json-schema.unit.ts

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

packages/repository/src/query.ts

deepakrkris

LGTM

bajtos · 2020-02-28T15:07:34Z

@raymondfeng Feel free to merge the PR after my comments are addressed, no need to wait for another review from me (unless you want to).

hacksparrow

👏

Implements #1749

raymondfeng requested review from agnes512, bajtos, deepakrkris, emonddr, hacksparrow, jannyHou and nabdelgadir as code owners February 25, 2020 22:07

raymondfeng force-pushed the fix-1721 branch from 03d7109 to be725df Compare February 25, 2020 23:25

raymondfeng mentioned this pull request Feb 25, 2020

[DO NOT MERGE] feat(docs): march milestone #4594

Closed

7 tasks

dougal83 approved these changes Feb 25, 2020

View reviewed changes

raymondfeng force-pushed the fix-1721 branch from babe088 to e2a4222 Compare February 26, 2020 00:18

raymondfeng changed the title ~~feat: add type and schema for filter excluding where for findById~~ feat: simplify filter and where usage for constraint, schema, and OpenAPI mapping Feb 26, 2020

raymondfeng requested a review from dougal83 February 26, 2020 00:22

raymondfeng force-pushed the fix-1721 branch from e2a4222 to bdfa28a Compare February 26, 2020 02:00

nabdelgadir reviewed Feb 27, 2020

View reviewed changes

packages/openapi-v3/src/decorators/parameter.decorator.ts Outdated Show resolved Hide resolved

raymondfeng force-pushed the fix-1721 branch from bdfa28a to 5e3703d Compare February 27, 2020 04:21

raymondfeng force-pushed the fix-1721 branch from 5e3703d to 01eec6c Compare February 27, 2020 04:23

raymondfeng requested a review from nabdelgadir February 27, 2020 04:25

nabdelgadir approved these changes Feb 27, 2020

View reviewed changes

dougal83 approved these changes Feb 27, 2020

View reviewed changes

achrinza approved these changes Feb 27, 2020

View reviewed changes

packages/repository/src/repositories/repository.ts Show resolved Hide resolved

bajtos reviewed Feb 27, 2020

View reviewed changes

packages/cli/snapshots/integration/generators/controller.integration.snapshots.js Outdated Show resolved Hide resolved

packages/openapi-v3/src/decorators/parameter.decorator.ts Show resolved Hide resolved

packages/repository/src/query.ts Show resolved Hide resolved

raymondfeng force-pushed the fix-1721 branch 3 times, most recently from 2c2b093 to a0e2e6e Compare February 27, 2020 18:18

raymondfeng requested a review from bajtos February 27, 2020 19:50

dougal83 reviewed Feb 27, 2020

View reviewed changes

dougal83 approved these changes Feb 27, 2020

View reviewed changes

raymondfeng force-pushed the fix-1721 branch 2 times, most recently from 31177ff to 137aa2d Compare February 27, 2020 20:23

feat(repository): skip undefined property values for toJSON

8e049ec

raymondfeng force-pushed the fix-1721 branch from 137aa2d to 282c371 Compare February 27, 2020 21:18

emonddr approved these changes Feb 28, 2020

View reviewed changes

jannyHou approved these changes Feb 28, 2020

View reviewed changes

bajtos reviewed Feb 28, 2020

View reviewed changes

deepakrkris approved these changes Feb 28, 2020

View reviewed changes

bajtos requested review from achrinza and nabdelgadir February 28, 2020 15:08

bajtos added developer-experience Issues affecting ease of use and overall experience of LB users feature labels Feb 28, 2020

hacksparrow approved these changes Feb 28, 2020

View reviewed changes

raymondfeng added 5 commits February 28, 2020 07:52

feat: improve filter schema to allow exclusion

9db4056

feat(openapi-v3): add sugar decorators for filter/where params

db7b1f9

Implements #1749

feat: constrain filter to exclude where for findById

9f64d83

feat: use @param.filter and @param.where decorators

dc21fe3

feat(docs): add @param.filter and @param.where to OpenAPI decorators

60514c6

raymondfeng force-pushed the fix-1721 branch from 282c371 to 60514c6 Compare February 28, 2020 15:52

nabdelgadir approved these changes Feb 28, 2020

View reviewed changes

achrinza approved these changes Feb 28, 2020

View reviewed changes

raymondfeng merged commit deba8b4 into master Feb 28, 2020

raymondfeng deleted the fix-1721 branch February 28, 2020 16:40

This was referenced Feb 28, 2020

Filter on findById is ignoring the where clause #3336

Closed

Sugar decorators for Filter and Where parameters #1749

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: simplify filter and where usage for constraint, schema, and OpenAPI mapping #4745

feat: simplify filter and where usage for constraint, schema, and OpenAPI mapping #4745

raymondfeng commented Feb 25, 2020 •

edited

Loading

dougal83 left a comment

raymondfeng commented Feb 26, 2020

nabdelgadir commented Feb 27, 2020

raymondfeng commented Feb 27, 2020

nabdelgadir left a comment

derdeka commented Feb 27, 2020

achrinza left a comment

bajtos left a comment

dougal83 commented Feb 27, 2020

raymondfeng commented Feb 27, 2020

dougal83 Feb 27, 2020

raymondfeng Feb 27, 2020

raymondfeng Feb 27, 2020

jannyHou left a comment

bajtos left a comment

deepakrkris left a comment

bajtos commented Feb 28, 2020

hacksparrow left a comment

feat: simplify filter and where usage for constraint, schema, and OpenAPI mapping #4745

feat: simplify filter and where usage for constraint, schema, and OpenAPI mapping #4745

Conversation

raymondfeng commented Feb 25, 2020 • edited Loading

Checklist

dougal83 left a comment

Choose a reason for hiding this comment

raymondfeng commented Feb 26, 2020

nabdelgadir commented Feb 27, 2020

raymondfeng commented Feb 27, 2020

nabdelgadir left a comment

Choose a reason for hiding this comment

derdeka commented Feb 27, 2020

achrinza left a comment

Choose a reason for hiding this comment

bajtos left a comment

Choose a reason for hiding this comment

dougal83 commented Feb 27, 2020

raymondfeng commented Feb 27, 2020

dougal83 Feb 27, 2020

Choose a reason for hiding this comment

raymondfeng Feb 27, 2020

Choose a reason for hiding this comment

raymondfeng Feb 27, 2020

Choose a reason for hiding this comment

jannyHou left a comment

Choose a reason for hiding this comment

bajtos left a comment

Choose a reason for hiding this comment

deepakrkris left a comment

Choose a reason for hiding this comment

bajtos commented Feb 28, 2020

hacksparrow left a comment

Choose a reason for hiding this comment

raymondfeng commented Feb 25, 2020 •

edited

Loading