Merge pull request #290 from bugsnag/fix-invalid-truncated-payloads

Preserve field types when truncating large payloads

lib/bugsnag/helpers.rb

@@ -1,24 +1,21 @@
 require 'uri'
+require 'set' unless defined?(Set)
+require 'json' unless defined?(JSON)
+
 
 module Bugsnag
   module Helpers
     MAX_STRING_LENGTH = 4096
+    MAX_PAYLOAD_LENGTH = 128000
+    MAX_ARRAY_LENGTH = 400
 
-    def self.reduce_hash_size(hash)
-      return {} unless hash.is_a?(Hash)
-      hash.inject({}) do |h, (k,v)|
-        if v.is_a?(Hash)
-          h[k] = reduce_hash_size(v)
-        elsif v.is_a?(Array) || v.is_a?(Set)
-          h[k] = v.map {|el| reduce_hash_size(el) }
-        else
-          val = v.to_s
-          val = val.slice(0, MAX_STRING_LENGTH) + "[TRUNCATED]" if val.length > MAX_STRING_LENGTH
-          h[k] = val
-        end
-
-        h
-      end
+    # Trim the size of value if the serialized JSON value is longer than is
+    # accepted by Bugsnag
+    def self.trim_if_needed(value)
+      return value unless payload_too_long?(value)
+      reduced_value = trim_strings_in_value(value)
+      return reduced_value unless payload_too_long?(reduced_value)
+      truncate_arrays_in_value(reduced_value)
     end
 
     def self.flatten_meta_data(overrides)
@@ -31,5 +28,95 @@ def self.flatten_meta_data(overrides)
         overrides
       end
     end
+
+    private
+
+    TRUNCATION_INFO = '[TRUNCATED]'
+    RAW_DATA_TYPES = [Numeric, TrueClass, FalseClass]
+
+    # Shorten array until it fits within the payload size limit when serialized
+    def self.truncate_arrays(array)
+      return [] unless array.respond_to?(:slice)
+      array = array.slice(0, MAX_ARRAY_LENGTH)
+      while array.length > 0 and payload_too_long?(array)
+        array = array.slice(0, array.length - 1)
+      end
+      array
+    end
+
+    # Trim all strings to be less than the maximum allowed string length
+    def self.trim_strings_in_value(value, seen=[])
+      return value if is_json_raw_type?(value)
+      case value
+      when Hash
+        trim_strings_in_hash(value, seen)
+      when Array, Set
+        trim_strings_in_array(value, seen)
+      else
+        trim_as_string(value)
+      end
+    end
+
+    # Validate that the serialized JSON string value is below maximum payload
+    # length
+    def self.payload_too_long?(value)
+      ::JSON.dump(value).length >= MAX_PAYLOAD_LENGTH
+    end
+
+    # Check if a value is a raw type which should not be trimmed, truncated
+    # or converted to a string
+    def self.is_json_raw_type?(value)
+      RAW_DATA_TYPES.detect {|klass| value.is_a?(klass)} != nil
+    end
+
+    def self.trim_strings_in_hash(hash, seen=[])
+      return {} if seen.include?(hash) || !hash.is_a?(Hash)
+      result = hash.each_with_object({}) do |(key, value), reduced_hash|
+        if reduced_value = trim_strings_in_value(value, seen)
+          reduced_hash[key] = reduced_value
+        end
+      end
+      seen << hash
+      result
+    end
+
+    # If possible, convert the provided object to a string and trim to the
+    # maximum allowed string length
+    def self.trim_as_string(text)
+      return "" unless text.respond_to? :to_s
+      text = text.to_s
+      if text.length > MAX_STRING_LENGTH
+        length = MAX_STRING_LENGTH - TRUNCATION_INFO.length
+        text = text.slice(0, length) + TRUNCATION_INFO
+      end
+      text
+    end
+
+    def self.trim_strings_in_array(collection, seen=[])
+      return [] if seen.include?(collection) || !collection.respond_to?(:map)
+      result = collection.map {|value| trim_strings_in_value(value, seen)}
+      seen << collection
+      result
+    end
+
+    def self.truncate_arrays_in_value(value)
+      case value
+      when Hash
+        truncate_arrays_in_hash(value)
+      when Array, Set
+        truncate_arrays(value)
+      else
+        value
+      end
+    end
+
+    def self.truncate_arrays_in_hash(hash)
+      return {} unless hash.is_a?(Hash)
+      hash.each_with_object({}) do |(key, value), reduced_hash|
+        if reduced_value = truncate_arrays_in_value(value)
+          reduced_hash[key] = reduced_value
+        end
+      end
+    end
   end
 end

