MongoRepository delete by ObjectId deletes the wrong entity

[danmana]

Issue type:

[ ] question
[x] bug report
[ ] feature request
[ ] documentation issue

Database system/driver:

[ ] cordova
[x] mongodb
[ ] mssql
[ ] mysql / mariadb
[ ] oracle
[ ] postgres
[ ] cockroachdb
[ ] sqlite
[ ] sqljs
[ ] react-native
[ ] expo

TypeORM version:

[x] latest
[ ] @next
[ ] 0.x.x (or put your version here)

Steps to reproduce or a small repository showing the problem:

@Entity()
export class Post {

    @ObjectIdColumn()
    _id: ObjectID;

    @Column()
    title: string;

}

// try to delete a post by passing directly it's ObjectId (as the signature suggests should be possible)
// delete(criteria: string|string[]|number|number[]|Date|Date[]|ObjectID|ObjectID[]|FindConditions<Entity>): Promise<DeleteResult>
const post: Post = getSomePost();
repository.delete(post._id);

If you look at the actual query executed (in MongoQueryRunner) you will see deleteOne({}).
Notice the empty query! This will have a side effect of removing the first Post from the collection, whatever that is ...

If the Post entity has the id field renamed to postId the query will be deleteOne({ _id: Buffer}), which is still wrong, because the type should be ObjectId not Buffer. In this case the sideffect is less severe, it just doesn't delete anything

The workaround is to use repository.delete({_id: post._id}) which works correctly.

The problem is in MongoEntityManager.convertMixedCriteria which tests for instanceof ObjectId in the wrong order:

protected convertMixedCriteria(metadata: EntityMetadata, idMap: any): ObjectLiteral {
        if (idMap instanceof Object) {
            return metadata.columns.reduce((query, column) => {
                const columnValue = column.getEntityValue(idMap);
                if (columnValue !== undefined)
                    query[column.databasePath] = columnValue;
                return query;
            }, {} as any);
        }

        // means idMap is just object id
        const objectIdInstance = PlatformTools.load("mongodb").ObjectID;
        return {
            "_id": (idMap instanceof objectIdInstance) ? idMap : new objectIdInstance(idMap)
        };
    }

If idMap is an ObjectId it will obviously also be an instance of Object and it will enter wrongly in the first branch, never reaching the code at the bottom.