[PATCH conductor 2/2] BZ805909: Detect if paginated collection is out of bounds

ifarkas at redhat.com ifarkas at redhat.com
Wed Mar 28 09:24:11 UTC 2012


From: Imre Farkas <ifarkas at redhat.com>

https://bugzilla.redhat.com/show_bug.cgi?id=805909
---
 src/app/controllers/application_controller.rb |    8 +++++
 src/app/controllers/deployments_controller.rb |   22 +++++++-------
 src/app/controllers/images_controller.rb      |    2 +-
 src/app/controllers/instances_controller.rb   |   15 +++++----
 src/app/controllers/pools_controller.rb       |   41 ++++++++++++++----------
 src/app/models/pool.rb                        |    6 ----
 src/app/views/pools/_pretty_list.html.haml    |    2 +-
 7 files changed, 53 insertions(+), 43 deletions(-)

diff --git a/src/app/controllers/application_controller.rb b/src/app/controllers/application_controller.rb
index db471ba..9b89d0e 100644
--- a/src/app/controllers/application_controller.rb
+++ b/src/app/controllers/application_controller.rb
@@ -36,6 +36,7 @@ class ApplicationController < ActionController::Base
   rescue_from Aeolus::Conductor::API::Error, :with => :handle_api_error
 
   helper_method :check_privilege
+  helper_method :paginate_collection
 
   protected
 
@@ -369,4 +370,11 @@ class ApplicationController < ActionController::Base
     redirect_to path.merge(original_params).merge(extended_params)
   end
 
+  def paginate_collection(collection, page = nil, per_page = PER_PAGE)
+    result = collection.paginate(:page => page, :per_page => per_page)
+    result = collection.paginate(:page => result.total_pages, :per_page => per_page) if result.out_of_bounds? && result.total_pages > 0
+
+    result
+  end
+
 end
diff --git a/src/app/controllers/deployments_controller.rb b/src/app/controllers/deployments_controller.rb
index 504a050..8bb76b3 100644
--- a/src/app/controllers/deployments_controller.rb
+++ b/src/app/controllers/deployments_controller.rb
@@ -187,13 +187,13 @@ class DeploymentsController < ApplicationController
     if filter_view?
       @view = 'instances/list'
       params[:instances_preset_filter] = "" unless params[:instances_preset_filter]
-      @instances = Instance.apply_filters(:preset_filter_id => params[:instances_preset_filter], :search_filter => params[:instances_search]).
-                            list(sort_column(Instance), sort_direction).where("instances.deployment_id" => @deployment.id).
-                            paginate(:page => params[:page], :per_page => PER_PAGE)
+      @instances = paginate_collection(Instance.apply_filters(:preset_filter_id => params[:instances_preset_filter], :search_filter => params[:instances_search]).
+                                                list(sort_column(Instance), sort_direction).where("instances.deployment_id" => @deployment.id),
+                                       params[:page], PER_PAGE)
     else
       @view = 'pretty_view_show'
-      @instances = Instance.list(sort_column(Instance), sort_direction).where("instances.deployment_id" => @deployment.id).
-                            paginate(:page => params[:page], :per_page => PER_PAGE)
+      @instances = paginate_collection(Instance.list(sort_column(Instance), sort_direction).where("instances.deployment_id" => @deployment.id),
+                                       params[:page], PER_PAGE)
     end
     #TODO add links to real data for history, permissions, services
     @tabs = [{:name => t('instances.instances.other'), :view => @view, :id => 'instances', :count => @deployment.instances.count, :pretty_view_toggle => 'enabled'},
@@ -383,12 +383,12 @@ class DeploymentsController < ApplicationController
       { :name => t("providers.provider"), :sortable => false }
     ]
     @pools = Pool.list_for_user(current_user, Privilege::CREATE, Deployment)
-    @deployments = Deployment.includes(:owner, :pool, :instances).
-                              apply_filters(:preset_filter_id => params[:deployments_preset_filter], :search_filter => params[:deployments_search]).
-                              list_for_user(current_user, Privilege::VIEW).
-                              where('deployments.pool_id' => @pools).
-                              order(sort_column(Deployment, "deployments.name") +' '+ sort_direction).
-                              paginate(:page => params[:page], :per_page => PER_PAGE)
+    @deployments = paginate_collection(Deployment.includes(:owner, :pool, :instances).
+                                                  apply_filters(:preset_filter_id => params[:deployments_preset_filter], :search_filter => params[:deployments_search]).
+                                                  list_for_user(current_user, Privilege::VIEW).
+                                                  where('deployments.pool_id' => @pools).
+                                                  order(sort_column(Deployment, "deployments.name") +' '+ sort_direction),
+                                       params[:page], PER_PAGE)
   end
 
   def count_additional_quota(deployment)
