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 ClientErrorLogMiddleware for detailed 4xx error logging #1128

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

untidy-hair
Copy link

@untidy-hair untidy-hair commented Feb 20, 2021

Hi, thank you for creating/maintaining this great library!

I've created a PR to add a logging middleware to the library for 4xx errors.
Currently when there is a 400 error, Django's logging is something very simple like this:

[WARNING] 2021-02-17 06:20:58.277 Bad Request: /graphql

I checked actual errors on client side every time this happened and sometimes it was hard to reproduce the error that had happened in the past. I knew it wouldn't be too hard to write a middleware to show more details in the logs, but it had taken me about 2 months before I actually got to write one at work. So I think it would be handy for a lot of people if we bundle one in the library.

Once you enable the middleware (please see docs/installation.rst on how), you will see more details like so:

[WARNING] 2021-02-20 18:00:50.871 Graphql Error: [{'message': 'Cannot query field "createUser" on type "Mutation".', 'locations': [{'line': 2, 'column': 3}]}]
The Query is: {'query': 'mutation {\n  createUser(userData: {\n    name: "Hallo"\n  }) {\n    user {\n      id\n      name\n    }\n  }\n}\n', 'variables': None}
[WARNING] 2021-02-17 06:20:58.277 Bad Request: /graphql

Thanks in advance :)

@untidy-hair untidy-hair changed the title Add ClientErrorLogMiddleware for detailed logging Add ClientErrorLogMiddleware for detailed 4xx error logging Feb 20, 2021
@zbyte64
Copy link
Collaborator

zbyte64 commented Feb 20, 2021

Should this be implemented as graphene middleware? https://docs.graphene-python.org/en/latest/execution/middleware/

@untidy-hair
Copy link
Author

hmm, I tried implementing with graphene middleware as you suggested @zbyte64 but I cannot make it working. Could you help me with it?

logger = logging.getLogger(__name__)

class MyLogMiddleware:
    def resolve(self, next, root, info, **args):
        res = next(root, info, **args)
        if res.errors:
            # TODO: build up actual error messages. 
            logger.warning("3.141592")
        return res

def test_test(caplog):
    Reporter.objects.create(last_name="ABA")

    invalid_query = """
        query ReporterQuery {
          reporter {
            invalidAttrName 
          }
        }
    """

    schema = graphene.Schema(query=Query)
    schema.execute(invalid_query, middleware=[MyLogMiddleware()])

    # THIS FAILS, there is supposed to be one warning logged, but len() returns 0
    assert len(caplog.records) == 1

@zbyte64
Copy link
Collaborator

zbyte64 commented Feb 22, 2021

It could be that next raises an exception and res.errors gets populated further up in the stack. In PR #1122 we put the next call in a try catch block.

@untidy-hair
Copy link
Author

I'm sorry, I haven't been able to take much time. However I read Graphene doc, too, but it doesn't seem to elaborate on how errors are handled in graphene middleware. Where should I look if I want to understand? Maybe some file suggestions if you have any?

@ulgens
Copy link
Collaborator

ulgens commented Mar 3, 2021

@untidy-hair I have a PR on graphql-core repo, i think it can help you how to find how errors are handled: graphql-python/graphql-core-legacy#269 (I'm aware it's not a middleware, just a suggestion 🙂 )

@untidy-hair
Copy link
Author

untidy-hair commented Mar 12, 2021

@zbyte64 I was able to take some time so I looked into how graphql middleware works but I found that for some errors, middlewares will never run: https://github.com/graphql-python/graphql-core/blob/main/src/graphql/graphql.py#L166-L197.
So I think we should go with django middleware in this case? Let me know your thoughts. Thanks!

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

Successfully merging this pull request may close these issues.

4 participants