BP_Groups_Group::get() returns incorrect results when using meta_query with multiple meta_values and OR relationship

[richtelford]

I think I’ve found a bug in the BP_Groups_Group::get() function when performing meta_query with OR relationship.

Currently I have 4 groups to test with. Each group is assigned a meta_key => group_sport with values “running”, “swimming”, “mountain”, “winter” respectively. When I perform this:

$meta_query = array(
'relation' => 'OR',
array(
	"key" => "group_sport",
	"value" => 'mountain',
	"compare" => 'LIKE'
),
array(
	"key" => "group_sport",
	"value" => 'running',
	"compare" => 'LIKE'
));

$group_arr = BP_Groups_Group::get(array(
	'per_page'=>-1,
	'meta_query' => $meta_query
));

it retrieves mountain and running but also incorrectly retrieves swimming and winter.

On further investigation I noticed that table joins in BP_Groups_Group::get() and get_meta_query_sql() are done using commas rather than INNER JOIN, etc. I changed the functions to use INNER JOIN instead and it now works correctly on my test site. Can anyone else confirm this bug please? I'm running WP 4.0 and BP 2.0.2.

[boonebgorges]

Thanks for the report. This test ​https://buddypress.trac.wordpress.org/browser/trunk/tests/phpunit/testcases/groups/class-bp-groups-group.php#L137 passes, and it looks the same as your test. I also tested manually and it appears to be working as expected with data like you've presented here.

I noticed that table joins in BP_Groups_Group::get() and get_meta_query_sql() are done using commas rather than INNER JOIN, etc. I changed the functions to use INNER JOIN instead and it now works correctly on my test site

BuddyPress handles the necessary transformation - see ​https://buddypress.trac.wordpress.org/browser/trunk/src/bp-groups/bp-groups-classes.php#L849 There may be something on your installation that is causing the preg_match_all() checks there to fail. Could you try debugging in this area? You may also try disabling other plugins in case there is one that is doing something to the syntax of the query.

[richtelford]

I'm not seeing a test for the same key repeated with multiple values but I can see why that might not be necessary. I'll try on a vanilla install and report back. Thanks for quick response.

[richtelford]

Just tested on a fresh install and problem persists. To replicate I created 4 groups within wp-admin and then manually added unique meta data for each group to wp_bp_groups_groupmeta. Then to quickly test I added the following to the top of /wp-content/buddypress/bp-templates/bp-legacy/buddypress/groups/index.php:

$meta_query[] = array(
		"key" => "sport",
		"value" => 'running',
		"compare" => 'LIKE'
	);

$meta_query[] = array(
		"key" => "sport",
		"value" => 'mountain',
		"compare" => 'LIKE'
	);


if(count($meta_query) > 1)
{
	$meta_query = array_merge(array('relation' => 'OR'), $meta_query);
}

print_r($meta_query);

$group_result = BP_Groups_Group::get(array(
	'per_page'=>-1,
	'meta_query' => $meta_query
));	


print_r($group_result);

This returns all 4 groups instead of the 2 I was expecting.

[boonebgorges]

I've performed exactly the steps that you've laid out above (as much as I could - more below) and it's still working properly for me. Only the correct 2 groups are being returned.

When you say "manually added unique meta data" I'm assuming that means that you went to the mysql command line and did something like this:

INSERT INTO wp_bp_groups_groupmeta (group_id,meta_key,meta_value) VALUES (5,'sport','swimming');

If so, this makes me think that you might have a caching issue on your hands. Check to see if you have some sort of aggressive PHP or MySQL-level caching going on in your current environment. You might double check this by testing for the problematic behavior on a different server/computer.

[richtelford]

I manually added the meta data using Navicat but yeah similar to executing an SQL statement or using PHPMyAdmin. This is actually on a different server to when I first encountered the problem. I'll try again - this time using a localhost setup and see what happens.

Can I just confirm what versions of WP and BP you are running? Are they the latest yes?

I don't think there is any aggressive caching going on as both environments I've tried on currently are production servers which we currently use to host lots of other WordPress sites and we've not experienced any caching issues (apart from when using caching plugins).

I'll report back with my localhost results shortly.

[boonebgorges]

I am testing with BP r8985 and WP r29475.

