Fix duplicated entries for resolver classes that use inheritance #499
Conversation
…t use inheritance
Codecov Report
@@ Coverage Diff @@
## master #499 +/- ##
==========================================
- Coverage 95.14% 94.89% -0.25%
==========================================
Files 75 76 +1
Lines 1318 1333 +15
Branches 252 259 +7
==========================================
+ Hits 1254 1265 +11
- Misses 61 65 +4
Partials 3 3
Continue to review full report at Codecov.
|
This is a huge antipattern as TypeGraphQL was never designed to work in that way. The only "dynamic" part was added for generics support, so you can provide "schema name" of a field/query to prevent duplicates in schema when property names are used by default. TypeGraphQL is not designed for that case as the schema building is a super slow process, the decorator metadata are stored globally to be able to share definitions into a separate npm packages. What is your use case that you can't just "prebuild" a few schemas on bootstrap and then redirect to a proper apollo server instance port based on the request user? |
MichalLytek
added
Community 👨👧
|
Sorry I probably should have addressed the 2 things separately but this PR I think fixes a bug that is there regardless of whether or not you use the workflow I'm describing. It's relatively harmless for most people probably as 20-30 additional entries in the array is pretty benign but it's there none the less. Easiest way to reproduce would be create couple Resolver classes using inheritance and log the Obviously this was much easier to see when using my workflow because every time I built the schema the array would grow (and by a lot more than the number of queries I had) and the filtering of the arrays etc. would get slower and slower. The reason I'm building the one schema on every request is that the schema is dependent on database entries that can change fields name, available resolvers, etc. So for example if a user goes into an admin UI and clicks to hide a query property of rename a field I'd like the resulting schema to update. The easiest way make sure the graph is always in sync with the database I can think of is to build the schema on every request but then I run into the expanding arrays issue and the performance of Sound like supporting this sort of workflow is out of scope for your vision of this library which seems fair. Thanks for cool library! |
Don't feel offended but I think it's a terrible design 😱 Maybe it would be better if you just built-in the dynamic nature of fields into the schema? Like for this kind of query: {
user(id: 2137) {
fields {
name
value
}
}
}So you will return this kind of response: {
"user": {
"fields": [
{ "name": "firstname", "value": "John" },
{ "name": "age", "value": 12 }
]
}
}The whole point of GraphQL by design is a constant contract between client and server, so you can be 100% sure what you can ask and what you will receive. With your dynamic rebuild for every request, it's not true anymore and you need to do hackish query with introspection before every request 🙅♂ |
|
Lol. No offense I oversimplified my use case. I think you’re right that would be a terrible design if I were only making names dynamic. The reality is I’m making the entire graph dynamic creation and all types, queries, and mutations. The service I’m actually working on is a service that allows users to build graphql APIs via a UI. So for example in the UI you enter some credentials for a SQL database and I can connect to it, introspect it and turn the tables into types, the columns into fields, and then make some basic CRUD resolvers around the tables. Obviously as the user adds more tables, or makes migrations, or adds an Auth provider & Auth to a field or query etc I’ll need to rebuild the schema with the new configuration. I agree it would be self defeating to change the schema all the time but that’s not really the goal. The goal is they can build and test their schema as they build it and of course once it’s built they get the benefits of graphql contract. I know your library wasn’t built with this use case in mind but it actually works surprisingly well for it. I was originally using the raw graphql classes and wrapping resolvers with Auth etc but I’ve been playing around with using this library since I use it for the static API I built to import the databases etc in the more traditional way. Anyway building the users schema on every single request is probably unnecessary. Like I said I could build it and cache it until I detect that they’ve made changes. However I’ll still need to rebuild the same schema on occasion and I'd prefer not to restart the instance each time. And then there is the problem that I’d need to build different graphs for each user in the same instance which has a similar effect to rebuilding the same graph in that the arrays in this PR grow very quickly do to the fact that all the schemas share a global storage singleton. Looking at the code it seems like it would be feasible to add a way register a unique id for the schema & metadata calls then then create a new instance of storage for each id and allow the decorators to register themselves to an id but I very likely could be missing some of the complexity since I’ve mostly just looked at the metadata storage and query decorator at this point. I the thing I’m proposing looking something like: ...
@Query(() => type, {schemaId: “xxxx”})
async createRow(...) {
...
}
...
const schema = buildSchema({resolvers: [...], schemaId: “xxxx”})
...
resetSchemaMetadata({schemaId: “xxxx”})Something along those lines anyway. The idea would be the ability to scope & clear the metadata cache for folks building more than one schema or needing to rebuild a schema occasionally and not wanting to reboot the process. |
Ok, that makes sense 👍 No we know your use case and real root of the problem, instead of just applying a fix for something 😉
That's what I wanted to propose - it's really weird that you have the metadata (SQL database layout) and you dynamically create kinda-static classes with decorators that are again translated to metadata and then into Maybe you should try different wrappers around the builder pattern, like
It's not that the schemas need to have something global or read the metadata in runtime. It's about that the So the problem is that when you create new classes, it run the decorators and register new metadata but the old, not used anymore classes (as they are replaced by a new schema) are still in the storage, while they should be cleaned. One way to achieve that is to have some imperative API like you proposed, which is error prone. A second way is to make the metadata storage garbage collectable, so e.g. instead of storing the metadata and references to classes in a static array, it should be a As the first version of TypeGraphQL was a proof of concept, I haven't thought about such things and even don't consider the What do you think? And why you've not created an issue as I asked in readme and started with PR without even describing the context and use cases that motivated the changes? 😕 |
|
Sorry for not opening an issue. In trying to figure out what was going on with my use case I thought I found a legitimate bug. The PR here doesn't actually fix my issue as the global metadata storage is still used and so every time a Just run I would expect 1 entry per schema entry point you end up with one for the decorator call and 1 for each class it inherits from. Maybe not really a bug since the logic still works because you're using As for using something like That means I can write a function that returns a The |
The reason why I don't:
Is that I try to follow FP and immutability rules. So rather than modifying the object, I would make a copy with changes (like now) and then delete the metadata of superclass as they are not needed for resolvers, only for types inheritance.
I've tried that on the The classes as keys would be referenced in schema, so when you create a new schema and delete the old one, it could be GC then 😉 BTW, how do you swap the schema after applying the changes? You close the apollo server and create a new instance? |
|
Awesome! I'll check out the Currently I'm simply creating a new |
If you just register a new one without unregistering the old middleware, it may still be attached to express router but just don't called as the new middleware doesn't call |
That's basically what I do. I don't actually attach the router to the express app. My middleware looks something like... app.use(`/:graphId/graphql`, (req, res, next) => {
const { graphId }= req.params;
const config = await db.find(Config, graphId);
if (!config) return res.sendStatus(404);
const resolvers = await buildResolvers(config);
const authChecker = await buildAuthChecker(config);
const schema = buildSchema({
resolvers,
authChecker,
authMode: 'error'
});
const server = new ApolloServer({
schema,
context: async ({ req }) => { ... },
});
const router = server.getMiddleware({ path: "/" });
router(req, res, next);
})I think that should be enough to allow things to be gc as
I can update this PR to do this if you would like these changes. I guess maybe I'd make the export function mapSuperResolverHandlers<T extends BaseResolverMetadata>(
definitions: T[],
superResolver: Function,
resolverMetadata: ResolverClassMetadata,
): T[] {
return definitions.map(metadata => {
return metadata.target === superResolver ? {
...metadata,
target: resolverMetadata.target,
resolverClassMetadata: resolverMetadata,
} : {
...metadata
}
});
}
...
private buildExtendedResolversMetadata() {
...
this.queries = mapSuperResolverHandlers(this.queries, superResolver, def);
...
} |
…ractice in mapSuperResolverHandlers
|
@nphyatt Could you add a test case that ensures that future refactors won't affect this fix and we won't have a regression in that scope? |
|
Ok added tests that fail on master but pass after this PR. Added a |
MichalLytek
changed the title
MichalLytek
added
Enhancement 🆕
This is an attempt to at least partially solve what I believe is a memory leak issue regarding metadata-storage.
If you're using a workflow where you build the schema dynamically on every request the
queriesandmutationsarrays that are properties of the global metadata storage instance quickly grow in size for classes utilizing inheritance. From what I can tell it filters for the objects it's looking for in order to copy and modify metadata from the parent resolver class but creates new entries and adds them to an ever growing list. This solution simply modifies those object in place relying on pass by reference so that the array doesn't grow forever and cause memory issues as well as degraded performance on each subsequent request.In my workflow simply clearing the global metadata cache didn't work because I have 2 schemas. 1 which is created statically when the app is started and then an additional schema which is created dynamically on each request. Clearing the cache broke my static schema.
This PR only partially solves the issue though as the
querieslist does seem to continue to grow (all be it much more slowly) adding a single duplicate item for each@Queryor@Mutationdecorator that I call during the dynamic creation of the schema. For example:Every time I prepare the Resolvers before calling
buildSchematheQuerydecorator executes and adds a new item the my list of queries. This would be fine & necessary if I were intending to keep the schemas around for future use but they are meant to be thrown away after each request .I'm using Containers and reseting after each request but the metadata storage appears to be global and it's not clear to me currently whether there is even a way to associate metadata items with the id of a container so that perhaps they could be cleared more individually at the same time you reset the container.
Perhaps my use case is rather uncommon or perhaps I'm missing something but I think this PR solves a bug that is there regardless of my particular use cases as you can see duplicate items without even having build the schema multiple times. Any guidance you might have on how I could clear other items from the metadata storage based on Container ID or what I might attempt in terms of an approach to make a PR to support this would be greatly appreciated!
Thanks for the amazing library!