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

AC::Parameters no longer being a Hash has broken nested attributes #20922

Closed
twalpole opened this issue Jul 18, 2015 · 9 comments
Closed

AC::Parameters no longer being a Hash has broken nested attributes #20922

twalpole opened this issue Jul 18, 2015 · 9 comments

Comments

@twalpole
Copy link
Contributor

Since AC::Parameters is no longer derived from Hash, and each nested "hash" inside Parameters is also an instance of AC::Parameters, https://github.com/rails/rails/blob/master/activerecord/lib/active_record/nested_attributes.rb#L446 raises an error since it only allows Hash or Array, https://github.com/rails/rails/blob/master/activerecord/lib/active_record/nested_attributes.rb#L446 calls #map which isnt supported on AC::Parameters, and https://github.com/rails/rails/blob/master/activerecord/lib/active_record/nested_attributes.rb#L471 calls #with_indifferent_access which isn't supported on AC::Parameters.

I'm willing to submit a fix if wanted, just wondered whether the preferred solution would be to have AC::Parameters call #to_h on accessed elements so they return filtered hashes rather than AC::Parameters instances, or update the nested_attributes code to support AC::Parameters

@jonatack
Copy link
Contributor

Yes. Experiencing this issue as well.

@jonatack
Copy link
Contributor

Needs test coverage in addition to a fix. The current test suite is passing.

@robin850 robin850 added this to the 5.0.0 milestone Jul 18, 2015
@robin850
Copy link
Member

/cc @sikachu

@sgrif sgrif closed this as completed in 68af636 Jul 18, 2015
@sgrif
Copy link
Contributor

sgrif commented Jul 18, 2015

I've pushed a fix for this problem, and added an isolated test case for it. However I'm going to leave this issue open. This should have never passed CI and I consider our lack of integration test coverage a bug. We need to make sure that we have the tests required to prevent this from happening in the future.

@sgrif sgrif reopened this Jul 18, 2015
@jonatack
Copy link
Contributor

Thank you, Sean, for looking into this! However, with has_many associations the issue still remains.

Hash or Array expected, got ActionController::Parameters 

@twalpole
Copy link
Contributor Author

@sgrif Your fix only fixed this issue for singular associations - PR #20932 continues that for collection associations

@jonatack
Copy link
Contributor

@twalpole Good call. Tried out your PR #20932 and it appears to fix the remaining issue.

@sgrif sgrif removed this from the 5.0.0 milestone Jul 19, 2015
@sgrif
Copy link
Contributor

sgrif commented Jul 19, 2015

I've opened a separate issue for the testing problem.

@sgrif sgrif closed this as completed Jul 19, 2015
@sgrif
Copy link
Contributor

sgrif commented Jul 19, 2015

I forgot the link (#20945)

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

4 participants