fix: Fixed a NoMethodError when a format declined to decode an http request (#63)

Signed-off-by: Daniel Azuma <[email protected]>

Author: dazuma

CHANGELOG.md

@@ -2,15 +2,17 @@
 
 ### v0.5.0 / 2021-06-28
 
-This is a significant update that reworks the interface of the JSON formatter and applies fixes to of its payload encoding behavior. There are several breaking changes to internal interfaces, as well as breaking fixes to the encoding/decoding behavior of HttpBinding class, and a few deprecations.
-
-* FIXED: HttpBinding#decode_rack_env in binary content mode now parses JSON content-types and exposes the data attribute as a JSON value. Similarly, encoding in binary content mode serializes a JSON document even if the payload is string-typed. (BREAKING CHANGE)
-* CHANGED: HttpBinding#decode_rack_env raises CloudEvents::NotCloudEventError (rather than returning nil) if given an HTTP request that does not seem to have been intended as a CloudEvent. (BREAKING CHANGE)
+This is a significant update that provides several important spec-related and usability fixes. Some of the behavioral changes are breaking, so to preserve compatibility, new methods were added and old methods deprecated, particularly in the HttpBinding class. Additionally, the formatter interface has been simplified and expanded to support payload formatting.
+
+* CHANGED: Deprecated HttpBinding#decode_rack_env and replaced with HttpBinding#decode_event. (The old method remains for backward compatibility, but will be removed in version 1.0). The new decode_event method has the following differences:
+    * decode_event raises NotCloudEventError (rather than returning nil) if given an HTTP request that does not seem to have been intended as a CloudEvent.
+    * decode_event takes an allow_opaque argument that, when set to true, returns a CloudEvents::Event::Opaque (rather than raising an exception) if given a structured event with a format not known by the SDK. Opaque event objects cannot have their fields inspected, but can be reserialized for retransmission to another event handler.
+    * decode_event in binary content mode now parses JSON content-types and exposes the data attribute as a JSON value.
+* CHANGED: Deprecated the HttpBinding encoding entrypoints (encode_structured_content, encode_batched_content, and encode_binary_content) and replaced with a single encode_event entrypoint that handles all cases via the structured_format argument. (The old methods remain for backward compatibility, but will be removed in version 1.0). In addition, the new encode_event method has the following differences:
+    * encode_event in binary content mode now interprets a string-valued data attribute as a JSON string and serializes it (i.e. wraps it in quotes) if the data_content_type has a JSON format. This is for compatibility with the behavior of the JSON structured mode which always treats the data attribute as a JSON value if the data_content_type indicates JSON.
 * CHANGED: The JsonFormat class interface was reworked to be more generic, combine the structured and batched calls, and add calls to handle data payloads. A Format module has been added to specify the interface used by JsonFormat and future formatters. (Breaking change of internal interfaces)
-* ADDED: It is now possible to tell HttpBinding#decode_rack_env to return an opaque object if the request specifies a format that is not known by the SDK. This opaque object cannot have its fields inspected, but can be reserialized for retransmission to another event handler.
-* CHANGED: If opaque objects are disabled and an input request has an unknown format, HttpBinding#decode_rack_env now raises UnsupportedFormatError instead of HttpContentError. (The old exception name is aliased to the new name for backward compatibility, but is deprecated and will be removed in version 1.0.)
-* CHANGED: If format-driven parsing fails, HttpBinding#decode_rack_env will raise FormatSyntaxError instead of, for example, a JSON-specific error. The lower-level parser error can still be accessed as the "cause".
-* CHANGED: Consolidated HttpBinding encoding entrypoints into one HttpBinding#encode_event call. The older encode_structured_content, encode_batched_content, and encode_binary_content calls are now deprecated.
+* CHANGED: Renamed HttpContentError to UnsupportedFormatError to better reflect the specific issue being reported, and to eliminate the coupling to http. The old name remains aliased to the new name, but is deprecated and will be removed in version 1.0.
+* CHANGED: If format-driven parsing (e.g. parsing a JSON document) fails, a FormatSyntaxError will be raised instead of, for example, a JSON-specific error. The lower-level parser error can still be accessed from the exception's "cause".
 * FIXED: JsonFormat now sets the datacontenttype attribute explicitly to "application/json" if it isn't otherwise set.
 
 ### v0.4.0 / 2021-05-26

lib/cloud_events/http_binding.rb

@@ -28,8 +28,7 @@ class HttpBinding
     def self.default
       @default ||= begin
         http_binding = new
-        json_format = JsonFormat.new
-        http_binding.register_formatter json_format, JSON_FORMAT
+        http_binding.register_formatter JsonFormat.new, JSON_FORMAT
         http_binding.default_encoder_name = JSON_FORMAT
         http_binding
       end
@@ -125,12 +124,12 @@ def register_formatter_methods formatter,
     # @raise [CloudEvents::CloudEventsError] if an event could not be decoded
     #     from the request.
     #
-    def decode_rack_env env, allow_opaque: false, **format_args
+    def decode_event env, allow_opaque: false, **format_args
       content_type_string = env["CONTENT_TYPE"]
       content_type = ContentType.new content_type_string if content_type_string
       content = read_with_charset env["rack.input"], content_type&.charset
-      result = decode_binary_content content, content_type, env
-      result ||= decode_structured_content content, content_type, allow_opaque, **format_args
+      result = decode_binary_content(content, content_type, env) ||
+               decode_structured_content(content, content_type, allow_opaque, **format_args)
       if result.nil?
         content_type_string = content_type_string ? content_type_string.inspect : "not present"
         raise NotCloudEventError, "Content-Type is #{content_type_string}, and CE-SpecVersion is not present"
@@ -168,7 +167,7 @@ def encode_event event, structured_format: false, **format_args
         if event.is_a? ::Array
           raise ::ArgumentError, "Encoding a batch requires structured_format"
         end
-        encode_binary_content event, **format_args
+        encode_binary_content event, legacy_data_encode: false, **format_args
       else
         structured_format = default_encoder_name if structured_format == true
         raise ::ArgumentError, "Format name not specified, and no default is set" unless structured_format
@@ -183,6 +182,31 @@ def encode_event event, structured_format: false, **format_args
       end
     end
 
+    ##
+    # Decode an event from the given Rack environment hash. Following the
+    # CloudEvents spec, this chooses a handler based on the Content-Type of
+    # the request.
+    #
+    # @deprecated Will be removed in version 1.0. Use {#decode_event} instead.
+    #
+    # @param env [Hash] The Rack environment.
+    # @param format_args [keywords] Extra args to pass to the formatter.
+    # @return [CloudEvents::Event] if the request includes a single structured
+    #     or binary event.
+    # @return [Array<CloudEvents::Event>] if the request includes a batch of
+    #     structured events.
+    # @return [nil] if the request does not appear to be a CloudEvent.
+    # @raise [CloudEvents::CloudEventsError] if the request appears to be a
+    #     CloudEvent but decoding failed.
+    #
+    def decode_rack_env env, **format_args
+      content_type_string = env["CONTENT_TYPE"]
+      content_type = ContentType.new content_type_string if content_type_string
+      content = read_with_charset env["rack.input"], content_type&.charset
+      decode_binary_content(content, content_type, env, legacy_data_decode: true) ||
+        decode_structured_content(content, content_type, false, **format_args)
+    end
+
     ##
     # Encode a single event in structured content mode in the given format.
     #
@@ -232,14 +256,28 @@ def encode_batched_content event_batch, format_name, **format_args
     # @param format_args [keywords] Extra args to pass to the formatter.
     # @return [Array(headers,String)]
     #
-    def encode_binary_content event, **format_args
+    def encode_binary_content event, legacy_data_encode: true, **format_args
       headers = {}
       event.to_h.each do |key, value|
         unless ["data", "datacontenttype"].include? key
           headers["CE-#{key}"] = percent_encode value
         end
       end
-      body, content_type = encode_data event.spec_version, event.data, event.data_content_type, **format_args
+      if legacy_data_encode
+        body = event.data
+        content_type = event.data_content_type&.to_s
+        case body
+        when ::String
+          content_type ||= string_content_type body
+        when nil
+          content_type = nil
+        else
+          body = ::JSON.dump body
+          content_type ||= "application/json; charset=#{body.encoding.name.downcase}"
+        end
+      else
+        body, content_type = encode_data event.spec_version, event.data, event.data_content_type, **format_args
+      end
       headers["Content-Type"] = content_type.to_s if content_type
       [headers, body]
     end
@@ -302,7 +340,7 @@ def add_named_formatter collection, formatter, name
     def decode_structured_content content, content_type, allow_opaque, **format_args
       @event_decoders.reverse_each do |decoder|
         result = decoder.decode_event content: content, content_type: content_type, **format_args
-        event = result[:event] || result[:event_batch]
+        event = result[:event] || result[:event_batch] if result
         return event if event
       end
       if content_type&.media_type == "application" &&
@@ -316,13 +354,20 @@ def decode_structured_content content, content_type, allow_opaque, **format_args
     ##
     # Decode an event from the given Rack environment in binary content mode.
     #
-    def decode_binary_content content, content_type, env
+    # TODO: legacy_data_decode is deprecated and can be removed when
+    # decode_rack_env is removed.
+    #
+    def decode_binary_content content, content_type, env, legacy_data_decode: false
       spec_version = env["HTTP_CE_SPECVERSION"]
       return nil unless spec_version
       unless spec_version =~ /^0\.3|1(\.|$)/
         raise SpecVersionError, "Unrecognized specversion: #{spec_version}"
       end
-      data, content_type = decode_data spec_version, content, content_type
+      if legacy_data_decode
+        data = content
+      else
+        data, content_type = decode_data spec_version, content, content_type
+      end
       attributes = { "spec_version" => spec_version, "data" => data }
       attributes["data_content_type"] = content_type if content_type
       omit_names = ["specversion", "spec_version", "data", "datacontenttype", "data_content_type"]

test/test_http_binding.rb

@@ -92,7 +92,7 @@
       "rack.input"   => StringIO.new(my_json_struct_encoded),
       "CONTENT_TYPE" => "application/cloudevents+json"
     }
