[PATCH conductor] BZ805909: Detect if paginated collection is out of bounds (rev. 3)

Jan Provaznik jprovazn at redhat.com
Thu Apr 19 13:02:15 UTC 2012


On 04/13/2012 01:39 PM, ifarkas at redhat.com wrote:
> From: Imre Farkas<ifarkas at redhat.com>
>
> https://bugzilla.redhat.com/show_bug.cgi?id=805909
>
> Resending it because a part of the previous revision has already been committed.
> ---
>   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 9556dee..4604bd2 100644
> --- a/src/app/models/pool.rb
> +++ b/src/app/models/pool.rb
> @@ -137,12 +137,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

ACK



More information about the aeolus-devel mailing list