TransactionWrite sanitizes items

[ckhsponge]

TransactionWrite should call sanitize_item so that nils are not persisted. Otherwise GSI will fail with a nil range key.

Closes #885

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.88%. Comparing base (37572d7) to head (20d2eea).
Report is 27 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #897      +/-   ##
==========================================
+ Coverage   91.56%   91.88%   +0.31%     
==========================================
  Files          75       76       +1     
  Lines        3700     3856     +156     
==========================================
+ Hits         3388     3543     +155     
- Misses        312      313       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[andrykonchin]

It's kind of integration tests, so it makes sense to have them, but it will be great to have more thorough specs for each transactional method, like it's done for non-transactional ones:

https://github.com/Dynamoid/dynamoid/blob/master/spec/dynamoid/persistence_spec.rb#L2996-L3037

Such specs would test:

• what actual value of a nullified attribute is persisted (it either has null value or is removed in the item)
• and how store_attribute_with_nil_value option affects this behaviour

[ckhsponge]

I'm thinking to not add tests for store_attribute_with_nil_value = true when there is an index. This fails even without transactions. I suppose I could add a test that a ValidationException is thrown in this case. And that test should exist for both transactions and non-transactions.

  let(:klass_with_gsi) do
    new_class(class_name: 'Ferret') do
      field :name
      field :age, :number
      global_secondary_index hash_key: :name, range_key: :age, projected_attributes: :all, name: 'str_num'
    end
  end
...
    context 'store_attribute_with_nil_value = true', config: { store_attribute_with_nil_value: true } do
      it 'can set gsi field to nil' do
        doc = klass_with_gsi.new
        doc.name = 'abc'
        doc.age = 1
        doc.save!
        doc.age = nil
        doc.save! #      Aws::DynamoDB::Errors::ValidationException: Invalid attribute value type
      end
    end

[andrykonchin]

It seems it's more difficult than just skipping nil values. A nullified attribute of an existing item simply will not be updated at all (so old value will be kept unchanged). So correct logic is to remove such attribute at all.

It's done in the following way in non-transactional method #save:

• https://github.com/Dynamoid/dynamoid/blob/master/lib/dynamoid/persistence/save.rb#L44
• https://github.com/Dynamoid/dynamoid/blob/master/lib/dynamoid/adapter_plugin/aws_sdk_v3/item_updater.rb#L52-L66

[ckhsponge]

Indeed. The code I put in there just skips the nils which is not correct. I'll have to do the removing for all these methods like you said.

[ckhsponge]

This was bothering me again today. I'll renew my efforts to fix it.

[ckhsponge]

@andrykonchin A review of this would be appreciated. Let me know if you see some areas for cleanup.

[andrykonchin]

I'll review this later this week.

[andrykonchin]

This logic is already applied (except stringifying Hash keys) by Dynamoid::Dumping.dump_attributes call (in save, update_fields, upsert - where #sanitize_attributes is called) so #sanitize_attributes seems excessive.

[andrykonchin]

So it might make sense to stringify Hash keys during dumping as well. This way it can be shared by the transactional and non-transactional methods and can be removed from the aws plugin.

[ckhsponge]

Indeed, sanitize_attributes was excessive. I removed it. The stringifying evidently is done somewhere else and I didn't need that.

[andrykonchin]

It makes sense to have similar specs for #update_attributes and #update_attribute.

[ckhsponge]

I added update_attributes, update_fields and update_fields with set().

[ckhsponge]

@andrykonchin Please review again.