Property named required no longer works

[maxlinc]

I had been using a dash with a :required property. It worked fine in Hashie v3.3.2. It stopped working in v3.4.0.

The issue seems to be that Hashie is trying to define two conflicting required? methods. The first one is the Mash-like behavior to check if the value of the property is truthy that has always been part of Hashie Mash/Dash. The second is to check if a property is required that appears to have been introduced in #252.

The result is a very tricky to debug error:

wrong number of arguments (1 for 0) (ArgumentError)
/Users/Thoughtworker/repos/opensource/hashie/lib/hashie/dash.rb:46:in `block in property'
/Users/Thoughtworker/repos/opensource/hashie/lib/hashie/dash.rb:190:in `assert_property_set!'
/Users/Thoughtworker/repos/opensource/hashie/lib/hashie/dash.rb:185:in `block in assert_required_attributes_set!'
/Users/Thoughtworker/repos/opensource/hashie/lib/hashie/dash.rb:184:in `each_key'
/Users/Thoughtworker/repos/opensource/hashie/lib/hashie/dash.rb:184:in `assert_required_attributes_set!'
/Users/Thoughtworker/repos/opensource/hashie/lib/hashie/dash.rb:107:in `initialize'

Note that a class-level method named required? did exist and should be okay, because the Mash-like methods only exist at the instance level.

I'm hoping to see support for restored for required so I don't have to resort to workarounds to define the Swagger Parameter object in a Dash, but it would probably be good to add a list of reserved names to Dash and to avoid introducing any new instance-level methods. I suggest prefixing all internal methods with some common prefix, but I'm not sure what to use since underscore and double-underscore already have meaning in Hashie.

[maxlinc]

I was able to use reflection to find all the private methods that could conflict with property names. I basically found this via:

class DashWithAllExtensions < Hashie::Dash
  Hashie::Extensions.constants.map do | c |
    include Hashie::Extensions.const_get(c)
  end

  Hashie::Extensions::Dash.constants.map do | c |
    include Hashie::Extensions::Dash.const_get(c)
  end

  Hashie::Extensions::Mash.constants.map do | c |
    include Hashie::Extensions::Mash.const_get(c)
  end

  # Is this not includable?
  # Hashie::Extensions::Parsers.constants.map do | c |
  #   include Hashie::Extensions::Mash.const_get(c)
  # end
end

names_that_are_unnecessarily_reserved = (DashWithAllExtensions.new.private_methods - Object.new.private_methods).map do | name |
  next if name.to_s.start_with? '___'
  name.to_s.gsub(/\!\Z/, '').gsub(/\?\Z/, '').gsub(/=\Z/, '')
end.uniq

The names I found are: coerce_or_init, _deep_find, _deep_find_all, _recursive_merge, _regular_reader, _regular_writer, assert_property_exists, assert_required_attributes_set, assert_property_set, assert_property_required, fail_property_required_error, fail_no_property_error, required, flexibly_convert_to_hash, and _deep_transform_keys_in_object.

It seems unlikely that any of these would be used as property names other than required, but I think it's a good idea to not take away potential property names with private methods. So I'm planning to add a test that enforces a convention where all Hashie::Dash and extension private methods start with a triple underscore. I'm planning to send a PR to enable RSpec zero monkey patching mode first since it'll make it easier to use reflection on Hashie classes without needing to filter out RSpec methods. It's planned to be the default setting in RSpec 4 anyways.

I am also planning to add a test that ensures that any public methods defined by Hashie extensions are reserved and cannot be used as property names, but the MethodAccessWithOverride extension makes that interesting. I'm thinking a good way to do it would allow hash and hashie methods to be overriden by fields, but only after the user sets a flag to indicate they know what they're doing. This way you keep the flexibility but avoid surprises.

property :zip # raises exception saying the property would override a reserved method and may cause unexpected behavior
property :zip, allow_override: true # ok

[michaelherold]

So I'm planning to add a test that enforces a convention where all Hashie::Dash and extension private methods start with a triple underscore.

I like this idea. I picked a double underscore for the MethodOverridingWriter extension, so I think it should be standardized in that extension for sure. Off the top of my head I'm not sure what the other extensions use.

Regarding the MethodAccessWithOverride extension, the discussion in #119 covers the history of the issue. #222 is a related issue as well. I like the idea of a user toggleable override, as you suggest.

[maxlinc]

I have a branch with most of the work done to:

1. Add the tests that check if Hashie is polluting the method namespace.
2. Change protected/private methods to use triple underscores.
3. Add checks for required keywords.

I haven't submitted a PR yet because I'm not really sure about the best way to handle the last item. Apparently Hashie used to have this message:
hashie-2.0.0/lib/hashie/mash.rb:80: warning: redefiningobject_id' may cause serious problems`

That's basically how I feel about using a field like :key or :class, because it could interfere with normal behavior. If I define a field named :key I'm probably not thinking about how it conflicts with Hash/Hashie's own #key?. If I got an error or warning I'd either change the name or would know I need to carefully review code related to that field.

Note: tried it out... I guess key is okay because Hashie::Dash doesn't seem to get Mash's '?' methods, but class is an issue:

class DashWithClass < Hashie::Dash
  property :class
end

DashWithClass.new(class: 'foo')
# SystemStackError: stack level too deep

That would be annoying to find in a large code base, especially since it's tricky to get a stacktrace for SystemStackError.

However, a warning isn't really going to help, and raising an error would be a breaking change since Hashie has allowed that those fields in the past and explicitly allows them with MethodAccessWithOverride. Perhaps it would work if the allow_override option is mixed with Hashie.prevent_accidental_overrides! and a note saying that will be the default behavior in the next major release.

Another related idea is to allow those fields to be defined but without Mash-like methods. So you'd still be allowed to have a field named :class, and you'd be able to use it with coercion, hash initializers, indifferent access, etc., but you would need to read/write exclusively via dash[:class] and dash[:class] = 'foo'.

[Napolskih]

any news?

[dblock]

According to d03759f#commitcomment-10090931 this was fixed? @maxlinc?