Skip to content

Feature/array in fields #6517

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.

Sign up for GitHub

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

Merged
dhmlau merged 1 commit into from
Oct 14, 2020
Merged

Feature/array in fields #6517

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 12 additions & 5 deletions docs/site/Fields-filter.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,21 @@ Where:
- `<true|false>` signifies either `true` or `false` Boolean literal. Use `true`
to include the property or `false` to exclude it from results.

also can be used an array of strings with the properties

<pre>
{ fields: [<i>propertyName</i>, <i>propertyName</i>, ... ] }
</pre>

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.
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.
Copy link
Contributor

@hacksparrow hacksparrow Oct 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member

@bajtos bajtos Oct 9, 2020

Choose a reason for hiding this comment

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

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).


### REST API
Copy link
Contributor

@jannyHou jannyHou Oct 12, 2020

Choose a reason for hiding this comment

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

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


<pre>
filter[fields][<i>propertyName</i>]=true|false&filter[fields][<i>propertyName</i>]=true|false...
filter[fields]=<i>propertyName</i>&filter[fields]=<i>propertyName</i>...
</pre>

Note that to include more than one field in REST, use multiple filters.
Expand All @@ -47,14 +54,14 @@ fields, you can specify all required properties as:
{% include code-caption.html content="Node.js API" %}

```ts
await customerRepository.find({fields: {name: true, address: true}});
await customerRepository.find({fields: ['name', 'address']});
```

{% include code-caption.html content="REST" %}

Its equivalent stringified JSON format:

`/customers?filter={"fields":{"name":true,"address":true}}`
`/customers?filter={"fields":["name","address"]}`

Returns:

Expand Down
11 changes: 11 additions & 0 deletions examples/todo/src/__tests__/acceptance/todo.acceptance.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,6 +293,17 @@ describe('TodoApplication', () => {
]);
});

it('queries todos with exploded array-based fields filter', async () => {
await givenTodoInstance({
title: 'go to sleep',
isComplete: false,
});
await client
.get('/todos')
.query('filter[fields][0]=title')
.expect(200, toJSON([{title: 'go to sleep'}]));
});

