[Extends #787] - Improve json conversion with tests and support for R…

…ails older than 4.1 (#812) * Improve json_safe_and_pretty method to handle parse error * Add tests to json_safe_and_pretty method * Add old json escape method for Rails versions less than 4 * Add json safe and pretty argument class test * Add rails version less than method * Add tests for json encoding helper methods * Move rails_version_less_than method into Utils module * Extract json escaping logic into JsonOutput class * Remove "or equal" logic from rails_version_less_than util method * Use monkey patch json escape from Rails 4.2 instead of Rails 4 * Memoize result for static method rails_version_less_than * Update json escaping method into class methods
shakacode · Apr 23, 2017 · 6194e31 · 6194e31
1 parent 1db5170
commit 6194e31
Show file tree

Hide file tree

Showing 7 changed files with 202 additions and 32 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -5,7 +5,11 @@ Contributors: please follow the recommendations outlined at [keepachangelog.com]
 
 ## [Unreleased]
 
+### Improved
+- Improve json conversion with tests and support for older Rails 3.x. [#787](https://github.com/shakacode/react_on_rails/pull/787) by [cheremukhin23](https://github.com/cheremukhin23) and [Ynote](https://github.com/Ynote).
+
 ## [6.10.0] - 2017-04-13
+
 ### Added
 - Add an ability to return multiple HTML strings in a `Hash` as a result of `react_component` method call. Allows to build `<head>` contents with [React Helmet](https://github.com/nfl/react-helmet). [#800](https://github.com/shakacode/react_on_rails/pull/800) by [udovenko](https://github.com/udovenko).
 

diff --git a/app/helpers/react_on_rails_helper.rb b/app/helpers/react_on_rails_helper.rb
@@ -6,6 +6,7 @@
 require "react_on_rails/prerender_error"
 require "addressable/uri"
 require "react_on_rails/utils"
+require "react_on_rails/json_output"
 
 module ReactOnRailsHelper
   include ReactOnRails::Utils::Required
@@ -224,6 +225,17 @@ def server_render_js(js_expression, options = {})
     # rubocop:enable Style/RaiseArgs
   end
 
+  def json_safe_and_pretty(hash_or_string)
+    unless hash_or_string.class.in?([Hash, String])
+      raise "#{__method__} only accepts String or Hash as argument "\
+            "(#{hash_or_string.class} given)."
+    end
+
+    json_value = hash_or_string.is_a?(String) ? hash_or_string : hash_or_string.to_json
+
+    ReactOnRails::JsonOutput.escape(json_value)
+  end
+
   private
 
   def build_react_component_result_for_server_rendered_string(
@@ -290,26 +302,6 @@ def compose_react_component_html_with_spec_and_console(component_specification_t
     HTML
   end
 
-  def json_safe_and_pretty(hash_or_string)
-    # if Rails.env.development?
-    #   # TODO: for json_safe_and_pretty
-    #   # 1. Add test
-    #   # 2. Add error handler if cannot parse the string with nice message
-    #   # 3. Consider checking that if not a string then a Hash
-    #   hash_value = hash_or_string.is_a?(String) ? JSON.parse(hash_or_string) : hash_or_string
-    #   ERB::Util.json_escape(JSON.pretty_generate(hash_value))
-    # else
-    #
-    # Temp fix given that a hash may contain active record objects and that crashed with the new
-    # code to JSON.pretty_generate
-
-    # If to_json is called on a String, then the quotes are escaped.
-    json_value = hash_or_string.is_a?(String) ? hash_or_string : hash_or_string.to_json
-
-    ERB::Util.json_escape(json_value)
-    # end
-  end
-
   # prepend the rails_context if not yet applied
   def prepend_render_rails_context(render_value)
     return render_value if @rendered_rails_context

diff --git a/lib/react_on_rails/json_output.rb b/lib/react_on_rails/json_output.rb
@@ -0,0 +1,26 @@
+require "active_support/core_ext/string/output_safety"
+
+module ReactOnRails
+  class JsonOutput
+    ESCAPE_REPLACEMENT = {
+      "&" => '\u0026',
+      ">" => '\u003e',
+      "<" => '\u003c',
+      "\u2028" => '\u2028',
+      "\u2029" => '\u2029'
+    }.freeze
+    ESCAPE_REGEXP = /[\u2028\u2029&><]/u
+
+    def self.escape(json)
+      return escape_without_erb_util(json) if Utils.rails_version_less_than_4_1_1
+
+      ERB::Util.json_escape(json)
+    end
+
+    def self.escape_without_erb_util(json)
+      # https://github.com/rails/rails/blob/60257141462137331387d0e34931555cf0720886/activesupport/lib/active_support/core_ext/string/output_safety.rb#L113
+
+      json.to_s.gsub(ESCAPE_REGEXP, ESCAPE_REPLACEMENT)
+    end
+  end
+end
diff --git a/lib/react_on_rails/utils.rb b/lib/react_on_rails/utils.rb
@@ -24,6 +24,22 @@ def self.running_on_windows?
       (/cygwin|mswin|mingw|bccwin|wince|emx/ =~ RUBY_PLATFORM) != nil
     end
 
+    def self.rails_version_less_than(version)
+      @rails_version_less_than ||= {}
+
+      if @rails_version_less_than.key?(version)
+        return @rails_version_less_than[version]
+      end
+
+      @rails_version_less_than[version] = begin
+        Gem::Version.new(Rails.version) < Gem::Version.new(version)
+      end
+    end
+
+    def self.rails_version_less_than_4_1_1
+      rails_version_less_than("4.1.1")
+    end
+
     module Required
       def required(arg_name)
         raise ArgumentError, "#{arg_name} is required"

diff --git a/spec/dummy/spec/helpers/react_on_rails_helper_spec.rb b/spec/dummy/spec/helpers/react_on_rails_helper_spec.rb
@@ -10,24 +10,40 @@
     }
   end
 
-  describe "#sanitized_props_string(props)" do
-    let(:hash) do
-      {
-        hello: "world",
-        free: "of charge",
-        x: "</script><script>alert('foo')</script>"
-      }
-    end
+  let(:hash) do
+    {
+      hello: "world",
+      free: "of charge",
+      x: "</script><script>alert('foo')</script>"
+    }
+  end
 
-    let(:hash_sanitized) do
-      '{"hello":"world","free":"of charge","x":"\\u003c/script\\u003e\\u003cscrip'\
+  let(:hash_sanitized) do
+    '{"hello":"world","free":"of charge","x":"\\u003c/script\\u003e\\u003cscrip'\
       "t\\u003ealert('foo')\\u003c/script\\u003e\"}"
+  end
+
+  let(:hash_unsanitized) do
+    "{\"hello\":\"world\",\"free\":\"of charge\",\"x\":\"</script><script>alert('foo')</script>\"}"
+  end
+
+  describe "#json_safe_and_pretty(hash_or_string)" do
+    it "should raise an error if not hash nor string passed" do
+      expect { helper.json_safe_and_pretty(false) }.to raise_error
+    end
+
+    it "converts a hash to escaped JSON" do
+      escaped_json = helper.json_safe_and_pretty(hash)
+      expect(escaped_json).to eq(hash_sanitized)
     end
 
-    let(:hash_unsanitized) do
-      "{\"hello\":\"world\",\"free\":\"of charge\",\"x\":\"</script><script>alert('foo')</script>\"}"
+    it "converts a string to escaped JSON" do
+      escaped_json = helper.json_safe_and_pretty(hash_unsanitized)
+      expect(escaped_json).to eq(hash_sanitized)
     end
+  end
 
+  describe "#sanitized_props_string(props)" do
     it "converts a hash to JSON and escapes </script>" do
       sanitized = helper.sanitized_props_string(hash)
       expect(sanitized).to eq(hash_sanitized)

diff --git a/spec/react_on_rails/json_output_spec.rb b/spec/react_on_rails/json_output_spec.rb
@@ -0,0 +1,45 @@
+require_relative "spec_helper"
+require "react_on_rails/json_output"
+
+module ReactOnRails
+  describe JsonOutput do
+    let(:hash_value) do
+      {
+        simple: "hello world",
+        special: '<>&\u2028\u2029'
+      }
+    end
+
+    let(:escaped_json) do
+      '{"simple":"hello world","special":"\\u003c\\u003e\\u0026\\\\u2028\\\\u2029"}'
+    end
+
+    shared_examples :escaped_json do
+      it "returns a well-formatted json with escaped characters" do
+        expect(subject).to eq(escaped_json)
+      end
+    end
+
+    describe ".escape" do
+      subject { described_class.escape(hash_value.to_json) }
+
+      context "with Rails version 4.1.1 and higher" do
+        before { allow(Rails).to receive(:version).and_return("4.1.1") }
+
+        it_behaves_like :escaped_json
+      end
+
+      context "with Rails version lower than 4.1.1" do
+        before { allow(Rails).to receive(:version).and_return("4.1.0") }
+
+        it_behaves_like :escaped_json
+      end
+    end
+
+    describe ".escaped_without_erb_utils" do
+      subject { described_class.escape_without_erb_util(hash_value.to_json) }
+
+      it_behaves_like :escaped_json
+    end
+  end
+end
diff --git a/spec/react_on_rails/utils_spec.rb b/spec/react_on_rails/utils_spec.rb
@@ -0,0 +1,71 @@
+require_relative "spec_helper"
+
+module ReactOnRails
+  RSpec.describe Utils do
+    subject { Utils.rails_version_less_than("4") }
+
+    describe ".rails_version_less_than" do
+      before(:each) { Utils.instance_variable_set :@rails_version_less_than, nil }
+
+      context "with Rails 3" do
+        before { allow(Rails).to receive(:version).and_return("3") }
+
+        it { expect(subject).to eq(true) }
+      end
+
+      context "with Rails 3.2" do
+        before { allow(Rails).to receive(:version).and_return("3.2") }
+
+        it { expect(subject).to eq(true) }
+      end
+
+      context "with Rails 4" do
+        before { allow(Rails).to receive(:version).and_return("4") }
+
+        it { expect(subject).to eq(false) }
+      end
+
+      context "with Rails 4.2" do
+        before { allow(Rails).to receive(:version).and_return("4.2") }
+
+        it { expect(subject).to eq(false) }
+      end
+
+      context "with Rails 10.0" do
+        before { allow(Rails).to receive(:version).and_return("10.0") }
+
+        it { expect(subject).to eq(false) }
+      end
+
+      context "called twice" do
+        before do
+          allow(Rails).to receive(:version).and_return("4.2")
+        end
+
+        it "should memoize the result" do
+          2.times { Utils.rails_version_less_than("4") }
+
+          expect(Rails).to have_received(:version).once
+        end
+      end
+    end
+
+    describe ".rails_version_less_than_4_1_1" do
+      subject { Utils.rails_version_less_than_4_1_1 }
+
+      before(:each) { Utils.instance_variable_set :@rails_version_less_than, nil }
+
+      context "with Rails 4.1.0" do
+        before { allow(Rails).to receive(:version).and_return("4.1.0") }
+
+        it { expect(subject).to eq(true) }
+      end
+
+      context "with Rails 4.1.1" do
+        before { allow(Rails).to receive(:version).and_return("4.1.1") }
+
+        it { expect(subject).to eq(false) }
+      end
+    end
+  end
+end