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

Implement support for querying soft deleted elements #937

Merged
merged 25 commits into from
Sep 2, 2019
Merged

Implement support for querying soft deleted elements #937

merged 25 commits into from
Sep 2, 2019

Conversation

lorado
Copy link
Collaborator

@lorado lorado commented Aug 27, 2019

  • Added or updated tests
  • Added Docs for all relevant versions
  • Updated the changelog

Related Issue/Intent

Resolves #100

Changes

  • New enum Trash is defined. Possible values are ONLY, WITH and WITHOUT
  • @find, @all and @paginate directives searches for argument of enum type Trash and applies defined filter to fetch either onlyTrashed, withTrashed or withoutTrashed

Breaking changes

There are no breaking changes

Additional Note

I currently implemented this feature only for @find, @all and @paginate directives. I guess that probably it would be cool to be able to define such an argument on related items too? Didn't look for the possible implementation, but what you think?

UPDATE

Reworked to @softDeletes directive, #937 (comment)

CHANGELOG.md Outdated Show resolved Hide resolved
@spawnia
Copy link
Collaborator

spawnia commented Aug 29, 2019

Nice feature, you did a great job with the implementation.

I am still pondering on the best way to add a trashed filter to a query. We could do it through a schema-manipulating field directive, such as:

foos(): [Foo!]! @all @softDeletes

@lorado
Copy link
Collaborator Author

lorado commented Aug 29, 2019

Nice feature, you did a great job with the implementation.

Thanks!

I am still pondering on the best way to add a trashed filter to a query. We could do it through a schema-manipulating field directive, such as:

foos(): [Foo!]! @all @softDeletes

I was also thinking about it. But as people already discussed here I thought it is the idea people would like =)

Currently solution is simple, but it might be not really clear, that adding Trash argument will be managed by lighthouse, as there is no visible directive in the schema definition.

I didn't write a custom directive till now, so don't know how would I implement your solution...

@spawnia
Copy link
Collaborator

spawnia commented Aug 29, 2019

@softDeletes would be a FieldManipulator, with the sole purpose to add an argument to the field. The transformed field would look like:

foos(trashed: Trash @trash): [Foo!]! @all

The @trash directive is just a regular ArgBuilderDirective.

@lorado
Copy link
Collaborator Author

lorado commented Aug 29, 2019

Ok, I'll give a try today ;)

@lorado
Copy link
Collaborator Author

lorado commented Aug 30, 2019

So, just got ready new implementation via directives. Actually this approach is much better, as we don't have to duplicate code and take care of, where this parameter could also be entered (e.g. on relation fields).

By now it works also with @hasMany and I guess all other relation directives. With less code I was able to achieve more cases. Profit 😄

One thing took me much time... I didn't figure out, why is it happened... As I updated my tests, I had something like this:

        $this->graphQL('
        {
            users {
                tasks(trashed: ONLY) {
                    id
                    name
                }
            }
        }
        ')->assertJson(...);

        $this->graphQL('
        {
            users {
               tasks(trashed: WITH) {
                    id
                }
            }
        }
        ')->assertJsonCount(3, 'data.users.0.tasks')

As you can see, these queries are different - different trashed value and even different data set. But the second request became always the same response, as first query - with only trashed tasks. And this happened ONLY in this nested structure. Tests with plain structure work fine (if you wish you can take a look at other tests). I though it is my implementation fault, but why the plain fields work? I also tried to run test with disabled cache, but it didn't help.

What helped - using aliases in graphql Query. This way results were always fetched correctly.

I don't know, if this bug also appear in real world, we will see...

@lorado
Copy link
Collaborator Author

lorado commented Sep 2, 2019

@spawnia Should I do anything else, so this PR gets merged? I am trying to keep it all the time up to date... Would be nice if this feature could be released next week.

Copy link
Collaborator Author

@lorado lorado left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made changes as you wanted

P.s. First time in this situation on github. Was not sure what happens, when I click on "submit" :D

@spawnia
Copy link
Collaborator

spawnia commented Sep 2, 2019

Hey @lorado working it in right now.

I am changing the behaviour when using the directives upon models that do not use SoftDeletes to throw instead of silently doing nothing.

@spawnia
Copy link
Collaborator

spawnia commented Sep 2, 2019

The issue with multiple queries in a single test is caused by some caching mechanism within Laravel's HTTP helpers. It should not happen in production, as PHP dies after each request.

@spawnia spawnia added the enhancement A feature or improvement label Sep 2, 2019
@lorado
Copy link
Collaborator Author

lorado commented Sep 2, 2019

The issue with multiple queries in a single test is caused by some caching mechanism within Laravel's HTTP helpers. It should not happen in production, as PHP dies after each request.

Oh ok. Yea, I guessed that there is somewhere cache, but didn't know which one. Thanks for explanation. Your changes looks great. I was also thinking about throwing an exception, but was not sure if it is a good idea.

@lorado
Copy link
Collaborator Author

lorado commented Sep 2, 2019

Offtopic. Just for me to know how to work with github PR: Is it possible somehow to pull your changes you made in this PR?

@spawnia
Copy link
Collaborator

spawnia commented Sep 2, 2019

Offtopic. Just for me to know how to work this github PR: Is it possible somehow to pull your changes you made in this PR?

You can just pull them. I committed to your branch.

lorado and others added 2 commits September 2, 2019 23:12
@lorado lorado merged commit fc2a49c into nuwave:master Sep 2, 2019
@lorado lorado deleted the support_soft_delete_queries branch September 2, 2019 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A feature or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Soft deleted models arg directive
2 participants