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

[Question] How do we query to filter based on 2+ array items on the front-end? #482

Closed
kshitizshankar opened this issue May 11, 2020 · 12 comments

Comments

@kshitizshankar
Copy link

Considering a simple use case where I have the following records of Books loaded into Vuex store of the Books Service. Each book contains a list of tags.

Book 1 Coding Javascript Education
Book 2 Javascript Coding FeathersJS
Book 3 Javascript FeathersVuex FeathersJS Coding

How do I query for books that have Coding & FeathersJS tags?

query: {
    tag: {
         $in: ['Coding', 'FeathersJS']
    }
}

This gives results for the books that have the Coding or FeathersJS tags as expected. (Book1, Book2, Book3)

I am not sure how to query for books that contain both the tags.

On the backend with mongoose, I can use the $all operator to achieve this - but how do I go about doing this on the frontend?

Currently, my workaround was to have a custom getter on the service which executes the $in query and then loops through the results to find the items where both tags are present.

Is there a better way to do this? Did I miss something w.r.t querying?

@J3m5
Copy link
Contributor

J3m5 commented May 11, 2020

MongoDB allows to query a array with exact equality but in that case it respect the specific order of the items in the array.

You can do this if you are sure about the order of the items:

query: {
    tag: ['Coding', 'FeathersJS']
}

Or you can use the $all operator like that:

query: {
    tag: { $all: ['Coding', 'FeathersJS'] }
}

but you might need to whitelist the $all operator on the used service.

@kshitizshankar
Copy link
Author

@J3m5 Thanks a lot!! I just stumbled on it as well. Ended up digging around in the code, found out that sift is being used to filter the results and the whitelist option is sent as a parameter.

$all seems to work as expected after whitelisting it! :)

Can I safely whitelist $and or other mongodb operators as well? On the mongodb $all documentation page it says that the following queries are equivalent -
{ tags: { $all: [ "ssl" , "security" ] } }
{ $and: [ { tags: "ssl" }, { tags: "security" } ] }

However, even after whitelisting both, the results are not the same when passed to the find getter ($all works as expected but $and just returns everything without any filtering). Is that expected?

@J3m5
Copy link
Contributor

J3m5 commented May 11, 2020

Can I safely whitelist $and or other mongodb operators as well?

It depends on what you want your users to be able to do with your data, but for $and I would say yes, but there might be a risk of data leak with the $populate operator for example.

However, even after whitelisting both, the results are not the same when passed to the find getter ($all works as expected but $and just returns everything without any filtering). Is that expected?

You should try this
But sift should behave like mongo, you can try to open an issue to bring back the question.

@kshitizshankar
Copy link
Author

kshitizshankar commented May 11, 2020

If I add it to the additionalOperators=['$elemMatch'] in the 'service-module.getters.js' file and replace it with const additionalOperators = ['$elemMatch','$and'] - It starts working and let's me execute the { $and: [ { tags: "ssl" }, { tags: "security" } ] } query.

const FILTERS = ['$sort', '$limit', '$skip', '$select']
const OPERATORS = ['$in', '$nin', '$lt', '$lte', '$gt', '$gte', '$ne', '$or']
const additionalOperators = ['$elemMatch']
const defaultOps = FILTERS.concat(OPERATORS).concat(additionalOperators)

export default function makeServiceGetters() {
  return {
    list(state) {
      return state.ids.map(id => state.keyedById[id])
    },
    find: state => params => {
      params = { ...params } || {}

      // Set params.temps to true to include the tempsById records
      params.temps = params.hasOwnProperty('temps') ? params.temps : false

      const { paramsForServer, whitelist } = state
      const q = _omit(params.query || {}, paramsForServer)
      const customOperators = Object.keys(q).filter(
        k => k[0] === '$' && !defaultOps.includes(k)
      )
      const cleanQuery = _omit(q, customOperators)

      const { query, filters } = filterQuery(cleanQuery, {
        operators: additionalOperators.concat(whitelist)
      })

Why do the top-level $ operators get filtered out irrespective of the whitelist? Trying to get a better understanding. Not sure if there is any documentation/posts/issues that go into this..


It depends on what you want your users to be able to do with your data, but for $and I would say yes, but there might be a risk of data leak with the $populate operator for example.

Oh ok - Though I am not using any $populate, but it sounds like something that might cause future problems. Will try and simplify my stories for now and get back to this once I have a better understanding I guess.

@J3m5
Copy link
Contributor

J3m5 commented May 11, 2020

I think it's because of this:

const customOperators = Object.keys(q).filter(
  k => k[0] === '$' && !defaultOps.includes(k)
)
const cleanQuery = _omit(q, customOperators)

The filtering doesn't take the whitelist into account, I've seen it before but I don't know why it's done because the cleanQuery function is there for that and it is recursive.

@J3m5
Copy link
Contributor

J3m5 commented May 11, 2020

When the whitelist was implemented, it was not added in the filter.
So we need to know if this filtering is really necessary, if yes add the whitelist to the filtering and if not remove the whole filtering.

@kshitizshankar
Copy link
Author

I think it's because of this:

const customOperators = Object.keys(q).filter(
  k => k[0] === '$' && !defaultOps.includes(k)
)
const cleanQuery = _omit(q, customOperators)

The filtering doesn't take the whitelist into account, I've seen it before but I don't know why it's done because the cleanQuery function is there for that and it is recursive.

Yeah I get what is happening in that flow - just don't understand why 😅
So I have no idea if this is a bug or if there are other concerns that led to removing all '$' operators from the top-level of the query except those specifically hard-coded in there.

Hopefully, someone else can shed some light on this!

@J3m5
Copy link
Contributor

J3m5 commented May 11, 2020

I think that we can safely remove this code block because the filterQuery function doesn't keep the operator if it's not in the given operators list.

@kshitizshankar
Copy link
Author

@J3m5 Oh perfect - This would make my life a lot easier for sure if it can be safely removed!

@marshallswain
Copy link
Member

I would really appreciate a PR if this will benefit users. I'm a bit strapped for time for the next week or so, but I can review PRs and do releases. Thanks guys!

@kshitizshankar
Copy link
Author

@marshallswain @J3m5 started looking into how to contribute and get this fixed (I've never done a PR before so trying to figure it out). Along the way, I ran into some test cases that might need some fixing as well -

const customOperators = Object.keys(q).filter(
  k => k[0] === '$' && !defaultOps.includes(k)
)
const cleanQuery = _omit(q, customOperators)

If I remove the code for clean query from the service getters, 2 of the test cases fail -

  it('find with custom operator', function() {
    const { state } = this
    const params = { query: { test: false, $populateQuery: 'test' } }
    const results = find(state)(params)

    assert(results.data.length === 1, 'the length was correct')
    assert(results.data[0]._id === 3, 'the correct record was returned')
    assert(results.limit === 0, 'limit was correct')
    assert(results.skip === 0, 'skip was correct')
    assert(results.total === 1, 'total was correct')
  })

and

  it('find with non-whitelisted custom operator fails', function() {
    const { state } = this
    const params = { query: { $client: 'test' } }
    let results
    try {
      results = find(state)(params)
    } catch (error) {
      assert(error)
    }
    assert(!results[0])
  })

The first test case shouldn't pass at all given $populateQuery is not whitelisted (unless I am missing something) - It passes only because the existing cleanQuery code removes it and the final query that gets executed becomes {test: false}.

The 2nd test case passes right now because assert(!results[0]) is always true given results is an object. It never goes into the catch part since $client gets removed from the query like before.

      {
        total,
        limit: filters.$limit || 0,
        skip: filters.$skip || 0,
        data: values
      }

My experience with mocha hasn't been that much so don't know I missed something.
If we can safely remove the 1st test case and fix the 2nd one to something like -

  it('find with non-whitelisted custom operator fails', function() {
    const { state } = this
    const params = { query: { $client: 'test' } }
    let results
    try {
      results = find(state)(params)
    } catch (error) {
      assert(error, 'error was the correct response.')
    }
    assert(!results, 'should have failed without whitelisted custom operator')
  })

With those changes, the issue gets fixed and all test cases get passed.

(Just for reference, here is the change in the find function in module.getters -

find: state => params => {
      if (isRef(params)) {
        params = params.value
      }
      params = { ...params } || {}

      // Set params.temps to true to include the tempsById records
      params.temps = params.hasOwnProperty('temps') ? params.temps : false

      const { paramsForServer, whitelist, keyedById } = state
      const q = _omit(params.query || {}, paramsForServer)
      
      //COMMENTING OUT THE OPERATOR CLEANUP
      // const customOperators = Object.keys(q).filter(
      //   k => k[0] === '$' && !defaultOps.includes(k)
      // )
      // const cleanQuery = _omit(q, customOperators)
     
      //USING THE INITIAL QUERY INSTEAD OF THE CLEANED UP ONE.
      const cleanQuery = q;

      const { query, filters } = filterQuery(cleanQuery, {
        operators: additionalOperators.concat(whitelist)
      })
     ...
     ...

Let me know if this looks OK. I am still trying to figure out how to send a PR for this (Can both test-cases edit and code edits be part of the PR?)

marshallswain added a commit that referenced this issue Sep 2, 2020
This is intented to fix the issue uncovered in #482.

It also adds `$populateParams` to the `paramsForServer`  by default, which means it now supports feathers-graph-populate out of the box, now.
@marshallswain
Copy link
Member

@kshitizshankar thanks for the details in your last comment. I was able to make the PR and have published it as feathers-vuex@3.12.1. Please reopen this issue if you still find that there's a problem.

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

No branches or pull requests

3 participants