Bug #15409
closedOpenStruct error when attribute is called 'method'
Description
The following error is shown when you try to access an OpenStruct with a property called method:
`method': wrong number of arguments (given 0, expected 1) (ArgumentError)
To replicate:
require 'ostruct'
o = OpenStruct.new({ method: 'get' })
o.method
The expected behavior should be to return 'get'
Updated by Anonymous almost 6 years ago
According to the docs, OpenStruct
uses method_missing
, so it does not redefine existing methods of Object
or BasicObject
.
https://docs.ruby-lang.org/en/trunk/OpenStruct.html
o = OpenStruct.new(
method: 'method',
class: 'class',
display: 'display',
send: 'send',
__id__: '__id__',
frozen?: 'frozen?'
)
o.method #=> ArgumentError (wrong number of arguments (given 0, expected 1))
o.class #=> OpenStruct
o.display #=> #<OpenStruct method="method", class="class", display="display"...
o.send #=> ArgumentError (no method name given)
o.__id__ #=> 7539827944720
o.frozen? #=> false
Updated by mame (Yusuke Endoh) almost 6 years ago
- Status changed from Open to Assigned
- Assignee set to marcandre (Marc-Andre Lafortune)
Yes, the current behavior is intentional. OpenStruct prohibits redefinition of the superclass methods.
However, the current spec that prohibits overwrite is fragile against newly introduced methods to Object class. For example, Object#then is planned to be introduced in Ruby 2.6. It breaks OpenStruct({ :then => 42 })
which worked well in Ruby 2.5.
I considered this issue with some committers, and found two possible solutions:
- Just warn if a specified key name conflicts with any method of Object class. This does not solve the issue itself, but a user can notice the breakage.
- Allow overwrite. This solves the issue. But if a user gives untrusted input as a key of OpenStruct, an attacker might be able to overwrite some basic methods (for example, Object#dup, Object#object_id, etc.), which might lead to a vulnerability of the application. (It would be very rare, I guess, though.)
@marcandre (Marc-Andre Lafortune), a maintainer of OpenStruct, what do you think?
Updated by marcandre (Marc-Andre Lafortune) almost 6 years ago
I don't have a good solution.
OpenStruct
is kind of an anti-pattern. This is even more apparent when adding methods to Object/Kernel
.
I note that Struct
allows overriding builtin methods:
Struct.new(:method, keyword_init: true).new(method: :foo).method # => :foo
I'd be tempted to allow overriding of methods in OpenStruct
for setters (e.g. method=
), but not very confident about it.
Even if we allow overriding like this, this could still yield to potentially unexpected results.
o = OpenStruct.new
o.then # => nil in Ruby 2.5, Enumerator in Ruby 2.6
o.then = :foo # (assuming we allow overriding from setters)
o.then # => :foo
If the object is inited with the data like OpenStruct.new(then: :foo)
, then both versions would at least behave the same.
Updated by shevegen (Robert A. Heiler) almost 6 years ago
I think it should be overridable and a warning could be issued for those methods that would lead to breakage. I don't really use Struct and OpenStruct much at all these days though - I tend to just define what I need via simple classes.
Updated by mame (Yusuke Endoh) almost 6 years ago
OpenStruct
is kind of an anti-pattern.
Completely agreed. I'd like to prohibit the library itself, honestly.
I note that Struct allows overriding builtin methods:
Interesting. I believe no one passes untrusted keys to Struct. But I heard that OpenStruct is used for JSON.
o = OpenStruct.new o.then # => nil in Ruby 2.5, Enumerator in Ruby 2.6
Ah... It's just a design flaw.
Updated by tansaku (Sam Joseph) over 5 years ago
I just encountered this issue trying to pop a json structure into an OpenStruct. It would be great if there was a way to indicate to OpenStruct such that some keywords (e.g. method
) are safe to be overridden in a particular context.
Updated by marcandre (Marc-Andre Lafortune) about 5 years ago
- Related to Bug #12136: OpenStruct.new(format: :bar).send :format # => too few arguments added
Updated by mame (Yusuke Endoh) over 4 years ago
@marcandre (Marc-Andre Lafortune)
I talked with matz about this ticket, and he said it would be good to revert 3bf9b2f0473550caa73468908ac3e18e0f431b85 because the change brought a compatibility issue (#12055, #12136, #15409). The final decision is left to you as you are the maintainer of openstruct. Just for your information.
Updated by marcandre (Marc-Andre Lafortune) about 4 years ago
I opened a PR that resolves this: https://github.com/ruby/ostruct/pull/15
Note that using a field called :method
has never worked with OpenStruct
before; only private methods could be overridden.
The PR does a few things:
- reverts lazy initialization and restores overriding private methods
- allows overriding public methods (as in this issue, for flexibility and compatibility with
Struct
) - creates aliases ending with
!
for public instance methods (e.g.OpenStruct.new.class! # => OpenStruct
) - adds documentation about the caveats of using
OpenStruct
and recommending not using it at all - small refactoring to better handle frozen state
Updated by jeremyevans0 (Jeremy Evans) about 4 years ago
marcandre (Marc-Andre Lafortune) wrote in #note-9:
I opened a PR that resolves this: https://github.com/ruby/ostruct/pull/15
I reviewed the PR and the changes look good to me.
In regards to OpenStruct in general, I agree with @marcandre (Marc-Andre Lafortune) and @mame (Yusuke Endoh) that it is an antipattern. I think it is almost always preferable to use a plain hash. Personally, I would like to move ostruct from default gems to bundled gems, and at some point consider removing it as a bundled gem. The only complicating factor is json has a dependency on ostruct. JSON::GenericObject inherits from OpenStruct, though as far as I can tell, it doesn't seem to be used internally.
Updated by Hanmac (Hans Mackowiak) about 4 years ago
marcandre (Marc-Andre Lafortune) wrote in #note-9:
I opened a PR that resolves this: https://github.com/ruby/ostruct/pull/15
i have seen ruby making problems when using method_missing
without respond_to_missing
so it might cause problems there
Updated by marcandre (Marc-Andre Lafortune) about 4 years ago
Hanmac (Hans Mackowiak) wrote in #note-11:
i have seen ruby making problems when using
method_missing
withoutrespond_to_missing
so it might cause problems there
The PR doesn't change respond_to?
. The respond_to_missing
was added only for lazy initialization, which I'm removing. Otherwise it has always been standard behavior that:
os = OpenStruct.new
os.respond_to?(:foo) # => false
os.foo # => nil
That's in part because responding to :to_ary
, :to_str
, etc., could be problematic.
Updated by marcandre (Marc-Andre Lafortune) about 4 years ago
jeremyevans0 (Jeremy Evans) wrote in #note-10:
marcandre (Marc-Andre Lafortune) wrote in #note-9:
I opened a PR that resolves this: https://github.com/ruby/ostruct/pull/15
I reviewed the PR and the changes look good to me.
Thanks for the quick review 👍. I'll wait a few days and commit if there are no other comments.
I would like to move ostruct from default gems to bundled gems, and at some point consider removing it as a bundled gem.
I don't have a strong opinion on this.
Updated by byroot (Jean Boussier) about 4 years ago
The only complicating factor is json has a dependency on ostruct. JSON::GenericObject inherits from OpenStruct
It is of very questionable usefulness though, so it could probably be deprecated at the same time, or make lazy loaded and make it try to load ostruct
.
Updated by marcandre (Marc-Andre Lafortune) about 4 years ago
- Status changed from Assigned to Closed
Merged.
Updated by mame (Yusuke Endoh) over 3 years ago
- Related to Bug #18032: Openstruct is ~20..25x slower with Ruby 3.0.0 and 3.0.1 compared to earlier versions added