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

Provide an easy way to get NoMethodError instead of nil on undefined keys. #60

Open
myronmarston opened this issue Oct 18, 2012 · 16 comments

Comments

@myronmarston
Copy link

My request is similar to #17. Hashie::Mash, out of the box, doesn't provide a way to distinguish between a fat-fingered/typo'd key and a key that exists but is set to nil. I saw @mbleigh's comment on #17 saying that Hashie::Mash is meant to work like a hash, and since that's how a hash works, that's how a Hashie::Mash works. I get that. The suggested he suggested in that thread works OK for flat hash but falls apart once you start working with nested hashes:

1.9.3p194 :001 > require 'hashie/mash'
 => true 
1.9.3p194 :002 > h = Hashie::Mash.new('a' => { 'b' => 3 }) { raise NoMethodError }
 => #<Hashie::Mash a=#<Hashie::Mash b=3>> 
1.9.3p194 :003 > h.a
 => #<Hashie::Mash b=3> 
1.9.3p194 :004 > h.a.c
 => nil 
1.9.3p194 :005 > h.b
NoMethodError: NoMethodError
    from (irb):2:in `block in irb_binding'
    from /Users/myron/code/interpol/bundle/ruby/1.9.1/gems/hashie-1.2.0/lib/hashie/mash.rb:173:in `yield'
    from /Users/myron/code/interpol/bundle/ruby/1.9.1/gems/hashie-1.2.0/lib/hashie/mash.rb:173:in `default'
    from /Users/myron/code/interpol/bundle/ruby/1.9.1/gems/hashie-1.2.0/lib/hashie/mash.rb:173:in `method_missing'
    from (irb):5
    from /Users/myron/.rvm/rubies/ruby-1.9.3-p194/bin/irb:16:in `<main>'

I came up with a version that does work, but it feels like a lot of code for something that should be simpler:

require 'hashie/mash'

module Interpol
  module DynamicStruct
    DEFAULT_PROC = lambda do |hash, key|
      raise NoMethodError, "undefined method `#{key}' for #{hash.inspect}"
    end 

    def self.new(source)
      hash = Hashie::Mash.new(source)
      recursively_freeze(hash)
      hash
    end 

    def self.recursively_freeze(object)
      case object
        when Array
          object.each { |obj| recursively_freeze(obj) }
        when Hash
          object.default_proc = DEFAULT_PROC
          recursively_freeze(object.values)
      end 
    end 
  end 
end

On top of that, it took me several hours to come up with this working version; I had several previous attempts that did things like subclass Hashie::Mash but I kept getting errors on instantiation that I couldn't work around. This version I've come up with also suffers from a potential perf issue: it has to walk the entire nested hash tree to ensure every level gets this behavior.

Ideally, I'd love if Hashie supported one of these two things out of the box:

# A subclass of `Hashie::Mash` that raises errors on undefined keys.
mash = Hashie::ConservatieMash.new("a" => { "b" => 3 }) # or some better name

# ...or, have `Hashie::Mash` propogate the default proc to all levels of the hash (including to hashes w/in nested arrays)
mash = Hashie::Mash.new("a" => { "b" => 3 }) { raise NoMethodError }
@scottnicolson
Copy link

What about implementing a fetch method that raises a key error much like a hash does? This then stays with the spirit of a hash.

class Hashie::Mash

  def fetch(key)
    raise KeyError, 'key not found' unless self.include?(key)
    self[key]
  end

end

@myronmarston
Copy link
Author

@scottnicolson -- but that wouldn't apply to the hashie dot syntax, would it? I like the dot syntax a lot (e.g. users.first.address.city) and I want a way to make my mash (or mash-like object) raise appropriate errors when used in that fashion.

@myronmarston
Copy link
Author

BTW, for anyone who tries my snippet above: it works on 1.9 but I discovered it didn't work on 1.8.7 because there is no Hash#default_proc= method on 1.8. I found a work around for 1.8 that you can checkout if you're interested in using it.

@jch
Copy link
Contributor

jch commented Feb 12, 2013

I'd be happy to help review a pull if you've come up with a solution you like.

@lukaszx0
Copy link

@myronmarston any developments?

@dblock
Copy link
Member

dblock commented Mar 31, 2014

Bump.

