Add validation on update

[ssh24]

Description

Currently, update/updateAll function does not perform any validation if a validation is specified on the model. This fix adds the validation check on dao for update/updateAll function and tests with all the validatable functions.

fixes #771

[b-admike]

I suggest you check error messages when the validations fail and one property of the Employee instance that is updated when they pass on top of checking the existance of the error or the instance.

[b-admike]

Is this line needed to do the validation?

[ssh24]

No it is not. Good catch.

[b-admike]

🚢🇮🇹

[ssh24]

@slnode test please

[ssh24]

@slnode test please

[virkt25]

nitpick: newline.

[ssh24]

I don't think so. It does not have that red arrow thing.

[ssh24]

@slnode test please

[loay]

why do you need to declare update here?
I mean if you continue the code while removing the update variable, it will work just fine !
Also, I see it as a redundant use of data, which is not needed

[b-admike]

@loay update variable is used on line 2708

[loay]

true and 2699 as well .. any use for adding this variable?

[ssh24]

The whole point is to keep the data variable clean. If you scroll down along the lines, after making a copy of data into update, we modify update so that the data returned from the before save hook is not changed.

        data = ctx.data;
        var update = data;
        var inst = data;
        if (!(data instanceof Model)) {
          inst = new Model(data, {applyDefaultValues: false});
        }
        update = inst.toObject(false);

        // validation required
        inst.isValid(function(valid) {
          if (valid) {
            doUpdate(ctx.where, ctx.data);
          } else {
            cb(new ValidationError(inst), inst);
          }
        }, update, options);
      });

[loay]

Try with this:

        data = ctx.data;
        var inst = data;
        if (!(data instanceof Model)) {
          inst = new Model(data, {applyDefaultValues: false});
        }

        // validation required
        inst.isValid(function(valid) {
          if (valid) {
            doUpdate(ctx.where, ctx.data);
          } else {
            cb(new ValidationError(inst), inst);
          }
        }, options);
      });

You will see that it is not affected and works fine, because inst variable is assigned to be equal to data as well .. so it is either not needed at all, or the test coverage is not covering this part .. I think it is not needed, because inst variable is doing the job.

[ssh24]

Good catch. Fixed.

[ssh24]

@slnode test please

[ssh24]

@slnode test please

[loay]

LGTM
Don't forget to squash :)

[bajtos]

@ssh24 IMO, the tests you have modified in LoopBack (strongloop/loopback#3541) are a valid usage of update/updateAll API. If they are failing after your change here in #1445, then it's very likely that the change in juggler is actually a breaking change.

In order to support validations of partial updates (either via patchAttributes or updateAll), we need to implement partial validations that will validate only properties that are being changed.

If you think it still makes sense to run full validation in updateAll, then please add a feature flag to enable it. For example, many DAO options have options.validate that can be used to control whether validation should be run or skipped. You can also introduce model-level setting, similar to validateUpsert.

Regardless of the approach you take, all existing tests here in juggler and in loopback must keep passing without any changes. (The feature "validate updateAll" must be disabled by default for backwards compatibility.)

cc @kjdelisle

[mrfelton]

This change breaks https://github.com/fullcube/loopback-ds-cascade-update-mixin which uses updateAll to make partial updates to multiple models.

+1 on @bajtos comments - this is a breaking change.