balderdashy / sails

Realtime MVC Framework for Node.js
https://sailsjs.com
MIT License
22.84k stars 1.95k forks source link

Enhancement: Waterline API Issues #5565

Closed formula1 closed 5 years ago

formula1 commented 9 years ago

To start out, let's get some things clear

However, when using it with my own development, there were few usability issues.

Here is some samples that are somewhat familiar in mongoose. It's simple, clean, readable and makes sense. I'm making everything syncronous to avoid readability problems or promise vs callback wars. In anycase, es6 is right around the corner.... Any day now........... maybe any year now...

require("database").initialize();

var UserVal = createValidator({
  props:values
});
var UserLoginVal = createValidator({
  user{model:user}}
});

var User = require("database").loadModel(UserLogin);
var UserLogin = require("database").loadModel(User);

UserLogin.after("remove", function(values){
  if(this.find({user:values.user}).length > 0) return;
  User.find(values.user).destroyAll()
});

User.after("remove", function(values){
  UserLogin.find({user:values.user}).destroyAll()
});

var random_password = Math.round(Math.random()*10000);
var admin = User.find({name:"admin"});
UserLogin.update({user:admin}, {password:randomPassword});

console.log("The Admins current password is: "+random_password);

What's basically happening is

This isn't 100% accurate to Mongoose but a familiar gist ive written.

Middleware cannot be defined after inheriting collection properties

This turns everything from above into somewhat less readable. In order to be able to do it the old way, you'll have to create a wrapper object that has nothing to do with waterline except from a usability perspective or start editing the callbacks array

var UserLogin, User;

User = createCollection({
  attributes:{props:values},
  afterDestroy: function(values){
    UserLogin.find({user:values.user}).destroyAll()
  }
});
UserLogin = createCollection({
  attributes:{user{model:user}}
  afterDestory: function(values){
    if(this.find({user:values.user}).length == 0){
      User.find(values.user).destroyAll()
    }
  }
});

require("database").loadModel(UserLogin);
require("database").loadModel(User);

var random_password = Math.round(Math.random()*10000);
var admin = User.find({name:"admin"});
UserLogin.update({user:admin}, {password:randomPassword});

console.log("The Admins current password is: "+random_password);

Waterline is not global

I actually like this part about waterline. If I were to have different instances of waterline that were put into node vm's or made public to only certian modules, I have more ability to keep my stuff protected. That being said, it makes things a little more tedious. But not a lot more! It's an appropriate amount of code where As Control Increases, Difficulty Increases. And it isn't that much more difficult.

Waterline.Collection.extend does not return a model.

This forces me to have access to a Waterline Instance so that I can call other models within middleware and functions. That being said, a collection doesn't know what waterline instance it will be apart of until waterline loads it. In addition, a single collection can probably be loaded by two different waterline instances with two different adapters that have the same labels, so that in some ways makes sense.

These are two problems that can be solved with one stone. But it starts getting uglier where you are exposing the waterline object to the models. This also makes it so that I models will be bounded to an instance unless I provide two waterline instances or create a wrapper around the waterline instance.

The reason why you want to put it in a function is so that you can have a single waterline intance with multiple models. If you are making thing modular, generally you will want to have the waterline instance either global or provide it when it needs it. unfortunately, the models need it.

var UserLogin, User;

User = createCollection({
  attributes:{props:values},
  afterDestroy: function(values){
   this.collections.userlogin.find({user:values.user}).destroyAll()
  }
});
UserLogin = createCollection({
  attributes:{user{model:user}}
  afterDestory: function(values){
    if(this.collections.userlogin.find({user:values.user}).length  == 0){
      this.collections.user.findOne(values.user).destroyAll()
    }
  }
});

function compileUserModels(WaterlineInstance){

//Would have liked to get access to the model here, but not going to happen
/* UserModel =  */ WaterlineInstance.loadCollection(User);
/* UserLoginModel =  */ WaterlineInstance.loadCollection(UserLogin);

var random_password = Math.round(Math.random()*10000);
var admin = WaterlineInstance.collections.user.find({name:"dev_admin"});
WaterlineInstance.collections.userlogin.update({user:admin}, {password:randomPassword});
console.log("dev_admin's current password is: "+random_password);

}