diff --git a/src/app/controllers/images_controller.rb b/src/app/controllers/images_controller.rb
index cad12f9..d71c1b1 100644
--- a/src/app/controllers/images_controller.rb
+++ b/src/app/controllers/images_controller.rb
@@ -27,7 +27,7 @@ class ImagesController < ApplicationController
       { :name => t('images.index.architecture'), :sort_attr => :name },
       { :name => t('images.index.last_rebuild'), :sortable => false },
     ]
-    @images = Aeolus::Image::Warehouse::Image.all.paginate(:page => params[:page], :per_page => PER_PAGE)
+    @images = paginate_collection(Aeolus::Image::Warehouse::Image.all, params[:page], PER_PAGE)
     respond_to do |format|
       format.html
       format.js { render :partial => 'list' }
diff --git a/src/app/controllers/instances_controller.rb b/src/app/controllers/instances_controller.rb
index 7773d81..1153e96 100644
--- a/src/app/controllers/instances_controller.rb
+++ b/src/app/controllers/instances_controller.rb
@@ -248,14 +248,15 @@ class InstancesController < ApplicationController
 
   def load_instances
     if params[:deployment_id].blank?
-      @instances = Instance.includes(:owner).apply_filters(:preset_filter_id => params[:instances_preset_filter], :search_filter => params[:instances_search]).
-                            list_for_user(current_user, Privilege::VIEW).list(sort_column(Instance), sort_direction).
-                            where("instances.pool_id" => @pools).paginate(:page => params[:page], :per_page => PER_PAGE)
+      @instances = paginate_collection(Instance.includes(:owner).apply_filters(:preset_filter_id => params[:instances_preset_filter], :search_filter => params[:instances_search]).
+                                                list_for_user(current_user, Privilege::VIEW).list(sort_column(Instance), sort_direction).
+                                                where("instances.pool_id" => @pools),
+                                       params[:page], PER_PAGE)
     else
-      @instances = Instance.includes(:owner).apply_filters(:preset_filter_id => params[:instances_preset_filter], :search_filter => params[:instances_search]).
-                            list(sort_column(Instance), sort_direction).list_for_user(current_user, Privilege::VIEW).
-                            where("instances.pool_id" => @pools, "instances.deployment_id" => params[:deployment_id]).
-                            paginate(:page => params[:page], :per_page => PER_PAGE)
+      @instances = paginate_collection(Instance.includes(:owner).apply_filters(:preset_filter_id => params[:instances_preset_filter], :search_filter => params[:instances_search]).
+                                                list(sort_column(Instance), sort_direction).list_for_user(current_user, Privilege::VIEW).
+                                                where("instances.pool_id" => @pools, "instances.deployment_id" => params[:deployment_id]),
+                                       params[:page], PER_PAGE)
     end
   end
 
diff --git a/src/app/controllers/pools_controller.rb b/src/app/controllers/pools_controller.rb
index abc3203..347c180 100644
--- a/src/app/controllers/pools_controller.rb
+++ b/src/app/controllers/pools_controller.rb
@@ -44,25 +44,30 @@ class PoolsController < ApplicationController
     @details_tab = @tabs.find {|t| t[:id] == details_tab_name} || @tabs.first[:name].downcase
 
     @user_pools = Pool.list_for_user(current_user, Privilege::CREATE, Deployment)
+
     if filter_view?
       case @details_tab[:id]
       when 'pools'
-        @pools = Pool.includes(:deployments, :instances).apply_filters(:preset_filter_id => params[:pools_preset_filter], :search_filter => params[:pools_search]).
-                      list_for_user(current_user, Privilege::VIEW).list(sort_column(Pool), sort_direction).
-                      paginate(:page => params[:page], :per_page => PER_PAGE)
+        @pools = paginate_collection(Pool.includes(:deployments, :instances).
+                                          apply_filters(:preset_filter_id => params[:pools_preset_filter], :search_filter => params[:pools_search]).
+                                          list_for_user(current_user, Privilege::VIEW).list(sort_column(Pool), sort_direction),
+                                     params[:page], PER_PAGE)
       when 'instances'
         params[:instances_preset_filter] = "" unless params[:instances_preset_filter]
-        @instances = Instance.includes({:provider_account => :provider}).apply_filters(:preset_filter_id => params[:instances_preset_filter], :search_filter => params[:instances_search]).
-                              list_for_user(current_user, Privilege::VIEW).list(sort_column(Instance), sort_direction).
-                              paginate(:page => params[:page], :per_page => PER_PAGE)
+        @instances = paginate_collection(Instance.includes({:provider_account => :provider}).
+                                                  apply_filters(:preset_filter_id => params[:instances_preset_filter], :search_filter => params[:instances_search]).
+                                                  list_for_user(current_user, Privilege::VIEW).list(sort_column(Instance), sort_direction),
+                                         params[:page], PER_PAGE)
       when 'deployments'