@alex-fedorov
Copy link

👍 for that feature

@dblock
Copy link
Member

dblock commented Oct 25, 2015

I believe the StrictKeyAccess extension in 3.4.3 does this, can someone confirm and close this issue?

@michaelherold
Copy link
Member

@dblock When I was attempting to verify, I discovered that StrictKeyAccess doesn't work with Mash. See the following:

class StrictMash < Hashie::Mash
  include Hashie::Extensions::StrictKeyAccess
end

h = StrictMash.new('a' => {'b' => 3}) #=> Hashie::Extensions::StrictKeyAccess::DefaultError

The reason is because Mash implicitly checks whether #default_proc and #default exist. Those checks for existence raise a DefaultError in the current implementation.

I'm not a big fan of that, but I don't really see a clear way forward. We could make it so accessing #default_proc or #default doesn't raise an error, but I do feel that the current behavior is "correct" because have a default doesn't make sense in strict key access.

We could make a different extension that is only for Mash that is slightly less constraining, but I don't know if that's a good way forward either. What are your thoughts?

@dblock
Copy link
Member

dblock commented Nov 22, 2015

Maybe we should build a StringKeyAccess extension for Mash then that is based on Hashie::Extensions::StrictKeyAccess

@jbbarth
Copy link

jbbarth commented Nov 4, 2017

Pretty old issue but in case somebody needs that too: I tried a few approaches and for now, I'm on a variation of @scottnicolson suggestion that override Mash#method_missing instead:

require "hashie/mash"

class StrictMash < Hashie::Mash
  # Extends Hashie::Mash but raises a KeyError when trying to access a property
  # that doens't exist yet. A naive version of this consists in aliasing `[]` to
  # `fetch` but it makes it impossible to use nice Mash aliases such as `<field>!`,
  # `<field>?`, etc.
  # So instead we override `method_missing` with just a tiny patch inside.
  def method_missing(method_name, *args, &blk)
    return self.[](method_name, &blk) if key?(method_name)
    name, suffix = method_name_and_suffix(method_name)
    # PATCH
    if !key?(name) and !suffix
      raise KeyError, %(key not found: "#{name}")
    end
    # EOF PATCH
    case suffix
    when '='.freeze
      assign_property(name, args.first)
    when '?'.freeze
      !!self[name]
    when '!'.freeze
      initializing_reader(name)
    when '_'.freeze
      underbang_reader(name)
    else
      self[method_name]
    end
  end
end

The nice thing is that it preserves all Mash behaviours, including sub-hashes being StrictMash instances (s.foo!.bar!.meh!.class # => StrictMash).

StrictKeyAccess is actually too strict and will prevent using Mash's "!", "?", etc suffixes from what I saw. I think StrictKeyAccess not working in Mash is a bug in either Mash or StrictKeyAccess though: Mash first argument is a hash to be converted, not a default value, so it shouldn't trigger the DefaultError it triggers right now if you don't pass a default value / block.

If somebody finds the issue interesting enough, I'm up for discussing it and work on a clean PR that could be merged at some point. Else I'll keep my dirty hack, "it works" ;-)

@dblock
Copy link
Member

dblock commented Nov 4, 2017

I find it interesting enough for a PR!

@michaelherold
Copy link
Member

Instead of a module, what would we think of making the handler injectable? Basically, the else case of the case statement is the "default handler". If we make that something you can inject upon creation -- or via an accessor -- it gives an extension point to the Mash itself.

Of course, that increases the responsibilities of an already huge class ... 🤔

@betesh
Copy link

betesh commented Feb 5, 2020

@jbbarth's patch, simplified by using Module.prepend:

Module.new do
  def method_missing(method_name, *args, &blk)
    unless key?(method_name)
      name, suffix = method_name_and_suffix(method_name)
      raise KeyError, %(key not found: "#{name}") unless key?(name) || suffix
    end

    super
  end

  Hashie::Mash.prepend self
end

@dblock
Copy link
Member

dblock commented Jun 11, 2020

This is the oldest issue in Hashie. Can someone PR the version above as an extension? @betesh

@betesh
Copy link

betesh commented Jun 11, 2020

I'm not involved in any projects using hashie at the moment. If you just want someone to open a PR, feel free to use my code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants