Feature/array in fields #6517

mdbetancourt · 2020-10-08T06:35:31Z

Closes #5857

Checklist

DCO (Developer Certificate of Origin) signed in all commits
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

Acceptance criteria

Modify the Fields type to accept array (keyof MT)[]
Update code and test after the type change
Document the new behavior in LB4/Working-with-data/fields-filter page

bajtos

Great improvements, thank you @mdbetancourt for the pull request ❤️

You are generally on the right track, I have few comments to discuss and address.

Also, can you please add an end-to-end test making an HTTP call to verify all parts work together as expected? The goal is to specify fields as an array in the request query string, and verify that it was accepted by the validation code and correctly applied by the connector.

IIRC, we don't have a dedicated place for testing REST+Repository interaction, we use the Todo example app instead. I think it's ok to add new test(s) to examples/todo/src/__tests__/acceptance/todo.acceptance.ts

bajtos · 2020-10-08T15:12:01Z

packages/filter/src/__tests__/acceptance/query-builder.acceptance.ts

+    expect(filter).to.eql({
+      fields: {
+        a: true,
+        b: true,


Is it necessary to convert fields from an array to an object? I believe juggler and connectors should support both object and array format, so I don't think we need to implement this conversion in FilterBuilder.

We have test cases to verify that all connectors support both formats, see loopback-datasource-juggler/test/basic-querying.test.js. The base SQL connector seems to support both array and object format, see e.g. here and so does our MongoDB connector (see here).

I believe that internally, juggler will convert fields from an object into an array anyways, see ModelUtils._normalize()

My conclusion is that FilterBuilder should keep not modify filter.fields value and keep it as an object or as an array, depending on what was provided by the user.

@raymondfeng @jannyHou @hacksparrow what do you think?

hi @bajtos thank you for you feedback, i saw some disadvantage about that:

you need always to check what type is (array or object)

check if exists in object is faster than array (in performance O(n) vs O(1))

fewer keystroke to check a value fields.password vs fields.indexOf('password') >= 0 vs fields.includes('password')
also you say

My conclusion is that FilterBuilder should keep not modify filter.fields value and keep it as an object or as an array, depending on what was provided by the user.

but FilterBuilder allow do

builder.fields('property').fields(['property2']).fields({ property3: true }).build()

what is filter.fields?
another problem is

builder = builder.fields('property') // adding property to fields // ..some code builder.fields({property: false}) // i remove it

some edges case like this need to be cover (so fields function become more complex) if we keep the array (we need check if exists and remove it), instead with object that's not a problem because we merge the object

Thank you for a detailed explanation! Based on your arguments, I am fine to keep representing fields as an object internally inside FilterBuilder. (Unless other maintainers disagree.)

+1 appreciate all the considerations. Agree with do the conversion here.

bajtos · 2020-10-08T15:13:17Z

packages/filter/src/query.ts

   * included, or an Fields object for the inclusion/exclusion
   */
-  fields(...f: (Fields<MT> | (keyof MT)[] | keyof MT)[]): this {
+  fields(...f: (Fields<MT> | Extract<keyof MT, string>)[]): this {


Do we still need to specify all forms? I think Extract<...> is already covered by Fields<> type now, isn't it? Would the following work?

Suggested change

fields(...f: (Fields<MT> | Extract<keyof MT, string>)[]): this {

fields(...f: (Fields<MT>)[]): this {

that cover calling it with

builder.fields('property', 'property2')

while Fields<MT> only cover

builder.fields(['property', 'property2']) // and builder.fields({property: true, property2: true})

so i kept it in order to no introduce a breaking change

I see, I did not realize Extract<keyof MT, string> is describing a single item (field name) only, not an array.

+1 to preserve backwards compatibility 👍🏻

bajtos · 2020-10-08T15:15:41Z

packages/filter/src/query.ts

+      this.filter.fields = this.filter.fields.reduce(
+        (prev, current) => ({...prev, [current]: true}),
+        {},


As I mentioned above, I think we should keep fields as an array, no reducing should be needed here.

i did it to keep some consistency to make more easy the following code (instead of check if fields is an array or an object) also i used object aproach instead of array because object is more versatile. how could be converted the following object into an array?:

{ password: false }

Good point 👍🏻 As I wrote in #6517 (comment), you convinced me it's better to store fields as an object internally.

bajtos · 2020-10-08T15:18:38Z

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

+  const objTitle =
+    options.setTitle !== false
+      ? {
+          title: `${modelCtor.modelName}.Fields`,
+        }
+      : {};


I find this bit rather complex, how about the following?

Suggested change

const objTitle =

options.setTitle !== false

? {

title: `${modelCtor.modelName}.Fields`,

}

: {};

const objTitle =

options.setTitle !== false

? `${modelCtor.modelName}.Fields`

: undefined;

And later:

return { title: objTitle, oneOf };

It slightly changes the return value - the current version returns object with no title property while the new version will return title set to undefined, I don't know if that is a problem.

Alternatively, may we can keep the current code as is to avoid unnecessary code churn?

yes this could break some test because that approach remove from the object if already existed a title

{title: 'hello', ...getFieldSchema({}, {setTitle: false})} // returns {title: undefined} //instead of {title: 'hello'}

Interesting. Was the original version triggering this problem too? Do we have a test case to ensure that {title: 'hello', ...getFieldSchema({}, {setTitle: false})} works as intended?

Anyhow, if your proposed version works, then I am ok with it, but please do consider to add a test to prevent unintended regression in the future (if we don't have one already).

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

bajtos

Thank you for the updates and explanations 🙇🏻 The new version looks much better! I found few minor details to improve, see my comments below.

@raymondfeng @jannyHou @hacksparrow Ping, have you had a chance to look at the proposed changes?

bajtos · 2020-10-09T11:53:41Z

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

  beforeEach(() => {
    ajv = new Ajv();
    customerFilterSchema = getFilterJsonSchemaFor(Customer);
+    dynamiCustomerFilterSchema = getFilterJsonSchemaFor(DynamicCustomer);


Typo?

Suggested change

dynamiCustomerFilterSchema = getFilterJsonSchemaFor(DynamicCustomer);

dynamicCustomerFilterSchema = getFilterJsonSchemaFor(DynamicCustomer);

bajtos · 2020-10-09T11:55:32Z

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

    ]);
  });

+  it('non-strict model should allow any value in "fields"', () => {


We write test names to read like an English sentence, e.g. it disallows "include" for the test above.

Can you please improve names of the new test to follow that same style too?

For example:

Suggested change

it('non-strict model should allow any value in "fields"', () => {

it('allows free-form properties in "fields" for non-strict models', () => {

bajtos · 2020-10-09T11:56:06Z

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

+    expect(ajv.errors ?? []).to.be.empty();
+  });
+
+  it('strict model should  only allow defined values in "fields"', () => {


Ditto, e.g.

Suggested change

it('strict model should only allow defined values in "fields"', () => {

it('allows only defined properties in "fields" for strict models', () => {

bajtos · 2020-10-09T11:56:45Z

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

+    ]);
+  });
+
+  it('strict "fields" should not have duplicated items', () => {


Suggested change

it('strict "fields" should not have duplicated items', () => {

it('rejects "fields" with duplicated items for strict models', () => {

bajtos · 2020-10-09T11:57:06Z

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

+    ]);
+  });
+
+  it('non-strict "fields" should not have duplicated items', () => {


Suggested change

it('non-strict "fields" should not have duplicated items', () => {

it('rejects "fields" with duplicated items for non-strict models', () => {

bajtos · 2020-10-09T11:57:27Z

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

+  const schema: JsonSchema = {oneOf: []};
+  if (options.setTitle !== false) {
+    schema.title = `${modelCtor.modelName}.Fields`;
+  }


Much cleaner & easier to understand 👏🏻

hacksparrow · 2020-10-09T12:20:33Z

docs/site/Fields-filter.md

-query will include **only** those you specifically include with filters.
+specify at least one fields filter with a value of `true` or put it in the
+array, then by default the query will include **only** those you specifically
+include with filters.


Suggestion for re-structuring the initial sections to this:

A _fields_ filter specifies properties (fields) to include or exclude from the results. ### Node.js API #### `fields` as an array <pre> { fields: [propertyName1, propertyName2, ...] } </pre> Only the properties specified in the list will be returned. #### `fields` as an object <pre> { fields: {<i>propertyName</i>: <true|false>, <i>propertyName</i>: <true|false>, ... } } </pre> Where: - _propertyName_ is the name of the property (field) to include or exclude. - `<true|false>` signifies either `true` or `false` Boolean literal. Use `true` to include the property or `false` to exclude it from results. By default, queries return all model properties in results. However, if you specify at least one fields filter with a value of `true`, then by default the query will include **only** those you specifically include with filters.

@bajtos and others thoughts?

The suggested structure looks good to me.

Just note that we support array + object format also for REST API, not just in Node.js (TypeScript).

hacksparrow · 2020-10-09T12:28:14Z

@mdbetancourt thank you for the contribution. It's pretty close to landing, please address the inputs from @bajtos. I have left a doc suggestion for everyone to share their thoughts on.

jannyHou · 2020-10-12T03:39:55Z

packages/filter/src/__tests__/acceptance/query-builder.acceptance.ts

+    expect(filter).to.eql({
+      fields: {
+        a: true,
+        b: true,


+1 appreciate all the considerations. Agree with do the conversion here.

jannyHou · 2020-10-12T04:23:11Z

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

+      {
+        keyword: 'enum',
+        dataPath: '.fields[0]',
+        message: 'should be equal to one of the allowed values',


nitpick:

should be equal to one of the allowed values

Does error msg contain more details of the allowed values?

{ keyword: 'enum', dataPath: '.fields[0]', schemaPath: '#/properties/fields/oneOf/1/items/enum', params: { allowedValues: [ 'id', 'name' ] }, message: 'should be equal to one of the allowed values' }

this is the object

jannyHou · 2020-10-12T04:29:34Z

docs/site/Fields-filter.md

+array, then by default the query will include **only** those you specifically
+include with filters.

 ### REST API


nitpick: @mdbetancourt could you also update the shortcut for the REST API section? thanks.

bajtos

👏🏻

bajtos · 2020-10-13T11:57:32Z

@jannyHou do you have any more comments? Is this pull request good to be merged?

We will need to fix the git commit history to pass our commit linter check, I hope that rebasing it on top of the latest master should be good enough.

Signed-off-by: Michel Betancourt <michelbetancourt23@gmail.com>

jannyHou

👏 LGTM 🚢

dhmlau · 2020-10-14T18:08:36Z

@mdbetancourt, thanks for your contribution. Your PR has landed! 🎉

mdbetancourt requested review from agnes512, emonddr, hacksparrow and jannyHou as code owners October 8, 2020 06:35

dhmlau added the community-contribution label Oct 8, 2020

bajtos suggested changes Oct 8, 2020

View reviewed changes

mdbetancourt requested a review from bajtos October 9, 2020 03:02

bajtos reviewed Oct 9, 2020

View reviewed changes

bajtos self-assigned this Oct 9, 2020

hacksparrow reviewed Oct 9, 2020

View reviewed changes

raymondfeng approved these changes Oct 9, 2020

View reviewed changes

mdbetancourt requested a review from bajtos October 11, 2020 00:06

jannyHou reviewed Oct 12, 2020

View reviewed changes

mdbetancourt requested review from hacksparrow and jannyHou October 12, 2020 05:39

bajtos approved these changes Oct 12, 2020

View reviewed changes

bajtos mentioned this pull request Oct 13, 2020

feat(filter): implement ensureFields utility #6495

Closed

8 tasks

feat(filter): allow use an array in filter.fields

efaa444

Signed-off-by: Michel Betancourt <michelbetancourt23@gmail.com>

jannyHou approved these changes Oct 14, 2020

View reviewed changes

dhmlau merged commit ec386c1 into loopbackio:master Oct 14, 2020

mdbetancourt deleted the feature/array-in-fields branch October 16, 2020 04:14

	fields(...f: (Fields<MT> \| Extract<keyof MT, string>)[]): this {
	fields(...f: (Fields<MT>)[]): this {

	dynamiCustomerFilterSchema = getFilterJsonSchemaFor(DynamicCustomer);
	dynamicCustomerFilterSchema = getFilterJsonSchemaFor(DynamicCustomer);

	it('non-strict model should allow any value in "fields"', () => {
	it('allows free-form properties in "fields" for non-strict models', () => {

	it('strict model should only allow defined values in "fields"', () => {
	it('allows only defined properties in "fields" for strict models', () => {

	it('strict "fields" should not have duplicated items', () => {
	it('rejects "fields" with duplicated items for strict models', () => {

	it('non-strict "fields" should not have duplicated items', () => {
	it('rejects "fields" with duplicated items for non-strict models', () => {

Feature/array in fields #6517

Feature/array in fields #6517

Uh oh!

Conversation

mdbetancourt commented Oct 8, 2020 • edited by achrinza Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Acceptance criteria

Uh oh!

bajtos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdbetancourt Oct 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdbetancourt Oct 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdbetancourt Oct 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bajtos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hacksparrow Oct 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hacksparrow commented Oct 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos left a comment

Choose a reason for hiding this comment

Uh oh!

bajtos commented Oct 13, 2020

mdbetancourt commented Oct 8, 2020 •

edited by achrinza

Loading

mdbetancourt Oct 8, 2020 •

edited

Loading

mdbetancourt Oct 8, 2020 •

edited

Loading

mdbetancourt Oct 8, 2020 •

edited

Loading

hacksparrow Oct 9, 2020 •

edited

Loading