Bug with null in GSI in transaction

ckhsponge commented 1 month ago

@andrykonchin I found this bug. I think the transaction code needs to remove the value instead of trying to set it to null. I'll try to find a fix. I feel like this wasn't happening in earlier transaction code but perhaps it was.

class T
  include Dynamoid::Document

  table name: "numstr".to_sym, key: :id, capacity_mode: :on_demand

  field :str
  field :num, :number

  global_secondary_index hash_key: :str, range_key: :num, projected_attributes: :all, name: "str_num"
end

t = T.new
t.str = "STR"
t.num = nil

Dynamoid::TransactionWrite.execute do |txn|
  txn.save! t
end

/gems/aws-sdk-core-3.219.0/lib/seahorse/client/plugins/raise_response_errors.rb:17:in `call': Invalid attribute value type (Aws::DynamoDB::Errors::ValidationException)
ruby/gems/3.3.0/gems/aws-sdk-dynamodb-1.137.0/lib/aws-sdk-dynamodb/plugins/simple_attributes.rb:145:in `call'
ruby/gems/3.3.0/gems/aws-sdk-core-3.219.0/lib/aws-sdk-core/plugins/checksum_algorithm.rb:169:in `call'
ruby/gems/3.3.0/gems/aws-sdk-core-3.219.0/lib/aws-sdk-core/plugins/jsonvalue_converter.rb:16:in `call'
ruby/gems/3.3.0/gems/aws-sdk-core-3.219.0/lib/aws-sdk-core/plugins/invocation_id.rb:16:in `call'
ruby/gems/3.3.0/gems/aws-sdk-core-3.219.0/lib/aws-sdk-core/plugins/idempotency_token.rb:19:in `call'
ruby/gems/3.3.0/gems/aws-sdk-core-3.219.0/lib/aws-sdk-core/plugins/param_converter.rb:26:in `call'
ruby/gems/3.3.0/gems/aws-sdk-core-3.219.0/lib/seahorse/client/plugins/request_callback.rb:89:in `call'
ruby/gems/3.3.0/gems/aws-sdk-core-3.219.0/lib/aws-sdk-core/plugins/response_paging.rb:12:in `call'
ruby/gems/3.3.0/gems/aws-sdk-core-3.219.0/lib/seahorse/client/plugins/response_target.rb:24:in `call'
ruby/gems/3.3.0/gems/aws-sdk-core-3.219.0/lib/aws-sdk-core/plugins/telemetry.rb:39:in `block in call'
ruby/gems/3.3.0/gems/aws-sdk-core-3.219.0/lib/aws-sdk-core/telemetry/no_op.rb:29:in `in_span'
ruby/gems/3.3.0/gems/aws-sdk-core-3.219.0/lib/aws-sdk-core/plugins/telemetry.rb:53:in `span_wrapper'
ruby/gems/3.3.0/gems/aws-sdk-core-3.219.0/lib/aws-sdk-core/plugins/telemetry.rb:39:in `call'
ruby/gems/3.3.0/gems/aws-sdk-core-3.219.0/lib/seahorse/client/request.rb:72:in `send_request'
ruby/gems/3.3.0/gems/aws-sdk-dynamodb-1.137.0/lib/aws-sdk-dynamodb/client.rb:7004:in `transact_write_items'
ndle/ruby/3.3.0/bundler/gems/dynamoid-af79b8d0d879/lib/dynamoid/adapter_plugin/aws_sdk_v3/transact.rb:26:in `transact_write_items'

andrykonchin commented 1 month ago

Nice catch.

It seems transactions don't take into account the Dynamoid.config.store_attribute_with_nil_value config option, that is false by default.

Just for the record - I am planning to remove store_attribute_with_nil_value option (and to store nil values unconditionally) and some other legacy options that are needed only for compatibility reasons in the next major release.

But this is an interesting case so probably store_attribute_with_nil_value might be still useful and the global store_attribute_with_nil_value option should be replaced with a field-specific one (when a field is a part of a GSI).

ckhsponge commented 3 weeks ago

Thanks for the clarification on this. For DynamoDB, I would have preferred for a blank value to be able to be stored and just have the record omitted from from the GSI. Looks like that is not allowed.

I'll try to get a fix for this. And a test so you're future work doesn't break it ;)

ckhsponge commented 3 weeks ago

@andrykonchin I found the problem. Previously there were some sanitize_item calls in there.

Here's my first stab at a fix: https://github.com/Dynamoid/dynamoid/pull/897

andrykonchin commented 3 weeks ago

Thank you. I've left some comments.

Dynamoid / dynamoid

Bug with null in GSI in transaction #885