Documentation: Check the consistency with page number assignation change in 9+

[phyzical]

👀 Before submitting...

• I upgraded to pagy version 9.0.9
• I searched through the Documentation
• I searched through the Issues
• I searched through the Q&A

🧐 REQUIREMENTS

• I am providing a VALID code file that confirms the bug
• I am NOT posting any of the USELESS THINGS listed above
• I am aware that this issue will be automatically closed if the code file is missing or INVALID

💬 Description

Not sure if this is a bug but in 9 we had a set of tests fail that assumed that invalid page numbers result in the first page to be returned i.e providing
page: -1
page: 'blah'

in 8 would result in the first page of results
in 9 we get

Pagy::VariableError:
       expected :page >= 1; got -1

Pagy::VariableError:
       expected :page >= 1; got 0

TBH, i think its just a set of silly tests ourside but i could see it biting larger projects where people do dodge to reset pages ect.

Might just be worth getting added to the breaking changes list?

[phyzical]

incase others need this and this is closed as 'not fixing' i monkey patched it by

@vars[name] = 1 if name == :page && (@vars[name].instance_of?(String) || (@vars[name]&.respond_to?(:to_i) && @vars[name].to_i <= 0))

class Pagy
  module SharedMethods
 def assign_and_check(name_min)
      name_min.each do |name, min|
        @vars[name] = 1 if name == :page && (@vars[name].instance_of?(String) || (@vars[name]&.respond_to?(:to_i) && @vars[name].to_i <= 0))
        raise VariableError.new(self, name, ">= #{min}", @vars[name]) \
        unless @vars[name]&.respond_to?(:to_i) && \
               instance_variable_set(:"@#{name}", @vars[name].to_i) >= min
      end
    end
  end
end

[ddnexus]

@phyzical thank you for your report.

I am wondering how you don't get the automatic assignation of the page variable, through the pagy_get_page method.

Could you please use one of the Playground apps to show how to make it fail?

[phyzical]

I assume then it is just simply the fact its a redundant spec causing the issue to begin with, as its explicitly passing in the param.

Based on what your saying its probably safe to just close this. Just figured id raise it as it does seem to be a functionality change not mentioned in the breaking changes.

context 'when the client passes a nonsense page param' do
    it 'renders the first page instead of falling over' do
      login_as_user users(:global_admin)
      get users_path(page: -1)
      expect(response).to have_http_status(:ok)
      expect(response.body).to include('Displaying Users 1-10')

      get users_path(page: 'nonsense')
      expect(response).to have_http_status(:ok)
      expect(response.body).to include('Displaying Users 1-10')

      get users_path(page: '')
      expect(response).to have_http_status(:ok)
      expect(response.body).to include('Displaying Users 1-10')
    end
  end

[phyzical]

and why it happens
/lib/pagy/backend.rb

vars[:page] ||= pagy_get_page(vars) its just early exiting as its explicitly provided

[ddnexus]

That explains it. Indeed, if you explicitly pass a variable, you are responsible for passing it right.

Just figured id raise it as it does seem to be a functionality change not mentioned in the breaking changes.

I will check whether the docs and breaking changes are consistent with the current behavior before closing it.

[benkoshy]

I'm testing on Pagy 8 I was unable to reproduce the results such that the first page is retrieved?

I'm testing from the command line via the Pagy::Console on pagy 8.6.3.

I also added a custom test in pagy 8 and was unable to replicate such that the first page of results would be received, Sane for pagy 9.

      _ { Pagy.new(count: 100, page: "blah") }.must_be_instance_of Pagy       # fails
      _ { Pagy.new(count: 0, page: "-1") }.must_raise Pagy::VariableError      # passes
      _ { Pagy.new(count: 0, page: "blah") }.must_raise Pagy::VariableError    # passes
      _ { Pagy.new(count: 0, page: -1) }.must_raise Pagy::VariableError        # passes

Also tested via console.

/home/koshy/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/pagy-8.6.3/lib/pagy.rb:94:in `block in setup_vars': expected :page >= 1; got -1 (Pagy::VariableError)
	from /home/koshy/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/pagy-8.6.3/lib/pagy.rb:93:in `each'
	from /home/koshy/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/pagy-8.6.3/lib/pagy.rb:93:in `setup_vars'
	from /home/koshy/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/pagy-8.6.3/lib/pagy.rb:30:in `initialize'
	from /home/koshy/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/pagy-8.6.3/lib/pagy/extras/array.rb:11:in `new'
	from /home/koshy/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/pagy-8.6.3/lib/pagy/extras/array.rb:11:in `pagy_array'
	from (irb):8:in `<main>'
	from /home/koshy/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/irb-1.12.0/exe/irb:9:in `<top (required)>'
	from /home/koshy/.rbenv/versions/3.1.2/bin/irb:25:in `load'
	from /home/koshy/.rbenv/versions/3.1.2/bin/irb:25:in `<main>'
irb(main):009> pagy_array((1..1000).to_a, page: "blah")
/home/koshy/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/pagy-8.6.3/lib/pagy.rb:94:in `block in setup_vars': expected :page >= 1; got "blah" (Pagy::VariableError)
	from /home/koshy/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/pagy-8.6.3/lib/pagy.rb:93:in `each'
	from /home/koshy/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/pagy-8.6.3/lib/pagy.rb:93:in `setup_vars'
	from /home/koshy/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/pagy-8.6.3/lib/pagy.rb:30:in `initialize'
	from /home/koshy/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/pagy-8.6.3/lib/pagy/extras/array.rb:11:in `new'
	from /home/koshy/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/pagy-8.6.3/lib/pagy/extras/array.rb:11:in `pagy_array'
	from (irb):9:in `<main>'
	from /home/koshy/.rbenv/versions/3.1.2/lib/ruby/gems/3.1.0/gems/irb-1.12.0/exe/irb:9:in `<top (required)>'
	from /home/koshy/.rbenv/versions/3.1.2/bin/irb:25:in `load'
	from /home/koshy/.rbenv/versions/3.1.2/bin/irb:25:in `<main>'

It would seem that the behaviour is expected?

Any ideas on how to replicate the behaviour of pagy 8 such that the results are acheived?

[phyzical]

thanks for giving it a go, ill try by best to summarise the usecase that resulted in me raising this (closed source so its obviously a bit paraphrased)

So its in the context of a request with rails, so maybe rails request handling of the params has a say in all this maybe? that's all i can really think of code wise i cant seem to find anything super fancy

This is the spec chunk hitting an endpoint and manually providing the values

RSpec.describe Pagination, type: :request do
 context 'when the client passes a nonsense page param' do
    it 'renders the first page instead of falling over' do
      get users_path(page: -1)
      expect(response).to have_http_status(:ok)
      expect(response.body).to include('Displaying Users 1-10')

      get users_path(page: 'nonsense')
      expect(response).to have_http_status(:ok)
      expect(response.body).to include('Displaying Users 1-10')

      get users_path(page: '')
      expect(response).to have_http_status(:ok)
      expect(response.body).to include('Displaying Users 1-10')
    end
  end
end

This is that endpoint

module Users
  class UsersController < ApplicationController
    def index
      @pagy, @users = pagy(User.all)
    end
  end
end

class ApplicationController < ActionController::Base
  include Pagination
end

Pagination module (funny i know as there was logic that should have complained if these tests provided these values but i guess dead code?.. that became live in 9? idk...)

module Pagination
  extend ActiveSupport::Concern
  include Pagy::Backend

  included do
    rescue_from Pagy::VariableError do |err|
      # Blame the client if they've passed a nonsense per_page param like "0", "-1", or "pikachu"
      raise ActionController::BadRequest if err.message.start_with?('expected :limit >= 1')

      # Other problems are likely on our end. Re-raise to blame ourselves.
      raise err
    end
  end
end

Ill do one more dive to see if im looking down the wrong rabbit hole and there's a view or something that is hitting the code path causing the issue

nah it is 100% @pagy, @users = pagy(User.all) that started throwing in 9 that just didn't for us in 8 (8.6.3 to be super specific)

[ddnexus]

nah it is 100% @pagy, @users = pagy(User.all) that started throwing in 9 that just didn't for us in 8 (8.6.3 to be super specific)

So you should be able to edit a rails.ru of the playground apps to show that behavior. Just guessing make us all waste time running in circles.

[benkoshy]

I have been unable to reproduce this issue with a complete and self-contained example.

If I have made an error or if one is produced, please feel free to re-open this issue.