The middleware (after/before validate/create/update/destroy) have no this

The middleware (after/before validate/create/update/destroy) do not have access to the waterline instance

After looking through the source code some more, I've found that developers actually do have access to this. It must have been my implementation that caused the problem. However, though I have access to a this variable, this variable is bound to the literal collection rather than the class. And while the class has class.waterline and the instance has instance.constructor.waterline the literal collection does not.

Example:

schema = {
  afterCreate:function(values,next){
    console.log(this.waterline); //undefined
  }
}

waterline.initialize(function(err,self){
  console.log(self.collections.yourCollectionName.waterline); //A very large object
});

With access to this, I would be able to access the models current waterline instance via model.waterline or instance.constructor.waterline (the second I haven't tested yet). This would allow me avoid specifying the waterline instance to the constructor. This is a problem since when creating a model, I don't necessarilly know if the models will be loaded multiple times. which forces me to create new models for each waterline instance that wants them. It may be dry coding, but its wet in memory.

function(WaterlineInstance){

var UserLogin, User;

User = createCollection({
  attributes:{props:values},
  afterDestroy: function(values){
    WaterlineInstance.collections.userlogin.find({user:values.user}).destroyAll()
  }
});
UserLogin = createCollection({
  attributes:{user{model:user}}
  afterDestory: function(values){
    if(WaterlineInstance.collections.userlogin.find({user:values.user}).length  == 0){
      WaterlineInstance.collections.user.findOne(values.user).destroyAll()
    }
  }
});

//Would have liked to get access to the model here, but not going to happen
/* UserModel =  */ WaterlineInstance.loadCollection(User);
/* UserLoginModel =  */ WaterlineInstance.loadCollection(UserLogin);

var random_password = Math.round(Math.random()*10000);
var admin = WaterlineInstance.collections.user.find({name:"admin"});
WaterlineInstance.collections.userlogin.update({user:admin}, {password:randomPassword});
console.log("The Admins current password is: "+random_password);
}

Collections must be added before the database is initialized

This can also be considered a feature rather than a bug as during the initialization process many many fantastic things happen but it also creates usability issues. I'm going to pretend that Waterline is an event emitter here (which it is not) however, forcing it to be one is not an absurdly difficult process. However, this will result in some ugly logic separation

function(WaterlineInstance){

var UserLogin, User;

User = createCollection({
  attributes:{props:values},
  afterDestroy: function(values){
    WaterlineInstance.collections.userlogin.find({user:values.user}).destroyAll()
  }
});
UserLogin = createCollection({
  attributes:{user{model:user}}
  afterDestory: function(values){
    if(WaterlineInstance.collections.userlogin.find({user:values.user}).length  == 0){
      WaterlineInstance.collections.user.findOne(values.user).destroyAll()
    }
  }
});

WaterlineInstance.loadCollection(User);
WaterlineInstance.loadCollection(UserLogin);

WaterlineInstance.on("initialize",function(self){
  var random_password = Math.round(Math.random()*10000);
  var admin = self.collections.user.find({name:"admin"});
  self.collections.userlogin.update({user:admin}, {password:randomPassword});
  console.log("The Admins current password is: "+random_password);
});
}

Problems with DRY Browser/Server code

When attempting to reuse the same code on browser and server when it comes to waterline. These types of hooks become an issue. As simple example is

Browser - User.delete ->sends ajax call to server
Server - User.delete , runs middleware, UserProvider.delete, middleware finished -> returns it to the browser
Browser - Recieves response, runs middleware, UserProvider.delete -> sends ajax call to server
Server - UserProvider.delete, empty response -> returns it to the browser
Browser - middleware active on an empty array

Having access to Model.after("delete", function(values,next){}) would make things much easier to hook into on the fly without any other model needing another model to also interact with it.

 waterline.initialize({
    adapters: {},
    connections: {}
  }, function(err, models) {
    models.user.before("destroy", cb);
    models.userprovider.before("destroy", cb2);
    models.randommodel.before("destroy", cb3);
    models.randommodel2.before("destroy", cb4);
    //... and the beat goes on
  });

Problems Seperating Main Logic from Module Specific Logic

What is true is we have a callback after initialization.

 waterline.initialize({
    adapters: {},
    connections: {}
  }, function(err, models) {
    models.user.before("destroy", cb);
    models.userprovider.before("destroy", cb2);
    models.randommodel.before("destroy", cb3);
    models.randommodel2.before("destroy", cb4);
    //... and the beat goes on
  });

The number of models may not approach 100, maybe not even 25. And the number of callbacks may not ever reach the full 8. However, Its gross. The whole idea of having a module is that instead of mixing logic with the main code, you instead can put everything in its nice sweet little package and only call it when you need it. The reason why event emitters are useful is so you can do that sort of thing.

// in the userprovider module
waterline.on("initialize", function(waterline){
  models = waterline.collections;
  models.user.after("destroy", userprovidercb);
  models.userprovider.after("destroy", userprovidercb2);
});

// in the randomModel module
waterline.on("initialize", function(waterline){
  models = waterline.collections;
  models.user.after("destroy", randommodelcb1);
});

// in the randomModel2 module
waterline.on("initialize", function(waterline){
  models = waterline.collections;
  models.user.after("destroy", randommodel2cb1);
});

In this scenario, I never have to see the middleware logic in my main code. and honestly, I think that is a good thing. It reduces clutter.

Inability to Plugin, Plugout, and let waterline figure things out

The issue here is that I am forced to load all models into waterline before it gets initialized. From there, I can no longer add models. There are two main reasons, collections are dependent on connections which are dependent on adapters and collections are dependent on eachother. And i seems your handling these things at once.

The reason why this would be useful can be

It's quite possible to avoid this by setting up a dependency system. Then when a new collection gets loaded, it checks to see if anything depends on it or it depends on anything. However this would require an almost complete rewrite of the waterline-schema.

tjwebb commented 9 years ago

@formula1 I appreciate your thoughts. Give me some time to digest all this and I'll get back to you. Thanks.

dmarcelino commented 9 years ago

Hi @formula1, you've echoed some very good points. The one that particularly resonated with me was:

Inability to Plugin, Plugout, and let waterline figure things out The issue here is that I am forced to load all models into waterline before it gets initialized. From there, I can no longer add models. [...]

  • Content Management Systems hold plugins in high regard. If you have to restart your server in order to have waterline detect model/connection changes, You're in for a problem. It might be possible to create a new waterline instance with the exact same adapter credentials as the first, however this is untested.

Loading a collection after waterline is initialised would definitely be a welcomed feature. This should be fairly easy for collections without associations. The case were there are associations would require some sort of reloading previously loaded collections which is probably trickier...

formula1 commented 9 years ago

@tjwebb Take your time, the current consumers of waterline most likely using sails and/or don't interact with waterline too much. In addition, if a team wants to use waterline, These aren't impossible hoops to jump though.

@dmarcelino I was attempting to do it myself. The idea being a queue system of some sort but when looking through waterline I realized that I wouldn't easily be able to just rechop some code. I decided against it. I have a waterline branch I'm still working in, however at times it pains me to look at the code.

For what its worth

Turning waterline into an EventEmitter with current api

var db = new Waterline();
ee.call(db);
for(var i in ee.prototype){
  if(db[i]) throw new Error("Waterline has "+i);
  db[i] = ee.prototype[i];
}

Middleware/Callbacks with current api

Its not too difficult to make easy middleware pushes. You can also push and pop to the waterline middleware since its an array. So it's not impossible to add middleware after initialization. Just not pretty.

waterline.on("initialize",function(waterline){
  waterline.collections.a._callbacks.afterDestroy.push(fn1)
  waterline.collections.b._callbacks.afterDestroy.push(fn2)
  waterline.collections.c._callbacks.afterDestroy.push(fn3)
  waterline.collections.d._callbacks.afterDestroy.push(fn4)
});

Plugging in and Plugging out with current api (In theory....)

Waterline.prototype.addCollection = function(collection,next){
  this.preprocessedCollections.push(collection);
  if(this.adapters){
    var nw = new Waterline()
    this.preprocessedCollections.forEach(nw.addCollection.bind(nw));
    return nw.initialize({adapters:this.adapters,connections:this.connections},next);
  }
  this.oldAddCollection(collection);
  next(void(0),nw);
}
Waterline.prototype.removeCollection = function(collection,next){
  var nw = new Waterline()
  this.preprocessedCollections.forEach(function(col){
    if(col.name === collection) return;
    nw.addCollection(col);
  });
  if(this.adapters){
    return nw.initialize({adapters:this.adapters,connections:this.connections},next);
  }
  next(void(0),nw);
}

Thank goodness Waterline is a class amirite? ;)