-    event = http_binding.decode_rack_env env
+    event = http_binding.decode_event env
     assert_equal my_id, event.id
     assert_equal my_source, event.source
     assert_equal my_type, event.type
@@ -112,7 +112,7 @@
       "rack.input"   => StringIO.new(my_json_batch_encoded),
       "CONTENT_TYPE" => "application/cloudevents-batch+json"
     }
-    events = http_binding.decode_rack_env env
+    events = http_binding.decode_event env
     assert_equal 1, events.size
     event = events.first
     assert_equal my_id, event.id
@@ -151,7 +151,7 @@
       "HTTP_CE_SUBJECT"     => my_subject,
       "HTTP_CE_TIME"        => my_time_string
     }
-    event = http_binding.decode_rack_env env
+    event = http_binding.decode_event env
     assert_equal my_id, event.id
     assert_equal my_source, event.source
     assert_equal my_type, event.type
@@ -171,7 +171,7 @@
       "rack.input"   => StringIO.new(my_json_data_struct_encoded),
       "CONTENT_TYPE" => "application/cloudevents+json"
     }
-    event = http_binding.decode_rack_env env
+    event = http_binding.decode_event env
     assert_equal my_id, event.id
     assert_equal my_source, event.source
     assert_equal my_type, event.type
@@ -208,7 +208,7 @@
       "HTTP_CE_SUBJECT"     => my_subject,
       "HTTP_CE_TIME"        => my_time_string
     }
