create beforeFind lifecycle callback

[aclave1]

I created a beforeFind lifecycle callback to allow a user to always modify a model's find or findOne queries with some criteria. A good use case would be if you're using a soft-delete system and you'd like to tack on an isDeleted:false to any find query on a model. Please feel free to critique the code/suggest changes, I tried to stick to the style that already exists.

[mphasize]

As for implementing soft deletes, I would expect the soft delete to be set as a model attribute through waterline-schema... similar to autoCreatedAt and autoUpdatedAt (see here)
and then have the code parsing and normalizing query criteria watch out for the attribute.

But can imagine other great uses for a beforeFind callback, like implementing access control.

How's your soft delete implementation working with the beforeFind callback? Any issues you ran into?

[aclave1]

The only issue is, if your model looks something like this:

//models/User.js
module.exports = {
  attributes:{
    name:{type:'string'},
    isDeleted:{
      'type':'boolean',
       //not setting default to false
    }
  },
 beforeFind:function(values,cb){
     values.isDeleted = false;
     cb();
  }
}

Since you didn't default isDeleted to false, the query will not work as expected since isDeleted will be null. This isn't an issue in my implementation though.

I have an idea to improve my beforeFind though, I think a better idea for my implementation would be to allow the user's query to override the beforefind, so doing:

User.findOne({
  idDeleted:true
})
.then(function(user){
//currently, user is null since beforeFind overwrote it
});

will return users instead of what it currently does.

[aclave1]

@particlebanana any update on this?

[luislobo]

I definitelly +1 this one. This is really useful, for several scenarios.

[particlebanana]

If this is just for soft-deletes, thats on the roadmap. Not sure how I feel about a beforeFind method because it's not an actual lifecycle callback. You aren't tapping into the resource's lifecycle you are simply changing the query criteria, which is more on the application side rather than the ORM side. It could be confusing to track down issues if you are passing in a query criteria and not getting results based on what you passed in.

Are there examples of other ORMs that allow these kind of helpers? I know there is a concept of default scope in Rails but I tend to stay away from that as it's hard to change later (ex: User.unscoped). I want to keep Waterline on the lighter side and moving business logic into the ORM is a slippery slope.

We give you instance and class methods on models that could handle access control, though I absolutely agree that we need a way to instantiate instances without saving or creating (ex: new User(data)) and soft deletes are on the short term roadmap.

I'll leave this open for a while and get some feedback. I'm not totally opposed, just worried of the precedent it would set and need some convincing.

[luislobo]

@particlebanana yeah, I've been using it in Yii:
http://www.yiiframework.com/doc/api/1.1/CActiveRecord#beforeFind-detail

I've used it there.

[aclave1]

Soft deletes is not the only use I have for this. It can be used to pass any default value to a query on a model. Lets say I'm making an application for students. There is a class model and a semester model.

semester:{
  season:{type:'string'},
  year:{type:'integer'}
}
class:{
  semesterId:{type:'integer'},
  subject:{type:'string'},
  code:{type:'integer'},
  beforeFind:function(values,cb){
    values.semesterId = values.semesterId || global.CurrentSemester;
        cb();
  }
}

Class
.find({
    where:{
        subject:'english'
    }
})
.then(function(_classes){

    //do something with the english classes from the current semester
});

This prevents you from having to query for the current semester's ID first when you wish to search for a class, which is what I'd want in this scenario most of the time.

Continuing with the class scenario, imagine that users eventually "graduate", so they will not be taking classes with your app anymore. If you wanted to query user's, you wouldn't want the graduated user's to come back so you'd do this beforeFind.

beforeFind:function(values,cb){
  values.isGraduated = values.isGraduated || false;
}

If these user's graduate, maybe they can still access the app, but won't be enrolled in any classes. They shouldn't be deleted. but they won't be used very often.

Admittedly, the second option sounds kinda like soft delete but these are just contrived examples.

[particlebanana]

The issue here is most of the time. When you add this logic you don't have any other options for querying. I'd argue that these scenarios are what controllers in your app are for. You can define what logic you want to query with and attach them to routes.

Alternatively you could have class methods on the models for this. So for the students example you could have two methods, graduated and current which would allow you to do something like User.graduated() and User.current().

[luislobo]

I would like to just add cross logging... performance measures, or whatever
in that event.
The issue is not how to implement the options we are saying, is just having
the possibility to do whatever we want there.

On Mon, Aug 4, 2014 at 9:19 PM, Cody Stoltman [email protected]
wrote:

The issue here is most of the time. When you add this logic you don't
have any other options for querying. I'd argue that these scenarios are
what controllers in your app are for. You can define what logic you want to
query with and attach them to routes.

Alternatively you could have class methods on the models for this. So for
the students example you could have two methods, graduated and current
which would allow you to do something like User.graduated() and
User.current().

—
Reply to this email directly or view it on GitHub
#525 (comment).

[mphasize]

In order to achieve detailed access control I implemented hooks (or class methods) like controlBeforeFind, controlAfterFind, controlBeforeUpdate, controlBeforeDestroy, etc. . The before* hook is messing with the query criteria and the after* hook is messing with the result set after evaluating populated associations.
It works for me, but I have to agree with what @particlebanana that it doesn't feel right.

I think this kind of custom behavior belongs in the controller, but extending a default action in Sails is actually kind of hard and not very DRY. The current blueprints for find, findOne, create, update, etc. handle quite a lot of stuff, from parsing criteria and values to handling database queries to pub/sub'ing records.
My solution was to override the blueprints and add my custom hooks, but now I have to watch the default blueprints for changes and bugfixes to keep my overrides up to date. (I hope this will be more stable now, but during the beta phase we saw quite a few changes.)

So from my current point of view I would either appreciate hooks in the default blueprints to tap into the query criteria and later into the result set OR a thorough slimming / modularization of the code in the blueprints (beginning by exposing the actionUtils in the controllers) so that extending the default behavior through a controller action is much simpler.

[luislobo]

@mphasize Could you share your actions? I am just about to start doing something like that, for RBAC, and Entity Access Control

[mphasize]

@luislobo I decided to go down this way, because I am not doing classic RBAC, otherwise I would have opted for something like node-acl. And as some kind of warning: I still feel like my solution is somewhat brutish, but here's the gist of it: https://gist.github.com/mphasize/d62ad412e85bda06377c

I made a copy of the original Sails blueprints (and the actionUtils.js), put them in "api/blueprints" to override the defaults and adapted them as shown in the Gist. That gives me a very flexible set of model specific before/after hooks, but it also makes the code harder to read/follow.

[tjwebb]

Could you solve this with a mixin?

// PerennialModel.js
module.exports = {
  // ...
  destroy: function (xyz) {
    // custom implementation
  }
};

// MyModel.js
_.merge(module.exports, require('./PerennialModel'), {
  // lalala
});

[randallmeeker]

+1 for this pull request!

[KirillSuhodolov]

very useful improvement
what's current status?

[mphasize]

I created a Policy that enables this behavior. Here's the Gist: https://gist.github.com/mphasize/e9ed62f9d139d2152445

[aclave1]

I have to agree with particlebanana that this will set a bad precedent. The database shouldn't control access control. My main use was for default values in queries but that's really not very useful since we could easily just create a static function on the model to do that.

[tjwebb]

I'm working on solidifying sails-permissions for this kind of access control behavior: https://github.com/tjwebb/sails-permissions. There's still work to do, but hopefully some will find it to be a good starting point.

[randallmeeker]

I was hoping for this feature, not for access control, but for the scrubbing of query params among other reasons. I hate to see it go.

Another use case would be query limiting where you want to make sure people specify certain params always and set defaults when doing a query.

What this mostly does, it gives us freedom. By having this (and afterFind), give the ability for the users to implement specific use cases that waterline does not cover, helps keep controllers slim and our code DRY.

Not to mention that is falls in line with the convention of giving lifecycle callbacks to all core methods.

[tjwebb]

I liked this feature too. That said, it may be that it could be too easily used for evil.

I don't think it should be forgotten forever. I'm willing to re-open it given a sufficiently convincing argument.

[connor4312]

I think that @aclave1 made a good point. I found my way here looking for a way to do soft deletes in waterline. Particlebanana mentioned that they were on the roadmap, but disappointingly that no longer seems to be the case.

Are there examples of other ORMs that allow these kind of helpers?

Well, before_find in ActiveRecord, load() in SQLAlchemy, postLoad() in Doctrine, etc.

[randallmeeker]

I'm not sure why this is evil? Or any more or less evil than beforeValidate,beforeCreate,beforeUpdate ?

I'm trying to think of a framework that had callbacks, but did not have beforeFind and afterFind and I think sails is the only exception. (cause you know I have used everything, ever . .. probably not)

@tjwebb I'm not sure what a convincing argument would be that has not been made. What is the arguments for beforeValidate,beforeCreate,beforeUpdate?

Most people today in MVC are talking about skinny controllers and dry programming. When we put functionality in the model, we help accomplish these goals. I get that their needs to be a difference between controller logic and model logic, but we are already bluring these lines with the toJSON/toObject methods (as well as all the other callbacks).

A lot of these things can be done in Policies, but that is not ideal. Policies can be dangerous because they can be bypassed easy enough when writing out your model calls in a controller.

Example: I need to make sure that I include a security credential with every call to my user database to filter by. The check for this security credential is done at the policy level, however I could write a controller where I forgot to use the credential when searching despite being present. A beforeFind callback could check for the existence of this required field for searching, thus forcing me to specify it or returning an error. This goes a long way to providing better security for an app.

In the same manner I can also add checks in the before find to make sure that certain criteria are specified in the query. For instance if in order to limit bloated search results I required that every search require a date range.

BeforeFind: soft deletes. I know their is a plan in the works, but my preference on the the soft deletes is to use a date field deletedAt in the same manner that updatedAt and createdAt. I know this is not the current implementation that is being worked on, but a beforefind callback would allow me to implement this anyway I want. CRY FREEDOM!

BeforeFind: Scrub data for simplified data params. If I know that every search in my user table needs to search Last Name, First Name and Email Address, I can setup my before find to look for a generic _q field, and if found in a beforeFind(), then expand it to search for all 3 fields and drop the made up _q field before moving on. I have done this in other apps and it helps keep stuff really dry and eliminate long repetitive code for common but complex queries.

I could keep going. Currently I find that I'm using creative policies to do a lot of this work, but I don't think they belong in Policies because they are very model specific.

I'll revert to my previous comment

What this mostly does, it gives us freedom.

I get that freedom can be dangerous, but I don't think any more than the other callbacks.

This is also popular subject having been referenced here:

balderdashy/sails#1141
balderdashy/sails#1005
balderdashy/sails#2760
#798
https://github.com/balderdashy/waterline/issues/134

Its also been mentioned in different forms on StackOverflow and the Google Groups and I did not even look in the individual adapter repos.

[luislobo]

I still think we need this. There are tons of use cases.

[aclave1]

@luislobo can you provide us with some more use cases? I would love for this feature to be included, but I may have to rewrite some of this code as waterline has likely changed enough that merging this would be very hard.

[luislobo]

Some of them were already mentioned, by @randallmeeker and others, but I'll enumerate them:

