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.

Sign up for GitHub

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

Jump to bottom

Remove duplicate category_name, cat and tag_id from ES query when tax_query set #2241

Merged
merged 7 commits into from
Jul 20, 2021
Merged

Remove duplicate category_name, cat and tag_id from ES query when tax_query set #2241

merged 7 commits into from
Jul 20, 2021

Conversation

rebeccahum
Copy link
Contributor

Description of the Change

Fixes #1896. Core has a funky backwards compat feature where category_name, cat and tag_id are explicitly set: https://github.com/WordPress/WordPress/blob/5d99107bf3ab35aa3dda82c6b3903f5717771335/wp-includes/class-wp-query.php#L2193-L2215. However, this is interfering with the ES query when tax_query is set by creating duplicate filters from it.

Alternate Designs

I didn't consider any alternative designs since this was previously patched in and wanted to follow a similar pattern to #501. However, the patch was overwritten via #1617.

Benefits

No more duplicate filters in the ES query!

Possible Drawbacks

N/A

Verification Process

  1. Offload a WP_Query to ES:
$args = array(
    'post_type' => 'post',
    'tax_query' => array(
        'relation' => 'OR',
        array(
            'taxonomy' => 'post_tag',
            'field'    => 'slug',
            'terms'    => array( 'canada-traffic' ),
        ),
        array(
                        'relation' => 'AND',
                        array(
                    'taxonomy' => 'region',
                    'field'    => 'slug',
                    'terms'    => array( 'Canada' ),
                        ),
                        array(
                                'taxonomy' => 'category',
                                'field'    => 'slug',
                                'terms'    => array( 'traffic' ),
                        ),
        ),
    ),
);
  1. Inspect the ES query generated and notice the extra filters:
{
    "from": 0,
    "size": 10,
    "sort": [
        {
            "post_date": {
                "order": "desc"
            }
        }
    ],
    "query": {
        "match_all": {
            "boost": 1
        }
    },
    "post_filter": {
        "bool": {
            "must": [
                {
                    "bool": {
                        "should": [
                            {
                                "terms": {
                                    "terms.post_tag.slug": [
                                        "canada-traffic"
                                    ]
                                }
                            },
                            {
                                "bool": {
                                    "must": [
                                        {
                                            "terms": {
                                                "terms.region.slug": [
                                                    "Canada"
                                                ]
                                            }
                                        },
                                        {
                                            "terms": {
                                                "terms.category.slug": [
                                                    "traffic"
                                                ]
                                            }
                                        }
                                    ]
                                }
                            },
                            {
                                "terms": {
                                    "terms.category.slug": [
                                        "traffic"
                                    ]
                                }
                            },
                            {
                                "terms": {
                                    "terms.category.term_id": [
                                        123456
                                    ]
                                }
                            }
                        ]
                    }
                },
                {
                    "terms": {
                        "post_type.raw": [
                            "post"
                        ]
                    }
                },
                {
                    "terms": {
                        "post_status": [
                            "publish"
                        ]
                    }
                }
            ]
        }
    },
    "track_total_hits": true
}

You'll observe the duplicate filters applied here:

 {
                                "terms": {
                                    "terms.category.slug": [
                                        "traffic"
                                    ]
                                }
                            },
                            {
                                "terms": {
                                    "terms.category.term_id": [
                                        123456
                                    ]
                                }
                            }
  1. Apply patch and viola! They should no longer be present.

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests passed.

Applicable Issues

#1896

Changelog Entry

Fixed duplicate category or tag filters when tax_query is set.

@felipeelia felipeelia mentioned this pull request Jun 28, 2021
Closed
@felipeelia
Copy link
Member

Hi @rebeccahum, it seems one of the unit tests failed. Do you mind taking a look? Thanks!

@rebeccahum
Copy link
Contributor Author

@felipeelia Hiya! I've fixed the failing test now, can you please approve the running workflow?

@felipeelia
Copy link
Member

Thank you very much @rebeccahum! I've approved the workflow again.

It looks like #2242 is related to the bug you are solving here but there seems to be yet another problem directly related to it. Just out of curiosity, is that by any chance something you are also facing?

@rebeccahum
Copy link
Contributor Author

rebeccahum commented Jun 29, 2021 •
edited
Loading

@felipeelia I was able to reproduce that additional bug in regards to the terms.category.slug....but I believe that is a separate issue.

@mckdemps mckdemps requested a review from oscarssanchez July 13, 2021 17:30
@oscarssanchez oscarssanchez merged commit 6357996 into 10up:develop Jul 20, 2021
@oscarssanchez
Copy link
Contributor

Thanks for this @rebeccahum!

@rebeccahum rebeccahum deleted the rebecca/fix_1896 branch July 20, 2021 16:28
rebeccahum added a commit to Automattic/ElasticPress that referenced this pull request Aug 3, 2021
@rebeccahum
1:1 with upstream patch 10up#2241
ec59f49
@rebeccahum rebeccahum mentioned this pull request Aug 3, 2021
Merged
4 tasks
rinatkhaziev pushed a commit to Automattic/ElasticPress that referenced this pull request Aug 4, 2021
@rebeccahum
Account for tag_id when using tag__and in WP_Query (#93)
06cb065 
* Fix tag__and in use with tag_id

* 1:1 with upstream patch 10up#2241

* Fix syntax error
@jeffpaul jeffpaul added this to the 3.6.2 milestone Aug 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oscarssanchez oscarssanchez oscarssanchez approved these changes

Assignees

@oscarssanchez oscarssanchez

Labels
high priority
Projects
None yet
Milestone
3.6.2
Development

Successfully merging this pull request may close these issues.

Duplicate Filters in category_name when tax_query is set using OR
6 participants
@rebeccahum @felipeelia @oscarssanchez @jeffpaul @mckdemps @Rahmon