Support updating attributes that throw custom errors

[jkav77]

This now works for updating attributes. However, if you have an attribute that is required() and has a custom error() specified in the Joi schema it will fail to update the item but return no error at all.

[jkav77]

This is for issue #103. @Giners check it out. It's not done so not ready to merge.

[coveralls]

Changes Unknown when pulling a7934cd1abd164226bff7ce68ffa1cf5695628cc on dangerginger:feature/custom-error into on clarkie:master.

[coveralls]

Changes Unknown when pulling 6975b61fc38d95c8319d9ce3ad6d8847477fd5de on dangerginger:feature/custom-error into on clarkie:master.

[jkav77]

Actually I added a test and it seems to work. @Giners let me know what you think of this.

[jkav77]

@Giners thanks for the feedback. The console.log was an oversight. I should have removed that.

Bottom line: I think the functionality to validate the schema on an update should be removed entirely because it doesn't work and is attempting to duplicate functionality of Joi.

After looking at this more I think the validateItemFragment method should be removed. It is attempting to implement functionality that Joi does not have and it does so by parsing error messages which is hackish and probably unreliable. (BTW I wrote it a while ago lol). Also, it simply doesn't work for some cases (including the custom error messages you just brought up). For example, updating an item that doesn't exist yet allows an item that is missing required attributes. I'm not sure how it reacts to nested objects or who knows what else. The bottom line is this method is attempting to validate a schema which is what we use Joi for. Joi doesn't provide any partial schema validation.

I think the only reliable way to validate that an item is valid after an update is to get() the item from dynamodb first, apply changes, then validate the schema using Joi. All of the functionality to do this already exists in Model.get() and Model.validate().

@cdhowie and @clarkie I would like to know what you think of this suggestion.

[clarkie]

@dangerginger ah ok, it took me a while to understand the issue here but I think I do now and I think you're right. I don't think we should validate the full schema on update as dynamo allows PATCH requests. Having required fields on a PATCH request doesn't make sense to me. You shouldn't have to "change" all the required fields to update one other field. Perhaps we could modify the schema for updates something like this:

e.g.

const schema = {
  a: joi.string().required(),
  b: joi.string().required(),
  c: joi.string()
};

const patchB = { b: 'buses' };
const newSchema = omit(difference(keys(schema), keys(patchB)), schema);
// { b: joi.string().required() }

const patchC = { c: 'clouds' };
const newSchema = omit(difference(keys(schema), keys(patchC)), schema);
// { c: joi.string() }

[clarkie]

I take it this is related to the failing test introduced here? https://github.com/clarkie/dynogels/commit/d16eb786fcfa27276eb95e5924c5a1f851098d1f

[cdhowie]

I agree, this functionality has caused issues for us in the past and we no longer use it.

In particular, any Joi schema that has relationships between data elements is going to be problematic when all of those elements are not included in the same update. Think or(), xor(), ref(), etc.

Joi.object().keys({
  max: Joi.number().integer().min(1),
  min: Joi.number().integer().min(0).max(Joi.ref('max')).default(0)
})

There's no way to properly validate an update of only one of those two attributes without knowing the value of the other; you need both to check the max constraint on the min attribute.

[clarkie]

eugh, yeah, I hadn't thought of that. Joi is just too powerful! 😀

[jkav77]

Ok, I am going to close this PR then and I will reopen a new one to remove the (failing) partial validation.