If you are still reproducing the problem with another setup, I encourage you to continue your debugging by looking here ​https://buddypress.trac.wordpress.org/browser/trunk/src/bp-groups/bp-groups-classes.php#L849 to see if you can identify the root cause.

[richtelford]

Yeah this is happening on my localhost as well.

I know that joining tables together with comma syntax can be problematic - as is hinted at in the get_meta_query_sql() code. When debugging this initially, I took the SQL output created by get_meta_query_sql() and get() from bp-groups-classes.php, dropped it into Navicat to see what was going on. The first thing I tried was replacing the comma joins with inner joins and the problem (which I'm experiencing) was sorted. So perhaps the problem is related to ambiguous joins.

I'll try updating to the trunk revisions you are testing with - see if that makes a difference. There is no caching enabled on my localhost setup.

[boonebgorges]

I know that joining tables together with comma syntax can be problematic - as is hinted at in the get_meta_query_sql() code. When debugging this initially, I took the SQL output created by get_meta_query_sql() and get() from bp-groups-classes.php, dropped it into Navicat to see what was going on. The first thing I tried was replacing the comma joins with inner joins and the problem (which I'm experiencing) was sorted. So perhaps the problem is related to ambiguous joins.

As noted above and in the comments to the code, the INNER JOINs from WP_Meta_Query are supposed to be swapped out with the proper comma syntax in get_meta_query_sql(). If this is not happening - if the SQL being returned from get_meta_query_sql() still contains INNER JOINs (and the WHERE clause does not contain the necessary column mappings that are produced by ON in the case of JOINs), then there something is awry in get_meta_query_sql(). I'd urge you to debug there a bit more, or, barring that, please share the SQL chunks that are being passed into and out of get_meta_query_sql()

[richtelford]

What I'm saying is that maybe the comma syntax (which I try and steer clear of) is the cause of the problem. The solution would be to replace the comma syntax with the correct relevant JOINs (as I have done). Comma syntax can result in ambiguous JOINs and is reflected by the incorrect results I'm getting.

Perhaps you need a unit test for different meta_value but using the same key?

$meta_query[] = array(
	"key" => "samekey",
	"value" => 'value1',
	"compare" => 'LIKE'
);

$meta_query[] = array(
	"key" => "samekey",
	"value" => 'value2',
	"compare" => 'LIKE'
);

if(count($meta_query) > 1)
{
	$meta_query = array_merge(array('relation' => 'OR'), $meta_query);
}

$group_result = BP_Groups_Group::get(array(
	'per_page'=>-1,
	'meta_query' => $meta_query
));	

[boonebgorges]

What I'm saying is that maybe the comma syntax (which I try and steer clear of) is the cause of the problem.

Yes, I get that, and I agree that it's good to avoid. But it is very complicated to change this syntax, because we have a variety of backward compatibility issues to solve first. See #5451.

The purpose of the transformations in get_meta_query_sql() is to continue using the comma syntax even though WP_Meta_Query::get_sql() outputs INNER JOIN syntax. This is working in our unit tests, and yours is the first instance I've heard of where this issue specifically (SQL syntax) is causing meta_query to break. So, for the time being, I'm strongly inclined to find out what's causing the problem in your specific case, rather than doing the major refactoring your suggestion would require.

You may also take a look at #5824. It's possible that the changes in 2.1/trunk related to this ticket would also address what you're seeing (though tbh I can't see at a glance how this would be the case).

Perhaps you need a unit test for different meta_value but using the same key?

I have written such a test locally, and I have also done the test manually as you described above, and in all cases I'm unable to reproduce.

If you can share the pre- and post-processed SQL, I might be able to see what's up.

[richtelford]

Yes, I get that, and I agree that it's good to avoid.

Great I wasn't sure if that's what you were meaning or not.

But it is very complicated to change this syntax, because we have a variety of backward compatibility issues to solve first. See #5451.

I understand.

Here is the SQL output ($paged_groups_sql) before changing anything:

SELECT DISTINCT g.id, g.*, gm1.meta_value AS total_member_count, gm2.meta_value AS last_activity 
FROM wp_bp_groups_groupmeta gm1, wp_bp_groups_groupmeta gm2,wp_bp_groups_groupmeta,wp_bp_groups_groupmeta AS mt1,wp_bp_groups_groupmeta AS mt2, wp_bp_groups g WHERE g.id = gm1.group_id 
AND g.id = gm2.group_id 
AND gm2.meta_key = 'last_activity' A
ND gm1.meta_key = 'total_member_count' 
AND g.status != 'hidden' 
AND ( (g.id = wp_bp_groups_groupmeta.group_id AND wp_bp_groups_groupmeta.meta_key = 'group_sport' AND CAST(wp_bp_groups_groupmeta.meta_value AS CHAR) LIKE '%mountain%') OR (mt1.meta_key = 'group_sport' AND CAST(mt1.meta_value AS CHAR) LIKE '%running%') OR (mt2.meta_key = 'group_sport' AND CAST(mt2.meta_value AS CHAR) LIKE '%winter%') ) 
ORDER BY g.date_created DESC

You can see the problem in the second last line. The brackets don't form correctly around the arguments. Here is the SQL output after my changes:

SELECT DISTINCT g.id, g.*, gm1.meta_value AS total_member_count, gm2.meta_value AS last_activity 
FROM wp_bp_groups g 
INNER JOIN wp_bp_groups_groupmeta gm1 ON g.id = gm1.group_id 
INNER JOIN wp_bp_groups_groupmeta gm2 ON g.id = gm2.group_id 
INNER JOIN wp_bp_groups_groupmeta ON (g.id = wp_bp_groups_groupmeta.group_id) 
INNER JOIN wp_bp_groups_groupmeta AS mt1 ON (g.id = mt1.group_id) 
INNER JOIN wp_bp_groups_groupmeta AS mt2 ON (g.id = mt2.group_id) 
WHERE gm2.meta_key = 'last_activity' 
AND gm1.meta_key = 'total_member_count' 
AND g.status != 'hidden' 
AND ( (wp_bp_groups_groupmeta.meta_key = 'group_sport' AND CAST(wp_bp_groups_groupmeta.meta_value AS CHAR) LIKE '%mountain%') OR (mt1.meta_key = 'group_sport' AND CAST(mt1.meta_value AS CHAR) LIKE '%running%') OR (mt2.meta_key = 'group_sport' AND CAST(mt2.meta_value AS CHAR) LIKE '%winter%') ) 
ORDER BY g.date_created DESC

Executing the first query returns 4 results (all groups I've created). Executing the second returns 2 results (there are no groups with group_sport = 'winter').

[boonebgorges]

Thanks, richtelford. The problem is not improper bracketing (at least as far as I can see). It appears to be, as you suggested earlier, ambiguous joining. The first clause of the second-to-last line contains g.id = wp_bp_groups_groupmeta.group_id, but the second and third clauses do not contain corresponding subclauses.

Is it possible for you to test this against the latest BP trunk, after the changes in r8949? It's possible that this is a duplicate issue, and is already solved in BP trunk.

[richtelford]

the second and third clauses do not contain corresponding subclauses

That's kinda what I meant by improper bracketing.

I've got latest BP trunk running on my localhost site but still same issue. The SQL output from that is:

SELECT DISTINCT g.id, g.*, gm1.meta_value AS total_member_count, gm2.meta_value AS last_activity 
FROM wp_bp_groups_groupmeta gm1, wp_bp_groups_groupmeta gm2,wp_bp_groups_groupmeta,wp_bp_groups_groupmeta AS mt1,wp_bp_groups_groupmeta AS mt2, wp_bp_groups g WHERE g.id = gm1.group_id 
AND g.id = gm2.group_id 
AND gm2.meta_key = 'last_activity' 
AND gm1.meta_key = 'total_member_count' 
AND g.status != 'hidden' 
AND ( (g.id = wp_bp_groups_groupmeta.group_id 
AND wp_bp_groups_groupmeta.meta_key = 'sport' 
AND CAST(wp_bp_groups_groupmeta.meta_value AS CHAR) LIKE '%running%') 
OR (mt1.meta_key = 'sport' AND CAST(mt1.meta_value AS CHAR) LIKE '%mountain%') 
OR (mt2.meta_key = 'sport' AND CAST(mt2.meta_value AS CHAR) LIKE '%winter%') ) 
ORDER BY g.date_created DESC

[boonebgorges]

richtelford - Can you please share the value of $meta_sql as it's being generated inside of get_meta_query_sql()? I'm trying to figure out why the regex is failing.

[boonebgorges]

In 9021:

Add unit tests for group meta_query when queries with the same 'key' are joined by OR

This query is failing when the 'compare' operator is 'LIKE'.

See #5874

[boonebgorges]

I've managed to reproduce with a unit test (test_get_with_meta_query_multiple_clauses_relation_or_shared_meta_key_relation_like()). The following conditions have to hold to demonstrate the failure:

• you have to use relation 'OR'
• subqueries must share the same 'key'
• subqueries must use compare 'LIKE' (though this might also happen with other compare operators - not sure)

richtelford - The good news is that, for your current purpose, you should just be able to remove the 'compare' => 'LIKE' lines from your query. WP_Meta_Query will assume you mean '=', which it seems is what you actually do mean here. My tests suggest that this will solve your specific problem.

I have not examined carefully why changing 'compare' breaks things. It's possible that it has to do with the fact that the pseudo-ON clauses (AND g.id = gm1.group_id) are not properly grouped into the bracketed OR clauses - though I have no idea why this wouldn't affect '=' clauses. Let's have a closer look for 2.2.

[boonebgorges]

richtelford - Meant to say thanks for helping to debug. Thanks!

[richtelford]

No problem. It was fortunate I needed to perform these meta data queries for the project I'm working on.

Are you sure this only happens when using LIKE? I think the same issue occurs using "=" as well. I can't check right now but will do so when I get into the office tomorrow morning.

I can submit a patch based on the diff between my two files if that helps? I'm not sure I'll have time to fully test it against all the other query arguments as well though as I'm in the middle of a project which needs completing fairly sharpish.

[boonebgorges]

Are you sure this only happens when using LIKE?

As noted above, I'm not sure if it happens with other compare operators. I can say for certain that it does not happen if you leave out 'compare', which in your case seems like will be the correct behavior anyway.

I can submit a patch based on the diff between my two files if that helps?

You can provide it if you'd like, but again (assuming you're talking about the INNER JOINs) it's not going to be possible to change the comma syntax at the moment. The immediate goal is to figure out why the translation is not working.

[richtelford]

The immediate goal is to figure out why the translation is not working.

What do you mean by translation?

I can confirm that using LIKE, "=" or no compare operator reproduces the same bug which makes sense considering it's the joining of tables via the comma syntax that is causing the problem.

it's not going to be possible to change the comma syntax at the moment

Assuming that by using the comma syntax, INNER JOINs were desired originally and the current mechanism is already faulty, changing to actual INNER JOINs wouldn't be such a radical change? I understand there are other tickets on the roadmap though so fair enough if this one has to wait.

[boonebgorges]

In 9082:

Fix group meta query SQL manipulation.

The wildcard operators were acting too greedily, preventing some JOIN clauses
from being recognized and resulting in invalid SQL syntax. This problem had
previously been masked by ideosyncracies of WP's WP_Meta_Query SQL syntax,
which changed in ​[WP29887].

This change should be considered a band-aid fix for what is already a band-aid
fix. The proper resolution for this issue is to migrate away from the comma
syntax for JOINs used in BP_Groups_Group. See #5874, #5451.

[boonebgorges]

Removing failing test from repo. See #6223.

[boonebgorges]

In 11071:

Groups: Overhaul BP_Groups_Group::get() SQL query.

The new query follows the WP and BP convention of "split" queries: one
for the IDs matching the query parameters, and a second one for the
objects corresponding to the matched IDs. This configuration allows for
improved caching and better performance, especially on installations
with large numbers of groups.

The rewrite serves as a convenient excuse to address a number of
longtime annoyances with the way group queries work:

• Previously, comma syntax was used for table joins, rather than the JOIN keyword. This required some string manipulation when using external tools for generating SQL fragments, such as WP_Tax_Query and WP_Meta_Query. See #5099, #5874. We now use the more standard JOIN syntax.
• The logic for assembling the "total" query is overwhelmingly simpler.
• Previously, group queries required three joined tables: the groups table, one groupmeta table (for last_activity), and a second groupmeta table (for total_member_count). After this changeset, these tables will only be joined when needed for sorting (orderby or type). The last_activity and total_member_count properties, when needed for display in a group loop, are lazyloaded from groupmeta - where they're precached by default (see update_meta_cache) - rather than pulled as part of the primary query, and are made available via __get() for backward compatibility.

See #5451, #5874. Fixes #5099.