• Ability to add criteria to all finds, independant of the actual filters a user might select, for example, injecting userId, customerId, ownerId, etc (owners of data)
• Filtering soft deleted objects
• Filtering sitewide data, for example, an international site might show certain data based on their domain and country.
• Adding conditions to user selected filters, that are hidden for them.
• Enforcing date ranges
• Enforcing limits
• The need to always retrieve info from other table/collection that needs to be expanded/used as a condition to the current model. For example: a site that have users, that are assigned to groups, groups can also be assigned to groups, and data "belong" to some of those groups. We have to be able to ALWAYS query and ENFORCE for all finds in the whole application, that for a particular Equipment, it belongs to a user, based on the groups the user is member of, and the group hierarchy.

Some considerations mentioned:

• Having this in a single method, you don't have to repeat it everywhere, as those conditions added are cross site. It helps in making sure that you do it once, and it will be enforced always. I don't want to add deleted=false for all my models and queries!
• Along the same lines: Model specific stuff in models, not in controllers/services

Those are some of the reasons

[devinivy]

Bear in mind that this is doable and much more configurable using custom collection methods. You could alternately extend find. Based upon what everyone is saying, though, maybe a more particular feature for waterline would be default/active criteria.

[randallmeeker]

I don't think anyone is saying, "I can't do XXXX because I don't have the beforeFind() callback." I think what is being said is the beforeFind() solves many use cases, features and issues that have been brought up.

I think plenty of reasons have been given and I understand their are bad use cases out there, but can someone make the case of why implementing a beforeFind (AND afterFind) is the wrong thing for sails to do? How it would affect the framework in a negative fashion? and why its more evil than other model callbacks?

[devinivy]

Actually I think it's a reasonable feature! Was just wondering if there's a solution that solves a particular need more precisely. It sounded like most of the use-cases had to do with default criteria.

[aclave1]

Should I reopen this?

[aclave1]

I meant to click comment instead of reopem. Oh well!

[dmarcelino]

👍 I'm good with this as well.

[particlebanana]

Alright guys you've convinced me. The community has spoken! I'm not ready for a 0.11 release yet but let's create a branch for it. Then we can keep master clean and available for hot fixes while we flush out any breaking changes we want to go ahead and get in 0.11. I'll spin that up and then we can re-submit this on that branch.

You guys cool with that?

[dmarcelino]

@particlebanana, I'm cool with that. In my opinion our strategy should be:

1. 0.10.x
   • Fix priority bugs;
   • Get waterline - adapters integration tests to pass:
2. 0.11.0: roll out new relevant features, suggestions:
   • beforeFind;
   • many-to-many through associations;
   • Custom join table names;
   • Recursive populate? (it's our top request)

[devinivy]

Seems reasonable to me! beforeFind, through associations, and custom join tables are in sight. I know @particlebanana has made strides on recursive populate. So we're more-or-less on the right track feature-wise. I'd be be curious to add composite indexes too. What about other features for which we already have PRs?

[randallmeeker]

If I could offer the suggestion that 0.11.0 also include afterFind as well. Might as well since were doing a beforefind and it would complete our lifecycle callbacks on all model methods. I don't think there is a ready pull request, but I can work on that one. The only argument to be made agaist is that is duplicates some of the work being done with toJson/toObject

balderdashy/sails#1005
#798

[aclave1]

@particlebanana thanks! if you want me to implement afterFind as well, let me know! i'll submit another PR with that feature.

[particlebanana]

Alright guys, added a 0.11 branch. Can we re-submit this against that branch?

[connor4312]

I think that baked-in support for dealing with soft deletes (would be preferable to having lifecycle callbacks, for that particular problem. Any chance of those being roadmapped in 0.11?

[randallmeeker]

@connor4312 the soft delete is still open here https://github.com/balderdashy/waterline/issues/134

[taylorlapeyre]

👍

[aclave1]

resubmitting against 0.11

[aclave1]

new pull request: #902

[ghost]

Any news about this feature?

[ChrisWorks]

Yes, what happened to this? Did it get in or not?

[abalad]

Yes, what happened to this? Did it get in or not?

[silverark]

Any news? I can see there are comments in it for "FUTURE: this is where it would go. It's also in the list of allowed overrides, but just no code.