-        @deployments = Deployment.includes(:pool, :instances).apply_filters(:preset_filter_id => params[:deployments_preset_filter], :search_filter => params[:deployments_search]).
-                                  list_for_user(current_user, Privilege::VIEW).list(sort_column(Deployment), sort_direction).
-                                  paginate(:page => params[:page], :per_page => PER_PAGE)
+        @deployments = paginate_collection(Deployment.includes(:pool, :instances).
+                                                      apply_filters(:preset_filter_id => params[:deployments_preset_filter], :search_filter => params[:deployments_search]).
+                                                      list_for_user(current_user, Privilege::VIEW).list(sort_column(Deployment), sort_direction),
+                                           params[:page], PER_PAGE)
       end
     else
-      @pools = Pool.includes(:deployments, :instances, :quota, :catalogs).list_for_user(current_user, Privilege::VIEW).list(sort_column(Pool), sort_direction).
-                    paginate(:page => params[:page], :per_page => PER_PAGE)
+      @pools = paginate_collection(Pool.includes(:deployments, :instances, :quota, :catalogs).
+                                        list_for_user(current_user, Privilege::VIEW).list(sort_column(Pool), sort_direction),
+                                   params[:page], PER_PAGE)
     end
 
     statistics
@@ -109,16 +114,18 @@ class PoolsController < ApplicationController
 
     details_tab_name = params[:details_tab].blank? ? 'deployments' : params[:details_tab]
     @details_tab = @tabs.find {|t| t[:id] == details_tab_name} || @tabs.first[:name].downcase
-    @deployments = @pool.deployments.includes(:owner, :pool, :instances, :events).
-                         apply_filters(:preset_filter_id => params[:deployments_preset_filter], :search_filter => params[:deployments_search]).
-                         list_for_user(current_user, Privilege::VIEW).
-                         paginate(:page => params[:page], :per_page => PER_PAGE) if @details_tab[:id] == 'deployments'
+    @deployments = paginate_collection(@pool.deployments.includes(:owner, :pool, :instances, :events).
+                                                         apply_filters(:preset_filter_id => params[:deployments_preset_filter], :search_filter => params[:deployments_search]).
+                                                         list_for_user(current_user, Privilege::VIEW),
+                                       params[:page], PER_PAGE) if @details_tab[:id] == 'deployments'
     @view = @details_tab[:view]
     respond_to do |format|
       format.html { render :action => :show}
       format.js { render :partial => @view }
-      format.json { render :json => @pool.as_json(:with_deployments => true, :current_user => current_user,
-                                                  :page => params[:page], :per_page => PER_PAGE) }
+      format.json do
+        deployments = paginate_collection(@pool.deployments.list_for_user(current_user, Privilege::VIEW), params[:page], PER_PAGE).map {|d| d.as_json}
+        render :json => @pool.as_json.merge({:deployments => deployments})
+      end
     end
   end
 
diff --git a/src/app/models/pool.rb b/src/app/models/pool.rb
index 57a6caf..2604701 100644
--- a/src/app/models/pool.rb
+++ b/src/app/models/pool.rb
@@ -136,12 +136,6 @@ class Pool < ActiveRecord::Base
 
     })
 
-    if options[:with_deployments]
-      result[:deployments] = deployments.list_for_user(options[:current_user], Privilege::VIEW).
-                                         paginate(:page => options[:page], :per_page => options[:per_page]).
-                                         map {|d| d.as_json}
-    end
-
     result
   end
 
diff --git a/src/app/views/pools/_pretty_list.html.haml b/src/app/views/pools/_pretty_list.html.haml
index 47e7a8b..9b869b1 100644
--- a/src/app/views/pools/_pretty_list.html.haml
+++ b/src/app/views/pools/_pretty_list.html.haml
@@ -25,7 +25,7 @@
         %a.control{:href => '#'}
           %span Expand/Collapse
       %div.content
-        - deployments = pool.deployments.includes(:owner, :pool, :instances, :events).list_for_user(current_user, Privilege::VIEW).ascending_by_name.paginate(:page => params[:page], :per_page => PER_PAGE)
+        - deployments = paginate_collection(pool.deployments.includes(:owner, :pool, :instances, :events).list_for_user(current_user, Privilege::VIEW).ascending_by_name, params[:page], PER_PAGE)
         = render :partial => 'deployments', :locals => {:pool => pool, :deployments => deployments}
       %ul.content.actions
         %li
-- 
1.7.6.5




More information about the aeolus-devel mailing list