clarkie / dynogels

DynamoDB data mapper for node.js. Originally forked from https://github.com/ryanfitz/vogels
Other
490 stars 110 forks source link

v7 validation and null properties #60

Closed cmawhorter closed 6 years ago

cmawhorter commented 7 years ago

dynogels treats null essentially as a special $delete command for properties. I can live with that, but v7 is now even more strict with regard to how nulls are handled and I'm stuck at 6.2.

Repro:

  1. Create a schema that allows null. e.g.: something: Joi.string().allow(null)
  2. Model.update({ something: null }, console.log)
  3. Validation error complaining that something is required

I have all my objects in json schema. These objects have nearly everything required and allow nulls. This is so I can guarantee the model the consumer gets and ajv handles this.

I use enjoi to turn that json schema into a joi schema.

However with v7, null values are stripped prior to doing validation and this leads to the problem.

Would you ever consider accepting a PR supports reading/writing null to ddb?

clarkie commented 7 years ago

I think this may be an issue with the underlying database not accepting null values in previous versions (I'm guessing here). I've had a look and it does support NULL types which are in fact booleans! haha!

NULL — (Boolean) An attribute of type Null. For example: "NULL": true http://docs.aws.amazon.com/AWSJavaScriptSDK/latest/AWS/DynamoDB.html#updateItem-property

I have no issues with you submitting a PR for supporting this in dynogels. We'd need to keep the ability to set undefined to act in the same way as it does now.

Cheers

jkav77 commented 7 years ago

I think there are two aspects to this issue. The reason for the change is PR #54 for v7 enforces the schema. It prevents updating an item to be in violation of its schema, but I probably missed the case for .allow(null). I believe the previous behavior for setting an attribute to null was to delete that attribute. This makes sense because of the second aspect of this issue.

DynamoDB does not allow null values. If you look at the DynamoDB.putItem() page, it explicitly states this. If you look at the DynamoDB Attribute Values page it elaborates on the NULL data type, which is a boolean. I haven't used the NULL type, but it seems like if you checked the value it would be true or false, and not null.

Attribute values cannot be null. String and Binary type attributes must have lengths greater than zero. Set type attributes cannot be empty. Requests with empty values will be rejected with a ValidationException exception.

I think the desired behavior for dynogels should be that setting any attribute value to null is permitted and it deletes the attribute in DynamoDB. I am not sure, but I think requesting a non-existent attribute with DynamoDB.getItem() should return null.

cmawhorter commented 7 years ago

sorry for delay in responding (just had a baby). i forked and patched it to support null but it ended up breaking a lot of tests, which lead me to believe that a lot of old code might depend on the behavior.

perhaps a table option flag nullables t/f with default being false? that would give backwards compat.

DynamoDB does not allow null values

it does in typical aws fashion i.e. convoluted as F. DocumentClient translates this null data type to/from the DDB null type transparently. so using DocumentClient directly allows r/w null, which is what dynogels is using under the hood IIRC.

[...] but I think requesting a non-existent attribute with DynamoDB.getItem() should return null.

IMO getItem on a non-existent property should return undefined when the model schema is known and the property isn't defined.

A nullable boolean column type can have four states: undefined (non-existent), null, true, false. That type of thing can be useful in a pinch.

jkav77 commented 6 years ago

So according to the documentation:

// setting an attribute to null will delete the attribute from DynamoDB
Account.update({email: 'foo@example.com', age: null}, function (err, acc) {
  console.log('update account', acc.get('age')); // prints null
});

I think this is because when vogels was created DynamoDB did not allow null values and now it does. It does seem that Dynogels should be updated to reflect this change but I'm not sure exactly how that should look. And like @cmawhorter mentioned, this will break a lot of other areas.

If null values were allowed then how would you delete a value?

clarkie commented 6 years ago

@dangerginger I agree, this is a big change. I'm not convinced in the JS world that a null type is useful. At no point while using dynamo db have I felt I needed it. I'm guessing AWS added it more for the strongly-typed languages? Right now, I have no idea how to make this work.

Also, thanks for going through all the issues! 👍

jkav77 commented 6 years ago

So it seems that the only use case for this is when you have an attribute that is required, but that you can allow to be null. If the value is allowed to be null then it seems to me the attribute is not required and you can just remove it from the model. I am going to close this one. This would be a huge change and I don't think it is worth it.

cmawhorter commented 6 years ago

that's not the only use-case, but i understand adding support for this would be difficult to achieve in a backwards compat way.

i got around this by adding a special "$null" value and a translation step to my wrapper.