mail icon indicating copy to clipboard operation
mail copied to clipboard

Mail::Message.encoded breaks when Message is round-tripped through YAML

Open alexdowad opened this issue 3 years ago • 3 comments

I'm sorry if this has already been reported; I searched GH issues and didn't find it.

Version: 2.7.1 Repro:

m = Mail::Message.new
m.add_part(Mail::Part.new)
m2 = Mail::Message.from_yaml(m.to_yaml)
m2.encoded

That throws an exception: ArgumentError: wrong number of arguments (given 1, expected 0)

Reason: When Message is converted to YAML, PartsList appears like an Array ([]) when serialized out. When the YAML is read back in, it is deserialized to an Array. Then, line 1885 in message.rb sets @parts to that array. Then, when #encoded is called, it attempts to call #sort! on that Array, which fails (that's on line 133 in body.rb).

Recommended fix: Make PartsList round-trip correctly through YAML. Alternative fix: Convert Array to PartsList in Message.from_yaml.

alexdowad avatar Jul 03 '22 19:07 alexdowad

By the way, if you want me to open a PR with a fix for this problem, I can do that.

alexdowad avatar Jul 03 '22 19:07 alexdowad

It turns out my original analysis was not 100% correct... it's not message.parts which is the problem. Each Mail::Part has a '.body' property, and that also has .parts. It's message.parts[0].body.parts which is an Array instead of Mail::PartsList.

Here is the monkey patch which I am currently using to fix this:

  module ::Mail
    def Part.from_yaml(str)
      decoded = super
      decoded.body.instance_variable_set(:@parts, Mail::PartsList.new(decoded.body.parts))
      decoded
    end
  end

alexdowad avatar Jul 05 '22 13:07 alexdowad

Hmm. It would be nicer if PartsList just emitted the right YAML.

PartsList actually defines a to_yaml method, which explicitly makes it serialize out in array format. If that method is deleted, then because of the use of DelegateClass, #to_yaml gets delegated to the wrapped @parts array, so it still gets serialized out in array format.

I've played with a couple of possible solutions but the use of Delegator (which inherits from BasicObject) here is tripping me up.

alexdowad avatar Jul 05 '22 13:07 alexdowad