apollographql / apollo-server

🌍  Spec-compliant and production ready JavaScript GraphQL server that lets you develop in a schema-first way. Built for Express, Connect, Hapi, Koa, and more.
https://www.apollographql.com/docs/apollo-server/
MIT License
13.79k stars 2.03k forks source link

Execution instrumentation requires shallow-cloning the context value, which has negative side effects #3146

Open cipsys opened 5 years ago

cipsys commented 5 years ago

If the context is a javascript Proxy, Apollo Server fails to bring the proxy in the resolver due to the fact the the context gets cloned on each and every request. Callining the proxy will return the underlying target but will fail to bring the context as a Proxy in the resolver, losing all traps.

Please refer to line 242: const context = cloneObject(options.context); https://github.com/apollographql/apollo-server/blob/master/packages/apollo-server-core/src/runHttpQuery.ts

function cloneObject<T extends Object>(object: T): T {
  return Object.assign(Object.create(Object.getPrototypeOf(object)), object);
}

Is it possible to avoid cloning? Since the application creates the context, seems normal to expect the same object created to get into the resolver and not a clone.

Umkus commented 4 years ago

Also cloning generates extra problems in case context object has getter properties.

dncrews commented 4 years ago

Rather than open a duplicate of this issue with a different use case caused by the same problem (though I'm willing to if that's a problem), I'll add to this issue:

TL;DR: I want to create something in the context function that has access to dataSources

As I've gotten to "at scale" in production system that requires immediate consistency, I've been struggling with dataSources that are "too smart". I want to keep dataSources simple, and I don't want my resolvers to have very much logic in them. I've also gotten to the point where I had complex to have mutations that had to cross multiple service domain boundaries. Rather than having micro services dependent directly on each other, I've implemented an orchestration layer.

e.g. createOrder - needs to validate the products exist and reserve quantity, verify that the payment is valid, finalize payment, finalize the order, etc

In the orchestration service, I often need to call multiple domain services, allowing them to make validations, and, well, orchestrate these mutations. I still want to keep all of the business logic out of the resolvers, so I've been working to introduce the concept of a system useCase or workflow, which would own the business rules around such intricate processes. It didn't make sense to add that to the dataSources object, since:

  1. we had dataSources that represented those affected domains. These useCases would need to call those actual dataSources
  2. these useCases didn't own any data, so it felt bad to call them dataSources

Instead, I opted to create these context.useCases during the context method, which brought me here, since the functions inside context.useCases do not have access to context.dataSources due to the aforementioned (and FIXME-documented) issue.

smyrick commented 2 years ago

The Apollo Server v4 branch still has this code but also has some more info about why the context is cloned on every request

https://github.com/apollographql/apollo-server/blob/2d5d564abe3d817f8902f07a712a6e105f1b2e41/packages/server/src/ApolloServer.ts#L1143-L1152

glasser commented 2 years ago

We'd still like to improve graphql-js so that there's a less hacky way of instrumenting execution that doesn't rely on adding per-execution data to the context object.