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

feat: add GraphQLContextBuilder #1663

Conversation

samuelAndalon
Copy link
Contributor

@samuelAndalon samuelAndalon commented Jan 30, 2023

📝 Description

The GraphQLContextFactory allows people to easily create the GraphQLContext that will be used during the execution of a graphQL operation, depending on each client, this context could contain a lot of entries, the process of creating a particular entry could involve IO operations, or expensive in memory operations.

This PR attempts to add another way to create the GraphQLContext by using composition of producers by entry in the context.

// entry factory functional interface
fun interface GraphQLContextEntryProducer<Request, K: Any, V> {
    suspend fun generate(
        request: Request,
        graphQLRequest: GraphQLServerRequest,
        accumulator: Map<Any, Any?>
    ): Pair<K, V>?
}

// context builder with a list of entry factory
interface GraphQLContextBuilder<Request> : GraphQLContextProvider<Request> {

    val producers: List<GraphQLContextEntryProducer<Request, Any, *>>

    override suspend fun generateContext(
        request: Request,
        graphQLRequest: GraphQLServerRequest,
    ): GraphQLContext =
        producers.fold(mutableMapOf<Any, Any?>()) { accumulator, producer ->
            accumulator.also {
                producer.produce(request, graphQLRequest, accumulator)?.let { entry ->
                    accumulator += entry
                }
            }
        }.toGraphQLContext()
}

when a Factory becomes to large the maintainability could be a problem, the testability also could become a headache.

using a factory by entry will make the process of creating the context more flexible, testable and maintainable.

the GraphQLContextFactory will still be available, as both, GraphQLContextFactory and GraphQLContextBuilder implement a base interface GraphQLContextProvider

interface GraphQLContextFactory<Request> : GraphQLContextProvider<Request> {
    override suspend fun generateContext(
        request: Request,
        graphQLRequest: GraphQLServerRequest
    ): GraphQLContext =
        emptyMap<Any, Any>().toGraphQLContext()
}

Server will be fully agnostic of which GraphQLContextProvider implementation was used

class GraphQLServer<Request>(
    private val requestParser: GraphQLRequestParser<Request>,
    private val contextProvider: GraphQLContextProvider<Request>,
    private val requestHandler: GraphQLRequestHandler
) {
  // val graphQLContext = contextProvider.generateContext(request, graphQLRequest)

}

@samuelAndalon samuelAndalon added changes: major Changes require a major version module: server Issue affects the server code labels Jan 30, 2023
@samuelAndalon samuelAndalon marked this pull request as ready for review February 10, 2023 00:34
@dariuszkuc
Copy link
Collaborator

dariuszkuc commented Feb 12, 2023

Hello 👋
I am unsure about the use case for this feature - i.e. it adds a lot of complexity for arguably small convenience feature. You could achieve the same functionality by invoking whatever builders/providers from the existing context factory.

class MyCustomSpringContextFactory(val provider: SomeContextProviders) : DefaultSpringGraphQLContextFactory() {
    override suspend fun generateContext(request: ServerRequest): GraphQLContext {
        return super.generateContext(request).plus(
            // whatever logic goes here
            // iterate over and call the providers OR call individual functions
        )
    }
    
    suspend fun foo1(): Map<Any, Any> {
        // or split it up by functions
    }
}

The major change is the addition of parsed GraphQLServerRequest to the context generation - IMHO accessing query string or variables doesn't make much sense in the context factory. Guessing the only potentially useful information is the operationName which might be a good piece of information to include in logging (outside of logging unsure what would be other use cases). If that is the only use case -> maybe we should just autopopulate operationName in the context (similar as we auto populate tracing info) and keep the logic simple?

--- edit ---
Or keep the logic as is and just ask folks to write custom server if they need the functionality?

request: Request,
graphQLRequest: GraphQLServerRequest,
): GraphQLContext =
producers.fold(mutableMapOf<Any, Any?>()) { accumulator, producer ->
Copy link
Collaborator

Choose a reason for hiding this comment

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

note: while producers define their invoke function as suspend, logic below invokes it sequentially - was the intent to run them in parallel?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

given that there is an accumulator the idea is to execute them sequentially.

@dariuszkuc
Copy link
Collaborator

dariuszkuc commented Feb 13, 2023

Was just thinking - it seems like the major feature that this PR brings is the ability to do some logic around parsed GraphQL request. Maybe we should generalize it and provide some server execution hooks?

i.e.

  • when generating the schema we can provide schema generation hooks
  • when processing raw HTTP requests we can use Spring WebFilter (or Ktor equivalent) - great for reading headers (e.g. doing some auth stuff) or adding headers to the response but we shouldn't be parsing the request body (which is our GraphQL request) in the filters
  • during GraphQL engine execution we can use instrumentation

What we are missing is the ability to customize server behavior - yes, folks can provide their own server and override the methods but it is pretty cumbersome. Default server logic is as follows

1. parse HTTP request to a GraphQL request
2. generate GraphQL context
3. process GraphQL request

Wondering whether we should update it to (hook names TBD)

1. [new] hook -> willParseRequest
3. parse HTTP request to a GraphQL request
4. [new] hook -> didParseRequest
5. generate GraphQL context
6. [new] hook -> willProcessRequest
7. process GraphQL request
8. [new] hook -> didProcessRequest

@samuelAndalon
Copy link
Contributor Author

The major change is the addition of parsed GraphQLServerRequest to the context generation - IMHO accessing query string or variables doesn't make much sense in the context factory.

the GraphQLContext from graphql-java is defined as a object that can contain key value pairs that can be useful as a "context" when executing a GraphQL operation, in this case the actual graphQL operation is something that we could use to populate entries in the context, the specific use case i have for this is that i need the MDC to be populated with the operationName (operationNames in case of a BatchRequest), and # operations if its a batch request. For this use case i could leverage instrumentations that are already passing the operation as an argument, but i am already populating some information in the ContextFactory.

maybe we should just autopopulate operationName in the context (similar as we auto populate tracing info) and keep the logic simple?

the main concern i have with this is that we are already adding some extra entries in the Context.

@samuelAndalon
Copy link
Contributor Author

You could achieve the same functionality by invoking whatever builders/providers from the existing context factory.

I agree that this functionality can be achieved with the current GraphQLContextFactory, this PR only provides the basic interfaces to make this pattern adoption easier, no major changes in the way how context is created given that GraphQLContextFactory is still available to be used, and change is backwards compatible given that now the builder and the factory implement the same provider interface

@dariuszkuc
Copy link
Collaborator

change is backwards compatible given that now the builder and the factory implement the same provider interface

Change is not backwards compatible as it changes the factory method signature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changes: major Changes require a major version module: server Issue affects the server code
Development

Successfully merging this pull request may close these issues.

None yet

2 participants