fix payload strip of invalid utf8 sequences from symbols besides strings

[kroky]

This is a fix for invalid utf8 sequences in payload symbols according to #85. Please note we cannot modify the value in-place as there are frozen strings and also we need to convert back to symbol, otherwise half of the specs are failing.

[jondeandres]

@kroky it looks good to me, however I think we can improve the readability of .enforce_valid_utf8 method in this way:

def enforce_valid_utf8(payload)
      normalizer = proc do |object|
        is_symbol = object.is_a?(Symbol)

        # return the object unless it's a symbol or a string. == object.to_s it's a fast way to check strings
        return object unless object == object.to_s || is_symbol

        value = object.to_s

        if value.respond_to? :encode
          encoded_value = value.encode('UTF-8', 'binary', :invalid => :replace, :undef => :replace, :replace => '')
        else
          encoded_value = ::Iconv.conv('UTF-8//IGNORE', 'UTF-8', value)
        end

        is_symbol ? encoded_value.to_sym : encoded_value
      end

      Rollbar::Util.iterate_and_update(payload, normalizer)
    end

What do you think? I've removed some redundant conditions after your changes. The test it's ok for me.

[jondeandres]

@kroky my diff over your changes is the next one, if you prefer to apply it:

diff --git a/lib/rollbar.rb b/lib/rollbar.rb
index 94abf7b..1a989d8 100644
--- a/lib/rollbar.rb
+++ b/lib/rollbar.rb
@@ -617,22 +617,23 @@ module Rollbar
     end

     def enforce_valid_utf8(payload)
-      normalizer = Proc.new do |value|
-        if is_symbol = value.is_a?(Symbol)
-          value = value.to_s
-        end
-        if value.is_a?(String)
-          if value.respond_to? :encode
-            value = value.encode('UTF-8', 'binary', :invalid => :replace, :undef => :replace, :replace => '')
-          else
-            value = ::Iconv.conv('UTF-8//IGNORE', 'UTF-8', value)
-          end
-          value = value.to_sym if is_symbol
+      normalizer = proc do |object|
+        is_symbol = object.is_a?(Symbol)
+
+        return object unless object == object.to_s || is_symbol
+
+        value = object.to_s
+
+        if value.respond_to? :encode
+          encoded_value = value.encode('UTF-8', 'binary', :invalid => :replace, :undef => :replace, :replace => '')
+        else
+          encoded_value = ::Iconv.conv('UTF-8//IGNORE', 'UTF-8', value)
         end
-        value
+
+        is_symbol ? encoded_value.to_sym : encoded_value
       end

-      Rollbar::Util::iterate_and_update(payload, normalizer)
+      Rollbar::Util.iterate_and_update(payload, normalizer)
     end

     def truncate_payload(payload, byte_threshold)

[kroky]

Thanks, @jondeandres. I don't mind if you apply such suggestions directly. I have applied your patch now.

[jondeandres]

Hey, thanks for these changes. Actually it's a simpler method and easier to read, IMHO. Just one thing, the tests are failing cause in the code we are doing return object unless object == object.to_s || is_symbol and we are using proc instead of lambda.

I've checked the change here, https://travis-ci.org/jondeandres/rollbar-gem/builds/36208349, so you just need to replace proc with lambda.

[kroky]

Nice catch :) I have added the change.

[brianr]

Awesome, thanks @kroky and @jondeandres! Will get this out in the next release.