apollographql / apollo

:rocket: Open source tools for GraphQL. Central repo for discussion.
https://www.apollographql.com
MIT License
2.62k stars 425 forks source link

Datasource initialization could use more clarity #1125

Open patrickwall57 opened 3 years ago

patrickwall57 commented 3 years ago

The datasource classes have some subtitles or pitfalls that I think could be more clearly documented. Interested in thoughts from the maintainers on this point

By my understanding, all data access objects should be initialised during server initialisation and added to the datasources section of the context when initialising the ApolloServer object. All of the docs very clearly show this. I think it would be very dangerous for someone to make an incorrect assumption about where the datasources (especially database connections/connection pools) should be initialised.

If someone makes an incorrect assumption about how the datasource classes work and add their connection initialisation to the actual implementation of the Datasource class, they could have a very bad time. In this case, they are creating and potentially not tearing down connections on each request to the server which may have negative consequences to whatever backend database they are using. This could easily lead to a denial-of-service

If my assumption about this potential consequence and the reason ALL of the examples show initialising data access objects at server start time are correct, then I would like to propose an amendment to the docs to provide a brief explanation of when/how to setup data access objects.

If there is another reason why data access is always shown in the docs to be initialised at server startup, then it may be worth clarifying those points in the documentation.

patrickwall57 commented 3 years ago

As I go to fetch an example - here i find this which is a very apt comment in the fullstack demo

// creates a sequelize connection once. NOT for every request const store = createStore();

https://github.com/apollographql/fullstack-tutorial/blob/master/final/server/src/index.js

I think this is great - maybe clarifying this in the actual documentation is a good idea - people may not find their way to this comment :)