fix(mongo): restrict object id conversion only to known properties

Related: #401
mikro-orm · Apr 26, 2020 · 86cd027 · 86cd027
1 parent 062baf6
commit 86cd027
Show file tree

Hide file tree

Showing 3 changed files with 53 additions and 27 deletions.
diff --git a/lib/connections/MongoConnection.ts b/lib/connections/MongoConnection.ts
@@ -70,7 +70,6 @@ export class MongoConnection extends Connection {
 
   async find<T extends AnyEntity<T>>(collection: string, where: FilterQuery<T>, orderBy?: QueryOrderMap, limit?: number, offset?: number, fields?: string[], ctx?: Transaction<ClientSession>): Promise<T[]> {
     collection = this.getCollectionName(collection);
-    where = this.convertObjectIds(where as Dictionary);
     const options: Dictionary = { session: ctx };
 
     if (fields) {
@@ -156,7 +155,6 @@ export class MongoConnection extends Connection {
 
   private async runQuery<T extends { _id: any }, U extends QueryResult | number = QueryResult>(method: 'insertOne' | 'updateMany' | 'deleteMany' | 'countDocuments', collection: string, data?: Partial<T>, where?: FilterQuery<T>, ctx?: Transaction<ClientSession>): Promise<U> {
     collection = this.getCollectionName(collection);
-    where = this.convertObjectIds(where as Dictionary);
     const options: Dictionary = { session: ctx };
     const now = Date.now();
     let res: InsertOneWriteOpResult<T> | UpdateWriteOpResult | DeleteWriteOpResultObject | number;
@@ -188,28 +186,6 @@ export class MongoConnection extends Connection {
     return this.transformResult(res!) as U;
   }
 
-  private convertObjectIds<T extends ObjectId | Dictionary | any[]>(payload: T): T {
-    if (payload instanceof ObjectId) {
-      return payload;
-    }
-
-    if (Utils.isString(payload) && payload.match(/^[0-9a-f]{24}$/i)) {
-      return new ObjectId(payload) as T;
-    }
-
-    if (Array.isArray(payload)) {
-      return payload.map((item: any) => this.convertObjectIds(item)) as T;
-    }
-
-    if (Utils.isObject(payload)) {
-      Object.keys(payload).forEach(k => {
-        payload[k] = this.convertObjectIds(payload[k]);
-      });
-    }
-
-    return payload;
-  }
-
   private transformResult(res: any): QueryResult {
     return {
       affectedRows: res.modifiedCount || res.deletedCount || 0,

diff --git a/lib/drivers/MongoDriver.ts b/lib/drivers/MongoDriver.ts
@@ -1,11 +1,12 @@
 import { ClientSession, ObjectId } from 'mongodb';
 import { DatabaseDriver } from './DatabaseDriver';
 import { MongoConnection } from '../connections/MongoConnection';
-import { EntityData, AnyEntity, FilterQuery, EntityMetadata, EntityProperty, Dictionary } from '../typings';
+import { AnyEntity, Dictionary, EntityData, EntityMetadata, EntityProperty, FilterQuery } from '../typings';
 import { Configuration, Utils } from '../utils';
 import { MongoPlatform } from '../platforms/MongoPlatform';
 import { FindOneOptions, FindOptions } from './IDatabaseDriver';
 import { QueryResult, Transaction } from '../connections';
+import { ReferenceType } from '../entity';
 
 export class MongoDriver extends DatabaseDriver<MongoConnection> {
 
@@ -166,10 +167,46 @@ export class MongoDriver extends DatabaseDriver<MongoConnection> {
         if (prop.fieldNames) {
           Utils.renameKey(data, k, prop.fieldNames[0]);
         }
+
+        let isObjectId: boolean;
+
+        if (prop.reference === ReferenceType.SCALAR) {
+          isObjectId = prop.type.toLowerCase() === 'objectid';
+        } else {
+          const meta2 = this.metadata.get(prop.type);
+          const pk = meta2.properties[meta2.primaryKeys[0]];
+          isObjectId = pk.type.toLowerCase() === 'objectid';
+        }
+
+        if (isObjectId) {
+          data[k] = this.convertObjectIds(data[k]);
+        }
       }
     });
 
     return data;
   }
 
+  private convertObjectIds<T extends ObjectId | Dictionary | any[]>(data: T): T {
+    if (data instanceof ObjectId) {
+      return data;
+    }
+
+    if (Utils.isString(data) && data.match(/^[0-9a-f]{24}$/i)) {
+      return new ObjectId(data) as T;
+    }
+
+    if (Array.isArray(data)) {
+      return data.map((item: any) => this.convertObjectIds(item)) as T;
+    }
+
+    if (Utils.isObject(data)) {
+      Object.keys(data).forEach(k => {
+        data[k] = this.convertObjectIds(data[k]);
+      });
+    }
+
+    return data;
+  }
+
 }
diff --git a/tests/issues/GH401.test.ts b/tests/issues/GH401.test.ts
@@ -11,6 +11,9 @@ class Entity401 {
   @Property()
   data: Dictionary;
 
+  @Property()
+  bar?: string;
+
   constructor(data = {}) {
     this.data = data;
   }
@@ -35,14 +38,24 @@ describe('GH issue 401', () => {
   afterAll(() => orm.close(true));
 
   test('do not automatically convert string to ObjectId in the all cases', async () => {
-    const a = new Entity401({ foo: '0000007b5c9c61c332380f78' });
-    expect(a.data.foo).toBe('0000007b5c9c61c332380f78');
+    const id = '0000007b5c9c61c332380f78';
+    const a = new Entity401({ foo: id });
+    a.bar = id;
+    expect(a.data.foo).toBe(id);
+    expect(a.bar).toBe(id);
     await orm.em.persistAndFlush(a);
     expect(a.data.foo).not.toBeInstanceOf(ObjectId);
+    expect(a.bar).not.toBeInstanceOf(ObjectId);
     orm.em.clear();
 
     const getA = await orm.em.findOneOrFail(Entity401, a._id);
     expect(getA!.data.foo).not.toBeInstanceOf(ObjectId);
+    expect(getA!.bar).not.toBeInstanceOf(ObjectId);
+    orm.em.clear();
+
+    const getA2 = await orm.em.findOneOrFail(Entity401, { bar: id });
+    expect(getA2!.data.foo).not.toBeInstanceOf(ObjectId);
+    expect(getA2!.bar).not.toBeInstanceOf(ObjectId);
   });
 
 });