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

Add expressions support to put-item and update-item. #73

Merged
merged 1 commit into from Nov 30, 2015

Conversation

theleoborges
Copy link
Collaborator

This adds support to Dynamo's expressions which can be used for conditions and updates. It's the preferred way documented in DynamoDB's API.

I've also taken the liberty to deprecate and print warnings for the legacy ways so it doesn't break any existing code but still gives users a hint to update their implementations.

We are currently using my fork in our project and it's working great so far.

@ptaoussanis
Copy link
Member

Hi Leonardo,

Thanks a lot for this! Merging for now so I can take a closer look-

ptaoussanis added a commit that referenced this pull request Nov 30, 2015
Add expressions support to put-item and update-item.
@ptaoussanis ptaoussanis merged commit 8974bd1 into taoensso:master Nov 30, 2015
@theleoborges
Copy link
Collaborator Author

Awesome, thanks!

For what's worth, we've just ran a bunch of load tests against our code using faraday - previously it was using amazonica - and, besides correct results - we got much better CPU usage.

@ptaoussanis
Copy link
Member

No problem, thanks for the PR!

Have just pushed [com.taoensso/faraday "1.9.0-alpha1"] to Clojars. Please note that I renamed some options to keep them shorter: expression->expr, values->vals, etc. I prefer the shorter forms when they're standard and/or unambiguous; AWS's API tends to be pretty verbose w/o much benefit.

Also removed the printed warnings to keep from swamping legacy users with output.

Not using DDB myself, so would appreciate if you could confirm at some point that this published version works as expected? Zero rush.

Cheers :-)

@theleoborges
Copy link
Collaborator Author

Sure thing. No issues around the renaming.

Regarding the warnings, I see them as a way to encourage the user to upgrade. But I'll leave it up to you. Ideally a new version would remove that option altogether.

I'll let you know in a few weeks but our tests so far work great.

Thanks for pushing the artefact, I can now delete the one a pushed this morning :)

@ptaoussanis
Copy link
Member

Ideally a new version would remove that option altogether.

That'd be a little quick for a hard breaking change, I think. I'd be okay with printing a warning, but it'd need to be done in a delay to keep the warning from printing thousands of times in production - does that make sense?

@theleoborges
Copy link
Collaborator Author

Absolutely!

I didn't meant to imply this version should remove it :) I meant another version in the future, after this one :)

ptaoussanis added a commit that referenced this pull request Nov 30, 2015
ptaoussanis added a commit that referenced this pull request Nov 30, 2015
@ptaoussanis
Copy link
Member

Just pushed [com.taoensso/faraday "1.9.0-alpha2"] which adds warnings back, but will only print them once.

Re: removal in a future version- my general view is that we never ever break anything unless there's a strong reason to (e.g. we're forced to since the AWS lib itself is breaking backwards support).

If the deprecated path keeps working through the next 6 versions of the AWS lib, then I'd be happy for Faraday to keep working through its own next 6 versions. We can drop documentation eventually, but keep the legacy code working.

Forcing someone to spend time/money rewriting working production code is always something we should try avoid if possible. I'd note that Clojure itself is actually really good with this: very few unnecessary breaking changes since 1.0.

:-)

@theleoborges
Copy link
Collaborator Author

Sounds good to me!

Thanks

@ptaoussanis
Copy link
Member

Cool! Just shout if I broke anything with the changes. Thanks again for the PR!

ptaoussanis pushed a commit that referenced this pull request Dec 12, 2015
I wasn't comfortable with :proj-expr, :projection is a lot more readable.

I feel tempted to rename :expr-names to just :names, given that there is
no other `names` dictionary we can set for GetItemRequest, but #73 used
:expr-attr-names for put-item-request so I'm using this one as well for
consistency.
ptaoussanis added a commit that referenced this pull request Dec 19, 2015
ptaoussanis added a commit that referenced this pull request Dec 22, 2015
ptaoussanis added a commit that referenced this pull request Dec 22, 2015
ptaoussanis added a commit that referenced this pull request Jan 23, 2016
@joelittlejohn
Copy link
Collaborator

I've recently been working a lot with update expressions and I've been frustrated by the requirement to use strings rather than represent updates as data. This makes it difficult to add or remove clauses from the expression. This got me thinking...

Wouldn't it be preferable to re-implement the existing data-driven method of specifying updates so that it uses string-based expressions under-the-hood rather than deprecate it?

It seems like specifying updates as structured data rather than strings is a superior approach (and certainly idiomatic in Clojure-land). Faraday can implement these internally using the update-expression strings, but keeping the data-driven update API would provide a better API for Faraday users. Anyone that wants to use the update expression strings directly can of course do so by providing :update-expr etc.

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.

None yet

3 participants