update operations don't fail if required keys are missing.

[airhadoken]

I'm working on a not-really-serious feathers app right now, and I'm still getting to know it, but I think I've uncovered an issue with updates and required fields. Starting with a model "jogtimes" that's set up like this:

const mongoose = require('mongoose');
const Schema = mongoose.Schema;

const jogtimesSchema = new Schema({
  distance: { type: String, required: true },
  start: { type: Date, required: true },
  end:   { type: Date, required: true},
  user:  { type: Schema.ObjectId, ref: 'User', required: true },
  createdAt: { type: Date, 'default': Date.now },
  updatedAt: { type: Date, 'default': Date.now }
});

const jogtimesModel = mongoose.model('jogtimes', jogtimesSchema);

I have a create hook that ensures that the user is set to the authorized user on create, so this works as expected:

fetch(
          "/jogtimes", {
            method: "POST",
            headers: headers,
            body: JSON.stringify({
              token: usertoken,
              distance: "3000",
              start: Date.now() - 300000,
              end: Date.now()
            })

But then an update operation happens like such (note no user set):

// assume jogtime is the previously created object, headers are the standard extra headers
//  and token is the authorization token.
fetch(
          "/jogtimes/" + jogtime._id, {
            method: "PUT",
            headers: headers,
            body: JSON.stringify({
              __v: ++jogtime.__v,
              token: usertoken,
              distance: "5000",
              start: Date.now() - 400000,
              end: Date.now() - 100000
            })

With no hook to populate the user, this now unsets the user key (and changes the other set items) and returns me an object without it. I'm working around this with PATCH for this operation instead, but it seems like this should be throwing an error instead of allowing the operation to proceed.

[daffl]

update is supposed to replace the entire resource much like HTTP PUT (which it maps to) does. As described here:

The HTTP RFC specifies that PUT must take a full new resource representation as the request entity. This means that if for example only certain attributes are provided, those should be removed (i.e. set to null).

An additional method called PATCH has been proposed. The semantics of this call are like PUT in that it updates a resource, but unlike PUT, it applies a delta rather than replacing the entire resource.

If that is not your intended behaviour you can extend the adapter and map update to patch or do the same thing with a before hook for update:

app.service('myservice').before({
  update(hook) {
    return this.patch(hook.id, hook.data, hook.params).then(data => {
      // setting hook.result will skip the original method call
      hook.result = data;
      return hook;
    });
  }
});

[airhadoken]

Thanks for pointing me to extending adapters.

I just want to reiterate that the issue I think I'm seeing here is that {required: true} on the model isn't being respected, which would be fine in patch semantics (the key existed on the previous version of the object) but is not appropriate for put/update semantics (the proposed object is missing the required key).

[daffl]

Oh sorry, I think I totally missed the point then. So looking at https://github.com/feathersjs/feathers-mongoose/blob/master/src/service.js#L132 it sounds like Model.findOneAndUpdate doesn't validate?

[airhadoken]

That is what it sounds like. I haven't tried to trace it to be sure, but that's the effect I'm seeing.

[daffl]

Ok, thank you. We'll look into this once we circle back for the next release. Can this be solved with calling the Mongoose validation in an update or patch hook for now?

[airhadoken]

Yes. For PATCH, you would have to ensure non-null values for all required fields. For PUT, I wrote this hook that is currently working well enough:

function verifyRequired(serviceName) {
  return function(hook) {
    hook.app.service(serviceName).Model.schema._requiredpaths.forEach(function(key) {
      var badkeys = {};
      if(!hook.data[key]) {
        badkeys[key] = key + ' is required';
        throw new feathersErrors.BadRequest('required key "' + key + '" is missing', badkeys);
      }
    });
  };
}

[ekryski]

The reason it isn't validating is because we need to tell it do so. A pretty darn easy fix. We just need to add these to options on this line:

{
   runValidators: true,
   setDefaultsOnInsert: true
}

[whollacsek]

@ekryski Do you know when the runValidators is scheduled to be released?

[ekryski]

@whollacsek no one had done it yet. Looks like @marshallswain added a PR yesterday and it just needs some tests. We'll see if we can't get something out by the end of the weekend.