@delete directive does not return error when delete has failed through observer.

[ashwin0003]

Describe the bug

@delete directive does not return error when delete has failed through observer.

Expected behavior/Solution

My mutation in graphql-schema

extend Type Mutation {
  deleteUser(id: Int! @eq): User @delete
}

Note: Im using Int! @eq instead of ID! as my database has id as a incrementing integer.

My observer

    public function deleting(User $user)
    {
        return false;
        ...
        try {
            ...
        } catch (Exception $e) {
            ...
            return false;
        }
    }

Note: The first return false in the deleting method is there for testing purposes.

Expected behaviour.

1. When observer's deleting method(event-handler) returns false, delete actions fails.
2. GraphQL should consider that as a fail and return a error message.

First step is happening as in my test-case delete is not happening as expected.
But GraphQL is not considering that be a failure and is behaving as if it was a successful deletion.

Steps to reproduce

Output/Logs

Click to expand

# Add in log output/error messages here

Environment

Lighthouse Version: v4.8.1
Laravel Version: v6.9.0

[spawnia]

I think that this case, where a model is not being deleted, is not necessarily an error. If an exception is thrown during a callback, Lighthouse will abort execution - but just returning false might have other reasons.

How about we just return the result of calling delete() as the result of the mutation?

type Mutation {
	deleteUser(id: ID!): Boolean! @delete(model: "User")
}

[olivernybroe]

You could also add the exist field on your User class.
Then after running the delete query, you can check the returned user if exists is false client side, to make sure it was actually deleted.

@spawnia If this is a problem multiple people have, I think we could consider adding a flag to the directive for throwing an error when CRUD methods on models return false.

[spawnia]

@olivernybroe throwing is a perfectly fine way to bubble up errors and will produce a proper error in the response. I don't want to support the old PHP pattern of returning false on error.

[ashwin0003]

@spawnia I have started throwing exceptions instead of returning false in my observers, which responds with the thrown exception's message as debugMessage with category internal. This is allowing me to handle the failed deletion using the response. But I don't feel like its the way it should be done.

The ideas of returning Boolean and adding new exists field seem like bad ideas.
I like the idea of throwing a error when CRUD method on models return false.

And I also apologize for the delayed response and my incomplete replies due to lack of knowledge regarding internal specs.

[spawnia]

The solution for a single deletion is simple, but i struggle with what we should do when multiple models are deleted at once. We might buffer the exceptions and throw a list of errors for all ID's where deletion failed.