OPTIONS - Generic Object Definition

[AparnaKarve]

We need support for OPTIONS /api/generic_object_definitions so that the Generic Object Definitions UI populates the dropdowns with the allowed association types and allowed data types

Requires ManageIQ/manageiq#15922 to be merged first for the new Dictionary entries added for the allowed data types.

[abellotti]

This LGTM!! Thanks @AparnaKarve for the enhancement.

/cc @jntullo can I borrow your 👀 for a quick review ?

[miq-bot]

This pull request is not mergeable. Please rebase and repush.

[abellotti]

@AparnaKarve will need a rebase/repush when you get a chance. Thanks.

[imtayadeway]

Hi! It looks like your PR has been affected by the recently merged #40, and as a result will need to be rebased. First, I apologize for any inconvenience caused. After rebasing, you'll need to update some specs in order for them to pass. In particular you'll need to change any path helpers, applying the following pattern:

vms_url => api_vms_url
vms_url(vm.id) => api_vm_url(nil, vm) 
vms_url(vm.compressed_id) => api_vm_url(nil, vm.compressed_id)
"#{vms_url(vm.id)}/tags" => api_vm_tags_url(nil, vm)
"#{vms_url(vm.compressed_id}/tags" => api_vm_tags_url(nil, vm.compressed_id)
"#{vms_url(vm.id)}/tags/#{tag.id}" => api_vm_tag_url(nil, vm, tag)
"#{vms_url(vm.compressed_id)}/tags/#{tag.compressed_id}" => api_vm_tag_url(nil, vm.compressed_id, tag.compressed_id)

If you run into any issues, please ping me and I will try to help you out.

Many thanks!
Tim

[imtayadeway]

@AparnaKarve this LGTM. Just one suggestion - do you think that allowed_association_types and allowed_types could be centralized (possibly in the model) so that you don't have to reproduce the implementation in the tests? I'm fine with leaving that as a follow up if you'd rather get this merged first

[AparnaKarve]

@imtayadeway I did think about adding allowed_association_types and allowed_types in the model, but wanted to keep the model free of all the Dictionary/i18n related stuff.

In normal course (pre-API days), we would have implemented allowed_association_types and allowed_types in the view controller (controllers/generic_object_definition_controller.rb)
So to keep that parity, I decided to add the methods in allowed_association_types and allowed_types in the API controller (api/generic_object_definitions_controller.rb ) instead.

That was mainly the rationale behind adding the methods in api/generic_object_definitions_controller.rb

[imtayadeway]

I did think about adding allowed_association_types and allowed_types in the model, but wanted to keep the model free of all the Dictionary/i18n related stuff.

@AparnaKarve is that because you didn't want it to be executed at load time? Aren't there ways to delay it until call time?

Either way, if you didn't want to update the model, you could add a service object instead?

[AparnaKarve]

is that because you didn't want it to be executed at load time? Aren't there ways to delay it until call time?

@imtayadeway If we add the methods to the model, we probably do not have to worry about delayed loading etc. It should just work.
Not adding it to the model was a conscious decision, but I think I can reconsider that.

@abellotti Can you please merge this and I can create a follow-up PR to move the methods allowed_association_types and allowed_types out of the API controller. Thanks.

you could add a service object instead?

Not sure how applicable the service object is to this case.

[abellotti]

I'm ok with keeping those here, but still not crazy with the dup. As an option (no pun intended), why not just define those as class instance methods for now.

module Api
   class GenericObjectDefinitionsController
      def self.allowed_types
      end
      ...
      def self.allowed_association_types
      end
      ...

and used by both api controller and specs.

[AparnaKarve]

@abellotti When I make those methods as class instance methods, they work in the api controller when I access them like this -

GenericObjectDefinitionsController::allowed_association_types

But GenericObjectDefinitionsController::allowed_association_types does not seem to work in the spec. I'm seeing this -

Unable to autoload constant GenericObjectDefinitionsController, expected ~/master/manageiq/plugins/manageiq-api/app/controllers/api/generic_object_definitions_controller.rb to define it

[miq-bot]

Checked commits AparnaKarve/manageiq-api@9b87724~...9cfe639 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. 🏆

[AparnaKarve]

@abellotti Thanks for the tip on class instance method and Api::GenericObjectDefinitionsController.<method_name>

[AparnaKarve]

I like that there are no dups now.

Thanks @imtayadeway and @abellotti

[abellotti]

Thanks @AparnaKarve for the API enhancement !! will merge when 🍏