Skip to content

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

Merged
rinatkhaziev merged 3 commits intodevelopfrom
rebecca/remove_duplicate_queries_tax
Jun 22, 2021
Merged

Remove duplicate category_name, cat and tag_id from ES query when tax_query set#92
rinatkhaziev merged 3 commits intodevelopfrom
rebecca/remove_duplicate_queries_tax

Conversation

@rebeccahum
Copy link

Description

Core has a funky backwards compat feature where category_name, cat and tag_id are set explicitly:

https://github.com/WordPress/WordPress/blob/5d99107bf3ab35aa3dda82c6b3903f5717771335/wp-includes/class-wp-query.php#L2193-L2215

However, this interferes with the ES query when tax_query is also set. For example, take this WP_Query to be offloaded:

	$args = array(
		'es' => true,
		'post_type' => 'post',
		'tax_query' => array(
			'relation' => 'OR',
			array(
				'taxonomy' => 'post_tag',
				'field'    => 'slug',
				'terms'    => array( 'toronto-traffic' ),
			),
			array(
				'relation' => 'AND',
					array(
						'taxonomy' => 'post_tag',
						'field'    => 'slug',
						'terms'    => array( 'toronto' ),
					),
					array(
						'taxonomy' => 'category',
						'field'    => 'slug',
						'terms'    => array( 'traffic' ),
					),
			),
		),

What we would expect from this is posts with a tag of toronto-traffic OR category of traffic and post_tag of toronto. The ES query would look like:

{
    "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": [
                                        "toronto-traffic"
                                    ]
                                }
                            },
                            {
                                "bool": {
                                    "must": [
                                        {
                                            "terms": {
                                                "terms.post_tag.slug": [
                                                    "toronto"
                                                ]
                                            }
                                        },
                                        {
                                            "terms": {
                                                "terms.category.slug": [
                                                    "traffic"
                                                ]
                                            }
                                        }
                                    ]
                                }
                            }
                        ]
                    }
                },
                {
                    "terms": {
                        "post_type.raw": [
                            "post"
                        ]
                    }
                },
                {
                    "terms": {
                        "post_status": [
                            "publish"
                        ]
                    }
                }
            ]
        }
    },
    "track_total_hits": true
}

However, ES currently returns the following query, where results that may only just have traffic as a category are returned:

{
    "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": [
                                        "toronto-traffic"
                                    ]
                                }
                            },
                            {
                                "bool": {
                                    "must": [
                                        {
                                            "terms": {
                                                "terms.post_tag.slug": [
                                                    "toronto"
                                                ]
                                            }
                                        },
                                        {
                                            "terms": {
                                                "terms.category.slug": [
                                                    "traffic"
                                                ]
                                            }
                                        }
                                    ]
                                }
                            },
                            {
                                "terms": {
                                    "terms.category.slug": [
                                        "traffic"
                                    ]
                                }
                            },
                            {
                                "terms": {
                                    "terms.category.term_id": [
                                        12345 // traffic category term_id
                                    ]
                                }
                            },
                            {
                                "terms": {
                                    "terms.post_tag.term_id": [
                                        6789 // toronto-traffic post_tag 
                                    ]
                                }
                            }
                        ]
                    }
                },
                {
                    "terms": {
                        "post_type.raw": [
                            "post"
                        ]
                    }
                },
                {
                    "terms": {
                        "post_status": [
                            "publish"
                        ]
                    }
                }
            ]
        }
    },
    "track_total_hits": true
}

It ends up treating the three conditions separately as an OR.

Checklist

Please make sure the items below have been covered before requesting a review:

  • This change works and has been tested locally (or has an appropriate fallback).
  • This change works and has been tested on a Go sandbox.
  • This change has relevant unit tests (if applicable).
  • This change has relevant documentation additions / updates (if applicable).

Steps to Test

  1. Create post A and only assign it to category traffic.
  2. Create post B and assign it to category traffic and post_tag toronto.
  3. Create post C and assign it to post_tag toronto-traffic.
  4. Offload the WP_Query described in description.
  5. You will see that post A will be returned in the results (when it shouldn't be).
  6. Apply patch.
  7. You will not see post A returned in the results anymore.

Copy link

@rinatkhaziev rinatkhaziev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed in Slack, should be fine 🚢
But we should test thoroughly before merging to prod.

@rogertheriault
Copy link

Hiding whitespace changes is helpful, only 3 lines actually "changed"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants