Ensure actions are returned correctly in the API #14033

jntullo · 2017-02-22T22:03:53Z

This fixes an issue where subcollection actions were not being returned unless they were defined in the api.yml on the collection itself. As an example, the snapshots collection is defined as:

  :snapshots:
    :description: Snapshots
    :options:
    - :subcollection
    :verbs: *gpd
    :klass: Snapshot

which does not describe any actions. However, on the vms collection, the subcollection is defined as:

    :snapshots_subcollection_actions:
      :get:
      - :name: read
        :identifier: vm_snapshot_view
      :post:
      - :name: create
        :identifier: vm_snapshot_add
      - :name: delete
        :identifier: vm_snapshot_delete
    :snapshots_subresource_actions:
      :get:
      - :name: read
        :identifier: vm_snapshot_view
      :post:
      - :name: delete
        :identifier: vm_snapshot_delete
      :delete:
      - :name: delete
        :identifier: vm_snapshot_delete

This update ensures that subresource and subcollection actions are correctly returned and adds in tests to ensure this case is accounted for.

@miq-bot add_label bug, api
@miq-bot assign @abellotti
cc: @imtayadeway, @AllenBW

abellotti · 2017-03-16T15:39:36Z

hmm, seems to have lost collection level actions with this PR.

abellotti · 2017-03-22T19:55:44Z

ping @jntullo @imtayadeway we really need this fixed.

looked at this a bit, we should be able to extend gen_action_spec_for_collections and gen_action_spec_for_resources without adding new methods to fix this.

for the first, something like:

       def gen_action_spec_for_collections(collection, cspec, is_subcollection, href)
-        target = is_subcollection ? :subcollection_actions : :collection_actions
-        return [] unless cspec[target]
-        cspec[target].each.collect do |method, action_definitions|
+        if is_subcollection
+          target = :subcollection_actions
+          cspec_target = cspec[target] || collection_config.typed_subcollection_actions(@req.collection, collection)
+        else
+          target = :collection_actions
+          cspec_target = cspec[target]
+        end
+        return [] unless cspec_target
+        cspec_target.each.collect do |method, action_definitions|

and some variant for the second method.

abellotti

thanks @jntullo for updating this, a couple of question and need a new test.

abellotti · 2017-03-23T19:35:58Z

app/controllers/api/base_controller/renderer.rb

          next unless render_actions_for_method(cspec[:verbs], method)
          typed_action_definitions = fetch_typed_subcollection_actions(method, is_subcollection) || action_definitions
          typed_action_definitions.each.collect do |action|
            if !action[:disabled] && api_user_role_allows?(action[:identifier])
-              {"name" => action[:name], "method" => method, "href" => (href ? href : collection)}
+              { "name" => action[:name], "method" => method, "href" => (href ? href : collection) }


why this change here ? was this for Rubocop ?

abellotti · 2017-03-23T19:42:05Z

spec/requests/api/querying_spec.rb

@@ -643,6 +643,53 @@ def create_vms_by_name(names)
      expect(actions.first["name"]).to eq("suspend")
    end



Can you add a test on actions returned for the collection itself /api/vms, maybe authorize with vm_start and vm_stop. We seem to be missing that test.

abellotti · 2017-03-23T19:48:59Z

app/controllers/api/base_controller/renderer.rb

@@ -413,7 +425,7 @@ def render_options(resource, data = {})
                                                              collection.virtual_attribute_names),
              :virtual_attributes => options_attribute_list(collection.virtual_attribute_names),
              :relationships      => (collection.reflections.keys |
-                                       collection.virtual_reflections.keys.collect(&:to_s)).sort
+                collection.virtual_reflections.keys.collect(&:to_s)).sort


was this for Rubocop too ? looks odd, can collection on 428 align with collection on 427 while keeping Rubocop happy ?

abellotti · 2017-03-23T19:51:23Z

app/controllers/api/base_controller/renderer.rb

          next unless render_actions_for_method(cspec[:verbs], method)
-          typed_action_definitions = fetch_typed_subcollection_actions(method, is_subcollection) || action_definitions
+          typed_action_definitions = action_definitions || fetch_typed_subcollection_actions(method, is_subcollection)


what did this switch resolve ?

@abellotti the correct subresource actions were not being resolved - it was sending back subcollection actions.

add additional tests simplifying

miq-bot · 2017-03-23T21:23:11Z

Checked commit jntullo@70e89b6 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks good. 🍰

imtayadeway

👍

abellotti · 2017-03-24T20:19:42Z

Thanks @jntullo for fixing this. 😍

miq-bot added api bug labels Feb 22, 2017

miq-bot assigned abellotti Feb 22, 2017

jntullo closed this Feb 23, 2017

jntullo reopened this Feb 23, 2017

abellotti reviewed Mar 23, 2017

View reviewed changes

abellotti requested a review from imtayadeway March 23, 2017 19:58

return actions

70e89b6

add additional tests simplifying

imtayadeway approved these changes Mar 23, 2017

View reviewed changes

abellotti merged commit c8f898e into ManageIQ:master Mar 24, 2017

abellotti added this to the Sprint 57 Ending Mar 27, 2017 milestone Mar 24, 2017

chrisarcand deleted the fix_actions branch November 28, 2017 19:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure actions are returned correctly in the API #14033

Ensure actions are returned correctly in the API #14033

jntullo commented Feb 22, 2017

abellotti commented Mar 16, 2017

abellotti commented Mar 22, 2017

abellotti left a comment

abellotti Mar 23, 2017

abellotti Mar 23, 2017

abellotti Mar 23, 2017

abellotti Mar 23, 2017

jntullo Mar 23, 2017

miq-bot commented Mar 23, 2017

imtayadeway left a comment

abellotti commented Mar 24, 2017

		@@ -643,6 +643,53 @@ def create_vms_by_name(names)
		expect(actions.first["name"]).to eq("suspend")
		end

Ensure actions are returned correctly in the API #14033

Ensure actions are returned correctly in the API #14033

Conversation

jntullo commented Feb 22, 2017

abellotti commented Mar 16, 2017

abellotti commented Mar 22, 2017

abellotti left a comment

Choose a reason for hiding this comment

abellotti Mar 23, 2017

Choose a reason for hiding this comment

abellotti Mar 23, 2017

Choose a reason for hiding this comment

abellotti Mar 23, 2017

Choose a reason for hiding this comment

abellotti Mar 23, 2017

Choose a reason for hiding this comment

jntullo Mar 23, 2017

Choose a reason for hiding this comment

miq-bot commented Mar 23, 2017

imtayadeway left a comment

Choose a reason for hiding this comment

abellotti commented Mar 24, 2017