Merge pull request #726 from ForestAdmin/fix/prevent-auto-cast-to-objectid

Author: VincentMolinie

src/services/filters-parser.js

@@ -16,13 +16,15 @@ function FiltersParser(model, timezone, options) {
     if (value === 'false') { return false; }
     return typeof value === 'boolean' ? value : null;
   };
-  const parseString = (value) => {
-    // NOTICE: Check if the value is a real ObjectID. By default, the isValid method returns true
-    //         for a random string with length 12 (example: 'Black Friday').
+  const parseObjectId = (value) => {
+    // This fix issue where using aggregation pipeline, mongoose does not
+    // automatically cast 'looking like' string value to ObjectId
+    // CF Github Issue https://github.com/Automattic/mongoose/issues/1399
     const { ObjectId } = options.Mongoose.Types;
     if (ObjectId.isValid(value) && ObjectId(value).toString() === value) {
       return ObjectId(value);
     }
+
     return value;
   };
   const parseArray = (value) => ({ $size: value });
@@ -66,8 +68,8 @@ function FiltersParser(model, timezone, options) {
       case 'Number': return parseInteger;
       case 'Date': return parseDate;
       case 'Boolean': return parseBoolean;
-      case 'String': return parseString;
-      case _.isArray(type): return parseArray;
+      case 'ObjectId': return parseObjectId;
+      case Array.isArray(type): return parseArray;
       default: return parseOther;
     }
   };
