[Extends #787] - Improve json conversion with tests and support for Rails older than 4.1 #812

Ynote · 2017-04-19T14:45:15Z

Closes [Rails 3.2][v6.9.0] Parsing error when fetching ReactOnRails context from script tag #782.
Updates Improve json conversion with tests and support for older Rails 3.x #787 with some tests.

This change is

…into improve-json-safe-and-pretty # Conflicts: # app/helpers/react_on_rails_helper.rb

Ynote

I did not implement the test for Rails 10.0. It was interesting raising the point to implement a better Rails version comparator but I'm not sure it is useful as a test. What is your opinion about this?

Ynote · 2017-04-19T14:49:49Z

spec/dummy/spec/helpers/react_on_rails_helper_spec.rb

+    end
+
+    context 'with Rails 3' do
+      before { allow(Rails).to receive(:version).and_return(3) }


I'm not really sure that mocking the version is the better way to do.

Could it be a good idea to configure Travis to lauch the tests with differents Rails version? I'm just raising the question, I know this imply to extend the tests duration and to change the Travis configuration.

Ynote · 2017-04-19T15:01:30Z

It seems I cannot fetch the upstream to be up-to-date with the master branch.

justin808 · 2017-04-19T18:45:36Z

Please resolve the conflicts a add a test with a Rails version of 10. We can't easily run CI on rails 32. If you want to submit a PR on that, that would be awesome. Contact me if you want advice on how to do that.

justin808 · 2017-04-19T20:30:44Z

A couple comments. I'd like to see the escaping part refactored to it's own class for better testing.

CC: @cheremukhin23 @Ynote @squadette

Reviewed 2 of 3 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

CHANGELOG.md, line 9 at r2 (raw file):

### 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).

@Ynote you can add you as an author as well ;-)

app/helpers/react_on_rails_helper.rb, line 229 at r2 (raw file):

  def json_safe_and_pretty(hash_or_string)
    unless hash_or_string.class.in?([Hash, String])
      raise "#{hash_or_string.class} is unsupported argument class for this method"

list what types are supported in the error message

app/helpers/react_on_rails_helper.rb, line 243 at r2 (raw file):

  def rails_version_less_than(version)
    Gem::Version.new(Rails.version) <= Gem::Version.new(version)

need tests for this this

main issue is that we originally had string comparisons and "10.0" is < "4"

In order to best test private methods like this, I'd like to see this escaping functionality moved to a different ruby class.

That will work best with @squadette's idea to pretty print in some cases. See #760.

spec/dummy/spec/helpers/react_on_rails_helper_spec.rb, line 59 at r2 (raw file):

Previously, Ynote (Little Cheung) wrote…

I'm not really sure that mocking the version is the better way to do.

Could it be a good idea to configure Travis to lauch the tests with differents Rails version? I'm just raising the question, I know this imply to extend the tests duration and to change the Travis configuration.

Yes, but it would take (see #814):

/spec/dummy-rails3 (like spec/dummy, but with Rails 3)
pass env value on travis runs for different ruby versions and ONLY run rails 3 tests (spec/dummy-rails3) and unit tests) for the old ruby version. Don't run /spec/dummy and the generator tests.

Comments from Reviewable

…into improve-json-safe-and-pretty * upstream/master: Release 6.10.0 Update CHANGELOG.md Add ^ to react-helmut (shakacode#805)

Ynote · 2017-04-20T15:41:51Z

Review status: 0 of 7 files reviewed at latest revision, 4 unresolved discussions.

app/helpers/react_on_rails_helper.rb, line 243 at r2 (raw file):

Previously, justin808 (Justin Gordon) wrote…

need tests for this this

main issue is that we originally had string comparisons and "10.0" is < "4"

In order to best test private methods like this, I'd like to see this escaping functionality moved to a different ruby class.

That will work best with @squadette's idea to pretty print in some cases. See #760.

I moved this method into ReactOnRails::Util as it could be used elsewhere too. Is it ok for you?

spec/dummy/spec/helpers/react_on_rails_helper_spec.rb, line 59 at r2 (raw file):

Previously, justin808 (Justin Gordon) wrote…

Yes, but it would take (see #814):

/spec/dummy-rails3 (like spec/dummy, but with Rails 3)

pass env value on travis runs for different ruby versions and ONLY run rails 3 tests (spec/dummy-rails3) and unit tests) for the old ruby version. Don't run /spec/dummy and the generator tests.

Ok, I'll have a look at this later. I'll try to find some time to work on it.

Comments from Reviewable

justin808 · 2017-04-21T07:36:29Z

Wow! @Ynote GREAT JOB!

This is really OUTSTANDING!

Reviewed 7 of 7 files at r3.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

Comments from Reviewable

justin808 · 2017-04-21T07:38:38Z

@Ynote Please be sure that your PR passes CI.

You have many linter errors. Many will be fixed by running rubocop -a.

https://travis-ci.org/shakacode/react_on_rails/jobs/224013592

justin808 · 2017-04-22T08:33:33Z

@Ynote Do you need any help from me to get this one finished up? I want to do one last 6.x release before I merge in the webpacker changes and go to 7.0.0-beta.0

justin808 · 2017-04-22T08:45:44Z

Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

lib/react_on_rails/json_output.rb, line 13 at r3 (raw file):

    def escaped
      return escaped_without_erb_utils if Utils::rails_version_less_than("4")

We need to confirm this is less than 4 and not less than 4.2, per #815 and #816 by @EdmundLeex

Comments from Reviewable

Ynote · 2017-04-22T11:07:07Z

Hi,
I'm sorry, I wasn't available yesterday. I'm gonna have a look at these errors and fix it now.

Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

lib/react_on_rails/json_output.rb, line 13 at r3 (raw file):

Previously, justin808 (Justin Gordon) wrote…

We need to confirm this is less than 4 and not less than 4.2, per #815 and #816 by @EdmundLeex

Ok, I'm gonna change the tested version number.

Comments from Reviewable

Ynote · 2017-04-22T11:38:57Z

Sorry I didn't run rubocop before -_-" It is fixed now! Hope it will be ok for your release.

Review status: 3 of 7 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

lib/react_on_rails/json_output.rb, line 13 at r3 (raw file):

Previously, Ynote (Little Cheung) wrote…

Ok, I'm gonna change the tested version number.

After reading #815 and #816, I suppose what you meant was to confirm this is less than 4.2 and not 4 :)

Comments from Reviewable

coveralls · 2017-04-22T12:03:28Z

Coverage increased (+0.04%) to 98.02% when pulling 460eb37 on KissKissBankBank:improve-json-safe-and-pretty into 1f48e63 on shakacode:master.

EdmundLeex · 2017-04-22T16:46:10Z

Bumping into this because running into this json issue when I tried out the gem in our app.
I have a couple of maybe naive questions.

Why not just reimplement the ERB::Util::json_escape method since its a fairly simple one instead of checking the Rails version? (this seems adds unnecessary performance overhead, mainly in doing Gem.new(...))
Maybe a class method instead of a new JsonOutput class and instantiate an instance every time? Ruby object instantiation is costly and since there is not state changes, seems creating an object is just adding performance overhead. (I brought this up because we are at a stage where performance really matter)

This method is called in every request and maybe multiple times in each request. Think we might want to pay more attention to performance.

justin808 · 2017-04-22T20:45:32Z

@EdmundLeex:

Why not just reimplement the ERB::Util::json_escape method since its a fairly simple one instead of checking the Rails version? (this seems adds unnecessary performance overhead, mainly in doing Gem.new(...))

Suppose an XSS is discovered. We want to leverage what's in the Rails code.

@Ynote, @EdmundLeex: can we do some small memoization, maybe at the class level to address @EdmundLeex's concerns. I agree with him that anything in the rendering cycle should be fairly efficient. That being said. I bet if this were profiled, there would be ZERO difference if we optimized this.

I need to get this merged and shipped this weekend, as I'm about to go beta for 7.0.0

justin808 · 2017-04-22T20:48:34Z

Review status: 3 of 7 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

lib/react_on_rails/json_output.rb, line 13 at r3 (raw file):

Previously, Ynote (Little Cheung) wrote…

After readinghttps://github.com//issues/815 and #816, I suppose what you meant was to confirm this is less than 4.2 and not 4 :)

I'm not 100% sure on the exact version to compare. This sounds reasonable!

lib/react_on_rails/json_output.rb, line 19 at r4 (raw file):

    def escaped
      return escaped_without_erb_utils if Utils.rails_version_less_than("4.2")

@Ynote Let's add a method that encapsulates the 4.2 check and memoize the result.

We also can make the method static, since this class is so simple.

I believe that would address @EdmundLeex concerns.

Comments from Reviewable

justin808 · 2017-04-22T20:48:39Z

Reviewed 4 of 4 files at r4.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

Comments from Reviewable

EdmundLeex · 2017-04-22T20:50:29Z

FYI: 4.1.1 was the first version that corrected this issue.

Ynote · 2017-04-22T22:42:35Z

Thanks @EdmundLeex for your concern about performance!

@justin808, I added the memoization and follow @EdmundLeex's advice using the test on Rails 4.1.1.

Review status: 3 of 7 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

Comments from Reviewable

coveralls · 2017-04-22T22:52:37Z

Coverage increased (+0.05%) to 98.03% when pulling a168047 on KissKissBankBank:improve-json-safe-and-pretty into 1f48e63 on shakacode:master.

EdmundLeex · 2017-04-22T23:54:17Z

lib/react_on_rails/utils.rb

+      @rails_version_less_than ||= {}
+      @rails_version_less_than[version] = begin
+        Gem::Version.new(Rails.version) < Gem::Version.new(version)
+      end


hmm. I believe this block runs every time instead of using the memoized result?

True. I'm gonna fix it, thanks for the review. Can you tell me why my test passes and so, is not correct? https://github.com/shakacode/react_on_rails/pull/812/files#diff-ad2d9ab6a9a22bf3f00405aba4094cbaR38

this is because subject is memoized by rspec.

EdmundLeex · 2017-04-22T23:59:42Z

app/helpers/react_on_rails_helper.rb

+
+    json_value = hash_or_string.is_a?(String) ? hash_or_string : hash_or_string.to_json
+
+    ReactOnRails::JsonOutput.new(json_value).escaped


This is where the performance might suffer because a new JsonOutput object is instantiated each time. Rather, JsonOutput.escape_json(...), with escape_json being a class method can avoid this instantiation.

@EdmundLeex You are right. I'm gonna update it.

coveralls · 2017-04-23T00:38:31Z

Coverage increased (+0.05%) to 98.025% when pulling 06dd57d on KissKissBankBank:improve-json-safe-and-pretty into 1f48e63 on shakacode:master.

coveralls · 2017-04-23T00:38:31Z

Coverage increased (+0.05%) to 98.025% when pulling 06dd57d on KissKissBankBank:improve-json-safe-and-pretty into 1f48e63 on shakacode:master.

coveralls · 2017-04-23T02:00:19Z

Coverage increased (+0.05%) to 98.03% when pulling f5feb91 on KissKissBankBank:improve-json-safe-and-pretty into 1f48e63 on shakacode:master.

justin808 · 2017-04-23T05:37:23Z

Reviewed 3 of 3 files at r6, 2 of 2 files at r7.
Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

lib/react_on_rails/utils.rb, line 39 at r7 (raw file):

    end

    def self.rails_version_less_than_4_1_1

was the bug fixed in 4.1.1? (rather than 4.2?)

Comments from Reviewable

justin808 · 2017-04-23T05:37:41Z

@Ynote Please confirm that we should merge and release.

Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

Comments from Reviewable

Ynote · 2017-04-23T07:03:28Z

I just tested the PR again with my stack. Everything is ok for me. I think we can merge and release.

Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

lib/react_on_rails/utils.rb, line 39 at r7 (raw file):

Previously, justin808 (Justin Gordon) wrote…

was the bug fixed in 4.1.1? (rather than 4.2?)

This is the PR that fixes the eating quotes issue and the issue with \u2028 and \u2029special characters: rails/rails#13073. It was merged in Rails 4.1. So, I think it is ok.

Comments from Reviewable

justin808 · 2017-04-23T09:47:18Z

Thank you @Ynote!

cheremukhin23 and others added 11 commits March 31, 2017 15:03

Improve json_safe_and_pretty method to handle parse error

703e1d7

Add tests to json_safe_and_pretty method

5867246

Add old json escape method for Rails versions less than 4

68044df

Fix rubocop offenses

a3ef532

Merge branch 'master' of https://github.com/shakacode/react_on_rails …

fb697cd

…into improve-json-safe-and-pretty # Conflicts: # app/helpers/react_on_rails_helper.rb

Fixed rubocop offense

80d3dbc

Add json safe and pretty argument class test

358d572

Add rails version less than method

5ad43e8

Merge branch 'master' of https://github.com/shakacode/react_on_rails …

78a1c3c

…into improve-json-safe-and-pretty # Conflicts: # app/helpers/react_on_rails_helper.rb

Update CHANGELOG.md

19722db

Add tests for json encoding helper methods

c521f15

Ynote commented Apr 19, 2017

View reviewed changes

Ynote changed the title ~~Improve json safe and pretty~~ [Extends #787] - Improve json conversion with tests and support for older Rails 3.x Apr 19, 2017

Ynote mentioned this pull request Apr 19, 2017

[Rails 3.2][v6.9.0] Parsing error when fetching ReactOnRails context from script tag #782

Closed

justin808 mentioned this pull request Apr 19, 2017

[Rails 3] Create Rails 3 Unit tests #814

Closed

Ynote added 4 commits April 20, 2017 12:39

Move rails_version_less_than method into Utils module

d190765

Extract json escaping logic into JsonOutput class

633a7a6

Merge branch 'master' of https://github.com/shakacode/react_on_rails …

f78d48f

…into improve-json-safe-and-pretty * upstream/master: Release 6.10.0 Update CHANGELOG.md Add ^ to react-helmut (shakacode#805)

Update CHANGELOG

e7d0597

Fix error message output

21d502a

justin808 mentioned this pull request Apr 21, 2017

use json string #816

Closed

justin808 mentioned this pull request Apr 21, 2017

Improve json conversion with tests and support for older Rails 3.x #787

Closed

Ynote added 3 commits April 22, 2017 13:26

Remove "or equal" logic from rails_version_less_than util method

042f9c7

Use monkey patch json escape from Rails 4.2 instead of Rails 4

01c3926

Fix Rubocop offenses

460eb37

Memoize result for static method rails_version_less_than

a168047

EdmundLeex reviewed Apr 22, 2017

View reviewed changes

Update json escaping method into class methods

06dd57d

Fix rails_version_less_than helper method

f5feb91

Ynote changed the title ~~[Extends #787] - Improve json conversion with tests and support for older Rails 3.x~~ [Extends #787] - Improve json conversion with tests and support for Rails older than 4.1 Apr 23, 2017

justin808 merged commit 6194e31 into shakacode:master Apr 23, 2017

justin808 mentioned this pull request Apr 23, 2017

Putting back in pretty printing of JSON props? #799

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Extends #787] - Improve json conversion with tests and support for Rails older than 4.1 #812

[Extends #787] - Improve json conversion with tests and support for Rails older than 4.1 #812

Ynote commented Apr 19, 2017 •

edited

Loading

Ynote left a comment

Ynote Apr 19, 2017

Ynote commented Apr 19, 2017

justin808 commented Apr 19, 2017

justin808 commented Apr 19, 2017

Ynote commented Apr 20, 2017

justin808 commented Apr 21, 2017

justin808 commented Apr 21, 2017 •

edited

Loading

justin808 commented Apr 22, 2017

justin808 commented Apr 22, 2017

Ynote commented Apr 22, 2017

Ynote commented Apr 22, 2017 •

edited

Loading

coveralls commented Apr 22, 2017 •

edited

Loading

EdmundLeex commented Apr 22, 2017 •

edited

Loading

justin808 commented Apr 22, 2017

justin808 commented Apr 22, 2017

justin808 commented Apr 22, 2017

EdmundLeex commented Apr 22, 2017

Ynote commented Apr 22, 2017

coveralls commented Apr 22, 2017 •

edited

Loading

EdmundLeex Apr 22, 2017

Ynote Apr 23, 2017

EdmundLeex Apr 23, 2017

EdmundLeex Apr 22, 2017

Ynote Apr 23, 2017

coveralls commented Apr 23, 2017

coveralls commented Apr 23, 2017

coveralls commented Apr 23, 2017 •

edited

Loading

justin808 commented Apr 23, 2017

justin808 commented Apr 23, 2017

Ynote commented Apr 23, 2017

justin808 commented Apr 23, 2017


		json_value = hash_or_string.is_a?(String) ? hash_or_string : hash_or_string.to_json

		ReactOnRails::JsonOutput.new(json_value).escaped

[Extends #787] - Improve json conversion with tests and support for Rails older than 4.1 #812

[Extends #787] - Improve json conversion with tests and support for Rails older than 4.1 #812

Conversation

Ynote commented Apr 19, 2017 • edited Loading

Ynote left a comment

Choose a reason for hiding this comment

Ynote Apr 19, 2017

Choose a reason for hiding this comment

Ynote commented Apr 19, 2017

justin808 commented Apr 19, 2017

justin808 commented Apr 19, 2017

Ynote commented Apr 20, 2017

justin808 commented Apr 21, 2017

justin808 commented Apr 21, 2017 • edited Loading

justin808 commented Apr 22, 2017

justin808 commented Apr 22, 2017

Ynote commented Apr 22, 2017

Ynote commented Apr 22, 2017 • edited Loading

coveralls commented Apr 22, 2017 • edited Loading

EdmundLeex commented Apr 22, 2017 • edited Loading

justin808 commented Apr 22, 2017

justin808 commented Apr 22, 2017

justin808 commented Apr 22, 2017

EdmundLeex commented Apr 22, 2017

Ynote commented Apr 22, 2017

coveralls commented Apr 22, 2017 • edited Loading

EdmundLeex Apr 22, 2017

Choose a reason for hiding this comment

Ynote Apr 23, 2017

Choose a reason for hiding this comment

EdmundLeex Apr 23, 2017

Choose a reason for hiding this comment

EdmundLeex Apr 22, 2017

Choose a reason for hiding this comment

Ynote Apr 23, 2017

Choose a reason for hiding this comment

coveralls commented Apr 23, 2017

coveralls commented Apr 23, 2017

coveralls commented Apr 23, 2017 • edited Loading

justin808 commented Apr 23, 2017

justin808 commented Apr 23, 2017

Ynote commented Apr 23, 2017

justin808 commented Apr 23, 2017

Ynote commented Apr 19, 2017 •

edited

Loading

justin808 commented Apr 21, 2017 •

edited

Loading

Ynote commented Apr 22, 2017 •

edited

Loading

coveralls commented Apr 22, 2017 •

edited

Loading

EdmundLeex commented Apr 22, 2017 •

edited

Loading

coveralls commented Apr 22, 2017 •

edited

Loading

coveralls commented Apr 23, 2017 •

edited

Loading