-
Notifications
You must be signed in to change notification settings - Fork 643
Description
Hello! Recently, knex moved to use mssql v4 which dropped support for queueing queries within transactions (and to me, and it honestly looks like they aren't willing to recreate it, either). This change means that you should perform only sequential operations within them. Now, onto the issue with Objection.js:
objection.js/lib/queryBuilder/operations/eager/RelationJoinBuilder.js
Lines 34 to 44 in 5da8def
| /** | |
| * Fetches the column information needed for building the select clauses. | |
| * This must be called before calling `build`. `buildJoinOnly` can be called | |
| * without this since it doesn't build selects. | |
| */ | |
| fetchColumnInfo(builder) { | |
| const columnInfo = RelationJoinBuilder.columnInfo; | |
| const allModelClasses = findAllModels(this.expression, this.rootModelClass); | |
| return Promise.all( | |
| allModelClasses.map(ModelClass => { |
Here we have a explicit Promise.all which does things concurrently.
Now, we have two ways of solving this:
- Everywhere we use
Promise.all, we replace with a series implementation (bluebirdmade this quite counterintuitive...):
return Promise.reduce(allModelClasses, function (_, ModelClass) {
const table = builder.tableNameFor(ModelClass);
if (columnInfo[table]) {
return columnInfo[table];
} else {
columnInfo[table] = ModelClass.query()
.childQueryOf(builder)
.columnInfo()
.then(info => {
const result = {
columns: Object.keys(info)
};
columnInfo[table] = result;
return result;
});
return columnInfo[table];
}
}, null);(the code above works, can PR if needed, I currently "solve" the problem by either patching this code or using another eager algorithm)
- Or, in this specific case, it's possible to cache everything beforehand (by calling it outside a transaction), perhaps through a static method (I didn't see an easy way of doing this externally, so I didn't try it).
Now, I haven't looked much, but I suspect Promise.all is part of another feature which I'm not currently using which is graph inserts, which will probably suffer from the same problem with this transaction/mssql combo.