@@ -77,6 +79,7 @@ function FiltersParser(model, timezone, options) {
 
     // NOTICE: Mongoose Aggregate don't parse the value automatically.
     let field = SchemaUtils.getField(modelSchema, fieldName);
+    let fieldDefinition = model.schema.paths[fieldName];
 
     if (!field) {
       throw new InvalidFiltersFormatError(`Field '${fieldName}' not found on collection '${modelSchema.name}'`);
@@ -85,14 +88,17 @@ function FiltersParser(model, timezone, options) {
     const isEmbeddedField = !!field.type.fields;
     if (isEmbeddedField) {
       field = SchemaUtils.getField(field.type, subfieldName);
+      fieldDefinition = fieldDefinition[subfieldName];
     }
 
     if (!field) return (val) => val;
 
-    const parse = this.getParserForType(field.type);
+    const { ObjectId } = options.Mongoose.Schema.Types;
+    const fieldType = fieldDefinition instanceof ObjectId ? 'ObjectId' : field.type;
+    const parse = this.getParserForType(fieldType);
 
     return (value) => {
-      if (value && _.isArray(value)) {
+      if (value && Array.isArray(value)) {
         return value.map(parse);
       }
       return parse(value);
@@ -157,7 +163,7 @@ function FiltersParser(model, timezone, options) {
     if (!_.isObject(condition)) {
       throw new InvalidFiltersFormatError('Condition cannot be a raw value');
     }
-    if (_.isArray(condition)) {
+    if (Array.isArray(condition)) {
       throw new InvalidFiltersFormatError('Filters cannot be a raw array');
     }
     if (!_.isString(condition.field)

test/tests/services/filters-parser.test.js

@@ -15,6 +15,7 @@ describe('service > filters-parser', () => {
     Mongoose: mongoose,
     connections: { mongoose },
   };
+  const { ObjectId } = mongoose.Types;
 
   beforeAll(async () => {
     islandForestSchema = {
@@ -25,6 +26,7 @@ describe('service > filters-parser', () => {
       searchFields: ['name'],
       fields: [
         { field: 'id', type: 'Number' },
+        { field: 'myObjectId', type: 'String' },
         { field: 'name', type: 'String' },
         { field: 'size', type: 'Number' },
         { field: 'isBig', type: 'Boolean' },
@@ -44,6 +46,9 @@ describe('service > filters-parser', () => {
             fields: [{
               field: 'weapon',
               type: 'String',
+            }, {
+              field: '_id',
+              type: 'ObjectId',
             }],
           },
         },
@@ -60,6 +65,7 @@ describe('service > filters-parser', () => {
     await mongooseConnect();
     const IslandSchema = new mongoose.Schema({
       id: { type: Number },
+      myObjectId: { type: ObjectId },
       name: { type: String },
       size: { type: Number },
       isBig: { type: Boolean },
@@ -76,6 +82,12 @@ describe('service > filters-parser', () => {
           },
         },
       },
+      ships: {
+        type: {
+          weapon: { type: String },
+          _id: { type: ObjectId },
+        },
+      },
     });
 
     IslandModel = mongoose.model('Island', IslandSchema);
@@ -104,6 +116,33 @@ describe('service > filters-parser', () => {
 
   afterEach(() => jest.restoreAllMocks());
 
+  describe('getParserForType', () => {
+    describe('with a String type', () => {
+      it('should not try to cast it to ObjectId', () => {
+        expect.assertions(3);
+
+        const parser = defaultParser.getParserForType('String');
+
+        expect(parser('53c2ae8528d75d572c06adbc')).toStrictEqual('53c2ae8528d75d572c06adbc');
+        expect(parser('Star Wars')).toStrictEqual('Star Wars');
+        expect(parser('20')).toStrictEqual('20');
+      });
+    });
+
+    describe('with an ObjectId type', () => {
+      it('should try to cast it to ObjectId', () => {
+        expect.assertions(4);
+
+        const parser = defaultParser.getParserForType('ObjectId');
+
+        expect(parser('53c2ae8528d75d572c06adbc')).toStrictEqual(ObjectId('53c2ae8528d75d572c06adbc'));
+        expect(parser('star wars with the same ')).toStrictEqual('star wars with the same ');
+        expect(parser('Star Wars')).toStrictEqual('Star Wars');
+        expect(parser('20')).toStrictEqual('20');
+      });
+    });
+  });
+
   describe('getParserForField', () => {
     describe('with an embedded field', () => {
       it('should return the parser for the nested field', async () => {
@@ -126,6 +165,75 @@ describe('service > filters-parser', () => {
 
         spy.mockRestore();
       });
+
+      describe('with a nested ObjectId', () => {
+        it('should return the parser for the nested field', async () => {
+          expect.assertions(6);
+
+          const fakeParser = jest.fn().mockReturnValue('parsedValue');
+          const spy = jest.spyOn(defaultParser, 'getParserForType').mockReturnValue(fakeParser);
+
+          const parserForField = await defaultParser.getParserForField('ships:_id');
+
+          expect(defaultParser.getParserForType).toHaveBeenCalledTimes(1);
+          expect(defaultParser.getParserForType).toHaveBeenCalledWith('ObjectId');
+
+          expect(fakeParser).not.toHaveBeenCalled();
+
+          expect(parserForField('myValue')).toStrictEqual('parsedValue');
+
+          expect(fakeParser).toHaveBeenCalledTimes(1);
+          expect(fakeParser).toHaveBeenCalledWith('myValue');
+
+          spy.mockRestore();
+        });
+      });
+    });
+
+    describe('with a String', () => {
+      it('should return the string parser', async () => {
+        expect.assertions(6);
+
+        const fakeParser = jest.fn().mockReturnValue('parsedValue');
+        const spy = jest.spyOn(defaultParser, 'getParserForType').mockReturnValue(fakeParser);
+
+        const parserForField = await defaultParser.getParserForField('name');
+
+        expect(defaultParser.getParserForType).toHaveBeenCalledTimes(1);
+        expect(defaultParser.getParserForType).toHaveBeenCalledWith('String');
+
+        expect(fakeParser).not.toHaveBeenCalled();
+
+        expect(parserForField('myValue')).toStrictEqual('parsedValue');
+
+        expect(fakeParser).toHaveBeenCalledTimes(1);
+        expect(fakeParser).toHaveBeenCalledWith('myValue');
+
+        spy.mockRestore();
+      });
+    });
+
+    describe('with an ObjectId', () => {
+      it('should return the ObjectId parser', async () => {
+        expect.assertions(6);
+
+        const fakeParser = jest.fn().mockReturnValue('parsedValue');
+        const spy = jest.spyOn(defaultParser, 'getParserForType').mockReturnValue(fakeParser);
+
+        const parserForField = await defaultParser.getParserForField('myObjectId');
+
+        expect(defaultParser.getParserForType).toHaveBeenCalledTimes(1);
+        expect(defaultParser.getParserForType).toHaveBeenCalledWith('ObjectId');
+
+        expect(fakeParser).not.toHaveBeenCalled();
+
+        expect(parserForField('myValue')).toStrictEqual('parsedValue');
+
+        expect(fakeParser).toHaveBeenCalledTimes(1);
+        expect(fakeParser).toHaveBeenCalledWith('myValue');
+
+        spy.mockRestore();
+      });
     });
   });