HashWithIndifferentAccess causing unexpected behavior

[michaelherold]

When using a ModelFilter with an OmniAuth::AuthHash (from the omniauth gem) as the expected class, the Mutation::Command fails because the auth hash is cast to a HashWithIndifferentAccess.

You can see some examples in this repo. Here's a short example inline as well:

class OmniAuthAuthentication < Mutations::Command
  required do
    model :auth_hash, class: OmniAuth::AuthHash
  end

  def execute
    # Never runs because auth_hash fails the type check
    # auth_hash is cast to a HashWithIndifferentAccess
  end
end

I've started work on a fix that casts all input names to symbols, but it causes a lot of specs to need some tweaks and would not be backwards-compatible. Would such a solution be adequate? It seems like there is not really a need for HashWithIndifferentAccess, since it's overkill for the use that it's being put to.

[anicholson]

Hi @michaelherold, I'm a little confused. Do you mean that an OmniAuth::AuthHash passed to your mutation ends up as a HashWithIndifferentAccess by the time it arrives?

[michaelherold]

Yes, that's correct.

HashWithIndifferentAccess essentially "eats" any other kind of hash and transforms it into another HashWithIndifferentAccess. Thus, when you pass an OmniAuth::AuthHash as a "model" parameter for a mutation, it is eventually cast into a HashWithIndifferentAccess, thus failing the ModelFilter's class check.

This process starts at command.rb:58. If that args hash is kept as a normal Hash, then the first HashFilter (i.e. that filter created at command.rb:47) casts its args to a HashWithIndifferentAccess at hash_filter.rb:86 or hash_filter.rb:90.

The comment on the cast at hash_filter.rb:86 says that "we always want a hash with indifferent access". My question is: why? Is it merely convenience? If so, why don't we just settle on a key-type (i.e. either string or symbol) and cast all of the arg keys as such? It's not clear to me why we need a hash with indifferent access.

[anicholson]

Postel's Law, perhaps? Agree both that it's annoying and that it's a little fiddly to fix.

Hacky I know, but would recreating the AuthHash inside your mutation work? Looks like it contains very little state, so it should work...

[anicholson]

Another cheeky solution that just occurred to me. Utterly untested:

class OauthCommand < Mutations::Command
  def self.input_filters
    OauthFilter.new
  end
end

class OauthFilter
  def filter(data)
    return [data, :hash] unless data.is_a?(OmniAuth::AuthHash)
  end
end

With your original example becoming

class OmniAuthAuthentication < OauthCommand
  required do
    model :auth_hash, class: OmniAuth::AuthHash
  end

  def execute
    # Never runs because auth_hash fails the type check
    # auth_hash is cast to a HashWithIndifferentAccess
  end
end

Although this might only work if you only have one input...

[michaelherold]

Thanks for the replies. The cheeky one got a laugh out of me. 👍

I fixed this specific issue by changing the constraint to a Hash, which works but feels awful hacky to me. I'd rather just be able to say what I mean, so I thought I'd file the bug.

I was experimenting this evening with replacing the HashWithIndifferentAccess bit of Mutations with Hashie's IndifferentAccess mixin. This reduces Mutations' reliance on ActiveSupport to just the string extensions, though it does introduce that new dependency.

The bad thing about this approach is that it requires a lot of rework on the test suite, since the tests rely on the assert_equal some_hash_variable, some_result pattern quite a bit.

Is this something that the maintainers would be interested in? Since it's turning out to be non-trivial, I'd rather not sink more time into it if it's nothing something anyone else would find useful.

[anicholson]

No idea, I'm just a regular punter :)

@cypriss ?

[jkogara]

The same thing occurs when a Hashie::Mash is passed to a duck filter

:0 > duck = Hashie::Mash.new(hello: 'world')
{
    "hello" => "world"
}
:0 > duck.respond_to?(:hello)
true
:0 > class Test < Mutations::Command
:0 *   required do
:0 *     duck :test, methods: %w(hello)
:0 *   end
:0 *   def execute
:0 *   end
:0 * end
:execute
:0 > Test.run!(test: duck)
Mutations::ValidationException: Test is invalid

[michaelherold]

@jkogara The same reasoning applies: by the time test gets through the initial HashFilter, it has been cast to a HashWithIndifferentAccess since a Hashie Mash is a subclass of Hash.