dynamodb-toolbox icon indicating copy to clipboard operation
dynamodb-toolbox copied to clipboard

Updating an attribute to be undefined does not remove the attribute

Open mfbx9da4 opened this issue 4 years ago • 9 comments

if you have some item { id: 'asdf', foo: 'asdf'}

and you do Foo.update({id: 'asdf', foo: undefined } I would expect the field foo to be removed from dynamo but it persists

mfbx9da4 avatar Nov 18 '20 09:11 mfbx9da4

Isn't this what $remove does?

darbio avatar Nov 18 '20 09:11 darbio

Ah okay yeah makes sense. Shouldn't the toolbox then throw if (key in payload) { assert(payload[key] !== undefined, 'use $remove')

mfbx9da4 avatar Nov 18 '20 12:11 mfbx9da4

It depends on your use case I suppose.

Some people see empty/undefined values to have a meaning, so adding the exception carte blanche would be a big opinion.

darbio avatar Nov 19 '20 00:11 darbio

Well yeah, but if it has meaning then it should do something, otherwise it's an unknown parameter. Anyway, I don't feel that strongly about this, happy for it to be closed. It's just something that tripped me up coming from another dynamo client.

mfbx9da4 avatar Nov 19 '20 09:11 mfbx9da4

Agreed. I just get the opinion that this library is trying to be as unopinionated as possible.

Perhaps this could be an option to turn on? 'RemoveUndefinedAttributes: true' or similar?

darbio avatar Nov 19 '20 13:11 darbio

Jumping in late, but I think I agree with @mfbx9da4. There is a removeNullAttributes option (enabled by default) that will remove attributes set to null, but I can't remember why I didn't just use undefined. There was probably a reason, but I'll investigate further.

jeremydaly avatar Nov 19 '20 13:11 jeremydaly

I think it should probably set to null if null is provided as this a valid dynamo type right? and then remove if set to undefined

mfbx9da4 avatar Nov 19 '20 14:11 mfbx9da4

NULL is a valid type, but it doesn't make a lot of sense in NoSQL especially if you have a schema defined via the library. I ran a twitter poll on this and "remove the attribute" on nulls won out: https://twitter.com/jeremy_daly/status/1256259584819449856

I'll look more into the undefined thing. I'm guessing I didn't use that because undefined would get lost on serialization, but that might just be the way the parsing works. I could most likely figure out a workaround.

jeremydaly avatar Nov 19 '20 14:11 jeremydaly

I don't disagree with removing undefined, however I do note that $remove works just fine as well.

In the context of the DSL for this library, it's more explicit to use the DSL than it is to check for undefined attributes.

That doesn't mean that it's wrong to remove them, but it does introduce another vector for bugs and nuances in debugging that people who use the lib need to be aware of.

I suppose the broader question is to decide which method is the "way" to do it in this library - especially to get it to v1.0.0 rather than a WIP/beta.

darbio avatar Nov 20 '20 02:11 darbio