fix(sql): do not alias conditions for update queries with collection …

…operators Closes #4956
mikro-orm · Nov 25, 2023 · 5820d66 · 5820d66
1 parent 1833455
commit 5820d66
Show file tree

Hide file tree

Showing 4 changed files with 44 additions and 5 deletions.
diff --git a/packages/knex/src/query/CriteriaNode.ts b/packages/knex/src/query/CriteriaNode.ts
@@ -126,6 +126,10 @@ export class CriteriaNode<T extends object> implements ICriteriaNode<T> {
     return `${path}[pivot]`;
   }
 
+  aliased(field: string, alias?: string) {
+    return alias ? `${alias}.${field}` : field;
+  }
+
   /** @ignore */
   [inspect.custom]() {
     const o: Dictionary = {};

diff --git a/packages/knex/src/query/ObjectCriteriaNode.ts b/packages/knex/src/query/ObjectCriteriaNode.ts
@@ -30,9 +30,15 @@ export class ObjectCriteriaNode<T extends object> extends CriteriaNode<T> {
     if (this.shouldAutoJoin(nestedAlias)) {
       if (keys.some(k => ['$some', '$none', '$every'].includes(k))) {
         const $and: Dictionary[] = [];
-        const primaryKeys = this.metadata.find(this.entityName)!.primaryKeys.map(pk => `${alias}.${pk}`);
+        const primaryKeys = this.metadata.find(this.entityName)!.primaryKeys.map(pk => {
+          return [QueryType.SELECT, QueryType.COUNT].includes(qb.type!) ? `${alias}.${pk}` : pk;
+        });
 
         for (const key of keys) {
+          if (!['$some', '$none', '$every'].includes(key)) {
+            throw new Error('Mixing collection operators with other filters is not allowed.');
+          }
+
           const payload = (this.payload[key] as CriteriaNode<T>).unwrap();
           const sub = qb.clone(true).innerJoin(this.key!, qb.getNextAlias(this.prop!.type));
           sub.select(this.prop!.targetMeta!.primaryKeys);
@@ -126,7 +132,7 @@ export class ObjectCriteriaNode<T extends object> extends CriteriaNode<T> {
       if (Utils.isOperator(k, false)) {
         const tmp = payload[k];
         delete payload[k];
-        o[`${alias}.${field}`] = { [k]: tmp, ...(o[`${alias}.${field}`] || {}) };
+        o[this.aliased(field, alias)] = { [k]: tmp, ...(o[this.aliased(field, alias)] || {}) };
       } else if (this.isPrefixed(k) || Utils.isOperator(k) || !childAlias) {
         const idx = prop.referencedPKs.indexOf(k as EntityKey);
         const key = idx !== -1 && !childAlias && ![ReferenceKind.ONE_TO_MANY, ReferenceKind.MANY_TO_MANY].includes(prop.kind) ? prop.joinColumns[idx] : k;
@@ -138,7 +144,7 @@ export class ObjectCriteriaNode<T extends object> extends CriteriaNode<T> {
           o.$and = $and;
         } else if (Utils.isOperator(k) && Array.isArray(payload[k])) {
             o[key] = payload[k].map((child: Dictionary) => Object.keys(child).reduce((o, childKey) => {
-              const key = (this.isPrefixed(childKey) || Utils.isOperator(childKey)) ? childKey : `${childAlias}.${childKey}`;
+              const key = (this.isPrefixed(childKey) || Utils.isOperator(childKey)) ? childKey : this.aliased(childKey, childAlias);
               o[key] = child[childKey];
               return o;
             }, {} as Dictionary));
@@ -149,6 +155,7 @@ export class ObjectCriteriaNode<T extends object> extends CriteriaNode<T> {
         o[k] = payload[k];
       } else {
         o[`${childAlias}.${k}`] = payload[k];
+        o[this.aliased(k, childAlias)] = payload[k];
       }
     }
   }

diff --git a/packages/knex/src/query/ScalarCriteriaNode.ts b/packages/knex/src/query/ScalarCriteriaNode.ts
@@ -13,7 +13,7 @@ export class ScalarCriteriaNode<T extends object> extends CriteriaNode<T> {
       const path = this.getPath();
       const parentPath = this.parent!.getPath(); // the parent is always there, otherwise `shouldJoin` would return `false`
       const nestedAlias = qb.getAliasForJoinPath(path) || qb.getNextAlias(this.prop?.pivotTable ?? this.entityName);
-      const field = `${options?.alias}.${this.prop!.name}`;
+      const field = this.aliased(this.prop!.name, options?.alias);
       const type = this.prop!.kind === ReferenceKind.MANY_TO_MANY ? JoinType.pivotJoin : JoinType.leftJoin;
       qb.join(field, nestedAlias, undefined, type, path);
 

diff --git a/tests/issues/GH2916.test.ts → ...s/collection/collection-operators.test.ts b/tests/issues/GH2916.test.ts → ...s/collection/collection-operators.test.ts
@@ -1,5 +1,5 @@
 import { Collection, Entity, ManyToOne, MikroORM, OneToMany, ManyToMany, PrimaryKey, Property, SimpleLogger } from '@mikro-orm/better-sqlite';
-import { mockLogger } from '../helpers';
+import { mockLogger } from '../../helpers';
 
 @Entity()
 class Author {
@@ -294,3 +294,31 @@ test('allows only one of $some, $none and $every on the given level', async () =
     'Author 5',
   ]);
 });
+
+test('update query with $none', async () => {
+  const mock = mockLogger(orm);
+  await orm.em.nativeUpdate(Author, {
+    books: {
+      $none: { title: 'Foo' },
+    },
+  }, { name: 'foobar' });
+  expect(mock.mock.calls[0][0]).toBe("[query] update `author` set `name` = 'foobar' where `id` not in (select `a0`.`id` from `author` as `a0` inner join `book` as `b1` on `a0`.`id` = `b1`.`author_id` where `b1`.`title` = 'Foo')");
+});
+
+test('disallow mixing', async () => {
+  const mock = mockLogger(orm);
+  // separate branches work
+  await orm.em.fork().find(Author, {
+    $and: [
+      { books: { $none: { title: 'Foo' } } },
+      { books: { title: 'bar' } },
+    ],
+  });
+  // mixing throws
+  await expect(orm.em.fork().find(Author, {
+    books: {
+      $none: { title: 'Foo' },
+      title: 'bar',
+    },
+  })).rejects.toThrow('Mixing collection operators with other filters is not allowed.');
+});