lib/bugsnag/notification.rb

@@ -39,26 +39,9 @@ class Notification
 
     class << self
       def deliver_exception_payload(url, payload, configuration=Bugsnag.configuration, delivery_method=nil)
-
-        # If the payload is going to be too long, we trim the hashes to send
-        # a minimal payload instead
-        payload_string = ::JSON.dump(payload)
-
-        # Trim the hashes first then..
-        if payload_string.length > MAX_PAYLOAD_LENGTH
-          payload[:events] = payload[:events].map {|e| Bugsnag::Helpers.reduce_hash_size(e)}
-          payload_string = ::JSON.dump(payload)
-
-          #..if the payload is still too long, trim the stack trace
-          if payload_string.length > MAX_PAYLOAD_LENGTH
-            payload[:events].map{ |event| event[:exceptions] }.flatten.each do |exception|
-              exception[:stacktrace] = exception[:stacktrace].slice(0, MAX_STACKTRACE_LENGTH)
-            end
-            payload_string = ::JSON.dump(payload)
-          end
-        end
-
-        Bugsnag::Delivery[delivery_method || configuration.delivery_method].deliver(url, payload_string, configuration)
+        payload_string = ::JSON.dump(Bugsnag::Helpers.trim_if_needed(payload))
+        delivery_method = delivery_method || configuration.delivery_method
+        Bugsnag::Delivery[delivery_method].deliver(url, payload_string, configuration)
       end
     end
 

spec/helper_spec.rb

@@ -1,30 +1,119 @@
 # encoding: utf-8
 
 require 'spec_helper'
+require 'set'
 
 describe Bugsnag::Helpers do
-  it "reduces hash size correctly" do
-    meta_data = {
-      :key_one => "this should not be truncated",
-      :key_two => ""
-    }
 
-    1000.times {|i| meta_data[:key_two] += "this should be truncated " }
+  describe "trim_if_needed" do
 
-    expect(meta_data[:key_two].length).to be > 4096
+    context "payload length is less than allowed" do
 
-    meta_data_return = Bugsnag::Helpers.reduce_hash_size meta_data
+      it "does not change strings" do
+        value = SecureRandom.hex(4096)
+        expect(Bugsnag::Helpers.trim_if_needed(value)).to be value
+      end
 
-    expect(meta_data_return[:key_one].length).to eq(28)
-    expect(meta_data_return[:key_one]).to eq("this should not be truncated")
+      it "does not change arrays" do
+        value = 1000.times.map {|i| "#{i} - #{i + 1}" }
+        expect(Bugsnag::Helpers.trim_if_needed(value)).to be value
+      end
 
-    expect(meta_data_return[:key_two].length).to eq(4107)
-    expect(meta_data_return[:key_two].match(/\[TRUNCATED\]$/).nil?).to eq(false)
+      it "does not change hashes" do
+        value = Hash[*1000.times.map{|i| ["#{i}", i]}.flatten]
+        expect(Bugsnag::Helpers.trim_if_needed(value)).to be value
+      end
+    end
 
-    expect(meta_data[:key_two].length).to be > 4096
-    expect(meta_data[:key_two].match(/\[TRUNCATED\]$/).nil?).to eq(true)
+    context "payload length is greater than allowed" do
 
-    expect(meta_data[:key_one].length).to eq(28)
-    expect(meta_data[:key_one]).to eq("this should not be truncated")
+      context "value is a String" do
+        it "trims length" do
+          value = Bugsnag::Helpers.trim_if_needed(SecureRandom.hex(500_000/2))
+          expect(::JSON.dump(value.length).length).to be < Bugsnag::Helpers::MAX_STRING_LENGTH
+        end
+      end
+
+      context "value is an Array" do
+        it "trims nested string contents" do
+          value = [[30.times.map {|i| SecureRandom.hex(8192) }]]
+          json = ::JSON.dump(Bugsnag::Helpers.trim_if_needed(value))
+          expect(json.length).to be < Bugsnag::Helpers::MAX_PAYLOAD_LENGTH
+        end
+
+        it "trims string contents" do
+          value = 30.times.map {|i| SecureRandom.hex(8192) }
+          json = ::JSON.dump(Bugsnag::Helpers.trim_if_needed(value))
+          expect(json.length).to be < Bugsnag::Helpers::MAX_PAYLOAD_LENGTH
+        end
+      end
+
+      context "value is a Set" do
+        it "trims string contents" do
+          value = Set.new(30.times.map {|i| SecureRandom.hex(8192) })
+          json = ::JSON.dump(Bugsnag::Helpers.trim_if_needed(value))
+          expect(json.length).to be < Bugsnag::Helpers::MAX_PAYLOAD_LENGTH
+        end
+      end
+
+      context "value can be converted to a String" do
+        it "converts to a string and trims" do
+          value = Set.new(30_000.times.map {|i| Bugsnag::Helpers })
+          json = ::JSON.dump(Bugsnag::Helpers.trim_if_needed(value))
+          expect(json.length).to be < Bugsnag::Helpers::MAX_PAYLOAD_LENGTH
+        end
+      end
+
+      context "value is a Hash" do
+
+        before(:each) do
+          @metadata = {
+            :short_string => "this should not be truncated",
+            :long_string => 10000.times.map {|i| "should truncate" }.join(""),
+            :long_string_ary => 30.times.map {|i| SecureRandom.hex(8192) }
+          }
+
+          @trimmed_metadata = Bugsnag::Helpers.trim_if_needed @metadata
+        end
+
+        it "does not trim short values" do
+          expect(@trimmed_metadata[:short_string]).to eq @metadata[:short_string]
+        end
+
+        it "trims long string values" do
+          expect(@trimmed_metadata[:long_string].length).to eq(Bugsnag::Helpers::MAX_STRING_LENGTH)
+          expect(@trimmed_metadata[:long_string].match(/\[TRUNCATED\]$/)).to_not be_nil
+        end
+
+        it "trims nested long string values" do
+          @trimmed_metadata[:long_string_ary].each do |str|
+            expect(str.match(/\[TRUNCATED\]$/)).to_not be_nil
+            expect(str.length).to eq(Bugsnag::Helpers::MAX_STRING_LENGTH)
+          end
+        end
+
+        it "does not change the argument value" do
+          expect(@metadata[:long_string].length).to be > Bugsnag::Helpers::MAX_STRING_LENGTH
+          expect(@metadata[:long_string].match(/\[TRUNCATED\]$/)).to be_nil
+          expect(@metadata[:short_string].length).to eq(28)
+          expect(@metadata[:short_string]).to eq("this should not be truncated")
+          expect(@trimmed_metadata[:long_string_ary].length).to eq(30)
+        end
+      end
+
+      context "and trimmed strings are not enough" do
+        it "truncates long arrays" do
+          value = 100.times.map {|i| SecureRandom.hex(8192) }
+          trimmed_value = Bugsnag::Helpers.trim_if_needed(value)
+          expect(trimmed_value.length).to be > 0
+          trimmed_value.each do |str|
+            expect(str.match(/\[TRUNCATED\]$/)).to_not be_nil
+            expect(str.length).to eq(Bugsnag::Helpers::MAX_STRING_LENGTH)
+          end
+
+          expect(::JSON.dump(trimmed_value).length).to be < Bugsnag::Helpers::MAX_PAYLOAD_LENGTH
+        end
+      end
+    end
   end
 end