fixes #6248 - API V2 return object for POST/PUT/DELETE should not include root node #1518

isratrade · 2014-06-17T13:16:43Z

No description provided.

domcleal · 2014-06-17T13:21:35Z

Won't this change v1 too?

isratrade · 2014-06-17T13:24:17Z

@daviddavis, ping. Can you check if this fix makes this more consistent. The object returned on POST/PUT/DELETE now has no node. I don't think it affects anything on json parameters received or GET request that use RABL templates.

isratrade · 2014-06-17T13:25:04Z

@domcleal, yes, it will change v1. I guess it's a no go.

isratrade · 2014-06-17T13:26:01Z

@daviddavis, this will probably not be merged per @domcleal's comment above.

isratrade · 2014-06-17T13:28:20Z

@daviddavis, there are also several test failures locally caused by this change.

daviddavis · 2014-06-17T13:33:27Z

So, to sum up, I guess our options are:

Leave it as is for now (hopefully we can find a fix in the future)
Change katello to return root nodes for POST/PUT/DELETE calls to be more consistent
Find another solution that only affects v2.

I'm fine with #1 suppose. I think #2 is a bad idea. #3 seems best purely in theory but it's not feasible?

EDIT: Would there be a way to use the show rabl templates for DELETE/PUT/POST requests? I believe this is what we're doing in Katello. This sounds like the best in terms of consistency not only with Katello but also with show requests within foreman.

waldenraines · 2014-06-17T13:36:36Z

Is the third option really not feasible? I'm fine with option 1 or option 3. The second option would be largely destabilizing to katello.

thomasmckay · 2014-06-17T13:44:32Z

we agreed a long time ago to the non-nested format
http://theforeman.org/manuals/1.5/index.html#5.1.3JSONResponseFormatforSingleObjects

isratrade · 2014-06-18T08:40:31Z

app/controllers/api/v2/base_controller.rb

@@ -9,6 +9,11 @@ class BaseController < Api::BaseController
      end

      before_filter :setup_has_many_params, :only => [:create, :update]
+      # override true value defined in config/initializers/wrap_parameters.rb for V2 responses for POST, PUT, DELETE
+      before_filter :except => [:index] do
+        resource_class.include_root_in_json = false


@waldenraines, @daviddavis, I reviewed katello code and see how you are rendering RABL templates for each each action with respond_for_#{action}

Foreman is essentially just doing render :json => @resource for the response for PUT/POST/DELETE

This fix changing resource_class.include_root_in_json = false in Api::V2::BaseController fixes the issue without affecting V1.

In doing so, I noticed the inflection bug with "HostClass" and "HostgroupClass"

If this before_filter is also on the index actions, then several tests fail (dashboard, statistics, autosign, etc). I didn't investigate these failures as they don't affect our consistency in documentation between katello and foreman. All tests pass with :except => [:index]

isratrade · 2014-06-18T10:35:25Z

[test]

daviddavis · 2014-06-18T11:23:00Z

@isratrade thanks for fixing this.

isratrade · 2014-06-18T13:47:05Z

[test] interesting that SettingsControllerTest fails. This patch shouldn't touch it. My assumption is that once Setting.include_root_in_json = false is run by the API v2 controller, it persists for the model and affects everything else including API v1, so this solution won't work. Will need to re-think a new solution.

daviddavis · 2014-06-18T14:03:41Z

@isratrade interesting. what about putting the before filter into api base controller and then set include_root_in_json to true or false based on whether it's v1 or v2 respectively.

daviddavis · 2014-06-18T14:07:54Z

Also, since we're close to the end of the release and because this may cause instability, do we maybe want to wait on merging it?

isratrade · 2014-06-18T14:27:24Z

@domcleal, @daviddavis, this should work for V1 and V2 as expected. All tests pass.

daviddavis · 2014-06-18T16:10:34Z

@isratrade can you update your commit to point to http://projects.theforeman.org/issues/6248? #5880 got accidentally deleted. Thanks.

domcleal · 2014-06-19T13:52:46Z

test/functional/settings_controller_test.rb

@@ -13,7 +13,7 @@ def test_update_valid
    assert :success
    response = ActiveSupport::JSON.decode(@response.body)
    assert !response.empty?
-    assert_equal new_value, response['setting']['value']
+    assert_equal new_value, response['value']


This indicates a break in the UI which uses AJAX to update the saved value when editing settings. Mine's showing a load of AJAX when I change a value now.

I guess the change to the default will affect all UI controllers - do we need to add a before_filter in the ApplicationController so it remains true for them?

isratrade · 2014-06-19T20:24:39Z

app/controllers/application_controller.rb

@@ -13,6 +13,8 @@ class ApplicationController < ActionController::Base
  helper 'layout'
  helper_method :authorizer

+  # ensure include_root_in_json = true
+  before_filter { ActiveRecord::Base.include_root_in_json = true }


added this and re-committed.

domcleal · 2014-06-20T08:03:37Z

app/controllers/api/v2/base_controller.rb

@@ -9,6 +9,9 @@ class BaseController < Api::BaseController
      end

      before_filter :setup_has_many_params, :only => [:create, :update]
+      # ensure include_root_in_json = false in case it was overridden by Api::V1 to true


Perhaps use an around_filter I think, then we can restore it back to the previous value reliably?

@domcleal, that makes sense. good idea. will do.

isratrade · 2014-06-23T14:28:29Z

app/controllers/api/v2/base_controller.rb

@@ -9,6 +9,9 @@ class BaseController < Api::BaseController
      end

      before_filter :setup_has_many_params, :only => [:create, :update]
+      # ensure include_root_in_json = false for V2 only
+      around_filter :disable_json_root


updated PR using around_filter

…ld *not* include root node

domcleal · 2014-06-24T08:27:45Z

Merged as 301e9dc for Foreman 1.6.0, thanks @isratrade.

domcleal added Not yet reviewed and removed Waiting on contributor labels Jun 17, 2014

domcleal added Waiting on contributor and removed Not yet reviewed labels Jun 17, 2014

isratrade reviewed Jun 18, 2014
View reviewed changes

domcleal added Needs re-review and removed Waiting on contributor labels Jun 18, 2014

isratrade changed the title ~~fixes #4181 - API V2 return object for POST/PUT/DELETE should *not* include root node~~ fixes #5080 - API V2 return object for POST/PUT/DELETE should *not* include root node Jun 18, 2014

isratrade changed the title ~~fixes #5080 - API V2 return object for POST/PUT/DELETE should *not* include root node~~ fixes #6248 - API V2 return object for POST/PUT/DELETE should *not* include root node Jun 19, 2014

domcleal reviewed Jun 19, 2014
View reviewed changes

domcleal added Waiting on contributor and removed Needs re-review labels Jun 19, 2014

isratrade reviewed Jun 19, 2014
View reviewed changes

domcleal removed the Waiting on contributor label Jun 20, 2014

domcleal reviewed Jun 20, 2014
View reviewed changes

domcleal added Waiting on contributor and removed Needs testing labels Jun 20, 2014

isratrade reviewed Jun 23, 2014
View reviewed changes

fixes theforeman#6248 - API V2 return object for POST/PUT/DELETE shou…

a598df5

…ld *not* include root node

domcleal added Needs testing and removed Waiting on contributor labels Jun 23, 2014

domcleal closed this Jun 24, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixes #6248 - API V2 return object for POST/PUT/DELETE should not include root node #1518

fixes #6248 - API V2 return object for POST/PUT/DELETE should not include root node #1518

isratrade commented Jun 17, 2014

domcleal commented Jun 17, 2014

isratrade commented Jun 17, 2014

isratrade commented Jun 17, 2014

isratrade commented Jun 17, 2014

isratrade commented Jun 17, 2014

daviddavis commented Jun 17, 2014

waldenraines commented Jun 17, 2014

thomasmckay commented Jun 17, 2014

isratrade Jun 18, 2014

isratrade commented Jun 18, 2014

daviddavis commented Jun 18, 2014

isratrade commented Jun 18, 2014

daviddavis commented Jun 18, 2014

daviddavis commented Jun 18, 2014

isratrade commented Jun 18, 2014

daviddavis commented Jun 18, 2014

domcleal Jun 19, 2014

isratrade Jun 19, 2014

domcleal Jun 20, 2014

isratrade Jun 20, 2014

isratrade Jun 23, 2014

domcleal commented Jun 24, 2014

fixes #6248 - API V2 return object for POST/PUT/DELETE should *not* include root node #1518

fixes #6248 - API V2 return object for POST/PUT/DELETE should *not* include root node #1518

Conversation

isratrade commented Jun 17, 2014

domcleal commented Jun 17, 2014

isratrade commented Jun 17, 2014

isratrade commented Jun 17, 2014

isratrade commented Jun 17, 2014

isratrade commented Jun 17, 2014

daviddavis commented Jun 17, 2014

waldenraines commented Jun 17, 2014

thomasmckay commented Jun 17, 2014

isratrade Jun 18, 2014

Choose a reason for hiding this comment

isratrade commented Jun 18, 2014

daviddavis commented Jun 18, 2014

isratrade commented Jun 18, 2014

daviddavis commented Jun 18, 2014

daviddavis commented Jun 18, 2014

isratrade commented Jun 18, 2014

daviddavis commented Jun 18, 2014

domcleal Jun 19, 2014

Choose a reason for hiding this comment

isratrade Jun 19, 2014

Choose a reason for hiding this comment

domcleal Jun 20, 2014

Choose a reason for hiding this comment

isratrade Jun 20, 2014

Choose a reason for hiding this comment

isratrade Jun 23, 2014

Choose a reason for hiding this comment

domcleal commented Jun 24, 2014

fixes #6248 - API V2 return object for POST/PUT/DELETE should not include root node #1518

fixes #6248 - API V2 return object for POST/PUT/DELETE should not include root node #1518