Refactor item_operations to support upsert-like behaviour on item.save

[NielsKSchjoedt]

Not done yet:

• [ ] Upon item.save(perform_upsert: true) ensure item is reloaded with all attributes from dynamodb
• [ ] Write specs
• [ ] Support prevent_overwrite_expression for update_item?
• [ ] Document changes

[coveralls]

Coverage decreased (-0.1%) to 97.556% when pulling 9c4a439b921de340a9fda15c344abc02f8890d3d on NielsKSchjoedt:master into e2ad7e0556a425baedc1662278fa34dc71c46b1b on aws:master.

[coveralls]

Coverage decreased (-0.1%) to 97.556% when pulling 9c4a439b921de340a9fda15c344abc02f8890d3d on NielsKSchjoedt:master into e2ad7e0556a425baedc1662278fa34dc71c46b1b on aws:master.

[awood45]

I'm open to this change in behavior, associated with a minor version bump to 1.1.0. It does seem that your to-do checklist is correct. Consider (and I'm happy to do this for you, given that it does create/consume AWS resources temporarily) running the integration test suite as well, as those tests should all continue to pass unaltered.

[NielsKSchjoedt]

Cool, so what DSL do you prefer: .upsert or .save(perform_upsert: true) (I'm indifferent) and what is your opinion on supporting prevent_overwrite_expression in the item_update: nice-to-have or need-to-have?

[NielsKSchjoedt]

Another thing, I see that the _build_update_expression method currently "only" either uses SET or REMOVE actions. According to the documentation (http://docs.aws.amazon.com/amazondynamodb/latest/APIReference/API_UpdateItem.html) ADD is also an option. It's of cause more complicated to implement. Do you have any comments on why this is currently not supported (left out on purpose?). Do you think this should be taken into consideration or should we leave it out for another refactor or..? I personally don't need the functionality AFAIK.

[awood45]

It's a complexity thing. I do want to include "ADD" functionality and the associated "REMOVE" functionality, but getting it right when we don't have a guarantee on what is and is not present in our local copy is fraught with edge cases.

On Wed, Dec 14, 2016 at 1:35 PM, Niels Kristian Schjødt < notifications@github.com> wrote:

Another thing, I see that the _build_update_expression method currently "only" either uses SET or REMOVE actions. According to the documentation ( http://docs.aws.amazon.com/amazondynamodb/latest/ APIReference/API_UpdateItem.html) ADD is also an option. It's of cause more complicated to implement. Do you have any comments on why this is currently not supported (left out on purpose?). Do you think this should be taken into consideration or should we leave it out for another refactor or..? I personally don't need the functionality AFAIK.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/aws/aws-sdk-ruby-record/pull/50#issuecomment-267163762, or mute the thread https://github.com/notifications/unsubscribe-auth/ACBDofgsKLsYCzTc2P9FXVlSUi8mUXkvks5rIGEXgaJpZM4LNDdy .

[awood45]

I'm wondering to myself that very question on the "prevent overwrite" question. I'm starting to wonder if upserting should be presented as a separate method from #save.

On Wed, Dec 14, 2016 at 1:37 PM, Alex Wood awood45@gmail.com wrote:

It's a complexity thing. I do want to include "ADD" functionality and the associated "REMOVE" functionality, but getting it right when we don't have a guarantee on what is and is not present in our local copy is fraught with edge cases.

On Wed, Dec 14, 2016 at 1:35 PM, Niels Kristian Schjødt < notifications@github.com> wrote:

Another thing, I see that the _build_update_expression method currently "only" either uses SET or REMOVE actions. According to the documentation (http://docs.aws.amazon.com/amazondynamodb/latest/APIReferen ce/API_UpdateItem.html) ADD is also an option. It's of cause more complicated to implement. Do you have any comments on why this is currently not supported (left out on purpose?). Do you think this should be taken into consideration or should we leave it out for another refactor or..? I personally don't need the functionality AFAIK.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/aws/aws-sdk-ruby-record/pull/50#issuecomment-267163762, or mute the thread https://github.com/notifications/unsubscribe-auth/ACBDofgsKLsYCzTc2P9FXVlSUi8mUXkvks5rIGEXgaJpZM4LNDdy .

[NielsKSchjoedt]

I was initially thinking of something like adding another option to the attr definition like add_on_update: true:

class MyModel
  include Aws::Record

  integer_attr :id, hash_key: true
  string_attr  :name, range_key: true
  integer_attr :counter, add_on_update: true
  numeric_set_attr :top_scores, add_on_update: true
  boolean_attr :active, database_attribute_name: "is_active_flag"
end

Then it would be fairly easy I think to take that into account when constructing the update expression

[awood45]

Possibly - some sort of metadata solution may help with some of these (seems straightforward for numerics, but maps/lists/sets still have complications).

Would presenting upserting as a separate variant of save, say as a parameter into save called :upsert, meet your requirements? I'm getting increasingly nervous about changing default behaviors, but a simple option (which could even be applied at the model level to give a broader opt-in) solves that concern.

On Wed, Dec 14, 2016 at 1:42 PM, Niels Kristian Schjødt < notifications@github.com> wrote:

I was initially thinking of something like adding another option to the attire definition like add_on_update: true:

class MyModel include Aws::Record

integer_attr :id, hash_key: true string_attr :name, range_key: true integer_attr :counter, add_on_update: true numeric_set_attr :top_scores, add_on_update: true boolean_attr :active, database_attribute_name: "is_active_flag"end

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/aws/aws-sdk-ruby-record/pull/50#issuecomment-267165684, or mute the thread https://github.com/notifications/unsubscribe-auth/ACBDoYdgy27qs9yhXh53SxXEp30GEsUcks5rIGLUgaJpZM4LNDdy .

[NielsKSchjoedt]

It's all good to me :-) I'll see what I can come up with the coming days. Will keep you posted 👍

[awood45]

Awesome - if you get busy, I've added this to my feature request backlog as well. I think the upsert option + perhaps a table-wide option is the way to go on this.

On Wed, Dec 14, 2016 at 1:49 PM, Niels Kristian Schjødt < notifications@github.com> wrote:

It's all good to me :-) I'll see what I can come up with the coming days. Will keep you posted 👍

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/aws/aws-sdk-ruby-record/pull/50#issuecomment-267167415, or mute the thread https://github.com/notifications/unsubscribe-auth/ACBDoYgUCjcY5Eg3UZ19HDYJ9yodx-fbks5rIGRdgaJpZM4LNDdy .

[awood45]

I'm closing this at this point - I think between the Save and Update functionality offered, we cover this pretty well at the moment. If this isn't as well covered as I think, happy to revisit.