From 97df523a3d56b2f0715154c6c5dabc37df358bcd Mon Sep 17 00:00:00 2001 From: Alberto Bellotti Date: Thu, 9 Feb 2017 10:58:27 -0500 Subject: [PATCH 1/2] Api enhancement to support optional collection_class parameter - Allows subclassing any collection - specified via ?collection_class= i.e. GET /api/vms?collection_class=ManageIQ::Providers::CloudManager::Vm - Updated CLI to support --collection-class= --- app/controllers/api/base_controller/parser.rb | 50 +++++++++++++++---- tools/rest_api.rb | 2 +- 2 files changed, 40 insertions(+), 12 deletions(-) diff --git a/app/controllers/api/base_controller/parser.rb b/app/controllers/api/base_controller/parser.rb index 637b75b2d02..6835ae16521 100644 --- a/app/controllers/api/base_controller/parser.rb +++ b/app/controllers/api/base_controller/parser.rb @@ -31,17 +31,9 @@ def validate_api_request end def validate_optional_collection_classes - @collection_klasses = {} # Default all to config classes - param = params['provider_class'] - return unless param.present? - - raise BadRequestError, "Unsupported provider_class #{param} specified" if param != "provider" - %w(tags policies policy_profiles).each do |cname| - if @req.subcollection == cname || @req.expand?(cname) - raise BadRequestError, "Management of #{cname} is unsupported for the Provider class" - end - end - @collection_klasses[:providers] = Provider + @collection_klasses = {} # Default all to config classes + validate_provider_class + validate_collection_class end def validate_api_action @@ -331,6 +323,42 @@ def assert_all_required_fields_exists(data, type, required_fields) raise BadRequestError, "Resource #{missing_fields.join(", ")} needs be specified for creating a new #{type}" end end + + def validate_provider_class + param = params['provider_class'] + return unless param.present? + + raise BadRequestError, "Unsupported provider_class #{param} specified" if param != "provider" + %w(tags policies policy_profiles).each do |cname| + if @req.subcollection == cname || @req.expand?(cname) + raise BadRequestError, "Management of #{cname} is unsupported for the Provider class" + end + end + @collection_klasses[:providers] = Provider + end + + def validate_collection_class + param = params['collection_class'] + return unless param.present? + + begin + param_klass = param.constantize + rescue + raise BadRequestError, "Invalid collection_class #{param} specified" + end + + collection_klass = collection_config[@req.collection].klass.constantize + + # If it is the collection's class, then we're good to go as is. + return if param_klass == collection_klass + + if param_klass < collection_klass + @collection_klasses[@req.collection.to_sym] = param_klass + return + end + + raise BadRequestError, "Invalid collection_class #{param} specified for the #{@req.collection} collection" + end end end end diff --git a/tools/rest_api.rb b/tools/rest_api.rb index 2070b7c6924..9dbabf7747b 100755 --- a/tools/rest_api.rb +++ b/tools/rest_api.rb @@ -47,7 +47,7 @@ class Cli API_PARAMETERS = %w(expand hide attributes decorators limit offset depth search_options sort_by sort_order sort_options - filter by_tag provider_class requester_type).freeze + filter by_tag provider_class collection_class requester_type).freeze MULTI_PARAMS = %w(filter).freeze From dd8fd54d1ffb8e7cf8182edf0d75fd78772d399d Mon Sep 17 00:00:00 2001 From: Alberto Bellotti Date: Fri, 10 Feb 2017 15:03:22 -0500 Subject: [PATCH 2/2] Simplified logic, leveraging descendants, and added specs - Simplified validate_collection_class logic - Leveraging descendants to keep brakeman happy - Added rspecs --- app/controllers/api/base_controller/parser.rb | 15 ++----- spec/requests/api/querying_spec.rb | 40 +++++++++++++++++++ 2 files changed, 44 insertions(+), 11 deletions(-) diff --git a/app/controllers/api/base_controller/parser.rb b/app/controllers/api/base_controller/parser.rb index 6835ae16521..07fc8e469e2 100644 --- a/app/controllers/api/base_controller/parser.rb +++ b/app/controllers/api/base_controller/parser.rb @@ -341,18 +341,11 @@ def validate_collection_class param = params['collection_class'] return unless param.present? - begin - param_klass = param.constantize - rescue - raise BadRequestError, "Invalid collection_class #{param} specified" - end - - collection_klass = collection_config[@req.collection].klass.constantize - - # If it is the collection's class, then we're good to go as is. - return if param_klass == collection_klass + klass = collection_class(@req.collection) + return if param == klass.name - if param_klass < collection_klass + param_klass = klass.descendants.detect { |sub_klass| param == sub_klass.name } + if param_klass.present? @collection_klasses[@req.collection.to_sym] = param_klass return end diff --git a/spec/requests/api/querying_spec.rb b/spec/requests/api/querying_spec.rb index 2895cc3ef3e..1e10dfc1cda 100644 --- a/spec/requests/api/querying_spec.rb +++ b/spec/requests/api/querying_spec.rb @@ -706,4 +706,44 @@ def create_vms_by_name(names) expect(response.headers['Access-Control-Allow-Methods']).to include('OPTIONS') end end + + describe "with optional collection_class" do + before { api_basic_authorize collection_action_identifier(:vms, :read, :get) } + + it "fail with invalid collection_class specified" do + run_get vms_url, :collection_class => "BogusClass" + + expect_bad_request("Invalid collection_class BogusClass specified for the vms collection") + end + + it "succeed with collection_class matching the collection class" do + create_vms_by_name(%w(aa bb)) + + run_get vms_url, :collection_class => "Vm" + + expect_query_result(:vms, 2, 2) + end + + it "succeed with collection_class matching the collection class and returns subclassed resources" do + FactoryGirl.create(:vm_vmware, :name => "aa") + FactoryGirl.create(:vm_vmware_cloud, :name => "bb") + FactoryGirl.create(:vm_vmware_cloud, :name => "cc") + + run_get vms_url, :expand => "resources", :collection_class => "Vm" + + expect_query_result(:vms, 3, 3) + expect(response.parsed_body["resources"].collect { |vm| vm["name"] }).to match_array(%w(aa bb cc)) + end + + it "succeed with collection_class and only returns subclassed resources" do + FactoryGirl.create(:vm_vmware, :name => "aa") + FactoryGirl.create(:vm_vmware_cloud, :name => "bb") + vmcc = FactoryGirl.create(:vm_vmware_cloud, :name => "cc") + + run_get vms_url, :expand => "resources", :collection_class => vmcc.class.name + + expect_query_result(:vms, 2, 2) + expect(response.parsed_body["resources"].collect { |vm| vm["name"] }).to match_array(%w(bb cc)) + end + end end