-    event = http_binding.decode_rack_env env
+    event = http_binding.decode_event env
     assert_equal my_id, event.id
     assert_equal my_source, event.source
     assert_equal my_type, event.type
@@ -223,6 +223,43 @@
     assert_equal my_json_data_struct_encoded, body
   end
 
+  it "decodes and re-encodes a binary JSON rack env using deprecated methods" do
+    env = {
+      "rack.input"          => StringIO.new(my_simple_data),
+      "HTTP_CE_ID"          => my_id,
+      "HTTP_CE_SOURCE"      => my_source_string,
+      "HTTP_CE_TYPE"        => my_type,
+      "HTTP_CE_SPECVERSION" => spec_version,
+      "CONTENT_TYPE"        => my_json_content_type_string,
+      "HTTP_CE_DATASCHEMA"  => my_schema_string,
+      "HTTP_CE_SUBJECT"     => my_subject,
+      "HTTP_CE_TIME"        => my_time_string
+    }
+    event = http_binding.decode_rack_env env
+    assert_equal my_id, event.id
+    assert_equal my_source, event.source
+    assert_equal my_type, event.type
+    assert_equal spec_version, event.spec_version
+    assert_equal my_simple_data, event.data
+    assert_equal my_json_content_type, event.data_content_type
+    assert_equal my_schema, event.data_schema
+    assert_equal my_subject, event.subject
+    assert_equal my_time, event.time
+    headers, body = http_binding.encode_binary_content event, sort: true
+    expected_headers = {
+      "CE-id"          => my_id,
+      "CE-source"      => my_source_string,
+      "CE-type"        => my_type,
+      "CE-specversion" => spec_version,
+      "Content-Type"   => my_json_content_type_string,
+      "CE-dataschema"  => my_schema_string,
+      "CE-subject"     => my_subject,
+      "CE-time"        => my_time_string
+    }
+    assert_equal expected_headers, headers
+    assert_equal my_simple_data, body
+  end
+
   it "decodes a binary rack env using an InputWrapper and re-encodes as structured" do
     env = {
       "rack.input"          => Rack::Lint::InputWrapper.new(StringIO.new(my_simple_data)),
@@ -235,7 +272,7 @@
       "HTTP_CE_SUBJECT"     => my_subject,
       "HTTP_CE_TIME"        => my_time_string
     }
