fixes #2931 - API v2 hosts JSON response, add *_name for each *_id

[isratrade]

No description provided.

[isratrade]

Default response is *_name for all *_id fields. I would like to make this flexible for performance reasons if a user just wants hosts?fields=id,name to not run on the associated queries

[tstrachota]

Can we maybe move *_name methods out of the model? We'll need such functionality across whole API.

I can imagine a helper in the rabl templates. Something like:

associated_name :environment, :domain, :architecture, :medium
associated_name :compute_resource, :hostgroup, :name => :to_label
...

[isratrade]

@tstrachota, I didn't find documentation on creating rabl helpers. Can you send me an example.

[mbacovsky]

It seems that UI has own helpers to get the association names, so I wouldn't pollute the model with the *_name methods (maybe just in case of complex ones).
Having it as helper in Rabl the way Tomas suggested seems reasonable.
However I would find the last line more readable if it is as follows

 associated_name :compute_resource => :to_label, :hostgroup => :to_label

I'm also worried about the performance of the index command as you mentioned. Could we limit the columns displayed by default to lets say

• :name
• :id
• :operationd_system_id, :operating_system_name
• :hostgroup_id, hostgroup_name
• :environment_id, :environment_name
• :model_id, :model_name
• :last_report
• :ip
• :mac
• ;organization_id, :organization_name
• :location_id, :location_name

to match UI?

So I suggest to split details.rabl to index_datails and details (extended by index_details)

It may be better to also move org and location section to separate Rabl partial

Makes sense?

[mbacovsky]

@isratrade I believe there is no docs on how to extend rabl. You can find Tomas's attempt here https://github.com/tstrachota/katello/blob/409aa954f39e0bc6afa0cb4f447e2dd7b06f1b4a/config/initializers/rabl_init.rb. We didn't use this in the end, but associated_name could look similar to this

def associated_name *attrs
  attrs.each do |a|
    if a.is_a? Hash
      a.each do |res, col|
         node_name = (res.to_s+'_name).to_sym
         node(node_name){ |obj| obj.send(res).try(col) }
      end
    elsif a.is_a? Symbol
       node_name = (a.to_s+'_name).to_sym
       node(node_name){ |obj| obj.send(a).try(:name) }
    end
  end
end

[isratrade]

@tstrachota, @mbacovsky, makes sense. I'll work on this today.

[isratrade]

@tstrachota, @mbacovsky, PR re-committed with your suggested changes

#index extends details.rabl only
#show extends details.rabl and adds more node attributes

I included the boolean checks for org/loc in the method associated_attributes rather than having a separate partial.

[mbacovsky]

Looks good , I like the way you included loc and orgs into asociated_attrs.

I'd probably prefer following notation of associated attrs, to visually match rabl's attributes, but nothing crucial:

associated_attributes :operatingsystem => :to_label, hostgroup => :to_label
associated_attributes :environment, :model, :location, :organization

The rabl helper deserves some refactoring

I have a question about host_parameters. Why we have host_parameters in host and do not have domain parameters in domain? Are these the same case? Shouldn't we unify it?

[isratrade]

@mbacovsky, yes rabl usually doesn't allow mixing symbols and hashes in the the same line like this (what I did).

associated_attributes :model, :location, :operatingsystem => :to_label, hostgroup => :to_label

as their code expects the all attributes to be the same as the first one. If it's a hash, then they use find_pair.

I'll update it

I'm definitely agree to some re factoring of the rabl helper.

[isratrade]

@mbacovsky, recommited

[tstrachota]

@isratrade it looks good! Rabl template is quite clean now. I'd maybe do slight refactoring like this (I removed the comments to shorten the example):

    def associated_ids *attrs
      attrs.each do |a|
        if a.kind_of? Hash
          a.each do |klass, method_name|
            resource_id_nodes(klass, method_name) if show_node?(a)
          end
        elsif a.kind_of? Symbol or a.kind_of? String
          resource_id_nodes(a, :name) if show_node?(a)
        end
      end
    end

    def resource_id_nodes(klass, method_name)
      attributes klass.to_s+'_id'

      node_name = klass.to_s+'_name'
      node(node_name) do |obj|
        obj.send(klass).try(method_name)
      end
    end

I think it would be also handy to have such helpers for associated collections. Something like:

    def associated_collection_ids *attrs
      attrs.each do |a|
        if a.kind_of? Hash
          a.each do |klass, method_name|
            collection_id_nodes(klass, method_name) if show_node?(a)
          end
        elsif a.kind_of? Symbol or a.kind_of? String
          collection_id_nodes(a, :name) if show_node?(a)
        end
      end
    end

    def collection_id_nodes(klass, method_name)
      attributes klass.to_s.singularize+'_ids'

      node_name = klass.to_s.singularize+'_names'
      node(node_name) do |obj|
        obj.send(klass).collect{|a| a.try(method_name) }
      end
    end

I changed the names a bit but it's only cosmetics. I don't insist on them at all.

[mbacovsky]

@isratrade, looks good, if Tomas's refactoring is included I'm okay with merge. I prefer to stick with original associated_attributes and associated_collections naming though.

Thanks for the effort, this addition will be very helpfull!

[isratrade]

@mbacovsky, @tstrachota, I changed naming and some other minor changes to @tstrachota's refactoring.

I also used in #show

associated_collections :host_parameters

I see this as a spike, so let's add some testing after we are happy with this convention.

We also need to decide what to include in hosts#show by default. The host parameters were in v1, so I just included it in v2, but I think smart variables and smart class parameters should also be included. If not, PR #829 allows a user to GET "api/hosts/:host_id/smart_variables"

[tstrachota]

Looks good. +1 to merge.
I agree taking this as a spike. We can reevaluate after some time.

[mbacovsky]

it is starting to look powerful. The spike approach is good, when hosts are 'perfect' we can use it as pattern for fixing others controllers ourselves as we go through them during the CLI work.

For me this is ready to merge when the host_parameters are resolved. I think the use of associated_collections in the show is not getting us there as the parameters are key/value so printing just the keys makes no sense to me.

If I understand it correctly the associated_collections when used e.g. in architectures

  associated_collections :operatingsystem

would return

...
'operatingsystem_ids': [1,2]
'operatingsystem_names': ['RHEL 6.4', 'CentOS 6.4']
...

which is not the case for host's parameters. I suggest to remove host_parameters anyway to have more restfull API and used /host/:id/parameters instead.

And one minor thing 😉 - it may be more intuitive to rename detail.json.rabl to brief.json.rabl, or base as we use it more in that sense.

[isratrade]

@mbacovsky, @tstrachota
I re-committed and removed :host_parameters {} from the hosts#show.json.rabl as a user can call /host/:id/parameters.

I will include in the "API v2 conventions" etherpad doc (to be sent) about the file naming (details, brief, etc) as well as which associations to "expand" by default.

[mbacovsky]

Thanks for removing host_parameters. I saw in the comment you were mentioning 'brief' in file naming conventions, but there is 'details' still in the code base... just checking it was intentional 😉

[isratrade]

@mbacovsky, about naming convention of partial, I just thought we could wait for the etherpad discussion after I send an email to -dev. In the meantime, I renamed details.json.rabl it to base.json.rabl. There could be an additional partial call brief, short, small, etc that just include :id, :name

[isratrade]

@tstrachota, @mbacovsky, getter and setters *_names methods automatically added in PR #850 for all "has_many" in ActiveRecord. I think it's OK to have the getter methods *_names in AR even though we have a Rabl helper as well.

[mbacovsky]

👍
and here is 'foremans head' as an award for patience: 👷++
:)

[tstrachota]

Ok, now the two ways duplicate the same thing. Do we want to keep both rabl helpers and model methods?
I like PR #850 and would prefer it above this one.

[isratrade]

closing in favor or PR #850