Allow array in Filter.fields

[jannyHou]

Suggestion

Follow-up story from #4992 (comment)

Now the fields filter only allow object, like {fields: {title: true, desc: true}}, but doesn't allow the array shortcut like {fields: ['title', 'desc']}.

We should support the array shortcut as well.

Copy the details here:

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.

Use Cases

Filter with fields as array: {fields: ['title', 'desc']}

Examples

Filter with fields as array: {fields: ['title', 'desc']}

Acceptance criteria

• Modify the Fields<T> 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

🎆 Hacktoberfest 2020

Greetings 👋 to all Hacktoberfest 2020 participants!

Here are few tips 👀 to make your start easier, see also #6456:

• Before you start working on this issue, please leave a comment to let others know.
• If you are new to GitHub pull requests, then you can learn about the process in Submitting a pull request to LoopBack 4.
• If this is your first contribution to LoopBack, then please take a look at our Developer guide.
• Feel free to ask for help in #loopback-contributors channel, you can join our Slack workspace here.

[jeffersonlicet]

I would like to solve this issue. Is anybody working on this?

[bajtos]

@jeffersonlicet

I would like to solve this issue. Is anybody working on this?

Awesome, I am looking forward to your pull request!

Let me assign the issue to you to make it clear that you are working on it.

[jeffersonlicet]

@jeffersonlicet

I would like to solve this issue. Is anybody working on this?

Awesome, I am looking forward to your pull request!

Let me assign the issue to you to make it clear that you are working on it.

After working a little bit on the PR I found small details and questions to take into account:

1. Are we going to allow stringified JSON like: /customers?filter={"fields":["name","address"]}
   This is relevant because of the documentation and the schema validation, which currently only accepts an object.

2. Does it make sense to update the current example (await customerRepository.find({fields: {name: true, address: true}}); ) to use array syntax? (await customerRepository.find({fields: ['name', 'address']});) I think that changing this to use array shorthand (Use case 1) and keeping the Use case 2, can demonstrate both ways of removing and allowing fields.

(In case you need to check the current docs here is the link)

3. Does it make sense to keep only string keys from MT since typescript complains about other types?

This works: Extract<keyof MT, string>[]
This doesn't: (keyof MT)[]

And I want to apologize, I know that this is an easy good-first-issue but this is my first contribution and I want to be sure.

Thank you.

[bajtos]

Good questions, @jeffersonlicet!

1. Are we going to allow stringified JSON like: /customers?filter={"fields":["name","address"]}
   This is relevant because of the documentation and the schema validation, which currently only accepts an object.

Yes, we want to allow JSON-enconding of query string values (i.e. the top-level filter parameter). Please note this is handled by our REST layer, the schema should not need any specific changes to support JSON-encoding. Just describe the fields property as an array or an object in the schema. I think it's a good idea to show both flavors (JSON and "explode" encodings) in the docs 👍🏻

To avoid possible confusion, we DO NOT want to support JSON encoding of the nested fields property only, i.e. mix the two encoding styles as in /customers?filter[fields]=["name","address"].

2. Does it make sense to update the current example (await customerRepository.find({fields: {name: true, address: true}}); ) to use array syntax? (await customerRepository.find({fields: ['name', 'address']});) I think that changing this to use array shorthand (Use case 1) and keeping the Use case 2, can demonstrate both ways of removing and allowing fields.

(In case you need to check the current docs here is the link)

Yes please!

Personally, I find the array-based syntax easier to use than the current object-based one, so I think it makes a lot of sense to show the array version first, as the recommended one.

Just make sure to describe the other version too. It's best to be explicit about this. Keep all examples consistently using the same style, and then add new content to explain and show the alternate syntax. It would be great to also mention the version of @loopback/repository that added support for the new syntax, so that people on older versions are not trying to use something that's not supported.

1. Does it make sense to keep only string keys from MT since typescript complains about other types?

This works: Extract<keyof MT, string>[]
This doesn't: (keyof MT)[]

I don't have a strong opinion. Let's start with what works for you, we can discuss this aspect more deeply during pull request review.

And I want to apologize, I know that this is an easy good-first-issue but this is my first contribution and I want to be sure.

No need to apologize, we are happy to help 😄

Note: @achrinza is working on extracting filter-related code into a standalone package, his pull request #6182 is pretty much ready to be landed. Please keep an eye on it, I think you will have to rework your changes to accommodate the new structure of files & packages.

[bajtos]

@jeffersonlicet ping, have you have a chance to make any progress on this issue? Is there anything we can help you with?

[mdbetancourt]

since jefferson is not working any more on this issue, could i take this? and i have a question how is going to be handled in controllers methods?

findById(@param.filter() filter: Filter) {
   // how is going to work here te access should remain as an object? (no introducing breaking changes)
  filter.fields.name
  // or
  filter.fields.indexOf('name')
  // or
  filter.fields.has(name) // as a Set
}

[bajtos]

@mdbetancourt

since jefferson is not working any more on this issue, could i take this?

While I cannot speak for @jeffersonlicet, I think it should be ok to take over this work from him, since he haven't posted any update for a month.

how is going to be handled in controllers methods?

The top-level type Filter should remain the same as it it now. The underlying runtime (query builder, loopback-datasource-juggler, connectors) already support the array form, we just need to update type definitions to match the implementation.

At the moment, filter.fields can be a plain data object (example filter: {fields: {title: true, desc: true}}). Now we want to allow plain data arrays too (example filter: {fields: ['title', 'desc']}).

If you are accessing filter.fields from your controller method (why would you do that?), then you will need to update your controllers to handle both object and array variants.

findById(@param.filter() filter: Filter) {
   const hasName = Array.isArray(filter.fields) 
      ? filter.fields.includes('name')
      : filter.fields?.['name'];
}