-    event = http_binding.decode_rack_env env
+    event = http_binding.decode_event env
     assert_equal my_id, event.id
     assert_equal my_source, event.source
     assert_equal my_type, event.type
@@ -257,7 +294,7 @@
       "HTTP_CE_TYPE"        => my_type,
       "HTTP_CE_SPECVERSION" => spec_version
     }
-    event = http_binding.decode_rack_env env
+    event = http_binding.decode_event env
     assert_equal my_id, event.id
     assert_equal my_source, event.source
     assert_equal my_type, event.type
@@ -289,7 +326,7 @@
       "HTTP_CE_SPECVERSION"  => spec_version,
       "HTTP_CE_TRACECONTEXT" => my_trace_context
     }
-    event = http_binding.decode_rack_env env
+    event = http_binding.decode_event env
     assert_equal my_trace_context, event["tracecontext"]
     headers, body = http_binding.encode_event event
     expected_headers = {
@@ -330,17 +367,17 @@
       "HTTP_CE_TYPE"        => encoded_weird_type,
       "HTTP_CE_SPECVERSION" => spec_version
     }
-    reconstituted_event = http_binding.decode_rack_env env
+    reconstituted_event = http_binding.decode_event env
     assert_equal event, reconstituted_event
   end
 
   it "raises UnsupportedFormatError when a format is not recognized" do
     env = {
       "rack.input"   => StringIO.new(my_json_struct_encoded),
-      "CONTENT_TYPE" => "application/cloudevents+json"
+      "CONTENT_TYPE" => "application/cloudevents+hello"
     }
     assert_raises CloudEvents::UnsupportedFormatError do
-      minimal_http_binding.decode_rack_env env
+      http_binding.decode_event env
     end
   end
 
@@ -350,7 +387,7 @@
       "CONTENT_TYPE" => "application/cloudevents+json"
     }
     error = assert_raises CloudEvents::FormatSyntaxError do
-      http_binding.decode_rack_env env
+      http_binding.decode_event env
     end
     assert_kind_of JSON::ParserError, error.cause
   end
@@ -361,7 +398,7 @@
       "CONTENT_TYPE" => "application/cloudevents-batch+json"
     }
     error = assert_raises CloudEvents::FormatSyntaxError do
-      http_binding.decode_rack_env env
+      http_binding.decode_event env
     end
     assert_kind_of JSON::ParserError, error.cause
   end
@@ -374,16 +411,34 @@
       "HTTP_CE_SPECVERSION" => "0.1"
     }
     assert_raises CloudEvents::SpecVersionError do
-      http_binding.decode_rack_env env
+      http_binding.decode_event env
     end
   end
 
+  it "raises NotCloudEventError when a content-type is not recognized" do
+    env = {
+      "rack.input"   => StringIO.new(my_json_struct_encoded),
+      "CONTENT_TYPE" => "application/json"
+    }
+    assert_raises CloudEvents::NotCloudEventError do
+      http_binding.decode_event env
+    end
+  end
+
+  it "returns nil from the legacy decode method when a content-type is not recognized" do
+    env = {
+      "rack.input"   => StringIO.new(my_json_struct_encoded),
+      "CONTENT_TYPE" => "application/json"
+    }
+    assert_nil http_binding.decode_rack_env env
+  end
+
   it "decodes and re-encodes a structured event using opaque" do
     env = {
       "rack.input"   => StringIO.new(my_json_struct_encoded),
       "CONTENT_TYPE" => "application/cloudevents+json"
     }
-    event = minimal_http_binding.decode_rack_env env, allow_opaque: true
+    event = minimal_http_binding.decode_event env, allow_opaque: true
     assert_kind_of CloudEvents::Event::Opaque, event
     refute event.batch?
     headers, body = minimal_http_binding.encode_event event
@@ -396,7 +451,7 @@
       "rack.input"   => StringIO.new(my_json_batch_encoded),
       "CONTENT_TYPE" => "application/cloudevents-batch+json"
     }
-    event = minimal_http_binding.decode_rack_env env, allow_opaque: true
+    event = minimal_http_binding.decode_event env, allow_opaque: true
     assert_kind_of CloudEvents::Event::Opaque, event
     assert event.batch?
     headers, body = minimal_http_binding.encode_event event