deltacloud-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From David Lutterkort <lut...@redhat.com>
Subject Re: [PATCH core 2/3] CIMI: Filter sub-collections properly when using $select (DTACLOUD-431)
Date Sat, 26 Jan 2013 01:25:47 GMT
On Wed, 2013-01-23 at 16:51 +0100, mfojtik@redhat.com wrote:
> From: Michal Fojtik <mfojtik@redhat.com>
> 
> 
> Signed-off-by: Michal fojtik <mfojtik@redhat.com>

NAK because there's no test. Also a couple comments:

> ---
>  server/lib/cimi/models/base.rb   | 11 ++++++++---
>  server/lib/cimi/models/schema.rb |  1 +
>  2 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/server/lib/cimi/models/base.rb b/server/lib/cimi/models/base.rb
> index 5fdb69a..1393ae4 100644
> --- a/server/lib/cimi/models/base.rb
> +++ b/server/lib/cimi/models/base.rb
> @@ -170,8 +170,12 @@ class CIMI::Model::Resource
>    # Prepare to serialize
>    def prepare
>      self.class.schema.collections.map { |coll| coll.name }.each do |n|
> -      self[n].href = "#{self.id}/#{n}" unless self[n].href
> -      self[n].id = "#{self.id}/#{n}" if !self[n].entries.empty?
> +      if !@filter_attrs.empty? and !@filter_attrs.include?(n)
> +        @attribute_values[n] = nil

@attribute_values[n] is the same as self[n], and we should stick to one
way of writing that.

> +      else
> +        self[n].href = "#{self.id}/#{n}" if !self[n].href
> +        self[n].id = "#{self.id}/#{n}" if !self[n].entries.empty?
> +      end

This if block is basically saying 'if we are not selecting collection n,
set it to nil, otherwise set id/href to sth useful'

First off, we should rename everything called 'filter' in the code to
'select', since that's what it's used for (I had to do a bit of grepping
to realize all the filter stuff is only used for handling $select)

Second, the branches in the condition should be reversed to avoid the
'!' so that we can rewrite this as

        if @selected.include?(n)
          self[n].href = "#{self.id}/#{n}" if !self[n].href
          self[n].id = "#{self.id}/#{n}" if !self[n].entries.empty?
        else
          self[n] = nil
        end

> diff --git a/server/lib/cimi/models/schema.rb b/server/lib/cimi/models/schema.rb
> index 4acd2dc..24a5a0e 100644
> --- a/server/lib/cimi/models/schema.rb
> +++ b/server/lib/cimi/models/schema.rb
> @@ -241,6 +241,7 @@ class CIMI::Model::Schema
>      end
>  
>      def to_xml(model, xml)
> +      return if model[name].nil?
>        model[name].prepare
>        if model[name].entries.empty?
>          xml[xml_name] = { "href" => model[name].href }

The same thing needs to happen to to_json, no ?

David


Mime
View raw message