Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] .update() enforces schema #54

Merged
merged 2 commits into from
Jan 19, 2017
Merged

[WIP] .update() enforces schema #54

merged 2 commits into from
Jan 19, 2017

Conversation

jkav77
Copy link
Collaborator

@jkav77 jkav77 commented Dec 27, 2016

I created a function internals.validateItemFragment() to check that attribute modifications requested with .update() conform to the defined schema. This is more involved than just using Joi.validate() because .update() is not guaranteed to receive all required attributes. For example, modifying one attribute of an existing item would only require specifying the Hash Key, Range Key (if there is one), and the attribute to modify. This works as I mentioned, but I would be interested in any feedback on the below. @Si1kIfY?

A few issues/questions:

  1. This method will not check the item has all .isRequired() attributes if you create a new item with .update(). Rather it will ensure the fields specified match the schema. @deedw, any opinion on this?

  2. I didn't add anything to handle nested objects. The schema in the README does not cover nested objects or using the Joi.object() type. I am not really sure if this is possible or if people are doing that.

  3. Right now .update() validates the attributes it receives automatically. This would create problems if anyone was relying on .update() to create items that don't match their schema.

fixes #39

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 98.659% when pulling 534099b on dangerginger:update-enforces-schema into 2f4293c on clarkie:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 98.659% when pulling 534099b on dangerginger:update-enforces-schema into 2f4293c on clarkie:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 98.659% when pulling 534099b on dangerginger:update-enforces-schema into 2f4293c on clarkie:master.

@csi-lk
Copy link

csi-lk commented Jan 9, 2017

The code looks great to me and the tests seem to validate this would fix the issue posted

Thanks for putting the time into this 👍

@clarkie
Copy link
Owner

clarkie commented Jan 11, 2017

Hi, thanks for your PR. I'm really sorry I missed this. I'll hopefully have chance today to have a look.

@clarkie clarkie merged commit 096a812 into clarkie:master Jan 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

.update() does not enforce schema
4 participants