Skip to content
This repository

Fix an issue with proc options in child classes #141

Closed
wants to merge 3 commits into from

3 participants

Darcy Laycock Jake Dempsey John Nunemaker
Darcy Laycock
Sutto commented

This fixes an issue with serialization issues when using proc options (e.g. disable the rails query string normalizer) introduced by the use of Marshal.dump

John Nunemaker
Owner

Should we have a test for using dup as well?

Good idea - I'll add one shortly.

Darcy Laycock
Sutto commented

The more I look at this code, the more it perplexes me slightly (e.g. not behaving exactly as expected).

I'm going to take a look and see what I can do to make it clearer before updating this pull request as well.

Jake Dempsey

I have an issue open that prevents the use of debug_output. I added in a temp fix to resolve my specific issue but do you know if your patch resolves that issue? Your last comment said you needed to make some changes before updating the pull request.. just curious if you have had time to look at it more.

Jake Dempsey

Here is the outstanding issue I was referring to: #137

Darcy Laycock
Sutto commented

Indeed it's the same issue - The core issue is using Marshal is a bad way, when you can use dup / clone and check for duplicable objects as an alternative (so it handles it oddly).

The issue I have with the current implementation / why I've been trying to rethink it is that it merges the config every time you access it, which led to even more subtle bugs - the solution being to rework that to be a little less magical. I haven't gotten to look at it yet, will try and get to it today some time.

Darcy Laycock
Sutto commented

@angelo0000 @jnunemaker See above for further updates - I'm still not 100% happy, but it works as intended for the moment.

If you're happy with this for the moment, I'll also work on a version that cleans the method up to be a bit simpler.

John Nunemaker
Owner

Cool.

John Nunemaker
Owner

Any chance you could take a look at this based on master? One of your test cases is failing with master, but I'm not exactly sure what to do about it. Thanks!

Darcy Laycock

Hey John - Will take a look now (sorry for not getting back to it earlier, I still hadn't worked out a good solution).

Darcy Laycock

Just took a look - the failing spec is because it doesn't dup / clone the proc. It should be fine (since that change isn't 100% necessary).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
23 lib/httparty/module_inheritable_attributes.rb
... ... @@ -1,5 +1,22 @@
1 1 module HTTParty
2 2 module ModuleInheritableAttributes #:nodoc:
  3 +
  4 + def self.deep_clone(value)
  5 + if value.is_a?(Hash)
  6 + value.inject({}) do |result, (k, v)|
  7 + result[k] = deep_clone(v)
  8 + result
  9 + end
  10 + elsif value.is_a?(Proc)
  11 + value.dup # We can't serialize procs
  12 + elsif value.respond_to?(:map)
  13 + value.map { |value| deep_clone value }
  14 + else
  15 + # Other objects - use the default marshal dump / load strategy.
  16 + Marshal.load(Marshal.dump(value))
  17 + end
  18 + end
  19 +
3 20 def self.included(base)
4 21 base.extend(ClassMethods)
5 22 end
@@ -18,17 +35,19 @@ def inherited(subclass)
18 35 super
19 36 @mattr_inheritable_attrs.each do |inheritable_attribute|
20 37 ivar = "@#{inheritable_attribute}"
21   - subclass.instance_variable_set(ivar, instance_variable_get(ivar).clone)
  38 + # Initially, set the instance variable value to be a clone of the parents value.
  39 + subclass.instance_variable_set ivar, HTTParty::ModuleInheritableAttributes.deep_clone(instance_variable_get(ivar))
22 40 if instance_variable_get(ivar).respond_to?(:merge)
23 41 method = <<-EOM
24 42 def self.#{inheritable_attribute}
25   - #{ivar} = superclass.#{inheritable_attribute}.merge Marshal.load(Marshal.dump(#{ivar}))
  43 + #{ivar} = (superclass.#{inheritable_attribute}.merge(HTTParty::ModuleInheritableAttributes.deep_clone(#{ivar})))
26 44 end
27 45 EOM
28 46 subclass.class_eval method
29 47 end
30 48 end
31 49 end
  50 +
32 51 end
33 52 end
34 53 end
14 spec/httparty_spec.rb
@@ -539,6 +539,20 @@ def self.name
539 539 @child1.default_options[:headers].should == {'Accept' => 'application/xml'}
540 540 end
541 541
  542 + it "works with lambda values" do
  543 + @child1.default_options[:imaginary_option] = lambda { "This is a new lambda "}
  544 + @child1.default_options[:imaginary_option].should be_a Proc
  545 + end
  546 +
  547 + it 'should dup the proc on the child class' do
  548 + imaginary_option = lambda { "This is a new lambda" }
  549 + @parent.default_options[:imaginary_option] = imaginary_option
  550 + @parent.default_options[:imaginary_option].should be_equal imaginary_option
  551 + @child1.default_options[:imaginary_option]
  552 + @child1.default_options[:imaginary_option].should == imaginary_option
  553 + @child1.default_options[:imaginary_option].should_not be_equal imaginary_option
  554 + end
  555 +
542 556 it "inherits default_cookies from the parent class" do
543 557 @parent.cookies 'type' => 'chocolate_chip'
544 558 @child1.default_cookies.should == {"type" => "chocolate_chip"}

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.