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

Deep symbolize keys when parsing options #312

Merged
merged 1 commit into from
Feb 13, 2018

Conversation

jntullo
Copy link

@jntullo jntullo commented Feb 2, 2018

When parsing the options for creating / updating a request, only :symbolize_keys is being called. However, all of the keys must be symbolized via :deep_symbolize_keys

Related: ManageIQ/manageiq#16891
Fixes #260

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

cc: @lfu @evertmulder

@abellotti
Copy link
Member

LGTM!! should we add a test here ?

@jntullo jntullo force-pushed the deep_symbolize_request branch from bb5bdca to b5f152a Compare February 2, 2018 14:06
@miq-bot
Copy link
Member

miq-bot commented Feb 2, 2018

Checked commit jntullo@b5f152a with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍰

@@ -10,7 +10,7 @@ def self.parse_user(data)

def self.parse_options(data)
raise BadRequestError, "Request is missing options" if data["options"].blank?
data["options"].symbolize_keys
data["options"].deep_symbolize_keys
Copy link
Member

Choose a reason for hiding this comment

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

Can API save the data in the same format as what UI does? Currently the data saved via UI is like this.

---
:src_ids:
- '7000000000029'
:vm_memory: 1024
:disk_add:
- !ruby/hash:ActiveSupport::HashWithIndifferentAccess
  disk_size_in_mb: '1'
  persistent: true
  thin_provisioned: true
  new_controller_type: VirtualLsiLogicController
  dependent: true
  bootable: false
:request_type: :vm_reconfigure

Copy link
Author

Choose a reason for hiding this comment

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

@lfu we could possibly convert to yaml...however, at the moment it is not playing nice:

TypeError: no implicit conversion of Symbol into Integer
from /Users/jilliantullo/code/manageiq/app/models/mixins/miq_request_mixin.rb:4:in `[]'

After looking at it, it appears that the create_request method we are calling is expecting a hash.

Copy link
Member

Choose a reason for hiding this comment

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

@jntullo options is a serialized column. So DB saves the options data in yaml format. I was hoping to keep the data format in DB the same as what UI does. Hash would work if the format can't be exactly the same. Currently all the callers use the options key as a symbol.

@abellotti abellotti added the bug label Feb 13, 2018
@abellotti abellotti self-assigned this Feb 13, 2018
@abellotti
Copy link
Member

Thanks @jntullo for fixing this 👍

@abellotti abellotti merged commit 4e7edaa into ManageIQ:master Feb 13, 2018
@abellotti abellotti modified the milestone: Sprint 79 Ending Feb 12, 2018 Feb 13, 2018
@jntullo jntullo deleted the deep_symbolize_request branch February 14, 2018 19:33
@Fryguy
Copy link
Member

Fryguy commented Apr 13, 2018

@simaishi This is marked fine/yes, but @imtayadeway has opened ManageIQ/manageiq#17300 for backport.

@simaishi
Copy link
Contributor

@imtayadeway Is there a BZ associated with this PR?

@Fryguy
Copy link
Member

Fryguy commented Apr 13, 2018

@imtayadeway closed ManageIQ/manageiq#17300 because I think you can just backport even though it's a split repo. If you need/want it reopened, feel free.

@agrare
Copy link
Member

agrare commented Aug 16, 2018

@agrare
Copy link
Member

agrare commented Aug 16, 2018

@miq-bot remove-label bugzilla needed

@simaishi
Copy link
Contributor

simaishi commented Sep 4, 2018

Fine backport details:

$ git log -1
commit d2e16459393bdd1db0b4533a42c39bf12f6f88dd
Author: Alberto Bellotti <abellotti@users.noreply.github.com>
Date:   Tue Feb 13 10:13:40 2018 -0500

    Merge pull request #312 from jntullo/deep_symbolize_request
    
    Deep symbolize keys when parsing options
    (cherry picked from commit 4e7edaad50c50b266858ec071f0cc60ad127feec)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1623574

simaishi pushed a commit that referenced this pull request Sep 13, 2018
Deep symbolize keys when parsing options
(cherry picked from commit 4e7edaa)

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

Gaprindashvili backport details:

$ git log -1
commit 89e79662805a7c7c8c0cd28c51a124e5448e0111
Author: Alberto Bellotti <abellotti@users.noreply.github.com>
Date:   Tue Feb 13 10:13:40 2018 -0500

    Merge pull request #312 from jntullo/deep_symbolize_request
    
    Deep symbolize keys when parsing options
    (cherry picked from commit 4e7edaad50c50b266858ec071f0cc60ad127feec)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1623573

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.

7 participants