Dynamoid / dynamoid

Ruby ORM for Amazon's DynamoDB.
MIT License
582 stars 195 forks source link

Support binary type natively #822

Closed dalibor closed 4 days ago

dalibor commented 1 week ago

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 commented 5 days ago

Started a discussion https://github.com/aws/aws-sdk-ruby/discussions/3146.

dalibor commented 5 days ago

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 commented 5 days ago

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 commented 4 days ago

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