mickhansen/dataloader-sequelize

don't use ModelManager.forEachModel, it can't handle cyclic model references properly

jedwards1211 opened this issue · 6 comments

jedwards1211 commented

I have two models, Organization and UserGroup. Organization has many UserGroups. But Organization also "belongs to" one "All Users Group"; it has a FK constraint to this specific group. Perfectly valid DB schema.

But createContext crashes with this error:

Error: Cyclic dependency found. Organizations is dependent of itself.
Dependency chain: APITokens -> Organizations -> UserGroups => Organizations
    at visit (/Users/andy/clarity/node_modules/toposort-class/build/toposort.js:173:27)
    at visit (/Users/andy/clarity/node_modules/toposort-class/build/toposort.js:212:29)
    at visit (/Users/andy/clarity/node_modules/toposort-class/build/toposort.js:212:29)
    at Toposort.sort (/Users/andy/clarity/node_modules/toposort-class/build/toposort.js:252:29)
    at ModelManager.forEachModel (/Users/andy/clarity/node_modules/sequelize/lib/model-manager.js:86:21)
    at createContext (/Users/andy/clarity/node_modules/dataloader-sequelize/lib/index.js:446:26)

I've read that I can pass constraint: false when creating the associations, but this isn't really proper since both associations have constraints.

I don't think dataloader-sequelize needs to go through the models in toposorted order? Nothing in sequelize itself seems to be using it right now (we don't use sequelize.sync or whatever) and the idea of topologically sorting the models seems fundamentally flawed since it's perfectly possible to create cyclic constraints in SQL.

mickhansen commented

Is there another public API to iterate over models?

jedwards1211 commented

One can iterate over Object.values(sequelize.models) (not sure if that's technically public API or not?)

jedwards1211 commented

I argue in that linked issue that forEachModel shouldn't do a toposort, and the toposort should be confined to some internal API that sync uses.

mickhansen commented

Not sure if that's public API or not either, but it works for me.

scarlac commented

My team has spent half a day debugging this issue. It would be great if it was fixed.
It's possible to monkey patch the library before it monkey patches sequelize:

// given 'sequelize' is set to an instance of Sequelize (ie. new Sequelze({ ... }))
const originalForEachModel = sequelize.modelManager.forEachModel;
sequelize.modelManager.forEachModel = (...args) => {
  Object.values(sequelize.models).forEach(...args);
};
const dataloaderContext = createContext(sequelize);
sequelize.modelManager.forEachModel = originalForEachModel;
// Proceed to use dataloaderContext in the context

This monkey patch fixed our cyclic dependency issues entirely.

  • ❤️1
jedwards1211 commented

I need to make a PR for this

  • ❤️2