Support binary type natively

[dalibor]

aws-sdk-dynamodb expects StringIO or IO object for binary and returns back StringIO for binary values.

Current implementation incorrectly converts and stores binary values as strings that due to Base64 encoding use 33% more storage and read/write capacity.

Request Before:

{
  "TableName": "dynamoid_tests_documents_1732010637_594",
  "Item": {
    "image": {
      "S": "AIj/"
    },
    ...
}

Request After:

{
  "TableName": "dynamoid_tests_documents_1732010679_801",
  "Item": {
    "image": {
      "B": "AIj/"
    },
    ...
}

[andrykonchin]

Looks good!

Could you please take a look at the comments?

[andrykonchin]

Started a discussion aws/aws-sdk-ruby#3146.

[andrykonchin]

I suppose that now new values of a binary field will be stored in DynamoDB as binary but all existing values are stored as string. It might be a bit surprising for end users.

The common approach is to step-by-step migrate users from the current behaviour to the new one. We can introduce a new field's option to either keep old behaviour (by default, so there is no a breaking change in a minor release) or to switch to the new one (for new fields or new tables or in new projects).

Example is a boolean type that wasn't supported by DynamoDB initially so Dynamoid's boolean fields were stored as f and t. Later DynamoDB added support of the boolean type and a new option store_as_native_boolean was added:

field :active, :boolean, store_as_native_boolean: false

Later in the next major release such migration options can be removed and we can propose users to custom types to keep old storage formats.

So I propose to add a new field's option store_as_native_binary that is by default false. And check it at casting, dumping and undumping.

[dalibor]

I agree. It could cause issues for integrators even though they might not have noticed that the binary values aren't stored with binary type in DynamoDB.

I've added the store_as_native_binary option that should be a safe work-around, but there's still a risk with this change to not get noticed with the new major release since there will not be a breaking code change.

This concerns me along with the lack of alignment with DynamoDB's single table design which is why I've started exploring Aws::DynamoDB::Client directly for my project.

Let me know if you want me to do any other changes to this PR, or I'll leave it to you for how you want to get it out.

[andrykonchin]

The only way to prevent breaking changes being unnoticed that I see is to add deprecation warnings in the last minor release before the major one.

[dalibor]

Could also require the store_as_native_binary option be set, otherwise raise an error.

[andrykonchin]

Regarding the single-table design - it seems to me it's out of scope of Dynamoid. Or at least of the initial scope to implement the Active Record pattern.

I am not opposed to including the new functionality into the gem related to this "pattern". But I haven't found any common and widely accepted "interface" or API. So I postponed this.

There is an issue #568 but all I heard there were either some generic thoughts or requesting somebody's project specific features. So there was nothing I can work with.

[andrykonchin]

Looks great to me! Thank you.

[dalibor]

There's an issue with Code Climate:

File undumping.rb has 252 lines of code (exceeds 250 allowed).

I could fix it with this change, although not much ideal:

--- a/lib/dynamoid/undumping.rb
+++ b/lib/dynamoid/undumping.rb
@@ -267,11 +267,9 @@ module Dynamoid
       STRING_VALUES = %w[t f].freeze

       def process(value)
-        store_as_boolean = if @options[:store_as_native_boolean].nil?
-                             Dynamoid.config.store_boolean_as_native
-                           else
-                             @options[:store_as_native_boolean]
-                           end
+        store_as_boolean = @options[:store_as_native_boolean]
+        store_as_boolean = Dynamoid.config.store_boolean_as_native if store_as_boolean.nil?
+
         if store_as_boolean
           !!value
         elsif STRING_VALUES.include?(value)
@@ -284,11 +282,8 @@ module Dynamoid

     class BinaryUndumper < Base
       def process(value)
-        store_as_binary = if @options[:store_as_native_binary].nil?
-                            Dynamoid.config.store_binary_as_native
-                          else
-                            @options[:store_as_native_binary]
-                          end
+        store_as_binary = @options[:store_as_native_binary]
+        store_as_binary = Dynamoid.config.store_binary_as_native if store_as_binary.nil?

         if store_as_binary
           value.string

Code coverage seems to have already been broken in master. It might need a later ruby version?

[andrykonchin]

Code coverage seems to have already been broken in master. It might need a later ruby version?

Yeah, I didn't have a chance to fix this annoying issue. Please don't bother with it. With code climate as well - its rules aren't ideal.

The only thing left is to squash working commits. I can do it on my own today later.

[dalibor]

Cool - feel free to squash and merge when you'd like. Thank you!