Implement before/after validation hook

[fabien]

Consider the following scenario (which is somewhat similar to what @clarkorz describes here #433):

1. in beforeRemote('create', ...) uploaded files (multipart/form-data) are handled
2. this adds the stored filename/path to ctx.args.data so the model will save this
3. however, the model appears to be invalid for some reason (unrelated to the uploaded file)
4. now, validation fails and there's no way to purge the now orphaned uploaded file from storage/disk

Or more elaborately:

1. in beforeRemote('create', ...) uploaded files (multipart/form-data) are handled
2. this adds the stored temporary filename/path to ctx.args.data so the model can use this later
3. however, the model appears to be invalid for some reason (unrelated to the uploaded file)
4. now, validation fails and there's no way to purge the uploaded file from storage/disk
5. if the model would have been valid, an after validate hook handler would have moved the file to its permanent destination and update the instance's filepath attribute.
6. the model would be saved as expected

At point 4 there should be a way to cleanup any intermediate (but now invalidated/orphaned) data. In this example scenario there's no way to handle everything in the 'safe' after save handler, because the form-data needs to be parsed first, so there will always be artifacts already created; it cannot be done side-effect-free.

This PR introduces such an after validate hook, and pairs it with the expected before validate hook, which is executed after the data has been set - but before the model is actually validated. Just like the now deprecated hook event 'validate'.

Ref: #433 (the proposed solution proved unsatisfactory) and pending #262

/cc @raymondfeng @bajtos

[fabien]

The order of execution for a valid model is as follows:

1. before save
2. instance is updated with data
3. before validate
4. call isValid (async)
5. after validate (will receive context.isValid === true)
6. save through connector
7. after save

The order of execution for an invalid model is as follows:

1. before save
2. instance is updated with data
3. before validate
4. call isValid (async)
5. after validate (will receive context.isValid === false)

[bajtos]

I'll take a look at the code later. There is one thing I don't understand though: since you are creating the (temporary) files in a remote hook, why can't you clean it in a remote hook too?

1. in beforeRemote('create', ...) uploaded files (multipart/form-data) are handled
2. run the model method
3. in afterRemote('create', ...), if the response status is not 200, then cleanup the stored file

[fabien]

@bajtos I seem to recall that afterRemote is never called when an error (such as a validation error) occurs before it reaches the 'after' step. Please correct me if I'm wrong. Update: I can confirm that afterRemote hooks are never called in this case.

This doesn't change the fact that I think such a hook would make a lot of sense, because it does not need to be run a remoting context, making this a much more robust solution. I believe it also remedies the disparity between the new operation hooks and the now deprecated validate event.

[bajtos]

TL;DR

Instead of after validate + context.isValid === false, we should provide a hook save failed (or save error) that is triggered for any error encountered during a save operation. That is the only correct way how to ensure that temporary resources are cleaned up when the instance cannot be saved.

We should also provide a similar hook in strong-remoting, something like "afterRemoteError" - see strongloop/strong-remoting#189

Long version

First of all, I want to make it clear that I understand that the current version may not be powerful enough to support the use cases that you (and @clarkorz in #433) and that I want to find a good solution that will work for you.

IMO, the use-cases for before/after validation hooks I've seen so far are trying to use a wrong tool for the job. They are trying to work around a fact that the proper way is not available (yet).

Let's take your example of cleaning up a (temporary) file upload.

First of all, a valid model data does not imply that the database request will pass:

• The database schema may define extra validations (constraints) that are not checked by LoopBack.
• LoopBack validations are not atomic. For example, if both LoopBack and database perform a uniqueness check, and there are multiple clients creating the same value, then LoopBack validation may pass while database validation will not.
• Plus add all usual sources of failure (network connection problem, database is down or switching to failover node, etc.)

Therefore it's not enough to implement the cleanup in "after validate" hook, we need a more generic "save failed" hook.

Secondly, remoting and operations hooks are not meant to be the only way of customising model methods. In many cases, it's better to write a single cohesive custom method rather than split the implementation across multiple hooks.

MyModel.upload = function(data, req, cb) {
  // 1. handle uploaded files (multipart/form-data)
  // NOTE: this should be provided by strong-remoting OOTB
  // https://github.com/strongloop/strong-remoting/issues/35
  data.filepath = /* the file */;
  MyModel.create(data, function(err, created) {
    if (err) {
     // remove the file
     cb(err);
    }
    cb(null, created);
  });
};

// optionally remove "MyModel.create" from REST API
// an map "MyModel.upload" as "POST /MyModels"

You can object that you need this behaviour for multiple methods (create, update), which is a valid argument. However, I still believe it's a bad idea implement this feature using a mix of hooks from different abstraction levels (remoting, operations).

I seem to recall that afterRemote is never called when an error (such as a validation error) occurs before it reaches the 'after' step. Please correct me if I'm wrong. Update: I can confirm that afterRemote hooks are never called in this case.

I see. Let's fix the problem in strong-remoting then, don't add new stuff to LoopBack only because we need a workaround for a missing feature of strong-remoting.

strongloop/strong-remoting#189

This doesn't change the fact that I think such a hook would make a lot of sense, because it does not need to be run a remoting context, making this a much more robust solution. I believe it also remedies the disparity between the new operation hooks and the now deprecated validate event.

I agree we should have an operation hook that will allow users to clean up after an error.

I still don't think we should to add before/after validate hooks. Can you please describe scenarios/use-cases where you need these new hooks?

[fabien]

@bajtos sure, a failure hook makes sense. I would go as far as making it more generic, and then use ctx.error = err or something similar to differentiate between the different failures that would occur.

However, I still think before/after validate hooks make sense to have, as the validate event has not really been compensated for.

Finally, here's a personal issue I would like to point out: I (and many others) go through considerable efforts to create PR's like this - it's getting more and more painful to keep this up with the ongoing (long) discussions that tend to follow. Which frankly, quickly turn into 1:1 discussions.

I feel that true open discussions are hampered by the 'structure' which they take place: after the fact, in GH issues.

In my previous OS experience, the (core) developer community was much more approachable, with healthy IRC discussions and 1:1 chat sessions to discuss ideas like this, before or while the code is being fleshed out.

Sure, I can see you're being protective to keep quality standards and such. But personally I'm getting a bit tired of contributing in such a fashion. Remember, contributions are voluntary, and I often feel that there's more discussion, many more words than LOC, and instead of getting into such a detailed discussion, you could probably whip up a PR that would resolve the issue exactly as you see fit. And we both would be happy, because it feels like a waste of time, sorry.

/cc @altsang @raymondfeng

[bajtos]

In my previous OS experience, the (core) developer community was much more approachable, with healthy IRC discussions and 1:1 chat sessions to discuss ideas like this, before or while the code is being fleshed out.

We moved away from IRC to Gitter, you can find us here: https://gitter.im/strongloop/loopback Please do check with us before starting a larger or possibly controversial change.

For posterity, the discussion about what hooks to add is continuing here: strongloop/loopback#1226 (comment)

[ritch]

@fabien so we are doing all of the things that you'd expect ...but not in the right order and perhaps a bit too much (eg. long 1:1 discussions)?

I like the idea of having more discussions in realtime to avoid lengthy PR threads. Does gitter support PMs? If so maybe we could try that...

[bajtos]

@fabien based on our discussion in strongloop/loopback#1226, I am proposing to close this pull request in favour of #441, #559 and possibly also #560. Any objections?

[bajtos]

@fabien Also which of #441, #559 and #560 is the most important for you, i.e. which one should be the first to get implemented?

[tekand]

I was trying to implement before save hook, because the documentation says that it runs before the validation, but it runs after. Can you please also correct the documentation? before validation would solve my problem also.

[bajtos]

I was trying to implement before save hook, because the documentation says that it runs before the validation, but it runs after.

@tekand I am pretty sure the "before save" hook does run before the validation, see this unit-test. Could you please post your code where you observe that model validation is run before the hook?

[tekand]

I have a model for Attendance, which has a relation to the User model and an attribute as time. The id of the User is not known, but a unique identifier (cardId) is known. Therefore I implemented the before hook to query the user's id based on the cardId

Attendance.observe('before save', function getUserByTag(ctx, next) {
        var app = Attendance.app;
        var User = app.models.User;

        var studentId = "";
        var cardId = "";

        if (ctx.instance) {
            cardId = ctx.instance.cardId;
            ctx.instance.unsetAttribute('cardId');
        } else {
            cardId = ctx.data.cardId;
            ctx.data.unsetAttribute('cardId');
        }

        if (cardId) {
            User.findOne({where : {cardId : cardId}}, function (err, user) {
                if (user) {
                    studentId = user.id;
                    if (studentId) {
                        if (ctx.instance) {
                            ctx.instance.studentId = studentId;
                        } else {
                            ctx.data.studentId = studentId;
                        }
                    }
                }

            });
        }
        next();
    });

The validation I used:

Attendance.validatesPresenceOf('studentId');

After thinking about the issue, I think the next() is not at the correct place, because the callback of findOne can execute after the next() Is this the standard way to do such thing, or there is any other way to do it?

[bajtos]

After thinking about the issue, I think the next() is not at the correct place, because the callback of findOne can execute after the next()

@tekand I think you hit the nail on the head! You need to call next from the callback function passed to User.findOne.

if (!cardId) {
  return next();
}

User.findOne({where : {cardId : cardId}}, function (err, user) {
  if (err) return next(err);
  // modify instance/data
  next();
});

[bajtos]

@tekand FWIW, I think ctx.data.unsetAttribute('cardId') won't work, since ctx.data is a plain data (json-like) object. delete ctx.data.cardId should work better.

[tekand]

@bajtos Thank you very much, it works like charm. :)

[bajtos]

The new hook "persist" has been landed on master via #559, I am closing this pull request as no longer relevant. @fabien please do reopen if you disagree.

Here is my proposed solution to the problem described in the PR description:

• put "before validate" code into a "before save" observer
• put "after validate" code to a "persist" observer
• any temporary files created in "beforeRemote" hook should be cleaned up via "afterRemoteError" hook (see Model.afterRemoteError hook strongloop/loopback#1272).