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

Don't return a duplicate object from MiqGroup#settings #16572

Merged

Conversation

carbonin
Copy link
Member

Commit c0a662b made the return value of MiqGroup#settings a HashWithIndifferentAccess. Unfortunately, Hash#with_indifferent_access returns a duplicate object.

This behavior breaks callers when they attempt to alter the settings hash in-place, for example:

https://github.com/ManageIQ/manageiq-ui-classic/blob/7d2ef009ac536770b1415ee59fa79c3a01e4d53e/app/controllers/report_controller/menus.rb#L230-L231

This PR changes the #settings method to also set the settings attribute so that it returns the same instance as is set for the group object. Additionally the settings= method had to be changed so that we are consistent with the new type in the column.

This has the same net effect as a data migration and a serialization type change to ActiveSupport::HashWithIndifferentAccess. I'm okay with either route.

/cc @yrudman @skateman

https://bugzilla.redhat.com/show_bug.cgi?id=1516793

This commit ensures that #settings returns the same object that
exists in the object.

This ensures that calls like the following will work as expected:

group.settings[:a] = 1

Previously, #settings returned the duplicate object created by
Hash#with_indifferent_access

https://bugzilla.redhat.com/show_bug.cgi?id=1516793
@carbonin carbonin force-pushed the group_settings_with_indifferent_access branch from b7c89d3 to 57c8d63 Compare November 30, 2017 22:03
@miq-bot
Copy link
Member

miq-bot commented Nov 30, 2017

Checked commits carbonin/manageiq@66f86e4~...57c8d63 with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 👍

Copy link
Member

@skateman skateman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Seal of Approval

@gtanzillo gtanzillo added this to the Sprint 75 Ending Dec 11, 2017 milestone Dec 1, 2017
@gtanzillo gtanzillo merged commit 1ff04a9 into ManageIQ:master Dec 1, 2017
@carbonin carbonin deleted the group_settings_with_indifferent_access branch December 1, 2017 13:44
simaishi pushed a commit that referenced this pull request Dec 1, 2017
…nt_access

Don't return a duplicate object from MiqGroup#settings
(cherry picked from commit 1ff04a9)

https://bugzilla.redhat.com/show_bug.cgi?id=1519858
@simaishi
Copy link
Contributor

simaishi commented Dec 1, 2017

Gaprindashvili backport details:

$ git log -1
commit d6bba5e266134753b52fea120a41a86a4149e8ae
Author: Gregg Tanzillo <gtanzill@redhat.com>
Date:   Fri Dec 1 06:55:14 2017 -0500

    Merge pull request #16572 from carbonin/group_settings_with_indifferent_access
    
    Don't return a duplicate object from MiqGroup#settings
    (cherry picked from commit 1ff04a9ebc3ea848a9be591fac1685c2384f21bb)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1519858

@bdunne
Copy link
Member

bdunne commented Dec 1, 2017

The more I think about this, the more I like the migration route. I don't see any reason we can't still get a simple data migration into gaprindashvili for this. Then we can change to serialize :settings, ActiveSupport::HashWithIndifferentAccess and get rid of all of this tech debt. @Fryguy @chrisarcand Any opinions?

@carbonin
Copy link
Member Author

carbonin commented Dec 1, 2017

@bdunne Yeah, I was hoping to prompt a discussion and possibly make both PRs to compare approaches.

I should have left this as WIP until I could do that ... sorry about that.

I probably won't have time today, but I think we can still go that route and have the PR which changes the settings serialization class just remove the overrides for the settings setter and getter.

Would also like @imtayadeway 's opinion here, he seemed reluctant to go straight to HWIA in the original patch.

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

Successfully merging this pull request may close these issues.

6 participants