Skip to:
Content

BuddyPress.org

Opened 3 years ago

Closed 3 years ago

#7248 closed defect (bug) (fixed)

include = [ 0 ] should result in a "no results" query in `BP_User_Query`

Reported by: boonebgorges Owned by: boonebgorges
Milestone: 2.7 Priority: high
Severity: normal Version:
Component: Core Keywords: has-patch 2nd-opinion
Cc:

Description

A client site just ran into MySQL issues because of a bit of code that was occasionally doing queries like this:

$include_ids = run_some_function_to_get_include_ids();

// Ensure that an empty $include_ids doesn't return any results
if ( empty( $include_ids ) ) {
    $include_ids = array( 0 );
}

if ( bp_has_members( array( 'include' => $include_ids ) ) {
    // ...
}

Because user_id is never 0, MySQL is forced to scan the entire table just to return no results. This takes a very long time in some cases. Since we know that there will be no results, we should bail early in this special case (with 0=1).

Attachments (1)

7248.diff (1.8 KB) - added by boonebgorges 3 years ago.

Download all attachments as: .zip

Change History (6)

#1 @boonebgorges
3 years ago

  • Priority changed from normal to high

It turns out that this affects BP core in at least one place: BP_Group_Member_Query::get_include_ids() passes array( 0 ) to 'include' when the group has no non-admin members.

@boonebgorges
3 years ago

#2 @boonebgorges
3 years ago

  • Keywords has-patch 2nd-opinion added; needs-patch removed

7248.diff is what I have in mind. Can I get a review?

#3 @DJPaul
3 years ago

Looks okay. If you don't have time or interest to go through and look at all the other queries, this is a good fix to go in regardless. Only nit is swap the == for the type-strict version. :)

#4 @boonebgorges
3 years ago

  • Owner set to boonebgorges
  • Status changed from new to assigned

Thanks, @DJPaul. The good thing about fixing the issue in BP_User_Query is that it'll resolve the problem for BP_Group_Member_Query and any other consumer of BP_User_Query. But you're right that a review of other query classes would be useful.

The non-strict comparison was intentional, in case someone was passing in '0' instead of 0. If you don't think this is worth checking, it can be removed, though I have a feeling the loose check will catch more errors.

#5 @boonebgorges
3 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 11067:

Bail early if an array containing only 0 is passed to 'include' in user queries.

When 'include' is [ 0 ] or [ '0' ]`, there's no reason to run a costly
SQL query - we can immediately report no results.

Fixes #7248.

Note: See TracTickets for help on using tickets.