Model.pre('save', fn) is not working as documented

[pfurini]

I have something like:

EntryCategory.pre('save', function(next) {
  // some testing
  next(new errors.ValidationError('test error'));
});

This is what official docs suggest, that is calling next with an error instance, and that should invalidate the save. But the error get uncaught:

 Unhandled rejection ValidationError: test error

and the request never returns to the client.

The behaviour is different with pre('validate'), in that case calling next(new Error()) causes a 500 error to be returned to the client (I'd prefer a 400, but that's another issue).

Another issue is that every pre hook seems to be called multiple times (it happens with both 'validate' and 'save' hooks). This is not only inefficient, but dangerous when you try to perform consistency checks that depend on other tables.

How one is supposed to validate before saving and returning a 400 on error? And hopefully avoid calling hooks multiple times?
Thx

[neumino]

This works fine for me

var thinky = require('./lib/thinky.js')();
var type = thinky.type;
var errors = thinky.Errors;

var EntryCategory = thinky.createModel('EntryCategory', {
    id: type.string()
});
EntryCategory.pre('save', function(next) {
  // some testing
  next(new errors.ValidationError('test error'));
});

var entry = new EntryCategory({});
entry.save().then(function() {
  console.log('Saved');
}).error(function(e) {
  console.log('Error', e);
});

[pfurini]

Ok, so the problem must be with tastypie-rethink.. I suppose it's not handling errors properly.
Is it normal that hooks get called multiple times when saving a single document? Otherwise it's another issue in tastypie-rethink..
thx

[neumino]

It depends what hooks. Validate is called twice for a save (once before,
once after)

On Sun, Apr 24, 2016, 13:09 Paolo Furini [email protected] wrote:

Ok, so the problem must be with tastypie-rethink.. I suppose it's not
handling errors properly.
Is it normal that hooks get called multiple times when saving a single
document? Otherwise it's another issue in tastypie-rethink..
thx

—
You are receiving this because you commented.

Reply to this email directly or view it on GitHub
#485 (comment)

[pfurini]

While save should be called once, right before actual saving?

Anyway, it looks like validate hook is the only one working properly with tastypie, so I'm going to use it exclusively. But I'd like to add a check that involves the model itself, basically I want to check if a field is unique in the table, and abort saving if it's not.
In ReQL this is the condition that must be true: r.table("tbl").getAll(valueToBeInserted, {index: "field"}).isEmpty().
But I don't know how to get this working in a validate hook. I tried this:

  EntryCategory.getAll(this.label, {index: 'label'}).run().then(function (results) {
    if (results.length) {
      next(new errors.ValidationError('label must be unique.'));
    } else next();
  }, next);

and ended up in an infinite loop.. I also tried querying using r directly, but with the same outcome.
It seems that querying on the same table triggers another validation and so on.
Do you know the right way to achieve that?
thx

[neumino]

The right way to enforce uniqueness is to set the field as a primary key. That's the normal and safe way to do it. If you don't want to set it as a primary key, you need to maintain a distributed lock, but that's kind of pain to do properly.

What you wrote seems reasonable to me if you want a weak uniqueness, but I'm not sure where your inifnite loop come from, I would need to see more code for that.

[neumino]

And yes the pre hook save is called once before saving a document.

[pfurini]

I fixed some code in tastypie-rethink, but the problem with the validate pre hook remains..
I mean, the fact that it gets called twice, one before and one after save, poses problems for some validation scenarios.
For example, if I check for a field uniqueness, the check will be successful before save, but it will fail after save, and the whole request will return an error. (yes, I know it's not the correct/safe way to check for uniqueness, but I can't change the primary key, at least not until RethinkDB will support composite keys).
Is there a way to skip the after-save validation hook?

[neumino]

You can use the save pre hook in your case I think.

[pfurini]

Unfortunately the save pre hook causes an unhandled rejection error in hapijs/tastypie..

Can I ask why a pre validate method should be called after save? It drives me crazy.. :-/
Without reading the doc (and the docs don't mention it anyway) one should expect that a pre hook runs before an action, and a post hook runs after that action. If I want to ensure model consistency after a successful save, I can just attach a post hook and do my checks there..
If this behaviour is not so tied to the library internals, is it possible to avoid calling the pre validation hook after save?
thx

[neumino]

The hook is triggered twice because thinky validates the input before saving it, and validate the output before returning it.

There is a debate on whether thinky should validate when retrieving a document (I somehow can't find it right now, but there is). The solution is probably to make it an option but it's not available at the moment.

[pfurini]

Hi, sorry to bother again, but with this code:

doc.saveAll( function( err ){
          if (err) {
            if (err.name === 'ValidationError') {
              bundle.data = {code: 403, message: err.message};
              return that.respond(bundle, http.forbidden);
            } else {
              bundle.data = {code: 500, message: err.message};
              return that.respond(bundle, http.serverError)
            }
          }
          // omissis..
});

if I return an error (using next(new Error(..)) in a save pre hook the callback function never gets called, and the app crashes.
But if I throw an error in a validate pre hook the callback gets called with the err parameter correctly set.

If I change the code above using the promise pattern

doc.saveAll().then(function() {
  // omissis
}).error(function(err) {
  // return an appropriate response like above
});

then both the save and validate hooks can return errors without crashing the app.

The only change in my code is the above, and the behaviour is consistent. There's definitely something wrong here, let me know if you're able to reproduce the issue.

P.

[neumino]

Can you provide a script the trigger your issue? I have a hard time understanding what the issue is with your snippet

[pfurini]

Sure, see https://ide.c9.io/nexbit/thinky-issue485

To avoid flooding the console with connection errors, run the rethinkdb instance I pre-installed in the container (go to the terminal tab on the bottom, and run the usual rethinkdb --bind all command).
Then run the project and you'll see that the first save will gracefully handle the validation error, while the second one (using the callback syntax) throw an unhandled rejection.

Paul

[neumino]

I can't access your snippet, can you make it a gist?

[pfurini]

Here it is: https://gist.github.com/nexbit/a805d1bf2ae7e3cdd5c87907f19641f2

BTW, the workspace on cloud9 is public, you only need to create a free account. If you don't know c9, it's wonderful to quickly test and share complete working projects, or demonstrate issues in a full dev env (e.g. in that workspace there's a working instance of rethinkdb, and a full node/npm project, so you can test the code as if it was on your machine).
PS: I don't work for them, but I do appreciate a good product ;)

P.

[neumino]

Thanks for the snippet.
Fixed in 2.3.2