it('queries todos with exploded array-based order filter', async () => {
const todoInProgress = await givenTodoInstance({
title: 'go to sleep',
Expand Down
12 changes: 11 additions & 1 deletion packages/filter/src/__tests__/acceptance/query-builder.acceptance.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,17 @@ describe('FilterBuilder', () => {
},
});
});

it('builds a filter object with array', () => {
const filterBuilder = new FilterBuilder();
filterBuilder.fields(['a', 'b']);
const filter = filterBuilder.build();
expect(filter).to.eql({
fields: {
a: true,
b: true,
Copy link
Member

@bajtos bajtos Oct 8, 2020

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@mdbetancourt mdbetancourt Oct 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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

  1. you need always to check what type is (array or object)
  2. check if exists in object is faster than array (in performance O(n) vs O(1))
  3. 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

Copy link
Member

@bajtos bajtos Oct 9, 2020

Choose a reason for hiding this comment

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

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.)

Copy link
Contributor

@jannyHou jannyHou Oct 12, 2020

Choose a reason for hiding this comment

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

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

},
});
});
it('builds a filter object with limit/offset', () => {
const filterBuilder = new FilterBuilder();
filterBuilder.limit(10).offset(5);
Expand Down
15 changes: 11 additions & 4 deletions packages/filter/src/query.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,9 @@ export type Order<MT = AnyObject> = {[P in keyof MT]: Direction};
* Example:
* `{afieldname: true}`
*/
export type Fields<MT = AnyObject> = {[P in keyof MT]?: boolean};
export type Fields<MT = AnyObject> =
| {[P in keyof MT]?: boolean}
| Extract<keyof MT, string>[];

/**
* Inclusion of related items
Expand Down Expand Up @@ -560,16 +562,21 @@ export class FilterBuilder<MT extends object = AnyObject> {
* @param f - A field name to be included, an array of field names to be
* 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 {
Copy link
Member

@bajtos bajtos Oct 8, 2020

Choose a reason for hiding this comment

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

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 {

Copy link
Contributor Author

@mdbetancourt mdbetancourt Oct 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

@bajtos bajtos Oct 9, 2020

Choose a reason for hiding this comment

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

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 👍🏻

if (!this.filter.fields) {
this.filter.fields = {};
} else if (Array.isArray(this.filter.fields)) {
this.filter.fields = this.filter.fields.reduce(
(prev, current) => ({...prev, [current]: true}),
{},
Comment on lines +569 to +571
Copy link
Member

@bajtos bajtos Oct 8, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@mdbetancourt mdbetancourt Oct 8, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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 }

Copy link
Member

@bajtos bajtos Oct 9, 2020

Choose a reason for hiding this comment

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

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

);
}
const fields = this.filter.fields;
for (const field of f) {
if (Array.isArray(field)) {
(field as (keyof MT)[]).forEach(i => (fields[i] = true));
field.forEach(i => (fields[i] = true));
} else if (typeof field === 'string') {
fields[field as keyof MT] = true;
fields[field] = true;
} else {
Object.assign(fields, field);
}
Expand Down
69 changes: 60 additions & 9 deletions packages/openapi-v3/src/__tests__/unit/decorators/query.decorator.unit.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,27 @@ describe('sugar decorators for filter and where', () => {
'x-typescript-type': '@loopback/repository#Filter<MyModel>',
properties: {
fields: {
oneOf: [
{
type: 'object',
additionalProperties: false,
properties: {
name: {
type: 'boolean',
},
},
},
{
type: 'array',
uniqueItems: true,
items: {
enum: ['name'],
type: 'string',
example: 'name',
},
},
],
title: 'MyModel.Fields',
type: 'object',
properties: {name: {type: 'boolean'}},
additionalProperties: false,
},
offset: {type: 'integer', minimum: 0},
limit: {type: 'integer', minimum: 1, example: 100},
Expand Down Expand Up @@ -76,10 +93,27 @@ describe('sugar decorators for filter and where', () => {
'x-typescript-type': '@loopback/repository#Filter<MyModel>',
properties: {
fields: {
oneOf: [
{
type: 'object',
additionalProperties: false,
properties: {
name: {
type: 'boolean',
},
},
},
{
type: 'array',
uniqueItems: true,
items: {
enum: ['name'],
type: 'string',
example: 'name',
},
},
],
title: 'MyModel.Fields',
type: 'object',
properties: {name: {type: 'boolean'}},
additionalProperties: false,
},
offset: {type: 'integer', minimum: 0},
limit: {type: 'integer', minimum: 1, example: 100},
Expand Down Expand Up @@ -124,10 +158,27 @@ describe('sugar decorators for filter and where', () => {
'x-typescript-type': '@loopback/repository#Filter<MyModel>',
properties: {
fields: {
oneOf: [
{
type: 'object',
additionalProperties: false,
properties: {
name: {
type: 'boolean',
},
},
},
{
type: 'array',
uniqueItems: true,
items: {
enum: ['name'],
type: 'string',
example: 'name',
},
},
],
title: 'MyModel.Fields',
type: 'object',
properties: {name: {type: 'boolean'}},
additionalProperties: false,
},
offset: {type: 'integer', minimum: 0},
limit: {type: 'integer', minimum: 1, example: 100},
Expand Down
87 changes: 69 additions & 18 deletions packages/openapi-v3/src/__tests__/unit/filter-schema.unit.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,30 @@ describe('filterSchema', () => {
additionalProperties: true,
},
fields: {
type: 'object',
oneOf: [
{
type: 'object',
additionalProperties: false,
properties: {
id: {
type: 'boolean',
},
age: {
type: 'boolean',
},
},
},
{
type: 'array',
uniqueItems: true,
items: {
enum: ['id', 'age'],
type: 'string',
example: 'id',
},
},
],
title: 'my-user-model.Fields',
properties: {
id: {type: 'boolean'},
age: {type: 'boolean'},
},
additionalProperties: false,
},
offset: {type: 'integer', minimum: 0},
limit: {type: 'integer', minimum: 1, example: 100},
Expand All @@ -57,13 +74,30 @@ describe('filterSchema', () => {
'x-typescript-type': '@loopback/repository#Filter<MyUserModel>',
properties: {
fields: {
type: 'object',
oneOf: [
{
type: 'object',
additionalProperties: false,
properties: {
id: {
type: 'boolean',
},
age: {
type: 'boolean',
},
},
},
{
type: 'array',
uniqueItems: true,
items: {
enum: ['id', 'age'],
type: 'string',
example: 'id',
},
},
],
title: 'my-user-model.Fields',
properties: {
id: {type: 'boolean'},
age: {type: 'boolean'},
},
additionalProperties: false,
},
offset: {type: 'integer', minimum: 0},
limit: {type: 'integer', minimum: 1, example: 100},
Expand Down Expand Up @@ -98,13 +132,30 @@ describe('filterSchema', () => {
additionalProperties: true,
},
fields: {
type: 'object',
oneOf: [
{
type: 'object',
additionalProperties: false,
properties: {
id: {
type: 'boolean',
},
age: {
type: 'boolean',
},
},
},
{
type: 'array',
uniqueItems: true,
items: {
enum: ['id', 'age'],
type: 'string',
example: 'id',
},
},
],
title: 'CustomUserModel.Fields',
properties: {
id: {type: 'boolean'},
age: {type: 'boolean'},
},
additionalProperties: false,
},
offset: {type: 'integer', minimum: 0},
limit: {type: 'integer', minimum: 1, example: 100},
Expand Down
55 changes: 55 additions & 0 deletions packages/repository-json-schema/src/__tests__/unit/filter-json-schema.unit.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,15 @@ import {
describe('getFilterJsonSchemaFor', () => {
let ajv: Ajv.Ajv;
let customerFilterSchema: JsonSchema;
let dynamicCustomerFilterSchema: JsonSchema;
let customerFilterExcludingWhereSchema: JsonSchema;
let customerFilterExcludingIncludeSchema: JsonSchema;
let orderFilterSchema: JsonSchema;

beforeEach(() => {
ajv = new Ajv();
customerFilterSchema = getFilterJsonSchemaFor(Customer);
dynamicCustomerFilterSchema = getFilterJsonSchemaFor(DynamicCustomer);
customerFilterExcludingWhereSchema = getFilterJsonSchemaFor(Customer, {
exclude: ['where'],
});
Expand Down Expand Up @@ -127,6 +129,51 @@ describe('getFilterJsonSchemaFor', () => {
]);
});

it('allows free-form properties in "fields" for non-strict models"', () => {
const filter = {fields: ['test', 'id']};
ajv.validate(dynamicCustomerFilterSchema, filter);
expect(ajv.errors ?? []).to.be.empty();
});

it('allows only defined properties in "fields" for strict models"', () => {
const filter = {fields: ['test']};
ajv.validate(customerFilterSchema, filter);
expect(ajv.errors ?? []).to.containDeep([
{
keyword: 'enum',
dataPath: '.fields[0]',
params: {allowedValues: ['id', 'name']},
message: 'should be equal to one of the allowed values',
Copy link
Contributor

@jannyHou jannyHou Oct 12, 2020

Choose a reason for hiding this comment

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

nitpick:

should be equal to one of the allowed values

Does error msg contain more details of the allowed values?

Copy link
Contributor Author

@mdbetancourt mdbetancourt Oct 12, 2020

Choose a reason for hiding this comment

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

{
    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

},
]);
});

it('rejects "fields" with duplicated items for strict models', () => {
const filter = {fields: ['id', 'id']};
ajv.validate(customerFilterSchema, filter);
expect(ajv.errors ?? []).to.containDeep([
{
keyword: 'uniqueItems',
dataPath: '.fields',
message:
'should NOT have duplicate items (items ## 1 and 0 are identical)',
},
]);
});

it('rejects "fields" with duplicated items for non-strict models', () => {
const filter = {fields: ['test', 'test']};
ajv.validate(dynamicCustomerFilterSchema, filter);
expect(ajv.errors ?? []).to.containDeep([
{
keyword: 'uniqueItems',
dataPath: '.fields',
message:
'should NOT have duplicate items (items ## 1 and 0 are identical)',
},
]);
});

it('describes "include" as an array for models with relations', () => {
const filter = {include: 'invalid-include'};
ajv.validate(customerFilterSchema, filter);
Expand Down Expand Up @@ -499,3 +546,11 @@ class Customer extends Entity {
@hasMany(() => Order)
orders?: Order[];
}

@model({
settings: {strict: false},
})
class DynamicCustomer extends Entity {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
[key: string]: any;
}
Loading