[PATCH 4/4] BZ 802571 refactor provider_account sort-by-priority to correctly sort nils, regardless of db

Matt Wagner matt.wagner at redhat.com
Mon Mar 26 16:00:56 UTC 2012


On Fri, Mar 23, 2012 at 09:52:48AM -0400, Tzu-Mainn Chen wrote:
> ---
>  src/app/models/deployment.rb       |    3 ++-
>  src/app/models/instance.rb         |    2 +-
>  src/app/models/pool_family.rb      |   14 ++++++++++++++
>  src/app/models/provider_account.rb |    1 -
>  4 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/src/app/models/deployment.rb b/src/app/models/deployment.rb
> index fe02ad3..cd2eb73 100644
> --- a/src/app/models/deployment.rb
> +++ b/src/app/models/deployment.rb
> @@ -491,7 +491,8 @@ class Deployment < ActiveRecord::Base
>        errors = e.map {|e| "#{instance.name}: #{e}"}
>        m
>      end
> -    pool.pool_family.provider_accounts.ascending_by_priority.each do |account|
> +
> +    pool.pool_family.provider_accounts_by_priority.each do |account|
>        matches_by_account = all_matches.map do |m|
>          m.find {|m| m.provider_account.id == account.id}
>        end
> diff --git a/src/app/models/instance.rb b/src/app/models/instance.rb
> index 60a0f09..95e2717 100644
> --- a/src/app/models/instance.rb
> +++ b/src/app/models/instance.rb
> @@ -403,7 +403,7 @@ class Instance < ActiveRecord::Base
>      return [[], errors] unless errors.empty?
>  
>      matched = []
> -    pool.pool_family.provider_accounts.ascending_by_priority.each do |account|
> +    pool.pool_family.provider_accounts_by_priority.each do |account|
>        account.instance_matches(self, matched, errors)
>      end
>  
> diff --git a/src/app/models/pool_family.rb b/src/app/models/pool_family.rb
> index 8f5ea24..dd474a9 100644
> --- a/src/app/models/pool_family.rb
> +++ b/src/app/models/pool_family.rb
> @@ -70,6 +70,20 @@ class PoolFamily < ActiveRecord::Base
>      !provider_accounts.empty? and !provider_accounts.any? {|acc| acc.provider.enabled?}
>    end
>  
> +  def provider_accounts_by_priority
> +    provider_accounts.sort {|a,b|
> +      if a.priority.nil? and b.priority.nil?
> +        0
> +      elsif a.priority.nil?
> +        1
> +      elsif b.priority.nil?
> +        -1
> +      else
> +        a.priority <=> b.priority
> +      end
> +    }
> +  end
> +


Another "for the record" based on our IRC discussion -- I really, really
don't like this, since we're moving a trivial database operation into
our code base, which feels like it's asking for trouble. But the
reasoning here is compelling enough that I'll go along with it --
sorting on tables containing nils (as is the case with priority)
apparently doesn't behave consistently across databases.


>    def statistics
>      max = quota.maximum_running_instances
>      total = quota.running_instances
> diff --git a/src/app/models/provider_account.rb b/src/app/models/provider_account.rb
> index 9647dd5..d0323a9 100644
> --- a/src/app/models/provider_account.rb
> +++ b/src/app/models/provider_account.rb
> @@ -73,7 +73,6 @@ class ProviderAccount < ActiveRecord::Base
>    before_destroy :destroyable?
>  
>    scope :enabled, lambda { where(:provider_id => Provider.enabled) }
> -  scope :ascending_by_priority, :order => 'provider_accounts.priority ASC'
>  
>    # We set credentials hash as protected so that it is not set during mass assign on new
>    # This is to avoid the scenario where the credentials are set before provider which
> -- 
> 1.7.6.5

ACK, with inline comment.

-- Matt



More information about the aeolus-devel mailing list