Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Missing next page link when the resource is a postgres table has json columns #439

Open
MattFenelon opened this issue Oct 18, 2022 · 1 comment

Comments

@MattFenelon
Copy link
Contributor

The next link isn't included (and the last page link does not have a page number) when the resource has json columns because of the use of distinct.count(:all) in:

def count(scope, attr)
if attr.to_sym == :total
scope.distinct.count(:all)
else
scope.distinct.count(attr)
end
end

In postgresql, json columns can’t be included in a DISTINCT because postgresql does not have an equality operator for json columns, only jsonb.

In addition, the usual comparison operators shown in Table 9.1 are available for jsonb, though not for json.
PostgreSQL: Documentation: 15: 9.16. JSON Functions and Operators

Digging into this a bit more, I believe the Graphiti::Delegates::Pagination.links method is responsible for building the pagination links. That method calls has_next_page?:

def links
@links ||= {}.tap do |links|
links[:self] = pagination_link(current_page)
links[:first] = pagination_link(1)
links[:last] = pagination_link(last_page)
links[:prev] = pagination_link(current_page - 1) if has_previous_page?
links[:next] = pagination_link(current_page + 1) if has_next_page?
end.select { |k, v| !v.nil? }
end

In a rails console I ran:

> Graphiti::Delegates::Pagination.new(ModelWithJsonColumnsResource.all).has_next_page?
=> false

The return value of false is odd because my development environment has 18k records for that resource:

> ModelWithJsonColumns.count
=> 18000

has_next_page? calls the private methods current_page and last_page. current_page returns 1, as expected (it's the first page). last_page, on the other hand, returns nil. I expected it to return a high number (18,000 / page_size).

last_page calls item_count and calling that method returns 0, not the expected actual count of 18,000.

So it looks like there is something wrong with item_count.

item_count is assigned using:

@item_count = item_count_from_proxy || item_count_from_stats

item_count_from_proxy returns nil. I believe that's expected.

item_count_from_stats however returns an exception:

> Graphiti::Delegates::Pagination.new(ModelWithJsonColumnsResource).send(:item_count_from_stats)

   (0.8ms)  SELECT COUNT(*) FROM (SELECT DISTINCT  FROM (SELECT DISTINCT ON () "model_with_json_columns".* FROM "model_with_json_columns" WHERE  ORDER BY ) model_with_json_columns ORDER BY "model_with_json_columns"."id" ASC) subquery_for_count
ActiveRecord::StatementInvalid: PG::UndefinedFunction: ERROR:  could not identify an equality operator for type json
LINE 1: ..."another_column", "model_with_json_columns"."some_json_column", "model_with_json_columns...
                                                                           ^

from /usr/local/bundle/gems/activerecord-6.1.6.1/lib/active_record/connection_adapters/postgresql_adapter.rb:672:in `exec_params'
Caused by PG::UndefinedFunction: ERROR:  could not identify an equality operator for type json
LINE 1: ..."another_column", "model_with_json_columns"."some_json_column", "model_with_json_columns...
                                                                           ^

from /usr/local/bundle/gems/activerecord-6.1.6.1/lib/active_record/connection_adapters/postgresql_adapter.rb:672:in `exec_params'

And what does item_count do when it sees an exception? Return 0:

rescue
# FIXME: Unable to log because of how rspec mocks were
# created for the logger. In other words, logging here will
# break tests.
# Graphiti.logger.warn(e.message)
@item_count = 0

That's why has_next_page? is returning false, and why the :next link is not being included in the output.

I'm not sure what the fix could be. Is there a reason why distinct is the default over a simple count? Could the distinct be dropped? Could the count(:all) be changed to count(:id)?

@Zeneixe
Copy link

Zeneixe commented May 7, 2024

BUMP on this issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants