VulcanJS / Vulcan

🌋 A toolkit to quickly build apps with React, GraphQL & Meteor
http://vulcanjs.org
MIT License
7.98k stars 1.89k forks source link

Input filter operator _nin is not yet implemented #2638

Closed adalidda closed 3 years ago

adalidda commented 4 years ago

Hi,

I just notice that in the current version, in the input filter, while the operator _in is implemented, the operator _nin is not yet implemented.

Thank you Adalidda

eric-burel commented 4 years ago

For the record you can nest _not and _in to simulate _nin. Happy to accept a PR with _nin someday.

Note that this part of the code is being transitioned from Meteor to a NPM package at the moment, we'll be back at adding feature when it's done.

adalidda commented 4 years ago

Thank You @eric-burel

ErikDakoda commented 3 years ago

@eric-burel I just noticed that in vulcan-lib/lib/server/graphql/typedefs.js in String_Selector and others many of the selector options are commented out. I needed to use _gt for a string input parameter, so I uncommented it, and the selector worked just fine, returning documents filtered correctly.

Can you let me know what the status of this is? It actually appears that _gte and _lte are even unit tested in vulcan:lib/mongoParams/keep multiple filters.

eric-burel commented 3 years ago

Good question, ping @SachaG

I've noticed that but I am not sure why, I have started to convert the whole thing in TS but haven't run much filters yet https://github.com/VulcanJS/vulcan-npm/blob/devel/packages/mongo/mongoParams.ts

eric-burel commented 3 years ago

They live here now: https://github.com/VulcanJS/vulcan-npm/blob/devel/packages/graphql/server/defaultSchema.ts

I remember asking the same for Date_selector. I think they indeed just lack testing, if you have unit tests for them then I think we're good activating them. Let me know because I'll need to reenable them on Vulcan NPM too until we have unified both.

The main security threat is not the options in themselves, but the ability to remove document that you shouldn't have been able to filter, and it is correctly handled and unit tested in current version. So adding operators shouldn't be a problem as long as you confirm they work as expected.

kevinashworth commented 3 years ago

EDIT: Oops. Ignore what's below. I see Erik already changed things!

Should this issue be marked as resolved?


Sacha had previously said it was commented out because it wasn't implemented yet. See https://vulcanjs.slack.com/archives/C02L990AF/p1595281668431400

The minimal change to get _nin working for me was only for a string (_id) so just to uncomment its first occurence in typedefs.js. Seems to me that now uncommenting most of the selectors would work fine. I don't see the "ilike" or "contain_all" operators being in place, but otherwise I count maybe a dozen selectors to uncomment in that file.

ErikDakoda commented 3 years ago

Implemented in PR #2672