new integration spec for rails 7 exhibiting failure when executing de…

[aflansburg]

New integration spec for Rails 7 to exhibit failure mentioned in #559 when applying deep_transform_keys (or any deep_<op>_keys for that matter on Hashie::Dash or inheriting classes.

[dblock]

Did you expect more failures than this one @aflansburg?

  1) rails #deep_transform_keys sucessfully deep transforms keys
     Failure/Error:
       raise ArgumentError,
             "The property '#{property}' #{self.class.required_properties[property][:message]}"

     ArgumentError:
       The property 'foo_baz' is required for HashieDashKlass.
     # ./lib/hashie/dash.rb:222:in `fail_property_required_error!'
     # ./lib/hashie/dash.rb:214:in `assert_property_set!'
     # ./lib/hashie/dash.rb:209:in `block in assert_required_attributes_set!'
     # ./lib/hashie/dash.rb:208:in `each_key'
     # ./lib/hashie/dash.rb:208:in `assert_required_attributes_set!'
     # ./lib/hashie/dash.rb:183:in `update_attributes!'
     # ./lib/hashie/dash.rb:192:in `initialize_attributes'
     # ./lib/hashie/dash.rb:107:in `initialize'
     # ./spec/integration/rails_7/integration_spec.rb:50:in `block (3 levels) in <top (required)>'
     # ./spec/integration/rails_7/integration_spec.rb:13:in `block (2 levels) in <top (required)>'

I think we should mark that spec pending with a link to the issue, update CHANGELOG saying we added an integration test for Rails 7, and merge this PR so we have it.

That unless you can fix the issue?

[aflansburg]

Did you expect more failures than this one @aflansburg?
that was it!

@dblock

I think we should mark that spec pending with a link to the issue, update CHANGELOG saying we added an integration
test for Rails 7, and merge this PR so we have it.

yeah that makes sense -> 9033f06

That unless you can fix the issue?

short on time atm, but happy to look into it when able

[aflansburg]

@dblock not to muddy the waters with further commentary, but could this have something to do with this change here?

HashWithIndifferentAccess#deep_transform_keys` now returns a `HashWithIndifferentAccess` instead of a `Hash`.

This would also impact #deep_symbolize_keys also mentioned in #559

[michaelherold]

I have looked into the failure here and while the example makes sense from a use perspective, the implementation is weird because Dash keys are always Symbols, but the test is doing deep_transform_keys(&:to_s). This is always invalid for a Dash.

I'm wondering what the correct behavior should be here - the expected behavior of failing to convert the keys, which is consistent with ActiveSupport's behavior? Or implicitly casting the result to a regular Hash, which would allow the method to work but would break the definition of ActiveSupport's method.

The latter would at least allow it to work, albeit inconsistently. The former doesn't make sense though because you wouldn't transform the keys of a Dash; that would make no sense since the entire point of a Dash is that it has coerced and validated keys.

An alternative would be to disable or no-op #deep_stringify_keys, #deep_stringify_keys!, #deep_symbolize_keys, #deep_symbolize_keys!, #deep_transform_keys, and #deep_transform_keys! in Rails 7+.

In fact, this should probably be universal for #transform_keys and #transform_keys! as well, though that would be strictly backward-incompatible.

---

A final alternative, and what I think might make the most sense would be to add a note to the readme for Dash that:

1. Discusses why you don't want to transform keys on a Dash
2. Gives a pattern to use dash.to_h.transform_keys or dash.with_indifferent_access.transform_keys in Rails. This is the only pattern that makes sense for a Dash since Dashes validate their properties

The downside to this approach is that there is undefined behavior for something like #deep_transform_keys since that iterates through an Enumerable that may contain a Dash, which leads to this bug.

There isn't a pretty solution that I see.

[aflansburg]

An alternative would be to disable or no-op #deep_stringify_keys, #deep_stringify_keys!, #deep_symbolize_keys, #deep_symbolize_keys!, #deep_transform_keys, and #deep_transform_keys! in Rails 7+.

TL;DR - needing to use #deep_transform_keys on a Dash is probably an edge case. We really don't need it after digging deeper into our use case (not my code).

@michaelherold - personally, this seems very reasonable. Our use case is building request and response bodies using Hashie::Trash. We then .to_json the hash after doing the deep transforms on the keys. As I was not previously very familiar with Hashie, I now notice in the documentation we can do the basic key transforms we needed using the documented PropertyTranslation and there's really no need to do something like:

    def body
      @body ||= req.to_hash.deep_transform_keys { |key| key.to_s.camelize(:lower).to_sym }.to_json
    end

[dblock]

I made some nitpicky comments. I'd like to merge this, thanks for hanging in here!

[dblock]

Put this back please?

[aflansburg]

My apologies - just circling back to this.

[aflansburg]

03a2217

[dblock]

This is missing a period at the end.

I think the change should be "Added support for Rails 7" because by adding specs we say that we support it. There are bugs, but c'est la vie.

[aflansburg]

03a2217

[dblock]

I think these are global. I would do this to avoid polluting namespaces:

let(:klass) do
   Class.new(Hashie::Dash) do
     property ...
   end
end

[aflansburg]

03a2217

[dblock]

Move pending here, I think we use that pattern elsewhere, so pending "deep transforms keys".

[aflansburg]

d8a8997

[dblock]

Fixed up CHANGELOG, put the issue number in pending spec and merged, thx.