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

CrudAuth filter as a mandatory filter in query #327

Merged
merged 2 commits into from
Dec 5, 2019
Merged

CrudAuth filter as a mandatory filter in query #327

merged 2 commits into from
Dec 5, 2019

Conversation

kereslas
Copy link
Contributor

This fixes the issue explained in #323 and #302.

For use case of explained issue it works fine. Only concern is that this line:

const defaultSearch = this.getDefaultSearchCondition(options, parsed);

not only gets parsed.authFilter, but also options.query.filter and parsed.paramsFilter. Because of that I don't know if the fix can affect this two parameters in an unexpected behavior. Review necessary by a project author for other use cases.

This fixes the issue explained in #323 and #302. 

For use case of explained issue it works fine. Only concern is that this line:

const defaultSearch = this.getDefaultSearchCondition(options, parsed);

not only gets parsed.authFilter, but also options.query.filter and parsed.paramsFilter. Because of that I don't know if the fix can affect this two parameters in an unexpected behavior. Review necessary by a project author for other use cases.
@kereslas
Copy link
Contributor Author

Please @zMotivat0r, review this as author of CrudAuth. Thanks!

@michaelyali
Copy link
Member

Thanks a lot for your effort @kereslas
I'm gonna merge this. And my plan is to mark filter and or params as deprecated and totally get rid of it in v5 so that we could use the new search mechanism.

@michaelyali michaelyali merged commit 543d186 into nestjsx:master Dec 5, 2019
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

Successfully merging this pull request may close these issues.

2 participants