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

feat(builder): add support to nested filters #141

Merged
merged 8 commits into from
Nov 9, 2020
Merged

Conversation

JoaoPedroAS51
Copy link
Collaborator

@JoaoPedroAS51 JoaoPedroAS51 commented Nov 6, 2020

Add support to nested filters to where and whereIn.

Closes #81

TODO

  • Update documentation of where and whereIn

Usage

await Model.where(['user', 'status'], 'active').get()
// GET /resource?filter[user][status]=active

await Model.whereIn(['user', 'status'], ['active', 'inactive']).get()
// GET /resource?filter[user][status]=active,inactive

@Peter-Krebs
Copy link
Collaborator

Peter-Krebs commented Nov 6, 2020

That's a good step forward, thanks!

Only problem I can see is the lack of CRUD, which I like to implement for anything.
Currently, you would not be able to add or remove values for an existing key, only overwrite them, correct? It's not strictly neccessary but worth thinking about, you know someone is going to request that sooner or later.

@JoaoPedroAS51
Copy link
Collaborator Author

@Peter-Krebs I'm afraid I didn't understand. Can you open a new issue for this, so we could discuss more about it?

@JoaoPedroAS51
Copy link
Collaborator Author

@Peter-Krebs This PR is only related to nested filters support. Any other improvement will be handle in a new PR.

@Peter-Krebs
Copy link
Collaborator

@JoaoPedroAS51 Yes I'm talking about the filters.

Consider this call:
await Model.whereIn(['user', 'status'], ['active', 'inactive'])

It would be nice to modify filters as well instead of just replacing the whole value.
But I'm fine with your current solution, I just wanted to hear what you think about it.

@JoaoPedroAS51
Copy link
Collaborator Author

@Peter-Krebs I'm afraid that making such change would break the current structure we are following in this package, which I believe was inspired by Laravel’s Eloquent.

Maybe you could provide an usage example where adding and removing filters would be useful. That way we can think together in a solution :)

I recommend opening a new issue to discuss about this, because such change is outside the scope of this PR.

What do you think @robsontenorio?

@robsontenorio
Copy link
Owner

I think we can get started with the simple solution.

@JoaoPedroAS51 JoaoPedroAS51 marked this pull request as ready for review November 6, 2020 19:05
@JoaoPedroAS51 JoaoPedroAS51 merged commit faf3d1e into dev Nov 9, 2020
@JoaoPedroAS51 JoaoPedroAS51 deleted the feat/nested-filters branch November 9, 2020 03:14
@github-actions
Copy link

🎉 This PR is included in version 1.8.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@dionysiosarvanitis
Copy link

dionysiosarvanitis commented Nov 27, 2020

Seems like solution does not support two nested values or am I doing something wrong?

query.where(['schedule', 'start'], start)
    .where(['schedule', 'end'], end)

@JoaoPedroAS51
Copy link
Collaborator Author

Hi @dionysiosarvanitis! Thank you for your report. I'm working on a fix for your issue :)

@JoaoPedroAS51
Copy link
Collaborator Author

@dionysiosarvanitis Done #154 😉

@dionysiosarvanitis
Copy link

@JoaoPedroAS51 that was really quick! Thank you!

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

Successfully merging this pull request may close these issues.

Nested filters
4 participants