taoensso / faraday

Amazon DynamoDB client for Clojure
https://www.taoensso.com/faraday
Eclipse Public License 1.0
238 stars 84 forks source link

Add sanitization multimethod to strip empty values #67

Closed jeffh closed 9 years ago

jeffh commented 9 years ago

This is just a helper function that can be used to recursively strip empty values before giving to dynamodb. It converts empty sets and strings.

See invalid values at http://docs.aws.amazon.com/amazondynamodb/latest/APIReference/API_AttributeValueUpdate.html

ptaoussanis commented 9 years ago

Hi Jeff, thanks for this (and the doc reference, that was helpful).

I don't use DDB myself so I'm curious: is this actually a common issue that folks run into? And it's a common solution to just strip empty values? Is it not the case that the presence of an empty/invalid value implies some sort of error (i.e. something that you'd want to be made explicitly aware of through an exception, etc.)? Sincere question - I don't have any opinion on the subject.

Assuming folks would find this handy (feedback please?), I'd be on board with merging.

Side note: the implementation could be made significantly faster. I'll sort that out if/when merging - so please restrict comments to the general idea, not the particular implementation.

Thanks, cheers! :-)

sheelc commented 9 years ago

Hey Peter! I work with @jeffh and can help give a little bit more context.

We run into it a lot with optional fields, like address2 inside an address part of the document. Sometimes it's an empty string or sometimes it's nil but we don't really mind either way and just want it coerced to nil so DDB can store it. Or other times it's a vector of discounts, which should be empty when no discounts are applied. We've thought of two main choices given DDB's restrictions: save it as a nil and change it back to a vector if we really it need it as an empty vector on the other side (which we haven't really had to do) or use Nippy. We sometimes build local secondary indices off of these optional values and so we've preferred nil coercion over Nippy.

Rather than coerce nils of explicit fields we are okay with being nil, we've found this full "sanitization" behavior handy on most of our projects. Even if we're just sanitizing nested parts of a map. It definitely matters on the use case, which is why we use it as an opt-in function as opposed to all objects passing through Faraday have things stripped out.

We'd love to hear how other people in the community are working around this issue. It can definitely be a hiccup in the dream of putting in and getting out the same Clojure object.

ptaoussanis commented 9 years ago

Hi Sheel, thanks for the extra info.

Sounds like the approach you're taking is quite reasonable. Barring any other ideas (?), will merge this and make some changes for performance.

ptaoussanis commented 9 years ago

Okay, have a modified implementation with https://github.com/ptaoussanis/faraday/commit/f5f4e87112b7b30ed65caaa2edc6eeb28ca1e6c9

Let me know if that's still working as you expect?

Still open to alternative ideas if anyone has anything better btw.

jeffh commented 9 years ago

:+1: Thanks @ptaoussanis! It works as expected for us.