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

GraphQL logging support #490

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

agmoss
Copy link

@agmoss agmoss commented Jun 21, 2022

This PR adds the ability for GraphQL requests to be logged via the LoggingInterceptor.

Description

Nest provides a base ExecutionContext that allows for REST and GraphQL apps to be written with similar constructs. To obtain req and res metadata (headers, status, etc.) about a GraphQL request, one must first create a GqlExecutionContext from the base ExecutionContext.

Via this pr, if a request is determined to be a GraphQL, a GqlExecutionContext is created and metadata is obtained and logged in the same format as a REST request.

While this gets the "job done", I have a number of issues with this implementation:

  1. Reliance on the @nestjs/graphql dependency

To differentiate between REST and GrapQL requests, a dependency(via a peerDependency) on GqlContextType from @nestjs/graphql is required. I see this as problematic because most of the current users of the LoggingInterceptor will not have this within their project. It doesn't make much sense to require rest users of this lib to install @nestjs/graphql just to use the existing functionality.

  1. Reliance on context to have a req and res parameter

To obtain the req and res objects from the gqlContext, you must configure your GraphQL module (see apollo-server-context. This isn't as big of an issue as 1 IMO as passing in req and res to the context is very standard. I have also added error handling to this.

I could use some advice on point 1. Perhaps it is advisable to create a separate GqlLoggingInterceptor class within this package? Or perhaps I can do some work with the imports and package.json deps to negate this.

Motivation and Context

This is not a required change but it solves the issue of not being able to use this package for GraphQL apps!

#489

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

@ccoeurderoy
Copy link
Contributor

Hey @agmoss, thanks a lot for your contribution! I will look at it as soon as I can (probably this week-end) 🙂

The problem that I see here, is that peer dependencies related to graphQL could be a bit annoying for users using the express platform. So I'm thinking of creating a specific package for GraphQL (and also fastify).

Anyway, I will get back to you asap!

@ccoeurderoy ccoeurderoy force-pushed the master branch 3 times, most recently from 9caab2c to a554202 Compare March 13, 2023 10:34
@youngkiu
Copy link

I converted two components, LoggingInterceptor and HttpExceptionFilter for GraphQL, including test code.

https://github.com/youngkiu/nestjs-graphql-components/tree/feat/graphql

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.

3 participants