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

added support for specifying the ReturnValues option in update #350

Conversation

manast
Copy link

@manast manast commented Apr 25, 2018

Model.update always returns all the new values in every call. This can have a severe performance penalty in some scenarios. This PR provides a backwards compatible option returnValues when calling update so that the 5 different settings provided by Dynamo can be used:

  • NONE - If ReturnValues is not specified, or if its value is NONE, then nothing is returned. (This setting is the default for ReturnValues.)
  • ALL_OLD - Returns all of the attributes of the item, as they appeared before the UpdateItem operation.
  • UPDATED_OLD - Returns only the updated attributes, as they appeared before the UpdateItem operation.
  • ALL_NEW - Returns all of the attributes of the item, as they appear after the UpdateItem operation.
  • UPDATED_NEW - Returns only the updated attributes, as they appear after the UpdateItem operation.

@coveralls
Copy link

coveralls commented Apr 25, 2018

Coverage Status

Coverage increased (+0.1%) to 82.61% when pulling c9d2d87 on manast:feature/support-for-specifying-return-values into 388662c on automategreen:master.

@fishcharlie
Copy link
Member

@manast This looks amazing. Thanks so much!! Is there any chance you could add documentation regarding this new feature? We should also probably confirm to make sure there is a test to ensure backwards compatibility, and if not we should add one to confirm the backwards compatibility.

We will try to get this in for version 0.9. Thanks again!!

@fishcharlie fishcharlie added this to the v0.9.0 milestone Apr 25, 2018
@fishcharlie fishcharlie self-requested a review April 25, 2018 16:02
Copy link
Member

@fishcharlie fishcharlie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fishcharlie
Copy link
Member

Thanks again @manast! We'll get this in for version 0.9.

@fishcharlie fishcharlie modified the milestones: v0.9.0, v1.0 Jun 13, 2018
@fishcharlie fishcharlie merged commit c559e8f into dynamoose:master Jun 13, 2018
@fishcharlie fishcharlie mentioned this pull request Jun 13, 2018
@dobrynin
Copy link
Contributor

dobrynin commented Jan 9, 2019

@manast, @fishcharlie

Can we add a note to the documentation clearly stating that the Dynamoose default differs from the DynamoDB default? The current phrasing led me to believe that NONE was the default for Dynamoose.

Also, what do you think about adding an option defaultReturnValues so that you can globally override the current default of ALL_NEW?

@fishcharlie
Copy link
Member

@dobrynin Both ideas sound great! PR would be awesome.

@dobrynin dobrynin mentioned this pull request Jan 11, 2019
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants