create beforeFind lifecycle callbacks, will write afterFind if there is ...

[aclave1]

Resubmitting against 0.11 branch

[aclave1]

Old discussion: #525

[randallmeeker]

+1 to afterfind !!!

[aclave1]

One potential issue with beforeFind and afterFind is that queries can have certain criteria ALWAYS. I suggest using _.defaults or similar to make the criteria specified just default, meaning, if the user passes a value to the query, the beforeFind shouldn't overwrite it. This is up to the user to do this.

[randallmeeker]

@aclave1 I'm not sure I understand. can you be a little more specific, give me an example of what you mean?

[aclave1]

I'll use a soft delete example:

//MyModel.js

module.exports = {
  attributes:{
    name:"string",
    isDeleted:"boolean"
  },
  //wrong way to write this code:
  beforeFind:function(query,cb){
    query.isDeleted = false;
    cb();
  }
};

//somewhere else
MyModel.find({name:'bob',isDeleted:true})
  .then(function(found){
   //found[0].isDeleted === false
  });

If you write the beforeFind like that, you're overwriting the user's query params no matter what. A better way to write it would be:

module.exports = {
  attributes:{
    name:"string",
    isDeleted:"boolean"
  },
  //better way to write this code:
  beforeFind:function(query,cb){
    _.defaults(query,{isDeleted:false});
    cb();
  }
};

MyModel.find({name:'bob',isDeleted:true})
  .then(function(found){
   //found[0].isDeleted === true
  });

What we're doing with lodash here is, we're specifying that the default query parameter for isDeleted is false, but that it can be overridden. The beforeFind implementation doesn't do that for you, because there may be a situation where no matter what, a certain value should be attached, and we shouldn't keep the user from doing that.

An example of when a certain value should be attached, maybe we need to use a database partition key for a query which is set in the environment. Or maybe a locale key which is retrieved from another service.

[randallmeeker]

That is a good example, but it is a unique example. I think your claim that beforeUpdate should not overwrite variables passed is too broad of a statement and that although people make dumb mistakes all the time, it is easy to recognize when I'm overwriting a variable every time.

An exception might be that if I am querying over a date range and I want to accept all types of dates, but the end result sent to my DB needs to be an integer. I could check the dates in my beforeFind and transform them to an integer if they are dates. This would require directly manipulating those values.

[aclave1]

@randallmeeker Agreed, I think I really just wanted to mention this potential issue that people may face.

[randallmeeker]

👍

[dmarcelino]

Back in #525 the general consensus was that this was ready to go. It would be good to merge this to 0.11 so adventurers can start using the 0.11 branch and provide some feedback.

@particlebanana, @tjwebb, @randallmeeker, @devinivy do you have any concerns regarding these changes?

[randallmeeker]

needs docs

[dmarcelino]

Created balderdashy/waterline-docs#59 to address that.

[cyrilchapon]

Hey.

I'm very interested in this PR. Though, I installed it and I identified 2 issues on it:

• the fact that the criteria is passed by ref and meant to be mutated from the callback is not a reliable strategy: if you go Model.find() without any criteria, then the ref is null and you have no way to mutate query criteria at all. A callback accepting a criteria as a second parameter would be very much better.
• calling callback(new Error()) doesn't throw anything

I'll try to submit a PR with failing unit tests soon if that can help!

[dmarcelino]

A callback accepting a criteria as a second parameter would be very much better.

@cyrilchapon, currently no lifecycle callback accepts criteria as a parameter. Any change to this behaviour should be done in a similar manner to what is being discussed in #1004. Our lifecycle callbacks should be as consistent as possible.

[cyrilchapon]

@dmarcelino

currently no lifecycle callback accepts criteria as a parameter

Indeed... However, no similar situation was faced before: due to the fact that update() cannot be called without where argument, thus before/afterUpdate are never invoked with a null criteria parameter. If that wasn't the case, such an issue would have been raised waaay sooner, I think.

Any change to this behaviour should be done in a similar manner to what is being discussed in #1004. Our lifecycle callbacks should be as consistent as possible.

I was afraid of this. But as I just stated, I think this is the first time ever any lifecycle callback has a chance to be invoked with a null argument =/

[dmarcelino]

this is the first time ever any lifecycle callback has a chance to be invoked with a null argument =/

Good point, still, wouldn't it be simpler to do criteria.where = criteria.where || {} before basic.js#L44 instead of adding something to the callback?

[cyrilchapon]

Yes. As long as one has a chance to add some criteria. Your solution seems good =)

There is still the callback(err) issue that is not handled

[dmarcelino]

There is still the callback(err) issue that is not handled

I totally agree with you on that onde ;)

[jvanalst]

I have a situation where an AfterFind would be incredibly useful.

My Model is actually working as a side cache of an LDAP server. Before creating a instance it looks up the data from the LDAP (the primary information source) and then sets all it's own appropriate attributes appropriately.

It would be nice to have an afterFind LifeCycle callback, so that I could do a bit of custom code to check the MySql UpdatedAt field and if the record is stale enough run an automatic update, rather than having to call an update explicitly.

[aclave1]

I have another situation where beforeFind/afterFind are useful. When searching for users by email address, the search needs to be case INsensitive, since emails are case insensitive. When we create a user, we toLowerCase their email in a beforeFind, but we have no way to do this in a query without manually doing so every time we search a user by email.

[dmarcelino]

@cyrilchapon, any updates on the changes you wanted to make?

[cyrilchapon]

@dmarcelino Nope =(, unfortunately, I won't have a chance to fix it myself in the future weeks.

I said

I'll try to submit a PR with failing unit tests soon if that can help!

And won't be able to, sorry. Shouldn't be hard though.

[kevinob11]

What is the status here?

[joncodo]

Any progress here or a way to implement this in the meantime?

[randallmeeker]

closed w/o comment?

[mikermcneil]

@randallmeeker @joncodo @kevinob11 @cyrilchapon @dmarcelino @aclave1 Hey y'all, just posted in #1198 (comment) -- going to go through and cap off these other threads so that we can funnel discussion into one place. (Also the link will cause them to get automatic back-references, so it'll be easier for folks from search engines to get up-to-date, relevant information this way)