Opened 10 years ago
Last modified 8 years ago
#5874 new defect (bug)
BP_Groups_Group::get() returns incorrect results when using meta_query with multiple meta_values and OR relationship
Reported by: | richtelford | Owned by: | |
---|---|---|---|
Milestone: | Awaiting Contributions | Priority: | normal |
Severity: | normal | Version: | |
Component: | Groups | Keywords: | needs-patch |
Cc: |
Description
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.
Attachments (1)
Change History (26)
#2
@
10 years ago
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.
#3
@
10 years ago
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.
#4
@
10 years ago
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.
#5
@
10 years ago
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.
#6
@
10 years ago
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.
#7
@
10 years ago
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.
#8
@
10 years ago
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()
#9
@
10 years ago
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 ));
#10
@
10 years ago
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.
#11
@
10 years ago
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').
#12
@
10 years ago
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.
#13
@
10 years ago
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
#14
@
10 years ago
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.
#16
@
10 years ago
- Keywords needs-patch added
- Milestone changed from Awaiting Review to 2.2
- Severity changed from major to normal
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.
#18
@
10 years ago
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. But the changes I made corrected the issue.
#19
@
10 years ago
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.
#20
@
10 years ago
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.
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.
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.