fixes #6696 - API v2 - specify the key in which parameters would be wrapped rather than passing model name

[isratrade]

specify :host as key rather than passing class name Host::Base which was incorrectly resolved to params[:base]

[ohadlevy]

@isratrade can we have a test that validates the json output?

[isratrade]

@ohadlevy, this PR deals with json input for PUT and POST. What do you mean by testing the json output? I think all of our functional tests use the root node Ex.

      post :create, { :host => valid_attrs }

The current functional host test ignores wrap_parameters Host::Base. Not sure how to test it. It seems like an isolated unit test. What are your thoughts?

[isratrade]

@ohadlevy, if we change the functional test to

- post :create, { :host => valid_attrs }
+ post :create, { valid_attrs }

then it will test the wrapping, since the controller uses

Host.new(params[:host])

[daviddavis]

@isratrade if you want to test the param wrapping in minitest, you can can set the headers. Here's an example:

https://github.com/Katello/katello/blob/ddbde13b02ba2a50bbd0229475412dfcda23f156/test/controllers/api/v2/content_views_controller_test.rb#L43-L44

[isratrade]

@daviddavis, thanks. I set the headers in PR #1601

[daviddavis]

👍

[domcleal]

@isratrade should this be "fixes #6674" ?

[domcleal]

@isratrade I also see #6696 open, which looks like a dupe of 6674, could you tidy it up please?

[isratrade]

re-committed. Added @request.env['HTTP_ACCEPT'] here as well as password_confirmation to 'api/v2/users_controller.rb' since api v2 tests pass password_confirmation

[isratrade]

@daviddavis, an you please check how this PR would break Katello's products_controller_test

[daviddavis]

@isratrade will do.

[isratrade]

@daviddavis, did you find anything here about the katello test failure?

[daviddavis]

@isratrade no, I put this on hold while looking into nested params. Let me look into it now.

[daviddavis]

The test failure is because now we're sending json for all api requests which is triggering wrap_params and organization_id is a product param so it's not hitting the bad request anymore. Instead, it's continuing on and hitting some dynflow code. I'm tempted to remove the test.

[isratrade]

@daviddavis, good find. So I assume that the katello product tests needs to be updated as all api requests are in json.

[isratrade]

I think this good to merge in Foreman. Any ACK's?

[daviddavis]

Here's the fix for Katello. I can open a PR but I'll probably need a BZ#.

[isratrade]

@daviddavis, looks good, but this should be merged in the katello repo. I think you or somebody else can merge it without a BZ number.

[daviddavis]

[test]

[daviddavis]

Katello/katello#4479 has been merged.

[isratrade]

this can now be merged, now Katello patch has been merged and all tests pass. If there is another ACK, then I'll merge it.

[domcleal]

I agree with @ohadlevy's comment about the need for a test, we should have had it to begin with I guess. A functional test with some unwrapped host parameters and verifying that this results in params[:host] seems right?

[daviddavis]

Wrote some tests:

https://gist.github.com/daviddavis/2277610a6b3e71c67e94

[isratrade]

@daviddavis, it doesn't seem that the tests you show in your gist are testing the wrap_parameters method works as expected. It test that the response is successfully but not that the name of the root node of the post parameters.

[daviddavis]

@isratrade are you sure? They don't pass on develop currently but they do pass on this branch. They aren't directly checking that the params[:host] is getting set but you can't create a host without that.

[daviddavis]

@isratrade to be clear, each test is sending unwrapped params. It's not clear from the tests but valid_attrs is just an unwrapped hash.

[isratrade]

@daviddavis, I understand that the test is sending unwrapped params, but the test is doing assert_response :success which is good, but it does not isolate a test that checks if
{:name => 'special_compute', :provider => 'EC2'}
is converted to
{:compute_resource => {:name => 'special_compute', :provider => 'EC2'} }
By virtue that the response is success, it is assumed that the params[:compute_resource] was created, but it is not an isolated test, that I think @ohadlevy and @domcleal wanted unless I'm mistaken.

[isratrade]

@domcleal, I think this PR can be merged to get this fix it in for wrap_parameters :host so it doesn't resolve to params[:base]'. I will create another PR to addwrap_parameters` to the other controllers to ensure that unwrap PUT/POST params even though it wont be documented.

[isratrade]

This PR had changes to api/v2/compute_resources_controller.rb and api/v2/users_controller.rb but they were moved to PR #1668. This PR only fixes a bug that was introduced in api/v2/hosts_controller.rb that causes incorrect wrapping to params[:base] instead of params[:host]

[isratrade]

[test] don't understand failures to 1.8.7 only.

[isratrade]

All tests were passing. It seems like it's an apipie-rails issue with 0.2.3.
@iNecas , the raw output showed this

rails-0.2.3/lib/apipie/validator.rb:178: syntax error, unexpected ')', expecting '='

rake aborted!
/usr/local/rvm/gems/ruby-1.8.7-p371@test_develop_pr_core-1/gems/apipie-rails-0.2.3/lib/apipie/validator.rb:178: syntax error, unexpected ')', expecting '='
/usr/local/rvm/gems/ruby-1.8.7-p371@test_develop_pr_core-1/gems/apipie-rails-0.2.3/lib/apipie/validator.rb:223: class definition in method body
/usr/local/rvm/gems/ruby-1.8.7-p371@test_develop_pr_core-1/gems/apipie-rails-0.2.3/lib/apipie/validator.rb:245: class definition in method body
/usr/local/rvm/gems/ruby-1.8.7-p371@test_develop_pr_core-1/gems/apipie-rails-0.2.3/lib/apipie/validator.rb:269: class definition in method body
/usr/local/rvm/gems/ruby-1.8.7-p371@test_develop_pr_core-1/gems/apipie-rails-0.2.3/lib/apipie/validator.rb:356: class definition in method body
/usr/local/rvm/gems/ruby-1.8.7-p371@test_develop_pr_core-1/gems/apipie-rails-0.2.3/lib/apipie/validator.rb:373: class definition in method body
/usr/local/rvm/gems/ruby-1.8.7-p371@test_develop_pr_core-1/gems/apipie-rails-0.2.3/lib/apipie/validator.rb:394: class definition in method body
/usr/local/rvm/gems/ruby-1.8.7-p371@test_develop_pr_core-1/gems/apipie-rails-0.2.3/lib/apipie/validator.rb:411: class definition in method body
/usr/local/rvm/gems/ruby-1.8.7-p371@test_develop_pr_core-1/gems/apipie-rails-0.2.3/lib/apipie/validator.rb:453: syntax error, unexpected kEND, expecting $end

[domcleal]

They've been addressed. [test]

[domcleal]

Thanks @isratrade, merged as a2610e9 for Foreman 1.6.0.