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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Failed to exclude properties by setting false in fields filter via url #4992

Closed
4 of 5 tasks
agnes512 opened this issue Mar 29, 2020 · 9 comments
Closed
4 of 5 tasks
Assignees
Labels
Milestone

Comments

@agnes512
Copy link
Contributor

agnes512 commented Mar 29, 2020

Description

From the docs https://loopback.io/doc/en/lb3/Fields-filter.html ( it's the same for LB4), filter fields can be set to true/false in a query to include/exclude model properties. fields works with API Explorer and Node. But it couldn't set properties to false (exclude) via url.

Current Behavior

For example, model Order has three properties: id, name, and price. To exclude the name property from the result, as the docs suggested, the query can be written as ?filter[fields][name]=false. But it returns:

[{name: 'order 1'}, {name: 'order 2'}..]

i.e it includes only the name property instead. I checked the filter sent to the controller, it shows that the filter is being converted to

{fields: { name: 'false'}}  // string

while with API Explorer, the filter is converted to

{fields: { name: false}}  // boolean

The wrong type might cause the problem.

In order to make it work with urls, the query needs to be set as ?filter[fields][id]=true&filter[fields][price]=true

The issue might relate to #2208

Expected Behavior

fields should be able to exclude properties by setting properties to false via url just like what API Explorer/Node does.

Acceptance

  • check where clause if it has the same issue ( see this).
  • fix it so that API Explorer/Node/url all have the same behavior
  • add tests to check the filter that is sent to the controllers
  • fix docs in LB4/Working-with-data/fields-filter page
  • Based on the code, it seems accept arrays {fields: ['foo', 'bar']}. Check if array works for fields. Add proper normalization and error checking if needed. (explained in comment Failed to exclude properties by setting false in fields filter via url #4992 (comment))
@dhmlau
Copy link
Member

dhmlau commented Mar 30, 2020

@agnes512, could you please add acceptance criteria? Thanks.

@agnes512
Copy link
Contributor Author

@agnes512
Copy link
Contributor Author

Note: stringified JSON works well e.g ?filter[fields][name]=false works

@deepakrkris deepakrkris self-assigned this May 13, 2020
@dhmlau dhmlau added this to the Jun 2020 milestone May 20, 2020
@InvictusMB
Copy link
Contributor

@agnes512 @raymondfeng
I see some more inconsistencies in handling filter.fields in general. Do we expect fields to be {[k: string]: boolean} objects in future or arrays should also be universally acceptable?

https://github.com/strongloop/loopback-next/blob/a81ce7e1193f7408d30d984d0c3ddcec74f7c316/packages/repository/src/query.ts#L508-L523

The code above, for example, accepts both arrays and objects as arguments but misbehaves if this.filter.fields is array. Therefore if I set scope: {fields: ['foo', 'bar']} in a model decorator, all inclusions of that model will have broken filters as a result.

Should there be a normalization step somewhere?

@agnes512
Copy link
Contributor Author

I checked {fields: ['foo', 'bar']} on my end. It won't let me have such filter because of some type constraints.

But I didn't know it accepts arrays. I thought
{ fields: {propertyName: <true|false>, propertyName: <true|false>, ... } } was the only form that it took. This should be checked throughly. I will add it to the criteria. Thanks!

@InvictusMB
Copy link
Contributor

I think my confusion stems from loopback-datasource-juggler.
If I make a request via REST API and it goes directly into the juggler layer it works with arrays and does not report any violations.
If a filter is a part of include clause and gets amended inside @loopback/repository it breaks silently and that filter reaches the juggler mangled.

@jannyHou
Copy link
Contributor

fields: ['foo', 'bar'] is not allowed with bad request error. The reason is, type Fields<T> only allows object, so that type Filter<T> can only have fields as object. Array is now accepted. See:

https://github.com/strongloop/loopback-next/blob/ccea25fc382457f9436adfc0d8f6ce3a2d029c5e/packages/repository/src/query.ts#L196

and

https://github.com/strongloop/loopback-next/blob/ccea25fc382457f9436adfc0d8f6ce3a2d029c5e/packages/repository/src/query.ts#L162

But as @InvictusMB pointed out, the where builder allows array. And LB3 supports array syntax as well.

https://github.com/strongloop/loopback-next/blob/a81ce7e1193f7408d30d984d0c3ddcec74f7c316/packages/repository/src/query.ts#L508-L523

I am creating a new story to allow array in type Fields<T>. We can move the further discussion there.

@jannyHou
Copy link
Contributor

Created new story for the array fields: #5857

@jannyHou
Copy link
Contributor

Closed as PRs landed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants