fix: Deep-duplicated event attributes in to_h to avoid returning frozen objects (#44)

Signed-off-by: Daniel Azuma <dazuma@google.com>

Author: dazuma

lib/cloud_events/event/field_interpreter.rb

@@ -1,15 +1,16 @@
 # frozen_string_literal: true
 
+require "cloud_events/event/utils"
+
 module CloudEvents
   module Event
     ##
     # A helper that extracts and interprets event fields from an input hash.
-    #
     # @private
     #
     class FieldInterpreter
       def initialize args
-        @args = keys_to_strings args
+        @args = Utils.keys_to_strings args
         @attributes = {}
       end
 
@@ -26,6 +27,7 @@ def string keys, required: false
           case value
           when ::String
             raise AttributeError, "The #{keys.first} field cannot be empty" if value.empty?
+            value.freeze
             [value, value]
           else
             raise AttributeError, "Illegal type for #{keys.first}:" \
@@ -40,12 +42,12 @@ def uri keys, required: false
           when ::String
             raise AttributeError, "The #{keys.first} field cannot be empty" if value.empty?
             begin
-              [::URI.parse(value), value]
+              [Utils.deep_freeze(::URI.parse(value)), value.freeze]
             rescue ::URI::InvalidURIError => e
               raise AttributeError, "Illegal format for #{keys.first}: #{e.message}"
             end
           when ::URI::Generic
-            [value, value.to_s]
+            [Utils.deep_freeze(value), value.to_s.freeze]
           else
             raise AttributeError, "Illegal type for #{keys.first}:" \
                                   " String or URI expected but #{value.class} found"
@@ -58,15 +60,15 @@ def rfc3339_date_time keys, required: false
           case value
           when ::String
             begin
-              [::DateTime.rfc3339(value), value]
+              [Utils.deep_freeze(::DateTime.rfc3339(value)), value.freeze]
             rescue ::Date::Error => e
               raise AttributeError, "Illegal format for #{keys.first}: #{e.message}"
             end
           when ::DateTime
-            [value, value.rfc3339]
+            [Utils.deep_freeze(value), value.rfc3339.freeze]
           when ::Time
             value = value.to_datetime
-            [value, value.rfc3339]
+            [Utils.deep_freeze(value), value.rfc3339.freeze]
           else
             raise AttributeError, "Illegal type for #{keys.first}:" \
                                   " String, Time, or DateTime expected but #{value.class} found"
@@ -79,7 +81,7 @@ def content_type keys, required: false
           case value
           when ::String
             raise AttributeError, "The #{keys.first} field cannot be empty" if value.empty?
-            [ContentType.new(value), value]
+            [ContentType.new(value), value.freeze]
           when ContentType
             [value, value.to_s]
           else
@@ -94,6 +96,7 @@ def spec_version keys, accept:
           case value
           when ::String
             raise SpecVersionError, "Unrecognized specversion: #{value}" unless accept =~ value
+            value.freeze
             [value, value]
           else
             raise AttributeError, "Illegal type for #{keys.first}:" \
@@ -102,8 +105,17 @@ def spec_version keys, accept:
         end
       end
 
+      def data_object keys, required: false
+        object keys, required: required, allow_nil: true do |value|
+          Utils.deep_freeze value
+          [value, value]
+        end
+      end
+
       UNDEFINED = ::Object.new.freeze
 
+      private
+
       def object keys, required: false, allow_nil: false
         value = UNDEFINED
         keys.each do |key|
@@ -115,44 +127,10 @@ def object keys, required: false, allow_nil: false
           raise AttributeError, "The #{keys.first} field is required" if required
           return nil
         end
-        if block_given?
-          converted, raw = yield value
-          deep_freeze converted
-        else
-          converted = raw = value
-        end
-        @attributes[keys.first.freeze] = raw.freeze
+        converted, raw = yield value
+        @attributes[keys.first.freeze] = raw
         converted
       end
-
-      private
-
-      def deep_freeze obj
-        case obj
-        when ::Hash
-          obj.each do |key, val|
-            deep_freeze key
-            deep_freeze val
-          end
-        when ::Array
-          obj.each do |val|
-            deep_freeze val
-          end
-        else
-          obj.instance_variables.each do |iv|
-            deep_freeze obj.instance_variable_get iv
-          end
-        end
-        obj.freeze
-      end
-
-      def keys_to_strings hash
-        result = {}
-        hash.each do |key, val|
-          result[key.to_s] = val
-        end
-        result
-      end
     end
   end
 end

lib/cloud_events/event/utils.rb

@@ -0,0 +1,51 @@
+# frozen_string_literal: true
+
+module CloudEvents
+  module Event
+    ##
+    # A variety of helper methods.
+    # @private
+    #
+    module Utils
+      class << self
+        def deep_freeze obj
+          case obj
+          when ::Hash
+            obj.each do |key, val|
+              deep_freeze key
+              deep_freeze val
+            end
+          when ::Array
+            obj.each do |val|
+              deep_freeze val
+            end
+          else
+            obj.instance_variables.each do |iv|
+              deep_freeze obj.instance_variable_get iv
+            end
+          end
+          obj.freeze
+        end
+
+        def deep_dup obj
+          case obj
+          when ::Hash
+            obj.each_with_object({}) { |(key, val), hash| hash[deep_dup key] = deep_dup val }
+          when ::Array
+            obj.map { |val| deep_dup val }
+          else
+            obj.dup
+          end
+        end
+
+        def keys_to_strings hash
+          result = {}
+          hash.each do |key, val|
+            result[key.to_s] = val
+          end
+          result
+        end
+      end
+    end
+  end
+end

lib/cloud_events/event/v0.rb

@@ -3,6 +3,9 @@
 require "date"
 require "uri"
 
+require "cloud_events/event/field_interpreter"
+require "cloud_events/event/utils"
+
 module CloudEvents
   module Event
     ##
@@ -43,7 +46,7 @@ class V0
       #     field.
       #  *  **:type** [`String`] - _required_ - The event `type` field.
       #  *  **:data** [`Object`] - _optional_ - The data associated with the
-      #     event (i.e. the `data` field.)
+      #     event (i.e. the `data` field).
       #  *  **:data_content_encoding** (or **:datacontentencoding**)
       #     [`String`] - _optional_ - The content-encoding for the data (i.e.
       #     the `datacontentencoding` field.)
@@ -74,7 +77,7 @@ def initialize attributes: nil, **args
         @id = interpreter.string ["id"], required: true
         @source = interpreter.uri ["source"], required: true
         @type = interpreter.string ["type"], required: true
-        @data = interpreter.object ["data"], allow_nil: true
+        @data = interpreter.data_object ["data"]
         @data_content_encoding = interpreter.string ["datacontentencoding", "data_content_encoding"]
         @data_content_type = interpreter.content_type ["datacontenttype", "data_content_type"]
         @schema_url = interpreter.uri ["schemaurl", "schema_url"]
@@ -112,6 +115,8 @@ def with **changes
       #     event["time"]     # => String rfc3339 representation
       #     event.time        # => DateTime object
       #
+      # Results are also always frozen and cannot be modified in place.
+      #
       # @param key [String,Symbol] The attribute name.
       # @return [String,nil]
       #
@@ -121,12 +126,12 @@ def [] key
 
       ##
       # Return a hash representation of this event. The returned hash is an
-      # unfrozen copy. Modifications do not affect the original event.
+      # unfrozen deep copy. Modifications do not affect the original event.
       #
       # @return [Hash]
       #
       def to_h
-        @attributes.dup
+        Utils.deep_dup @attributes
       end
 
       ##

lib/cloud_events/event/v1.rb

@@ -3,6 +3,9 @@
 require "date"
 require "uri"
 
+require "cloud_events/event/field_interpreter"
+require "cloud_events/event/utils"
+
 module CloudEvents
   module Event
     ##
@@ -43,7 +46,7 @@ class V1
       #     field.
       #  *  **:type** [`String`] - _required_ - The event `type` field.
       #  *  **:data** [`Object`] - _optional_ - The data associated with the
-      #     event (i.e. the `data` field.)
+      #     event (i.e. the `data` field).
       #  *  **:data_content_type** (or **:datacontenttype**) [`String`,
       #     {ContentType}] - _optional_ - The content-type for the data, if
       #     the data is a string (i.e. the event `datacontenttype` field.)
@@ -71,7 +74,7 @@ def initialize attributes: nil, **args
         @id = interpreter.string ["id"], required: true
         @source = interpreter.uri ["source"], required: true
         @type = interpreter.string ["type"], required: true
-        @data = interpreter.object ["data"], allow_nil: true
+        @data = interpreter.data_object ["data"]
         @data_content_type = interpreter.content_type ["datacontenttype", "data_content_type"]
         @data_schema = interpreter.uri ["dataschema", "data_schema"]
         @subject = interpreter.string ["subject"]
@@ -108,6 +111,8 @@ def with **changes
       #     event["time"]     # => String rfc3339 representation
       #     event.time        # => DateTime object
       #
+      # Results are also always frozen and cannot be modified in place.
+      #
       # @param key [String,Symbol] The attribute name.
       # @return [String,nil]
       #
@@ -117,12 +122,12 @@ def [] key
 
       ##
       # Return a hash representation of this event. The returned hash is an
-      # unfrozen copy. Modifications do not affect the original event.
+      # unfrozen deep copy. Modifications do not affect the original event.
       #
       # @return [Hash]
       #
       def to_h
-        @attributes.dup
+        Utils.deep_dup @attributes
       end
 
       ##

test/event/test_v0.rb

@@ -240,4 +240,24 @@
     assert_nil event[:traceparent]
     refute_includes event.to_h, "traceparent"
   end
+
+  it "returns a deep copy from to_h" do
+    my_data = { "a" => [1, 2, 3, 4] }
+    event = CloudEvents::Event::V0.new id:           my_id,
+                                       source:       my_source_string,
+                                       type:         my_type,
+                                       spec_version: spec_version,
+                                       data:         my_data
+    assert Ractor.shareable? event if defined? Ractor
+
+    data_from_getter = event.data
+    assert_equal my_data, data_from_getter
+    assert data_from_getter.frozen?
+    assert data_from_getter["a"].frozen?
+
+    data_from_hash = event.to_h["data"]
+    assert_equal my_data, data_from_hash
+    refute data_from_hash.frozen?
+    refute data_from_hash["a"].frozen?
+  end
 end

test/event/test_v1.rb

@@ -231,4 +231,24 @@
     assert_nil event[:traceparent]
     refute_includes event.to_h, "traceparent"
   end
+
+  it "returns a deep copy from to_h" do
+    my_data = { "a" => [1, 2, 3, 4] }
+    event = CloudEvents::Event::V1.new id:           my_id,
+                                       source:       my_source_string,
+                                       type:         my_type,
+                                       spec_version: spec_version,
+                                       data:         my_data
+    assert Ractor.shareable? event if defined? Ractor
+
+    data_from_getter = event.data
+    assert_equal my_data, data_from_getter
+    assert data_from_getter.frozen?
+    assert data_from_getter["a"].frozen?
+
+    data_from_hash = event.to_h["data"]
+    assert_equal my_data, data_from_hash
+    refute data_from_hash.frozen?
+    refute data_from_hash["a"].frozen?
+  end
 end