berlindb / core

All of the required core code
MIT License
252 stars 27 forks source link

`$query->found_items` empty when paged is out of range #128

Closed rmorse closed 2 years ago

rmorse commented 2 years ago

Just noticed a quirk or maybe just an odd use case on my part.

What I noticed was $query->found_items is always 0 if you set paged to something out of range.

An example - if a user deletes some records, and ends up on a page number that no longer exists (by clicking back in the browser for example). I would want to use found_items to either: still show pagination, or redirect to the last available page.

My initial assumption was that found_items would ignore the paged value essentially.

I did a little digging (as far as I could) and it looks like SELECT FOUND_ROWS() is used to get this count, so perhaps its a limitation of that?

I've worked around this by running a seperate count query when needed, so no issue really - just curious if this is intended behaviour or possibly a problem that could be worth solving within berlindb.

Thanks

JJJ commented 2 years ago

Hey @rmorse 👋 Great question/observation!

I believe what you are experiencing was originally intended behavior, but perhaps should not be.


Inside the Query::set_found_items() method:

https://github.com/berlindb/core/blob/2d94af59a0d5642b659af7b62c7674811f25df00/src/Database/Query.php#L617-L618

...specifically...

is_array( $item_ids ) &&

...is basically an optimization that skips the SELECT FOUND_ROWS() query if no items were found in the related database query. The unintended consequence is, like you've discovered, some rows may exist even if the arguments for that query are returning an out-of-bounds empty result.


When you are running your separate count query, are you using SELECT FOUND_ROWS() or something else?

I am thinking that if we remove the above optimization, found_rows will return the value you are expecting.

JJJ commented 2 years ago

Oh, and no_found_rows to false also adds SQL_CALC_FOUND_ROWS to request clauses.

Perhaps it shouldn't be true by default? Hmm...

Looks like MySQL is deprecating FOUND_ROWS() starting with version 8.0.17. This may need more work, regardless!

https://dev.mysql.com/doc/refman/8.0/en/information-functions.html#function_found-rows

rmorse commented 2 years ago

👋

When you are running your separate count query, are you using SELECT FOUND_ROWS() or something else?

Yeah I'm running the same query, with paged removed and count arg set to true -

$args = (
    ...
    'count' => true,
);

Which I believe overrides the settings here:

// Never limit, never update item/meta caches when counting
if ( ! empty( $this->query_vars['count'] ) ) {
    $this->query_vars['number']            = false;
    $this->query_vars['no_found_rows']     = true;
    $this->query_vars['update_item_cache'] = false;
    $this->query_vars['update_meta_cache'] = false;
}

I think BerlinDB is not using FOUND_ROWS() under the hood for that...?

Looks like MySQL is deprecating FOUND_ROWS() starting with version 8.0.17. This may need more work, regardless!

Haha ok, that sounds fun! Let me know if there is anything I can do to help out, but these internals are a bit beyond me for now...

JJJ commented 2 years ago

Let me know if there is anything I can do to help out, but these internals are a bit beyond me for now...

Thank you for the kind and generous offer. 🙏

I'll get this issue fixed, but you 'gotta fix the next one! 💪😁

rmorse commented 2 years ago

Deal 🤝

JJJ commented 2 years ago

Mostly fixed/improved via #140.

Want to tackle the MySQL 8 support still. 🦾

JJJ commented 2 years ago

140 merged into release/2.1.0 branch. Give it a test! 🙏

rmorse commented 2 years ago

Awesome, thanks @JJJ - I'll test in my project over the next week