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

Add @whereHasConditions directive to filter relation existence #1140

Merged
merged 28 commits into from
Jan 15, 2020
Merged

Add @whereHasConditions directive to filter relation existence #1140

merged 28 commits into from
Jan 15, 2020

Conversation

chescos
Copy link
Collaborator

@chescos chescos commented Jan 13, 2020

  • Added or updated tests
  • Added Docs for all relevant versions
  • Updated CHANGELOG.md

Resolves #1112

Changes

This PR adds a new directive called @whereHasConditions. This directive is very similar to the @whereConstraints directive, except that the query conditions are applied to an Eloquent relationship.

The @whereConstraints directive has also been renamed to @whereConditions after a discussion in #1112.

Here's how the @whereHasConditions directive can be used in the schema:

type Query {
    users(
        hasPosts: _ @whereHasConditions(columns: ["title", "content"])
        hasComments: _ @whereHasConditions(relation: "posts.comments", columns: ["reply"])
    ): [User!]! @all
}

And here's an example query:

{
    users(
        hasPosts: { column: TITLE, operator: LIKE, value: "%some title%" },
        hasComments: { column: REPLY, operator: EQ, value: "hi" }
    ) {
        id
    }
}

Breaking changes

The @whereConstraints directive has been renamed to @whereConditions.

@chescos
Copy link
Collaborator Author

chescos commented Jan 13, 2020

@spawnia I'll start working on this PR soon if that's fine with you. Please let me know if there are any other requirements except what we discussed in #1112.

About renaming the @whereConstraints directive - do you want to do that in a separate PR or should we do this here as well?

@spawnia
Copy link
Collaborator

spawnia commented Jan 13, 2020

@chescos looking forward to seeing the results, nice! The tests look good.

Let's do the renaming now, i think that will be the least effort overall. It's bettter to make such breaking changes early, before this becomes so great that everybody will use it. We should be able to include it as a stable feature soon.

@chescos
Copy link
Collaborator Author

chescos commented Jan 13, 2020

@spawnia The @whereHasConditions directive is pretty much done, I think I covered everything. The code is mostly copied from the @whereConstraints directive as it follows the same logic. One question though... the Operator and SQLOperator classes can be fully shared between both directives - should they just stay in the WhereConstraints folder?

A code review would be awesome. After that, I'd add the documentation and add it to the changelog.

@spawnia
Copy link
Collaborator

spawnia commented Jan 13, 2020

All related code should be in a namespace WhereConditions, including everything for the previous @whereConstraints. Having one service provider for both is reasonable.

@chescos
Copy link
Collaborator Author

chescos commented Jan 13, 2020

@spawnia great call, thanks! That made things a lot cleaner already. Anything else that could be improved?

@chescos chescos marked this pull request as ready for review January 14, 2020 12:55
@chescos
Copy link
Collaborator Author

chescos commented Jan 14, 2020

@spawnia could you give it another review?

@spawnia
Copy link
Collaborator

spawnia commented Jan 14, 2020

This is starting to come together quite nicely. The refactorings and unification has brought the added line count down from ~800 to ~350, which i am much more comfortable with from a maintenance standpoint.

@chescos
Copy link
Collaborator Author

chescos commented Jan 14, 2020

Yes, looking nice so far. Anything else that needs improvement?

@spawnia
Copy link
Collaborator

spawnia commented Jan 15, 2020

This is good to go, awesome work.

We might consider marking this feature as stable in v5 and including it by default, i really like where it's at now.

@spawnia spawnia changed the title Add @whereHasConditions directive Add @whereHasConditions directive to filter relation existence Jan 15, 2020
@spawnia spawnia merged commit d6b6123 into nuwave:master Jan 15, 2020
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.

Add Relationship support for whereConstraints through whereHas and orWhereHas
2 participants