4
\$\begingroup\$

As a learning project I recently started writing a node module to generate / handle the creation of CRUD endpoints in a DRY way.

The problem I was initially wanting to solve was that in my generator-angular-fullstack project I had several basic CRUD endpoints that all had identical controllers (e.g. this).

My idea was a module that would generate those CRUD methods for different database types, something like:

var dryCrud = new DryCrud({
 type: 'mongoose',
 collection: MyMongooseCollection
});
app.get('/', dryCrud.read);
app.get('/:', dryCrud.read({
 query: {'_id':'^id'} // <-- the ^ = req.params.
}));

Then I had the idea of generating a full set of basic CRUD endpoints:

dryCrud.init('routePrefix', app);

The .init creates a create, read, read/:id, update/:id and delete endpoint using the supplied express app (or router) with each route prefixed by the 'routePrefix', connecting with the database specified in the dryCrud creation.

I have created a pretty basic module, and two database adaptors (Mongo native and Mongoose) here.

I feel like I have been staring at this in isolation for a while, and I would love to know if anyone else thinks a module like this would be useful. Is it worth developing it further? I would also welcome any feedback on my code / structure.

The Crudbrella module

index.js

var _ = require('lodash'),
 utils = require("./lib/utils-prototype"),
 Crudbrella;
Crudbrella = function(config){
 //Check for required config
 if(!config.collection || !config.type){
 //Raise an error here...
 console.error("dryCrud error: you must provide a database object and type");
 process.exit(e.code);
 return false;
 }
 //Constructor of crudbrella object being returned to the user
 var newCrud = function(){
 this.collection = config.collection;
 this.type = config.type;
 };
 //Include core functionality and utility methods
 _.extend(newCrud.prototype, utils);
 //If the type provided is a string
 if (typeof config.type == 'string' || config.type instanceof String){
 //attempt to load and use the module
 try {
 var adaptorModule = require(config.type);
 } catch(e) {
 console.error("Adaptor " + config.typre + " is not found");
 process.exit(e.code);
 }
 //If the type provided is a module
 }else{
 adaptorModule = config.type;
 }
 //use the module
 _.extend(newCrud.prototype, adaptorModule);
 return new newCrud();
};
module.exports = Crudbrella;

./libs/utils-prototype.js

var _ = require('lodash');
module.exports = {
 //Default success callback (can be replaced at runtime)
 successCallback: function(res, result){
 if(!result) { return res.send(404, ""); }
 res.send(result);
 },
 //Default error callback (can be replaced at runtime)
 errorCallback: function(res, error){
 res.send(500, error);
 },
 //Standard db callback
 dbCallback: function(data){
 //If there is an error
 if(data.err && data.err !== null){
 //Use a user defined or default error callback
 return data.onError(data.res, data.err || "empty");
 }
 if(data.req.method == 'DELETE'){
 data.result = true;
 }
 //Use a user defined or default success callback
 return data.onSuccess(data.res, data.result);
 },
 init: function(root, app){
 root = root.replace(/\/$/, "");
 app.get(root + '/', this.read());
 app.get(root + '/:id', this.read({query:{_id:'^id'}}));
 app.post(root + '/', this.create());
 app.put(root + '/:id', this.update());
 app.delete(root + '/:id', this.delete());
 },
 utils: {
 //Depopulate populated mongoose documents (move this to the mongoose adaptor)
 depopulate: function (model, doc){
 //loop through each item in the model
 _.each(model.schema.tree, function(item, key){
 var schemaDetails = JSON.stringify(item) || "";
 //If the item has a 'ref' property
 if (schemaDetails.indexOf('ref') !== -1){
 //For that item in the current document
 doc[key] = _.map(doc[key], function(value){
 return value._id;
 });
 }
 });
 }
 }
};

The mongoose adaptor

var _ = require('lodash');
module.exports = {
 //Create a new record
 create: function (options){
 return function(req, res){
 //If no options are provided default to an empty object
 options = options || {};
 this.collection.create(req.body, function(err, result) {
 //Pass results and req/res to the dbCallback
 this.dbCallback({
 req: req, 
 res: res, 
 err: err, 
 result: result,
 onSuccess:options.onSuccess || this.successCallback,
 onError:options.onError || this.errorCallback
 });
 }.bind(this));
 }.bind(this);
 },
 //Read an existing record / records
 read: function(options){
 return function (req, res){
 //If no options are provided default to an empty object
 options = options || {};
 options.query = options.query || {};
 //Create a local copy of the query definition to be parsed
 var query = JSON.parse(JSON.stringify(options.query)) || {};
 //Loop through every object in query
 _.each(query, function(item, key){
 //If the value starts with a ^ use the item value as a key for req.params
 if(item[0] === "^"){
 query[key] = req.params[item.replace("^","")];
 }
 });
 //Find using optional query or find all
 this.collection.find(query || {})
 //Process options populate string / object or default to empty string
 .populate(options.populate || '')
 //Execute the query
 .exec(function(err, items){
 if(items.length === 1){
 items = items[0];
 }
 //Pass results and req/res to the dbCallback
 this.dbCallback({
 req: req, 
 res: res, 
 err: err, 
 result: items,
 onSuccess:options.onSuccess || this.successCallback,
 onError:options.onError || this.errorCallback
 });
 }.bind(this)
 );
 }.bind(this);
 },
 //Update a record
 update: function(options){
 return function(req, res){
 //If no options are provided default to an empty object
 options = options || {};
 //If the id has been included in the body, remove it
 var x = req.body._id; 
 delete req.body._id;
 //Check if the body contains any populated fields and depopulate them
 this.utils.depopulate(this.collection, req.body);
 //Use crudbrella read to find the document to be updated
 this.read({
 query: {_id: x},
 //custom success handler to complete update, use default for errors
 onSuccess: function(innerRes, result){
 var updated = _.extend(result, req.body);
 updated.save(function (err, actualResult) {
 //Pass results and req/res to the dbCallback
 this.dbCallback({
 req: req, 
 res: res, 
 err: err, 
 result: actualResult,
 onSuccess:options.onSuccess || this.successCallback,
 onError:options.onError || this.errorCallback
 });
 }.bind(this));
 }.bind(this)
 })(req, res);
 }.bind(this);
 },
 //Delete a record
 delete: function(options){
 return function(req, res){
 //If no options are provided default to an empty object
 options = options || {};
 //If the id has been included in the body, remove it
 if(req.body._id) { delete req.body._id; }
 //Check if the body contains any populated fields and depopulate them
 this.utils.depopulate(this.collection, req.body);
 //Use dryCrud.read to find the document to be deleted
 this.collection.findOneAndRemove({_id: req.params.id}, function(err, result){
 //Pass results and req/res to the dbCallback
 this.dbCallback({
 req: req, 
 res: res, 
 err: err, 
 result: "",
 onSuccess:options.onSuccess || this.successCallback,
 onError:options.onError || this.errorCallback
 });
 }.bind(this));
 }.bind(this);
 }
};

The native mongo adaptor

var _ = require('lodash'),
 ObjectId = require('mongodb').ObjectID;
module.exports = {
 //Create a new record
 create: function (options){
 return function(req, res){
 //If no options are provided default to an empty object
 options = options || {};
 //Save the object into the collection
 this.collection.insert(req.body, function(err, result) {
 //Pass results and req/res to the dbCallback
 this.dbCallback({
 req: req, 
 res: res, 
 err: err, 
 result: result,
 onSuccess:options.onSuccess || this.successCallback,
 onError:options.onError || this.errorCallback
 });
 }.bind(this));
 }.bind(this);
 },
 //Check if a given string is a valid mongo ObjectId
 validId: function(id){
 checkForHexRegExp = /^(?=[a-f\d]{24}$)(\d+[a-f]|[a-f]+\d)/i;
 if (id.match(checkForHexRegExp)) {
 return true;
 }else{
 return false;
 }
 },
 //Read an existing record / records
 read: function(options){
 return function (req, res){
 //If no options are provided default to an empty object
 options = options || {};
 //If no query has been provided default to an empty object
 options.query = options.query || {};
 //Create a local copy of the query definition to be parsed
 var query = JSON.parse(JSON.stringify(options.query)) || {};
 //Loop through every object in query
 _.each(query, function(item, key){
 //If the value starts with a ^ use the item value as a key for req.params
 if(item[0] === "^"){
 if(this.validId(req.params[item.replace("^","")])){ 
 query[key] = ObjectId(req.params[item.replace("^","")]);
 }else{
 query[key] = req.params[item.replace("^","")]; 
 }
 }
 }.bind(this));
 //Find using optional query or find all
 this.collection.find(query || {}).toArray(function(err, items){
 if(items.length === 1){
 items = items[0];
 }
 //Pass results and req/res to the dbCallback
 this.dbCallback({
 req: req, 
 res: res, 
 err: err, 
 result: items,
 onSuccess:options.onSuccess || this.successCallback,
 onError:options.onError || this.errorCallback
 });
 }.bind(this));
 }.bind(this);
 },
 //Update a record
 update: function(options){
 return function(req, res){
 var requestId;
 //If no options are provided default to an empty object
 options = options || {};
 //If the id has been included in the body, remove it
 requestId = ObjectId(req.params.id); 
 delete req.body._id;
 //Check if the body contains any populated fields and depopulate them
 this.collection.update({'_id': requestId},{$set: req.body}, function(err, result){
 if(!err){
 result = req.body;
 }
 this.dbCallback({
 req: req, 
 res: res, 
 err: err, 
 result: result,
 onSuccess:options.onSuccess || this.successCallback,
 onError:options.onError || this.errorCallback
 });
 }.bind(this));
 }.bind(this);
 },
 //Delete a record
 delete: function(options){
 return function(req, res){
 //If no options are provided default to an empty object
 options = options || {};
 //If the id has been included in the body, remove it
 if(req.body._id) { delete req.body._id; }
 //Use dryCrud.read to find the document to be deleted
 this.collection.remove({_id: ObjectId(req.params.id)}, function(err, result){
 //Pass results and req/res to the dbCallback
 this.dbCallback({
 req: req, 
 res: res, 
 err: err, 
 result: "",
 onSuccess:options.onSuccess || this.successCallback,
 onError:options.onError || this.errorCallback
 });
 }.bind(this));
 }.bind(this);
 }
};
Jamal
35.2k13 gold badges134 silver badges238 bronze badges
asked Dec 1, 2014 at 0:24
\$\endgroup\$
0

