make it harder to shoot yourself in the foot with unrelate. closes #470

Author: koskimas

doc/includes/API.md

@@ -1056,6 +1056,9 @@ Doesn't delete the models. Only removes the connection. For ManyToMany relations
 deletes the join column from the join table. For other relation types this sets the
 join columns to null.
 
+Note that, unlike for `relate`, you shouldn't pass arguments for the `unrelate` method.
+Use `unrelate` like `delete` and filter the rows using the returned query builder.
+
 ##### Return value
 
 Type|Description

lib/queryBuilder/QueryBuilder.js

@@ -810,29 +810,35 @@ class QueryBuilder extends QueryBuilderBase {
 
   delete() {
     return writeOperation(this, () => {
-      const deleteOperation = this._deleteOperationFactory(this);
+      if (arguments.length) {
+        throw new Error(`Don't pass arguments to delete(). You should use it like this: delete().where('foo', 'bar').andWhere(...)`);
+      }
 
-      this.addOperation(deleteOperation, []);
+      const deleteOperation = this._deleteOperationFactory(this);
+      this.addOperation(deleteOperation, arguments);
     });
   }
 
   del() {
-    return this.delete();
+    return this.delete.apply(this, arguments);
   }
 
-  relate(ids) {
+  relate() {
     return writeOperation(this, () => {
       const relateOperation = this._relateOperationFactory(this);
 
-      this.addOperation(relateOperation, [ids]);
+      this.addOperation(relateOperation, arguments);
     });
   }
 
   unrelate() {
     return writeOperation(this, () => {
-      const unrelateOperation = this._unrelateOperationFactory(this);
+      if (arguments.length) {
+        throw new Error(`Don't pass arguments to unrelate(). You should use it like this: unrelate().where('foo', 'bar').andWhere(...)`);
+      }
 
-      this.addOperation(unrelateOperation, []);
+      const unrelateOperation = this._unrelateOperationFactory(this);
+      this.addOperation(unrelateOperation, arguments);
     });
   }
 

tests/integration/eager.js

@@ -1218,23 +1218,26 @@ module.exports = (session) => {
     describe('QueryBuilder.modifyEager', () => {
 
       it('should filter the eager query using relation expressions as paths', () => {
-        return Model1
-          .query()
-          .where('id', 1)
-          .modifyEager('model1Relation2.model2Relation1', builder => {
-            builder.where('Model1.id', 6);
-          })
-          .eager('model1Relation2.model2Relation1.[model1Relation1, model1Relation2]')
-          .filterEager('model1Relation2', builder => {
-            builder.where('model_2_prop_1', 'hejsan 2');
-          })
-          .then(models => {
-            expect(models[0].model1Relation2).to.have.length(1);
-            expect(models[0].model1Relation2[0].model2Prop1).to.equal('hejsan 2');
+        return Promise.all([Model1.WhereInEagerAlgorithm, Model1.JoinEagerAlgorithm].map(eagerAlgo => {
+          return Model1
+            .query()
+            .eagerAlgorithm(eagerAlgo)
+            .where('Model1.id', 1)
+            .modifyEager('model1Relation2.model2Relation1', builder => {
+              builder.where('Model1.id', 6);
+            })
+            .eager('model1Relation2.model2Relation1.[model1Relation1, model1Relation2]')
+            .filterEager('model1Relation2', builder => {
+              builder.where('model_2_prop_1', 'hejsan 2');
+            })
+            .then(models => {
+              expect(models[0].model1Relation2).to.have.length(1);
+              expect(models[0].model1Relation2[0].model2Prop1).to.equal('hejsan 2');
 
-            expect(models[0].model1Relation2[0].model2Relation1).to.have.length(1);
-            expect(models[0].model1Relation2[0].model2Relation1[0].id).to.equal(6);
-          });
+              expect(models[0].model1Relation2[0].model2Relation1).to.have.length(1);
+              expect(models[0].model1Relation2[0].model2Relation1[0].id).to.equal(6);
+            });
+        }));
       });
 
       it('should implicitly add selects for join columns if they are omitted in filterEager/modifyEager', () => {

tests/integration/unrelate.js

@@ -51,8 +51,7 @@ module.exports = (session) => {
         it('should unrelate', () => {
           return Model1
             .query()
-            .where('id', 1)
-            .first()
+            .findById(1)
             .then(model => {
               return model
                 .$relatedQuery('model1Relation1')
@@ -71,6 +70,25 @@ module.exports = (session) => {
             });
         });
 
+        it('should fail if arguments are given', (done) => {
+          Model1
+            .query()
+            .findById(1)
+            .then(model => {
+              return model
+                .$relatedQuery('model1Relation1')
+                .unrelate(1);
+            })
+            .then(numUpdated => {
+              done(new Error('should not get here'));
+            })
+            .catch(err => {
+              expect(err.message).to.equal(`Don't pass arguments to unrelate(). You should use it like this: unrelate().where('foo', 'bar').andWhere(...)`);
+              done();
+            })
+            .catch(done);
+        });
+
       });
 
       describe('has many relation', () => {
@@ -151,6 +169,25 @@ module.exports = (session) => {
             });
         });
 
+        it('should fail if arguments are given', (done) => {
+          Model1
+            .query()
+            .findById(1)
+            .then(model => {
+              return model
+                .$relatedQuery('model1Relation2')
+                .unrelate([1, 2])
+            })
+            .then(numUpdated => {
+              done(new Error('should not get here'));
+            })
+            .catch(err => {
+              expect(err.message).to.equal(`Don't pass arguments to unrelate(). You should use it like this: unrelate().where('foo', 'bar').andWhere(...)`);
+              done();
+            })
+            .catch(done);
+        });
+
       });
 
       describe('many to many relation', () => {
@@ -218,8 +255,7 @@ module.exports = (session) => {
         it('should unrelate multiple', () => {
           return Model2
             .query()
-            .where('id_col', 1)
-            .first()
+            .findById(1)
             .then(model => {
               return model
                 .$relatedQuery('model2Relation1')
@@ -239,6 +275,25 @@ module.exports = (session) => {
             });
         });
 
+        it('should fail if arguments are given', (done) => {
+          Model2
+            .query()
+            .findById(1)
+            .then(model => {
+              return model
+                .$relatedQuery('model2Relation1')
+                .unrelate([1, 2])
+            })
+            .then(numUpdated => {
+              done(new Error('should not get here'));
+            })
+            .catch(err => {
+              expect(err.message).to.equal(`Don't pass arguments to unrelate(). You should use it like this: unrelate().where('foo', 'bar').andWhere(...)`);
+              done();
+            })
+            .catch(done);
+        });
+
       });
 
       describe('has one through relation', () => {