Merge pull request #29 from feathersjs/internal-methods

Use internal methods instead of service methods directly

README.md

@@ -82,6 +82,10 @@ You can run this example by using `node server` and going to [localhost:8080/tod
 
 ## Changelog
 
+__2.1.0__
+
+- Use internal methods instead of service methods directly
+
 __2.0.0__
 
 - Refactoring to be independent from Knex module

package.json

@@ -49,7 +49,7 @@
   "dependencies": {
     "babel-polyfill": "^6.3.14",
     "feathers-errors": "^1.1.5",
-    "feathers-query-filters": "^1.3.0",
+    "feathers-query-filters": "^1.5.1",
     "is-plain-object": "^2.0.1",
     "uberproto": "^1.2.0"
   },

src/index.js

@@ -52,7 +52,7 @@ class Service {
 	}
 
   knexify(query, params, parentKey) {
-    Object.keys(params).forEach((key) => {
+    Object.keys(params || {}).forEach((key) => {
       const value = params[key];
 
       if (isPlainObject(value)) {
@@ -78,16 +78,9 @@ class Service {
     });
   }
 
-	find(params) {
-    params.query = params.query || {};
-
+	_find(params, count, getFilter = filter) {
     let query = this.db().select(['*']);
-    let filters = filter(params.query);
-
-    if(this.paginate.default) {
-      filters.$limit = Math.min(filters.$limit || this.paginate.default,
-				this.paginate.max || Number.MAX_VALUE);
-    }
+    let filters = getFilter(params.query || {});
 
 		// $select uses a specific find syntax, so it has to come first.
 		if (filters.$select) {
@@ -113,46 +106,69 @@ class Service {
 		if (filters.$skip) {
       query.offset(filters.$skip);
 		}
-
-    if (this.paginate.default && !params.query[this.id]) {
+
+    const executeQuery = total => {
+      return query.then(data => {
+        return {
+          total,
+          limit: filters.$limit,
+          skip: filters.$skip || 0,
+          data
+        };
+      });
+    };
+
+    if(count) {
       let countQuery = this.db().count('id as total');
 
       this.knexify(countQuery, params.query);
 
-      return countQuery.then(function(count) {
-        return query.then(data => {
-          return {
-  					total: count[0].total,
-  					limit: filters.$limit,
-  					skip: filters.$skip || 0,
-  					data
-  				};
-        }).catch(errorHandler);
-      }).catch(errorHandler);
-		}
+      return countQuery.then(count => count[0].total).then(executeQuery);
+    }
 
-    return query;
+    return executeQuery();
 	}
+
+  find(params) {
+    const paginate = !!this.paginate.default;
+    const result = this._find(params, paginate, query => filter(query, this.paginate));
+
+    if(!paginate) {
+      return result.then(page => page.data);
+    }
 
-	get(id, params) {
+    return result;
+  }
+
+	_get(id, params) {
     params.query = params.query || {};
     params.query[this.id] = id;
 
-    return this.find(params).then(data => {
-      if(data && data.length !== 1) {
-        throw new errors.NotFound(`No record found for id '${id}'`);
-      }
+    return this._find(params)
+      .then(page => {
+        if(page.data.length !== 1) {
+          throw new errors.NotFound(`No record found for id '${id}'`);
+        }
 
-      return data[0];
-    }).catch(errorHandler);
+        return page.data[0];
+      }).catch(errorHandler);
 	}
+
+  get(...args) {
+    return this._get(...args);
+  }
+
+  _create(data, params) {
+    return this.db().insert(data, this.id).then(rows => this.get(rows[0], params))
+      .catch(errorHandler);
+  }
 
 	create(data, params) {
     if(Array.isArray(data)) {
-      return Promise.all(data.map(current => this.create(current, params)));
+      return Promise.all(data.map(current => this._create(current, params)));
     }
 
-    return this.db().insert(data, this.id).then(rows => this.get(rows[0], params)).catch(errorHandler);
+    return this._create(data, params);
 	}
 
 	patch(id, data, params) {
@@ -169,25 +185,31 @@ class Service {
     delete data[this.id];
 
     return query.update(data).then(() => {
-      return this.find(params).then(items => {
-        if(items.length ===  0) {
-          throw new errors.NotFound(`No record found for id '${id}'`);
-        }
-
-        if(items.length === 1) {
-          return items[0];
+      return this._find(params).then(page => {
+        const items = page.data;
+
+        if(id !== null) {
+          if(items.length === 1) {
+            return items[0];
+          } else {
+            throw new errors.NotFound(`No record found for id '${id}'`);
+          }
         }
 
         return items;
-      }).catch(errorHandler);
+      });
     }).catch(errorHandler);
 	}
 
 	update(id, data, params) {
+    if(Array.isArray(data)) {
+      return Promise.reject('Not replacing multiple records. Did you mean `patch`?');
+    }
+
     // NOTE (EK): First fetch the old record so
     // that we can fill any existing keys that the
     // client isn't updating with null;
-    return this.get(id, params).then(oldData => {
+    return this._get(id, params).then(oldData => {
       let newObject = {};
 
       for (var key of Object.keys(oldData)) {
@@ -205,7 +227,7 @@ class Service {
         // NOTE (EK): Restore the id field so we can return it to the client
         newObject[this.id] = id;
         return newObject;
-      }).catch(errorHandler);
+      });
     }).catch(errorHandler);
 	}
 
@@ -218,17 +240,23 @@ class Service {
       params.query[this.id] = id;
     }
 
-    return this.find(params).then(items => {
-      let query = this.db();
+    return this._find(params).then(page => {
+      const items = page.data;
+      const query = this.db();
+
       this.knexify(query, params.query);
 
       return query.del().then(() => {
-        if(items.length === 1) {
-          return items[0];
+        if(id !== null) {
+          if(items.length === 1) {
+            return items[0];
+          } else {
+            throw new errors.NotFound(`No record found for id '${id}'`);
+          }
         }
 
         return items;
-      }).catch(errorHandler);
+      });
     }).catch(errorHandler);
 	}
 }