Fix for serializing with YAML

[AvaelKross]

Way to reproduce bug with current code:
YAML.load(Faraday::Utils::Headers.new("User-Agent" => "safari").to_yaml)

NoMethodError: undefined method `[]' for nil:NilClass
from /Users/Avael/.rvm/gems/ruby-2.3.0@showmojo/gems/faraday-0.9.2/lib/faraday/utils.rb:48:in `[]='

YAML is a standard tool for serializing, so I guess it should be supported. I believe that this bug can't be fixed outside of Faraday. I found this bug when I tried to serialize Faraday::Response instance for further storing in DB.

[iMacTia]

Hi @AvaelKross,

sorry if I get in the middle, but I just come across your PR and noticed something interesting.
If you look at the error, looks like the instance variable @names is actually nil, but if you look at the code, you'll see that the variable is set in all initialisers to be {}.

I made a quick research, and I found this SO answer: http://stackoverflow.com/a/12922463/4194669
As you can read, the initialize is not called during YAML.load, which cause the @names variable to be nil!

I understand you went for the simplest solution, but I'm not sure it's a proper implementation.
I would suggest you try to implement it as suggested in the SO answer, in the standard way.
Something similar to this inside the Faraday::Utils::Headers class could solve the issue as well:

def encode_with(coder)
  coder['names'] = @names
end

def init_with(coder)
  @names = coder['names']
end

[AvaelKross]

Thanks, @iMacTia ! I added only init_with method and seems like it's enough to fix this issue.

[iMacTia]

Looks strange to be honest, but I don't know how the YAML serialisation/deserialisation works exactly, so happy that it works now :)!

[sbleon]

@AvaelKross please add a test that demonstrates your fix in action. Thanks!

[mislav]

@AvaelKross Do you think you will be able to add a test to this PR? It's the only thing that's preventing it from being merged. Thanks!

[AvaelKross]

Yes, sure. I'll try to find some time later this week.

17 окт. 2016 г., в 21:10, Mislav Marohnić notifications@github.com написал(а):

@AvaelKross https://github.com/AvaelKross Do you think you will be able to add a test to this PR? It's the only thing that's preventing it from being merged. Thanks!

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub #579 (comment), or mute the thread https://github.com/notifications/unsubscribe-auth/ADWcRYpBDSpkM-H1-L8DBUj2u3tcsqpoks5q08gPgaJpZM4IPalH.

[iMacTia]

Hi @AvaelKross, any chance you can add the missing test? I would be happy to add your PR to faraday v0.10.1 release :)

[iMacTia]

Hi @AvaelKross, it would be nice to have your input on this.
I know we were the ones taking too much time to review, but I would love to pack this fix with v0.10.1 if you can add some tests in the next days

[olleolleolle]

Using this naive test, I noted that I got the NoMethodError when missing the #encode_with:

  # YAML parsing
  def test_yaml_roundtrip
    begin
      YAML.load(Faraday::Utils::Headers.new("User-Agent" => "safari").to_yaml)
      assert_equal true, true
    rescue => e
      assert_equal true, false, "Failed when loading YAML: #{e.message}"
    end
  end

Adding the #encode_with made it pass.

Edit: But that does not prove much yet. I'll edit this to something that I can use.

Update: It was possible to add a simpler-yet test.

  # YAML parsing

  def test_headers_yaml_roundtrip
    headers = Faraday::Utils::Headers.new('User-Agent' => 'safari', 'Content-type' => 'text/html')
    result = YAML.load(headers.to_yaml)

    assert result.include?('user-agent'), 'Unable to hydrate to a correct Headers'
    assert result.include?('content-type'), 'Unable to hydrate to a correct Headers'
  end

[iMacTia]

Closing as #634 was merged