Aka... Things can be done! It just may...

But I truly believe waterline has a LOT of potential and something I want to use in the future.

But the api isn't the best I've ever used : /

tjwebb commented 9 years ago

I'll bite off a couple pieces.

I'm making everything syncronous to avoid readability problems or promise vs callback wars.

I'm not sure what you mean by synchronous. There is no way to make any database call synchronous, nor is it every desirable. Also, there are no promise vs. callback "wars". Waterline supports both, and you should pick one. Picking either of these is far preferable than picking neither. Don't write custom craziness to suit your own preferences.

Collections must be added before the database is initialized

This is not true: https://github.com/sgress454/sails-hook-autoreload

dmarcelino commented 9 years ago

@tjwebb, sails-hook-autoreload seems to be a sails only solution. It would be interesting to see how to do the equivalent in waterline standalone.

devinivy commented 9 years ago

Looks like sails loading the collections, tearing down the adapters, then initializing the same instance of waterline again. It does this whenever a models file is modified.

formula1 commented 9 years ago

@tjwebb

My Preference Craziness

That was for psuedocode example. In anycase making asynchronous code synchronous is a new standard coming out if you haven't been paying attention. When yield and generators have landed in core, callbacks will be a thing of the past. I don't understand why you are attacking my method of writing psuedocode.

A more accurate example would be something like

//server.js
var config = require("config");
require("database").initiatize(require("config"),function(e,database){
  global.http = require("http").createServer()
  require("userprovider.js");
  http.listen(config.http.port);
});

// or

global.http = require("http").createServer()
require("userprovider.js");
require("database").initiatize(require("config"),function(e,database){
  http.listen(config.http.port);
});
//userprovider.js

var UserVal = createValidator({
  props:values
});
var UserLoginVal = createValidator({
  user{model:user}}
});

/*
  in mongoose it is a global object so I can require("database")
  however, it really doesn't make much of a difference between global.database
  or require("database")

  I personally believe it should be constructed and then set as a global if you really want it to be
  Which is one of the things that I like about waterline
*/
var User = require("database").loadModel(UserLogin);
var UserLogin = require("database").loadModel(User);

/*
  In mongoose, after errors don't expect a callback so this is not 100% accurate.
  The idea being that if something succeeded, then there is no going back.
  However, you can technically "go back" by undoing whatever you listening for
  But that's bad practice
*/
UserLogin.after("remove", function(values,next){
  if(this.find({user:values.user}).length > 0) return next();
  User.find(values.user).destroyAll(next)
});

User.after("remove", function(values,next){ 
  UserLogin.find({user:values.user}).destroyAll(next)
});

var random_password = Math.round(Math.random()*10000);
User.find({name:"admin"}, function(err,admin){
  if(err) throw err;
  UserLogin.update({user:admin.id}, {password:randomPassword},function(err,realadmin){
    if(err) throw err;
    console.log("The Admins current password is: "+random_password);
  });
});

What is and isn't true

Whenever something changes in those dirs, reload the ORM, controllers and blueprints.

This sounds expensive.

That is not hotswapping

At that point you're basically just doing my theoretical Plugin/Plugout. The reason I said that as a joke is its expensive and requires people to use the object as a key of another. At that point its an extremely good thing to have multiple waterline instances since something like this would result in something terrible.

var realdatabase = new Database();
(function(database){
  setImmediate(database.doSomething.bind(database));
})(realdatabase);
realdatabase.destroy();
realdatabase = new Database();

Instead what you'd have to do is

var framework = {
  db: new Database()
};
(function(framework){
  setImmediate(function(){
    framework.db.doSomething();
  });
})(framework)
framework.db.destroy();
framework.db = new Database();

// or

function DatabaseWrapper(){
  this.args = arguments.slice();
  this.database = new Database.apply(Database,arguments);
}

Object.keys(Database).forEach(function(key){
  DatabaseWrapper.prototype.[key] = function(){
    this.database[key].apply(this.database,arguments);
  }
});

DatabaseWrapper.prototype.reload = function(){
  this.database.destroy();
  this.database = new Database.apply(Database,this.args);
}

// or

var db1 = Database(connection_config);
var db2 = Database(connection_config); //create two connections to the same database

(function(database){
  setImmediate(database.doSomething.bind(database));
})(db1);
db2.destroy();
db2 = new Database();

aka More hoops to jump through/dirty programming. You're essentially invalidating any sort of object by reference passed to functions.

It's expensive because

Why I refuse to use sails

Sails reminds me of developing in drupal, I don't enjoy developing in drupal. I'm trying to escape my php past not find a new one.

That being said, drupal is used by many government and big companies because of how you can do almost everything within your browser in a simple-ish fassion. Things like create database objects, rules, and control the flow without ever touching or looking at code. I can also tell that Sails is going in the same direction which in my opinion is an awesome kind of ambitious.

Why you shouldn't be offended

Because waterline is great. I purposefully shower this project with compliments when I can because I like it's good parts. However, using it isn't as great in my opinion. However, It can be a lot worse : P. If you want to attack my criticisms or preferences or just me you can always view my code. Theres plenty of things you can find I'm not proud of.

mikehostetler commented 9 years ago

@formula1 This is fantastic, but I fear it may be a big large to process, which is why it's been stale for 24 days now.

Any thought to breaking it apart a bit?

I like a lot of your suggestions and would like to see them discussed a bit more, but grokking it all in one thread is tough.

formula1 commented 9 years ago

@mikehostetler Absolutely. Thanks for the feedback, I do know its a lot to request and for what its worth I tagged it as enhancement because I know how requests are often common and generlaly bugs are more important than enhancements anywhay. Would you like different issues or Just seperate it here?

particlebanana commented 9 years ago

@formula1 thanks for this and for taking the time to be so invested in Waterline. This started off as a crazy idea around a single database (mysql) and has turned into something much bigger. @mikermcneil wrote the original version over a holiday and over the past two years the number adapters and contributors has exploded. Something that we made to work for a few people is now being used by thousands. Thats pretty crazy.

The api needs work, this is very true. The first version of Waterline was simple and fairly elegant because it had to do so little. There were no associations or sql/nosql issues or complex queries, etc. In that codebase hot swapping models and adding event emitters would have been a breeze. But then we needed a way to add associations to the codebase AND have these associations work across a set of adapters that had zero notion of what an association was. This is where projects like waterline-schema and waterline-criteria came from. It all got really messy real quick. What we ended up with though was something that works for most people and most apps 80% of the time. It's not a clean API but 99% of the people never touch the WL api outside of it's query api.

Now we want more advanced features like transactions, recursive populations, advanced association queries, etc and this codebase just wasn't designed to handle these. It's pretty stretched as it is. What we need is to take the lessons learned and apply them to a clean api. Instead of having core do everything, let core handle a little and let the adapters tackle the majority of the implementation details. We need a clean, evented api that can be expanded to contain more features without relying on core to handle it all.

There has been a bit of work on Waterline2 but it hasn't been very active as other things have taken priority. There is a chance to start "fresh" and take what we know from WL and create a completely new API. We have the auxiliary pieces in place, waterline-adapter-tests, waterline-sequel and a plethora of adapters that make it easy to transition to a newer api. There are definitely challenges, believe me reloading models that have complex associations is HARD. Recursive populates are HARD. Where subqueries are HARD. Transactions are HARD. These things are all solvable though, especially if we let the adapters do what they are good at and not re-create the work of others in WL core.

If you are interested in this lets chat. I think waterline is pretty sweet, I've invested more hours than I would like to know in it and I obviously have a vested interest in seeing it succeed and grow up on its own. I echo almost everything you wrote but I don't see it in the current codebase.

Also everyone be nice, we are a friendly bunch 'round these parts!

formula1 commented 9 years ago

@particlebanana That sounds like development :)! I definitely can understand why waterline has become the way it is, both the good and the bad. I definitely don't blame you, Sails or anyone for not not fixing what isn't broken. I really appreciate everyone's attitude towards it and I can definitely sense an air of pride all criticisms aside. This took time, sweat, maybe some tears and thought to become what it is. For what its worth, my interactions with this team have been far nicer than that of

I recently joined a startup, so most of my time will be taken there. However, I'd be happy to just write code to see where it takes me. That being said, I'll definitely say it seems you have a passionate group of people here, so I don't doubt a solid plan is in the works. But I don't mind writing some things in good faith.

For what its worth, the "perfect" ORM is something hard to define. Like all products, everything is desired with no speed, usability or space cost whatsoever. Things like rethinkDB come out and all of a sudden, the standards change from just supporting middleware and passing the event to redis to just inherently having cluster support. Additionally, with transactions each database does it differently. SQL allows you to lock the table to disable anything else until you are finished. mongodb forces you to save your state along with the data your editing in case the database gets shut down midway through.

Heres a wip I'm sure whatever you guys pull off will probably be the bees knees, granted if you have time. But imo, the query logic can be kept, the adapter logic can be kept, validation (though something i may complain about later ;) can be kept and the instance logic can be kept. The important concepts here are

leejt489 commented 9 years ago

@particlebanana This is all very interesting. I've been watching Waterline2 and noticed that not a whole lot was going on with it.

Does Waterline have its own Roadmap like Sails does? I can't find anything on Trello, and the ROADMAP.md on GitHub is empty. Something like that would be great to let users know what may be coming soon. I read @mikermcneil 's slides on Waterline2 and it sounded pretty awesome.

As a Mongo user, I'd personally really like to see embedded document support, and I also was very excited by what seemed like the plans of Waterline2 support an in-memory data cache?

Unfortunately none of us have as much time as we'd like, but there certainly are a lot of things people want from Waterline. I'm sure you all leading the project have discussed and prioritized a lot of this. I would just suggest a blog to keep us all informed, and also to indicate what features you would like community support in development, so that we can plan the development of our apps with the expected evolution of Waterline in mind,

Thanks! I've really enjoyed working with Sails and Waterline over the past year and I'm excited to see where it goes, hopefully I'll be able to find some ways to contribute with my modest experience. Y'all are awesome!

dmarcelino commented 9 years ago

@leejt489, we are not currently maintaining a roadmap but we have been using github milestones so you can easily see what's coming next:

  1. 0.10.x
  2. 0.11
  3. WL_Next

indicate what features you would like community support in development

That's easy to answer: any issue tagged with help wanted :wink:

leejt489 commented 9 years ago

@dmarcelino cool thanks. That is helpful. I still think a periodic blog would be cool, to help consolidate some of the design philosophy and current thinking of the leadership team. Ember.js has this, and I've found it very valuable for not just planning my app, but understanding the thinking behind the instruction/docs. Don't want to hijack this thread though! I may open a separate issue...

sailsbot commented 9 years ago

Thanks for posting, @formula1. I'm a repo bot-- nice to meet you!

It has been 60 days since there have been any updates or new comments on this page. If this issue has been resolved, feel free to disregard the rest of this message. On the other hand, if you are still waiting on a patch, please:

Thanks so much for your help!