1 Answer 1

4
+50
\$\begingroup\$

To answer your question: yes, I think it could be interesting as a stand-alone module. There's can be a lot of repetitive boilerplate involved in connecting a REST api to a CRUD backend, reducing that is certainly worth a module, in my opinion.

Now to dive into the code...

index.js

if(!config.collection || !config.type){
 //Raise an error here...
 console.error("dryCrud error: you must provide a database object and type");
 process.exit(e.code);
 return false;
 }

It makes more sense to me to throw an exception here than to exit the process.

catch(e) {
 console.error("Adaptor " + config.typre + " is not found");
 process.exit(e.code);
}
  • typre strikes me as a typo.
  • Again, I would prefer an throwing an exception (e.g. e) over process.exit.

utils-prototype.js

successCallback

if(!result) { return res.send(404, ""); }

Are you positive you want to return 404 on falsy values? Also, as far as I can tell this is Express 3.x syntax, the two-argument version of send is not mentioned in the Express 4.x api documentation. Using res.status(404).send("Not Found") is compatible with either version.

errorCallback

Passing the error message directly to the user may be convenient during development, but it is generally considered insecure. Sending error to the user doesn't strike me as a good default.

dbCallback

 //If there is an error
 if(data.err && data.err !== null){
 //Use a user defined or default error callback
 return data.onError(data.res, data.err || "empty");
 }
  • the check in the if condition is redundant, as null is falsy.
  • the || "empty" is redundant as data.err is always truthy in this path.
if(data.req.method == 'DELETE'){
 data.result = true;
}

I did not expect to find this here at all. It seems to be working around the fact that there is no meaningful response body to send back for a delete request, but the result argument to onSuccess mustn't be falsy, or you'll get a 404. What I would in fact expect in this case is a 204 (No Content) response. To achieve this, you probably need different default success handlers for different request types.

utils.depopulate

This function definitely belongs in the mongoose adapter as the comment already states.

var schemaDetails = JSON.stringify(item) || "";
 //If the item has a 'ref' property
 if (schemaDetails.indexOf('ref') !== -1){

I sincerely doubt that this is what you want to do. What if the item has a prefered or prefix property (those also contain ref)? The fact that you're calling JSON.stringify here suggests to me that item actually has a richer structure and you might actually test if item.ref !== undefined or something like that (I am not familiar enough with mongoose to know the metadata structure you are traversing here).

The adapters

I'll cover both adapters in one go. The interaction with mongo / mongoose is sufficiently similar.

//Create a local copy of the query definition to be parsed
var query = JSON.parse(JSON.stringify(options.query)) || {};

Shouldn't you use something like _.clone or _.cloneDeep for this?

if(items.length === 1){
 items = items[0];
}

It seems like this is again working around the limitations in your default success handler. When I query a single object using the get('/:id') route, I expect a single object in the response. But when I query a route that can return multiple objects, like get('/'), I expect to always get an array back, even when there happens to be just one element in the array. Again, I think this logic should be replaced by having a different success callback for different routes.

The update and delete methods are missing any way to prevent concurrent modifications. It might be a little tricky to prevent this as it depends on information embedded in the data (like a modification timestamp or version field). From a usability perspective, preventing users from overwriting or removing data they haven't even seen is pretty important though, if you ask me.

answered Dec 31, 2014 at 13:45
\$\endgroup\$
1
  • 1
    \$\begingroup\$ Welcome to Code Review! You passed through the First Post Review with flying colors! \$\endgroup\$ Commented Dec 31, 2014 at 13:56

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.