From 8ad53a4c0ec10c5de7adeb79ba36942874e09fe8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Miroslav=20Bajto=C5=A1?= <mbajtos@cz.ibm.com>
Date: Fri, 29 Apr 2016 09:08:15 +0200
Subject: [PATCH] DAO.create: don't return the instance

Simplify DataAccessObject.create() and stop returning the
instance/array of instances. Users should always use callback (or
returned promise) to get the instance(s) created.
---
 3.0-RELEASE-NOTES.md      | 15 +++++++++++++++
 lib/dao.js                |  7 ++-----
 test/manipulation.test.js | 15 +++++++--------
 3 files changed, 24 insertions(+), 13 deletions(-)

diff --git a/3.0-RELEASE-NOTES.md b/3.0-RELEASE-NOTES.md
index 180d702c..61e33c50 100644
--- a/3.0-RELEASE-NOTES.md
+++ b/3.0-RELEASE-NOTES.md
@@ -34,3 +34,18 @@ checks `if (ctx.instance)` then you can remove this condition together with
 the branch that follows.
 
 See also the commit [30283291](https://github.com/strongloop/loopback-datasource-juggler/commit/30283291?w=1)
+
+## DAO.create no longer returns the instance(s) created
+
+While implementing support for promises in `DAO.create`, we found that this
+method returns the instance object synchronously. This is inconsistent with
+the usual convention of accessing the result via callback/promise and it may
+not work correctly in cases where some fields like the `id` property are
+generated by the database.
+
+We have changed the API to be consistent with other DAO methods: when invoked
+with a callback argument, the method does not return anything. When invoked
+without any callback, a promise is returned.
+
+See [pull request 918](https://github.com/strongloop/loopback-datasource-juggler/pull/918)
+for more details.
diff --git a/lib/dao.js b/lib/dao.js
index 73a85e77..a67957dd 100644
--- a/lib/dao.js
+++ b/lib/dao.js
@@ -271,7 +271,7 @@ DataAccessObject.create = function(data, options, cb) {
       }
       cb(errors, data);
     });
-    return data;
+    return;
   }
 
   var enforced = {};
@@ -402,10 +402,7 @@ DataAccessObject.create = function(data, options, cb) {
     }, obj, cb);
   }
 
-  // Does this make any sense? How would chaining be used here? -partap
-
-  // for chaining
-  return cb.promise || obj;
+  return cb.promise;
 };
 
 function stillConnecting(dataSource, obj, args) {
diff --git a/test/manipulation.test.js b/test/manipulation.test.js
index f2d21f6e..3317a3b8 100644
--- a/test/manipulation.test.js
+++ b/test/manipulation.test.js
@@ -115,14 +115,12 @@ describe('manipulation', function() {
 
     });
 
-    it('should return instance of object', function(done) {
+    it('should not return instance of object', function(done) {
       var person = Person.create(function(err, p) {
-        p.id.should.eql(person.id);
+        should.exist(p.id);
+        if (person) person.should.not.be.an.instanceOf(Person);
         done();
       });
-      should.exist(person);
-      person.should.be.an.instanceOf(Person);
-      should.not.exist(person.id);
     });
 
     it('should not allow user-defined value for the id of object - create', function(done) {
@@ -235,7 +233,8 @@ describe('manipulation', function() {
         { name: 'Boltay' },
         {},
       ];
-      Person.create(batch, function(e, ps) {
+      var res = Person.create(batch, function(e, ps) {
+        if (res) res.should.not.be.instanceOf(Array);
         should.not.exist(e);
         should.exist(ps);
         ps.should.be.instanceOf(Array);
@@ -254,8 +253,8 @@ describe('manipulation', function() {
           persons.should.have.lengthOf(batch.length);
           persons[0].errors.should.be.false;
           done();
-        }).should.be.instanceOf(Array);
-      }).should.have.lengthOf(3);
+        });
+      });
     });
 
     it('should create batch of objects with beforeCreate', function(done) {