New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Eagerload active_support/json/encoding in active_support/core_ext/object/to_json #12203

Merged
merged 1 commit into from Oct 30, 2013

Conversation

Projects
None yet
2 participants
@chancancode
Member

chancancode commented Sep 12, 2013

This is related to #12106. (The root cause for that ticket is that json/add defines Regexp#to_json among others, but here I'll reproduce the problem without json/add.)

Before:

>> require 'active_support/core_ext'
=> true
>> //.as_json
NoMethodError: undefined method `as_json' for //:Regexp
  from (irb):3
  from /Users/godfrey/.rvm/rubies/ruby-2.0.0-p195/bin/irb:16:in `<main>'
>> //.to_json
=> "\"(?-mix:)\""
>> //.as_json
=> "(?-mix:)"

After:

>> require 'active_support/core_ext'
=> true
>> //.as_json
=> "(?-mix:)"

When someone require 'active_support/core_ext', the expectation is that it would add certain methods to the core classes NOW. The previous behaviour causes additional methods to be loaded the first time you call to_json, which could cause nasty surprises.

@chancancode

This comment has been minimized.

Show comment
Hide comment
@chancancode

chancancode Sep 12, 2013

Member

By the way, I can squash the commits if preferred, but since they are not really related I think it's better to keep the isolated.

Member

chancancode commented Sep 12, 2013

By the way, I can squash the commits if preferred, but since they are not really related I think it's better to keep the isolated.

@chancancode

This comment has been minimized.

Show comment
Hide comment
@chancancode

chancancode Sep 12, 2013

Member

/cc @steveklabnik in case you have any objections

Member

chancancode commented Sep 12, 2013

/cc @steveklabnik in case you have any objections

@jeremy

View changes

Show outdated Hide outdated activesupport/lib/active_support/core_ext/object/to_json.rb
@jeremy

This comment has been minimized.

Show comment
Hide comment
@jeremy

jeremy Sep 12, 2013

Member
When someone require 'active_support/core_ext', the expectation is that it would
add certain methods to the core classes NOW. The previous behaviour causes
additional methods to be loaded the first time you call to_json, which could cause
nasty surprises.

Any way to test this?

Member

jeremy commented Sep 12, 2013

When someone require 'active_support/core_ext', the expectation is that it would
add certain methods to the core classes NOW. The previous behaviour causes
additional methods to be loaded the first time you call to_json, which could cause
nasty surprises.

Any way to test this?

@chancancode

This comment has been minimized.

Show comment
Hide comment
@chancancode

chancancode Sep 12, 2013

Member

I was thinking really hard about this and I couldn't come up with any good ideas, any hints? :P

I have some ideas that might work when the test file is run in isolation, but I'm not sure if there are any good ways to test this as part of the suite without relying the order the tests are run/loaded.

Member

chancancode commented Sep 12, 2013

I was thinking really hard about this and I couldn't come up with any good ideas, any hints? :P

I have some ideas that might work when the test file is run in isolation, but I'm not sure if there are any good ways to test this as part of the suite without relying the order the tests are run/loaded.

Moved all JSON core extensions into core_ext/object/json
TL;DR The primary driver is to remove autoload surprise.

This is related to #12106. (The root cause for that ticket is that
json/add defines Regexp#to_json among others, but here I'll reproduce
the problem without json/add.)

Before:

   >> require 'active_support/core_ext/to_json'
   => true
   >> //.as_json
   NoMethodError: undefined method `as_json' for //:Regexp
     from (irb):3
     from /Users/godfrey/.rvm/rubies/ruby-2.0.0-p195/bin/irb:16:in `<main>'
   >> //.to_json
   => "\"(?-mix:)\""
   >> //.as_json
   => "(?-mix:)"

After:

   >> require 'active_support/core_ext/to_json'
   => true
   >> //.as_json
   => "(?-mix:)"

This is because ActiveSupport::JSON is autoloaded the first time
Object#to_json is called, which causes additional core extentions
(previously defined in active_support/json/encoding.rb) to be loaded.

When someone require 'active_support/core_ext', the expectation is
that it would add certain methods to the core classes NOW. The
previous behaviour causes additional methods to be loaded the first
time you call `to_json`, which could cause nasty surprises and other
unplesant side-effects.

This change moves all core extensions in to core_ext/json. AS::JSON is
still autoloaded on first #to_json call, but since it nolonger
include the core extensions, it should address the aforementioned bug.

*Requiring core_ext/object/to_json now causes a deprecation warnning*
@chancancode

This comment has been minimized.

Show comment
Hide comment
@chancancode

chancancode Sep 13, 2013

Member

Sorry @jeremy, I changed my mind again – the final solution I proposed in the inline notes seems like the safest and cleanest solution to fix this, and on second thought the distinction between core_ext/object/to_json and core_ext/object/as_json seemed unnecessary, so I went ahead and moved everything to core_ext/object/json and deprecated core_ext/object/to_json.

With this change, chance of regressing on this same issue seems pretty slim, so I removed the test.

If we want to backport this (I think the bug might be worth fixing in other branches), we can probably just do the same thing minus the deprecation warning. I just didn't feel too good about silencing the circular require warning as I'm unsure of the impact.

This should be ready for merge barring any extra feedback 👍

Member

chancancode commented Sep 13, 2013

Sorry @jeremy, I changed my mind again – the final solution I proposed in the inline notes seems like the safest and cleanest solution to fix this, and on second thought the distinction between core_ext/object/to_json and core_ext/object/as_json seemed unnecessary, so I went ahead and moved everything to core_ext/object/json and deprecated core_ext/object/to_json.

With this change, chance of regressing on this same issue seems pretty slim, so I removed the test.

If we want to backport this (I think the bug might be worth fixing in other branches), we can probably just do the same thing minus the deprecation warning. I just didn't feel too good about silencing the circular require warning as I'm unsure of the impact.

This should be ready for merge barring any extra feedback 👍

@chancancode chancancode referenced this pull request Sep 14, 2013

Merged

JSON encoder refactor #12183

13 of 21 tasks complete

jeremy added a commit that referenced this pull request Oct 30, 2013

Merge pull request #12203 from chancancode/eager_load_json
Eagerload active_support/json/encoding in active_support/core_ext/object/to_json

@jeremy jeremy merged commit dae66a0 into rails:master Oct 30, 2013

1 check failed

default The Travis CI build failed
Details

chancancode added a commit to chancancode/rails that referenced this pull request Nov 7, 2013

Move the JSON extension require statements to the right place.
In #12203, the JSON core extensions were moved into the `core_ext`
folder. Unfortunately, there are some corresponding requires that
were left behind. The problem is partially addressed in #12710, this
commit fixes the rest.

chancancode added a commit that referenced this pull request Jul 12, 2014

Fixed a compatibility issue with the `Oj` gem
`Time#as_json`, `Date#as_json` and `DateTime#as_json` incorrectly depends on a
delegation that is set up in `active_support/json/encoding`. We cannot simply
require that file in `core_ext/object/json` because it would cause a circular
dependency problem (see #12203 for background). We should instead rely on AS's
autoload to load that file for us on-demand.

To trigger autoload correctly, we need to reference the `AS::JSON::Encoding`
constant instead of using the delegated version.

Fixes #16131.

chancancode added a commit that referenced this pull request Jul 26, 2014

Fixed a compatibility issue with the `Oj` gem
`Time#as_json`, `Date#as_json` and `DateTime#as_json` incorrectly depends on a
delegation that is set up in `active_support/json/encoding`. We cannot simply
require that file in `core_ext/object/json` because it would cause a circular
dependency problem (see #12203 for background). We should instead rely on AS's
autoload to load that file for us on-demand.

To trigger autoload correctly, we need to reference the `AS::JSON::Encoding`
constant instead of using the delegated version.

Fixes #16131.

chancancode added a commit that referenced this pull request Jul 28, 2014

Fixed a compatibility issue with the `Oj` gem
`Time#as_json`, `Date#as_json` and `DateTime#as_json` incorrectly depends on a
delegation that is set up in `active_support/json/encoding`. We cannot simply
require that file in `core_ext/object/json` because it would cause a circular
dependency problem (see #12203 for background). We should instead rely on AS's
autoload to load that file for us on-demand.

To trigger autoload correctly, we need to reference the `AS::JSON::Encoding`
constant instead of using the delegated version.

Fixes #16131.

chancancode added a commit that referenced this pull request Jul 29, 2014

Fixed a compatibility issue with the `Oj` gem
`Time#as_json`, `Date#as_json` and `DateTime#as_json` incorrectly depends on a
delegation that is set up in `active_support/json/encoding`. We cannot simply
require that file in `core_ext/object/json` because it would cause a circular
dependency problem (see #12203 for background). We should instead rely on AS's
autoload to load that file for us on-demand.

To trigger autoload correctly, we need to reference the `AS::JSON::Encoding`
constant instead of using the delegated version.

Fixes #16131.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment