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

fixes #3693 - API v2 - accept GET json format of object in PUT/POST requests to add/remove has_many associations #1040

Closed
wants to merge 1 commit into from

Conversation

isratrade
Copy link
Member

Currently we have to use _ids= methods to add/remove relationships.

{
  "id": 42,
  "name": "CentOS",
  "architecture_ids": [1,2]
}

This is what we want

{
  "id": 42,
  "name": "CentOS",
  "architectures": [ 
        {
        "id": 1,
        "name": "i386" 
         }, 
        {
         "id": 2,
         "name": "x86_64" 
         }
     ]
}

This DOES NOT update the associated objects (architectures). It's only for adding/removing relationships

If "id" is NOT present and "name" is, then it can add/remove relationships by name

{
  "id": 42,
  "name": "CentOS",
  "architectures": [ 
        {
        "name": "i386" 
         }, 
        {
         "name": "x86_64" 
         }
     ]
}

@komidore64
Copy link
Contributor

looks good

params.each do |k,v|
magic_method_ids = "#{k.singularize}_ids".to_sym
magic_method_names = "#{k.singularize}_names".to_sym
if controller_name.classify.constantize.instance_methods.include?(magic_method_ids) && v.any? && v.all? { |a| a.keys.include?("id") }
Copy link
Contributor

Choose a reason for hiding this comment

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

controller_name.classify.constantize can be replaced with resource_class

@domcleal
Copy link
Contributor

👍 when tests pass :)

klass_name = controller_name.classify
klass = LookupKey if ['SmartVariable', 'SmartClassParameter'].include?(klass_name)
klass ||= LookupValue if klass_name == 'OverrideValue'
klass ||= klass_name.constantize if Object.const_defined?(klass_name)
Copy link
Contributor

Choose a reason for hiding this comment

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

It still looks to me like it should use resource_class and then the smart* controllers (or the shared lookup keys concern) should override it to return LookupKey/Value etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

ping?

@isratrade
Copy link
Member Author

new commit to squash that uses 'resource_class'
test for os passes, but new test for puppetclass does not (and not committed). However, it's not due to the api/v2/base_controller code. I opened http://projects.theforeman.org/issues/3786
I think this can be merged as it works if the associations _ids= work.

@isratrade
Copy link
Member Author

@domcleal, ping. Tests passed!

def setup_has_many_params
params.each do |k,v|
if v.kind_of?(Array)
magic_method_ids = "#{k.singularize}_ids".to_sym
Copy link
Contributor

Choose a reason for hiding this comment

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

This is going to be a security issue, we can't convert arbitrary inputs (parameter names on the request) to symbols as it can cause memory exhaustion.

Can we keep these entirely as strings?

…PUT/POST requests to add/remove has_many associations
@isratrade
Copy link
Member Author

@domcleal, re-committed and changed to_sym to to_s. resource_class.instance_methods returns symbols in Ruby 1.9.3 and strings in 1.8.7 so the array is explicated converted to strings .

@domcleal
Copy link
Contributor

Great, thanks @isratrade, merged as bbf64d9 for Foreman 1.4.0.

@domcleal domcleal closed this Dec 16, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants