Skip to content
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

Fix custom marshalization with symbolize_names: true #476

Merged
merged 1 commit into from
Feb 24, 2021

Conversation

casperisfine
Copy link

Reported to me by @peterthesling.

Repo script

require 'psych'
class Foo
  def initialize(bar)
    @bar = bar
  end
  def init_with(coder)
    @bar = coder["bar"]
  end
  def encode_with(coder)
    coder["bar"] = @bar
  end
end
p Psych.load(Psych.dump(Foo.new(42))) # => #<Foo:0x00007fea51a63d88 @bar=42>
p Psych.load(Psych.dump(Foo.new(42)), symbolize_names: true) # => #<Foo:0x00007fea51a625a0 @bar=nil>

What happens is that the Hash built to be passed to init_with get its keys symbolized, which breaks the init_with.

cc @tenderlove

@casperisfine
Copy link
Author

jruby failures seem unrelated.

Copy link

@peterthesling peterthesling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fast response!

@@ -366,7 +366,7 @@ def revive_hash hash, o
hash[key] = val
end
else
if @symbolize_names
if !force_string_keys && @symbolize_names
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

force_string_keys is a misleading name. If you have a YAML doc that has Symbols, Numerics, Booleans, etc, as keys then setting force_string_keys = true passes those through without modification, it doesn't force them to be strings, it just skips the #to_sym call.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I wasn't very insprired at that time, I think I'll rename it to tagged or something.

@whitehat101
Copy link
Contributor

What's the use case for combining custom init_with and encode_with with Psych.load(..., symbolize_names: true)?

If you have an untyped/untagged document, and you're using YAML as a JSON replacement, symbolize_names: true makes sense since it's mimicking the JSON.load API, and lets you get symbols without any customization.

But if you do have custom init_with and encode_with, that's because you want fine control over how your object is (un)marshaled. If your object has an init_with, are we expecting symbolize_names: true is ignored? Or are we expecting that symbolize_names: true only applies to untagged mappings?

Behavior like this?:

doc = <<~YAML
---
baz: !ruby/object:Foo
  bar: 42
YAML

p Psych.load(doc)
# {"baz"=>#<Foo:0x000056141892d218 @bar=42>}
p Psych.load(doc, symbolize_names: true)
# {:baz=>#<Foo:0x0000561418cefe00 @bar=42>}

Using symbolize_names because you can't or don't want to use symbols in the dumped document?

doc = <<~YAML
---
:baz: !ruby/object:Foo
  bar: 42
YAML
p Psych.load(doc)
# {:baz=>#<Foo:0x0000561418cefe00 @bar=42>}

@casperisfine
Copy link
Author

Or are we expecting that symbolize_names: true only applies to untagged mappings?

Yes, and that was the behavior up to Psych 3.1.0, this PR fixes a regression.

@casperisfine casperisfine force-pushed the symbolize-name-ruby-object branch from b300d87 to ee26f26 Compare February 15, 2021 09:18
@tenderlove tenderlove merged commit 8c3afaa into ruby:master Feb 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants