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

Support custom update expressions when update has no other attribute changes #138

Merged
merged 5 commits into from
Oct 17, 2023

Conversation

lsglick
Copy link
Contributor

@lsglick lsglick commented Oct 14, 2023

Issue #, if available:
Resolves #137

Description of changes:

This is a minimal implementation to allow update expressions to be passed through directly to the api call. To avoid conflicts, an error is raised if an update expression would have been generated by the sdk to update dirty attributes.

Also: Adds support for pass-through API options to the class-level .update item operation.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…changes

This is a minimal implementation to allow update expressions to be passed through directly to the api call. To avoid conflicts, an error is raised if an update expression would have been generated by the sdk to update dirty attributes.

Also: Adds support for pass-through API options to the class-level `.update` item operation.

Resolves aws#137
@mullermp mullermp added the pr/needs-review This PR needs a review from a Member. label Oct 16, 2023
Copy link
Contributor

@mullermp mullermp left a comment

Choose a reason for hiding this comment

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

Looks good! This is on the right track. I think you will need a changelog for this. Are there any other improvements you can see while we're at it?

lib/aws-record/record/item_operations.rb Outdated Show resolved Hide resolved
@lsglick
Copy link
Contributor Author

lsglick commented Oct 17, 2023

I think you will need a changelog for this.

Would you like to see more than the changes already made in CHANGELOG.md?

Are there any other improvements you can see while we're at it?

There's a few subtle things here-and-there as we're using the sdk, though most we've been able to workaround or adjust our approach!

@mullermp
Copy link
Contributor

Would you like to see more than the changes already made in CHANGELOG.md?

Sorry... this is what I get for reviewing from mobile. :D

Copy link
Contributor

@mullermp mullermp left a comment

Choose a reason for hiding this comment

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

LGTM - requested one more reviewer and we can try to release this week.

Copy link
Contributor

@alextwoods alextwoods left a comment

Choose a reason for hiding this comment

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

Looks good - few minor comments

lib/aws-record/record/item_operations.rb Outdated Show resolved Hide resolved
@@ -581,19 +576,21 @@ def find_all(keys)
# Aws::DynamoDB::Client#update_item} call immediately on the table,
# using the attribute key/value pairs provided.
#
# @param [Hash] opts attribute-value pairs for the update operation you
# wish to perform. You must include all key attributes for a valid
# @param [Hash] new_params attribute-value pairs for the update operation
Copy link
Contributor

Choose a reason for hiding this comment

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

naming nit: maybe instead of new_params we could just use attributes?

Copy link
Contributor

Choose a reason for hiding this comment

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

new_params is what's used for all of the updates - I think it makes sense to stay consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately attributes would shadow the attributes method already used below:

attr_name = attributes.storage_name_for(attr_sym)
key[attr_name] = attributes.attribute_for(attr_sym).serialize(value)

I used new_params because it was consistent with the ItemOperations#update/ItemOperations#update! signatures also in this file. But maybe that's confusing since in this particular case it also must include the keys?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nvm - I think new_params makes sense for consistency then.

@jterapin jterapin merged commit 87d455f into aws:main Oct 17, 2023
25 checks passed
@lsglick lsglick deleted the custom-update-expressions branch October 18, 2023 12:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/needs-review This PR needs a review from a Member.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for custom Update expressions
4 participants