Generic Objects Subcollection #57

jntullo · 2017-09-18T22:40:47Z

This PR adds a Generic Objects Subcollection on the Generic Object Definition

Usage

GET /api/generic_object_definitions/:id/generic_objects

will return the collection of generic objects of that definition type:

{
  "name"      :"generic_objects",
  "count"     :2,
  "subcount"  :2,
  "resources" :[
   { "href" : "api/generic_object_definitions/:id/generic_objects/:id" },
   { "href" : "api/generic_object_definitions/:id/generic_objects/:id" },
  ]
}

GET /api/generic_object_definitions/:id/generic_objects/:id

will return property attributes by default:

{
  "href"                : "/api/generic_object_definitions/:id/generic_objects/:id",
  "id"                  : "1000000001",
  "name"                : "bar",
  ... 
  "property_attributes" : { "is_something" : true }
}

can specify associations on the request, ie:
GET /api/generic_object_definitions/:id/generic_objects/:id?associations=vms,services
returns:

{
  "href"                : "api/generic_object_definitions/:id/generic_objects/:id",
  "id"                  : "10000001",
  "name"                : "bar",
  "property_attributes" : { "is_something" : true },
  "vms"                 : [{ "id": "<vm id>" ...}],
  "services"            : [{"id": "<service id>"...}]
}

GET /api/services/:id/generic_objects

Returns the generic objects associated with a service

GET /api/services/:id?expand=resources,generic_objects&attributes=generic_objects.generic_object_definition

will return the service as well as the generic objects with associated generic object definition:

{'name':'services',
 'count':1,
 'subcount':1,
 'resources':
   [{'href':'/api/services/:id',
     'id':':id',
     'name':'svc',
     ...
     'generic_objects':
       [{
         'id':':id',
         'name':'generic_object_0000000000001',
         ...
         'generic_object_definition' : {
            'id' : ':id',
            'name' : 'generic_object_definition_name'
            ...
          }
        }
       ]
    }
   ]
}

POST /api/generic_object_definitions/:id/generic_objects

it can create a new generic object for the specified generic object definition:

{
  "name"                : "go_name1",
  "uid"                 : "optional_uid",
  "property_attributes" : {
    "widget" : "widget value",
  },
  "associations"        : {
    "vms" : [
      { "href" : "/api/vms/:id" },
    ]
  }
}

Background

There is shared code between the generic objects controller and the generic objects subcollection. The code is tightly coupled with the controllers, and therefore not suitable for a service, which was the design choice behind the mixin.

When writing the mixin, the initial intent was to not have it as a part of BaseController, it was instead to have Api::Mixins::GenericObjects, however, this caused sporadic test failures when included as include Mixins::GenericObjects. I think this may require greater refactoring to achieve it as Api::Mixins, which might be better suited in another PR.

abellotti · 2017-09-19T13:16:06Z

spec/requests/generic_object_definitions_spec.rb

+        },
+        'associations'        => {
+          'vms' => [
+            { 'href' => api_vm_url(nil, vm.id) },


with these helpers, you can simply pass in the object, i.e. api_vm_url(nil, vm)

abellotti · 2017-09-19T13:16:46Z

spec/requests/generic_object_definitions_spec.rb

+      get(api_generic_object_definition_generic_object_url(nil, object_def.id, object.id), :params => {:associations => 'vms'})
+
+      expected = {
+        'href'                => api_generic_object_definition_generic_object_url(nil, object_def.id, object.id),


same comment throughout, with these helpers just the object so ...(nil, object_def, object)

abellotti · 2017-09-19T13:19:36Z

app/controllers/api/subcollections/generic_objects.rb

+  module Subcollections
+    module GenericObjects
+      def generic_objects_query_resource(object_definition)
+        object_definition.generic_objects


what if the generic object definition has no generic object, is this nil here ? what is returned by API ?

@abellotti if it has no generic objects, it just returns an empty array, so the api returns "resources": []

abellotti · 2017-09-19T13:35:40Z

app/controllers/api/base_controller/generic_objects_mixin.rb

@@ -0,0 +1,40 @@
+module Api


I wonder if this should go in a new folder: app/controllers/api/mixins/
and dropping the BaseController in the module hierarchy, it would just be a submodule of the
class including the mixin.

@abellotti I explained a bit more in the PR description, but I have been looking into this (which is why it's still WIP) ...however, without the BaseController module hierarchy and with it in app/controllers/api/mixins, it has been creating sporadic test failures that I have been trying to figure out.

It seems to have to do with the way things are loaded, but haven't figured out the solution just yet.

abellotti · 2017-09-19T13:40:26Z

app/controllers/api/generic_object_definitions_controller.rb

@@ -114,6 +120,10 @@ def build_generic_object_definition_options

    private

+    def generic_objects_request


not quite sure what this does. why is it needed ?

I know, it's a bit funky, but basically this controller needs to have the before_action hooks to allow for the subcollection to set the associations for requests such as GET /api/generic_object_definitions/:id/generic_objects?associations=vmsas well as to set the hook to set @additional_attributes...we don't want to set the hooks unless the subject is generic_objects, so this method is the if condition on the hook. updated the name to is_generic_objects_request. Any other naming ideas?

jntullo · 2017-09-19T15:44:31Z

@abellotti exhibit A of sporadic test failures. Thought everything was fine with the mixin, finally, but Travis is now having failures I don't get locally 😭

jntullo · 2017-09-19T20:09:16Z

@miq-bot assign @abellotti

imtayadeway · 2017-09-19T23:38:09Z

app/controllers/api/generic_object_definitions_controller.rb

@@ -1,5 +1,11 @@
 module Api
  class GenericObjectDefinitionsController < BaseController
+    include Api::Mixins::GenericObjects


In short, I think you needed the Api qualification because the classic UI has a top-level Mixins namespace. If something in there gets loaded first, it will break the autoloading of this module. I'm proposing that the classic UI not do that and put their mixins in their own namespace.

thanks @imtayadeway !!! ✨

Or you could use include_concern Mixins::GenericObjects which I guess is the standard way that we deal with this problem (and may convey to the next person that there is some kind of contention over the Mixins namespace)

@imtayadeway unfortunately I tried this, and the API is not happy :(

miq-bot · 2017-09-21T13:29:36Z

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

bascar · 2017-09-29T18:25:22Z

@abellotti can we get this merged please? it is blocking other Generic Object items

jntullo · 2017-09-29T18:33:13Z

just squashed some commits.

abellotti · 2017-09-29T18:33:43Z

LGTM!!

@imtayadeway ?

abellotti · 2017-10-02T19:45:34Z

ping @imtayadeway can we borrow your 👀 please ?

miq-bot · 2017-10-02T21:09:41Z

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

return property attributes by default and return specified associations move mixin to its own folder move include from the sub collection to the collection

miq-bot · 2017-10-03T12:39:40Z

Checked commits jntullo/manageiq-api@27d50e2~...ff531b4 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
9 files checked, 0 offenses detected
Everything looks fine. ⭐

imtayadeway · 2017-10-03T16:31:29Z

spec/requests/generic_object_definitions_spec.rb

+    let(:vm) { FactoryGirl.create(:vm_amazon) }
+
+    before do
+      object_def.add_property_attribute('is_something', 'boolean')


The extensive setup here seems to suggest that the GO API is missing something....perhaps we could at a later point create a factory, or a builder, or at least enhance the factory girl definition to help simplify this process?

Yeah, some factories for "simple" objects with dummy attributes would be useful

imtayadeway

👍 Chatted with @jntullo on gitter about some possible "might be nice" changes to get some of the details of serializing and building generic objects out of the controller, but nothing blocking for this PR. Thanks @jntullo ! 🎈

abellotti · 2017-10-03T19:20:47Z

Thank you @imtayadeway for your review 🙇

miq-bot added the wip label Sep 18, 2017

abellotti reviewed Sep 19, 2017

View reviewed changes

jntullo changed the title ~~[WIP] Generic Objects Subcollection~~ Generic Objects Subcollection Sep 19, 2017

miq-bot removed the wip label Sep 19, 2017

miq-bot assigned abellotti Sep 19, 2017

imtayadeway reviewed Sep 19, 2017

View reviewed changes

miq-bot added unmergeable and removed unmergeable labels Sep 21, 2017

chalettu mentioned this pull request Sep 25, 2017

Implement Generic Objects on services details page ManageIQ/manageiq-ui-service#996

Merged

abellotti added the enhancement label Sep 25, 2017

abellotti approved these changes Sep 29, 2017

View reviewed changes

miq-bot added the unmergeable label Oct 2, 2017

add generic objects sub collection

27d50e2

return property attributes by default and return specified associations move mixin to its own folder move include from the sub collection to the collection

miq-bot removed the unmergeable label Oct 3, 2017

add generic objects sub collection to services

ff531b4

imtayadeway reviewed Oct 3, 2017

View reviewed changes

imtayadeway approved these changes Oct 3, 2017

View reviewed changes

abellotti merged commit f122f18 into ManageIQ:master Oct 3, 2017

abellotti added this to the Sprint 71 Ending Oct 16, 2017 milestone Oct 3, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generic Objects Subcollection #57

Generic Objects Subcollection #57

jntullo commented Sep 18, 2017 •

edited

Loading

abellotti Sep 19, 2017

abellotti Sep 19, 2017

abellotti Sep 19, 2017

jntullo Sep 19, 2017 •

edited

Loading

abellotti Sep 19, 2017

jntullo Sep 19, 2017

jntullo Sep 19, 2017

abellotti Sep 19, 2017

jntullo Sep 19, 2017 •

edited

Loading

jntullo commented Sep 19, 2017

jntullo commented Sep 19, 2017

imtayadeway Sep 19, 2017

jntullo Sep 20, 2017

imtayadeway Sep 21, 2017

jntullo Sep 21, 2017

miq-bot commented Sep 21, 2017

bascar commented Sep 29, 2017

jntullo commented Sep 29, 2017

abellotti commented Sep 29, 2017

abellotti commented Oct 2, 2017

miq-bot commented Oct 2, 2017

miq-bot commented Oct 3, 2017

imtayadeway Oct 3, 2017

Fryguy Oct 3, 2017

imtayadeway left a comment

abellotti commented Oct 3, 2017

		@@ -114,6 +120,10 @@ def build_generic_object_definition_options

		private

		def generic_objects_request

Generic Objects Subcollection #57

Generic Objects Subcollection #57

Conversation

jntullo commented Sep 18, 2017 • edited Loading

Usage

GET /api/generic_object_definitions/:id/generic_objects

GET /api/generic_object_definitions/:id/generic_objects/:id

GET /api/services/:id/generic_objects

GET /api/services/:id?expand=resources,generic_objects&attributes=generic_objects.generic_object_definition

POST /api/generic_object_definitions/:id/generic_objects

Background

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jntullo Sep 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jntullo Sep 19, 2017 • edited Loading

Choose a reason for hiding this comment

jntullo commented Sep 19, 2017

jntullo commented Sep 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Sep 21, 2017

bascar commented Sep 29, 2017

jntullo commented Sep 29, 2017

abellotti commented Sep 29, 2017

abellotti commented Oct 2, 2017

miq-bot commented Oct 2, 2017

miq-bot commented Oct 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imtayadeway left a comment

Choose a reason for hiding this comment

abellotti commented Oct 3, 2017

jntullo commented Sep 18, 2017 •

edited

Loading

jntullo Sep 19, 2017 •

edited

Loading

jntullo Sep 19, 2017 •

edited

Loading