From d445766761d22bd1c1d6ec3bde5af1e79bc4b4d3 Mon Sep 17 00:00:00 2001 From: Robin Cornett Date: Thu, 22 Oct 2026 11:54:57 -0400 Subject: [PATCH] Query: key the result cache on what the query-clauses filter changed. A {prefix}_{items}_query_clauses callback can narrow the rows on a query var with no registered default, or on state outside the query vars, and neither reached the cache key, so differently filtered queries shared one entry. Build the request before the cache lookup, record which clauses the filter changed, and hash those alongside the query vars. A query no callback changes keeps its previous key, so results-invariant vars still share one entry. Fixes #275. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 6 + CLAUDE.md | 2 +- src/Database/Traits/Query/Aggregates.php | 55 ++++--- src/Database/Traits/Query/Cache.php | 50 ++++++- src/Database/Traits/Query/Execution.php | 41 ++++-- src/Database/Traits/Query/Filters.php | 8 +- .../Database/Kern/Query/CacheKeyGuardTest.php | 58 ++++++++ tests/Database/Kern/Query/QueryCacheTest.php | 135 ++++++++++++++++++ 8 files changed, 323 insertions(+), 32 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 984d8c1a..2d16fa57 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,12 @@ Notable changes to BerlinDB are documented here. ## 3.1.0 - Unreleased +- Keys the result cache on what a `{prefix}_{items}_query_clauses` filter changed + (#275). A callback that narrows rows on a query var with no registered default, or + on state outside the query vars, no longer shares one cache entry with the + unfiltered query. The request is now built before the cache lookup, so the filter + runs on cache hits too; a query no callback changes keeps its previous key. + - Makes `MetaStore` the ownership boundary for item-meta cleanup (#262). A BerlinDB-owned Meta preset now derives its meta-row primary column and owning-object column from its registered Schema, so customized names are deleted through the diff --git a/CLAUDE.md b/CLAUDE.md index b955d2d6..c9aed7aa 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -46,7 +46,7 @@ hand-rolled queries — never more surprising. vendor/bin/phpcs ``` 5. **Don't invent APIs.** If unsure how something behaves, search `src/` and - `tests/` — the source and its 1755 test methods are the source of truth, ahead + `tests/` — the source and its 1760 test methods are the source of truth, ahead of memory or training data. (PHPUnit reports more cases: data providers expand methods at run time.) 6. **Keep changes focused and tested.** Bug fixes and new behavior ship with diff --git a/src/Database/Traits/Query/Aggregates.php b/src/Database/Traits/Query/Aggregates.php index 2c956731..cafe4a0d 100644 --- a/src/Database/Traits/Query/Aggregates.php +++ b/src/Database/Traits/Query/Aggregates.php @@ -270,30 +270,24 @@ private function empty_aggregate_result( array $aggregate ): array { } /** - * Run the 'aggregate' container and return its results. + * Build the request SQL for the 'aggregate' container. * - * Called from the query path (get_item_ids()) once the query vars are parsed, so - * it reads the compiled WHERE/JOIN directly. Each canonical entry renders + * Called from the query path (set_item_ids_request()) once the query vars are + * parsed, so it reads the compiled WHERE/JOIN directly. Each canonical entry renders * "FUNC( column ) AS alias". A JOIN-producing filter is wrapped in a * distinct-primary subquery (the same dedup the scalar methods use), so a - * one-to-many join does not fan out and double-count. - * - * Without a 'groupby' var the whole set is ONE row: an assoc keyed by every - * requested alias, each value null when its aggregate is NULL (an empty set) - - * unlike COUNT, an empty SUM/MAX is null, not zero. With 'groupby' the group - * column(s) join the SELECT and drive a GROUP BY (grouped AFTER the fan-out dedup), - * so it returns a LIST of rows, each carrying the group column(s) plus each alias; - * an empty set is an empty list (no groups). + * one-to-many join does not fan out and double-count. With a 'groupby' var the + * group column(s) join the SELECT and drive a GROUP BY (grouped AFTER the fan-out + * dedup). * * @since 3.1.0 * * @param array $aggregate The * canonical aggregate container (from normalize_aggregate_container()). * - * @return array|array> One row keyed by - * alias, or a list of grouped rows. + * @return string The request SQL, or '' when no entry renders. */ - private function run_aggregate( array $aggregate ): array { + private function get_aggregate_request( array $aggregate ): string { /* * A 'groupby' var puts the group column(s) into the SELECT and drives a GROUP BY, @@ -336,7 +330,7 @@ private function run_aggregate( array $aggregate ): array { // Nothing renderable resolves to an empty result set. if ( empty( $fields ) ) { - return array(); + return ''; } $group_clause = ( true === $grouped ) @@ -381,7 +375,36 @@ private function run_aggregate( array $aggregate ): array { $clauses[ 'orderby' ] = $this->resolve_aggregate_orderby( $group_names, array_keys( $fields ) ); } - $request = $this->parse_request_clauses( $clauses ); + return $this->parse_request_clauses( $clauses ); + } + + /** + * Run the 'aggregate' container's request and return its results. + * + * Runs the request get_aggregate_request() built. Without a 'groupby' var the + * whole set is ONE row: an assoc keyed by every requested alias, each value null + * when its aggregate is NULL (an empty set) - unlike COUNT, an empty SUM/MAX is + * null, not zero. With 'groupby' it returns a LIST of rows, each carrying the group + * column(s) plus each alias; an empty set is an empty list (no groups). + * + * @since 3.1.0 + * + * @param array $aggregate The + * canonical aggregate container (from normalize_aggregate_container()). + * @param string $request The + * request SQL from get_aggregate_request(). + * + * @return array|array> One row keyed by + * alias, or a list of grouped rows. + */ + private function run_aggregate( array $aggregate, string $request ): array { + + // Nothing renderable resolves to an empty result set. + if ( '' === $request ) { + return array(); + } + + $grouped = ! empty( $this->get_valid_groupby_columns( (array) $this->get_query_var( 'groupby' ) ) ); // Grouped: a list of rows (group column(s) + aliases), as the database returns them. if ( true === $grouped ) { diff --git a/src/Database/Traits/Query/Cache.php b/src/Database/Traits/Query/Cache.php index a4301b5b..8d3fa440 100644 --- a/src/Database/Traits/Query/Cache.php +++ b/src/Database/Traits/Query/Cache.php @@ -60,18 +60,23 @@ abstract protected function get_results_invariant_vars(): array; abstract protected function get_items_raw( string $where ): array; /** - * Get cache key from $query_vars and $query_var_defaults. + * Get cache key from $query_vars, $query_var_defaults, and filtered clauses. * * Performs the following operations to create a consistent cache-key: * - Removes the "fields" query_var, because whole objects/items are cached * - Removes unknown or unregistered query_var keys * - Sorts query_vars by query_var_default keys * - Removes query_vars with default values - * - Serializes and md5 hashes query_vars + * - Adds the clauses the query-clauses filter changed, if any + * - Serializes and md5 hashes the result * - Combines plural name and hash independently of the cache generation * + * The request must be built first (set_item_ids_request()), so the + * query-clauses filter has run. + * * @since 1.0.0 * @since 2.1.0 Correctly removes unique query_var_default_value values + * @since 3.1.0 Includes the clauses the query-clauses filter changed * * @return string */ @@ -102,13 +107,52 @@ private function get_cache_key(): string { $slice[ $key ] = $this->query_vars[ $key ]; } + /* + * A query-clauses callback can narrow the rows on a var with no registered + * default, or on state outside the query vars entirely, so neither reaches + * the slice. Key on what the filter changed; an unfiltered key is unchanged. + */ + $filtered = $this->get_current_array( 'filtered_clauses' ); + $hashable = empty( $filtered ) + ? $slice + : array( $slice, $filtered ); + // Hash the sliced query vars. serialize() is intentional and safe here. - $hash = md5( serialize( $slice ) ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_serialize + $hash = md5( serialize( $hashable ) ); // phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_serialize // Return the namespaced cache key. return "get_{$this->get_item_name_plural()}:{$hash}"; } + /** + * Record which query clauses the query-clauses filter changed. + * + * A removed clause is recorded as null. get_cache_key() hashes the result. + * + * @since 3.1.0 + * + * @param array $clauses The clauses before the filter. + * @param array $filtered The clauses after the filter. + * @return void + */ + private function set_filtered_clauses( array $clauses, array $filtered ): void { + $changed = array(); + + foreach ( $filtered as $key => $value ) { + if ( ! array_key_exists( $key, $clauses ) || ( $clauses[ $key ] !== $value ) ) { + $changed[ $key ] = $value; + } + } + + foreach ( array_keys( array_diff_key( $clauses, $filtered ) ) as $key ) { + $changed[ $key ] = null; + } + + ksort( $changed ); + + $this->set_current( 'filtered_clauses', $changed ); + } + /** * Get the current cache generations for tables used by relationship filters. * diff --git a/src/Database/Traits/Query/Execution.php b/src/Database/Traits/Query/Execution.php index 135b4a8b..b37c44a0 100644 --- a/src/Database/Traits/Query/Execution.php +++ b/src/Database/Traits/Query/Execution.php @@ -149,6 +149,9 @@ private function get_items(): array|int { return $this->items; } + // Build the request first, so the cache key sees what the query-clauses filter changed. + $this->set_item_ids_request(); + /* * Check the cache. EXPLAIN returns a plan (not rows) and must reflect the * current optimizer state, so it is never served from or stored in the cache. @@ -288,30 +291,46 @@ private function set_request(): void { } /** - * Used internally to get a list of item IDs matching the query vars. + * Build and store the request SQL for the resolved query mode. * - * @since 1.0.0 - * @since 3.0.0 Uses wp_parse_list() instead of wp_parse_id_list() + * Runs before the result-cache lookup, because get_cache_key() includes the + * clauses the query-clauses filter changed. * - * @return array|array[]|array|int Item IDs for a full query, int/rows for a count query, or aggregate results keyed by alias. + * @since 3.1.0 */ - private function get_item_ids(): array|int { + private function set_item_ids_request(): void { - /* - * Aggregate mode computes its result from its own request (run_aggregate()), - * with no primary-ID selection, so it returns before the shared request below. - */ + // Aggregate mode builds its own request, with no primary-ID selection. if ( 'aggregate' === $this->get_query_mode() ) { - return $this->run_aggregate( (array) $this->get_query_var( 'aggregate' ) ); + $this->set_current( 'request', $this->get_aggregate_request( (array) $this->get_query_var( 'aggregate' ) ) ); + return; } - // Count and rows both run the assembled request; build it once. + // Count and rows both run the assembled request. $this->set_query_clauses(); $this->set_request_clauses(); $this->set_request(); + } + /** + * Used internally to get a list of item IDs matching the query vars. + * + * Runs the request set_item_ids_request() built. + * + * @since 1.0.0 + * @since 3.0.0 Uses wp_parse_list() instead of wp_parse_id_list() + * @since 3.1.0 No longer builds the request. + * + * @return array|array[]|array|int Item IDs for a full query, int/rows for a count query, or aggregate results keyed by alias. + */ + private function get_item_ids(): array|int { $request = $this->get_current_string( 'request' ) ?? ''; + // Aggregate mode shapes its own result, with no primary-ID selection. + if ( 'aggregate' === $this->get_query_mode() ) { + return $this->run_aggregate( (array) $this->get_query_var( 'aggregate' ), $request ); + } + // Dispatch to the mode's execution. return ( 'count' === $this->get_query_mode() ) ? $this->execute_count( $request ) diff --git a/src/Database/Traits/Query/Filters.php b/src/Database/Traits/Query/Filters.php index 8fc82883..b5f3d8b5 100644 --- a/src/Database/Traits/Query/Filters.php +++ b/src/Database/Traits/Query/Filters.php @@ -173,6 +173,7 @@ protected function filter_found_items_query( $sql = '' ) { * Filter the query clauses before they are parsed into a SQL string. * * @since 3.0.0 + * @since 3.1.0 Records the clauses the filter changed, for the result-cache key. * * @param array $clauses All of the SQL query clauses. * @return array @@ -183,6 +184,7 @@ protected function filter_query_clauses( $clauses = array() ) { $filter_name = $this->apply_hook_prefix( $this->get_item_name_plural() . '_query_clauses' ); if ( '' === $filter_name ) { + $this->set_filtered_clauses( (array) $clauses, (array) $clauses ); return $clauses; } @@ -194,12 +196,16 @@ protected function filter_query_clauses( $clauses = array() ) { * @param array $clauses An array of query clauses. * @param Query $query Current query instance. */ - return (array) apply_filters_ref_array( + $filtered = (array) apply_filters_ref_array( $filter_name, array( $clauses, &$this, ) ); + + $this->set_filtered_clauses( (array) $clauses, $filtered ); + + return $filtered; } } diff --git a/tests/Database/Kern/Query/CacheKeyGuardTest.php b/tests/Database/Kern/Query/CacheKeyGuardTest.php index a4abfcd1..4d2edf72 100644 --- a/tests/Database/Kern/Query/CacheKeyGuardTest.php +++ b/tests/Database/Kern/Query/CacheKeyGuardTest.php @@ -228,6 +228,64 @@ public function test_results_affecting_vars_segment_cache_key() { ); } + /** + * Test the other direction: a var with no registered default that only a + * query-clauses callback reads still segments the key, because the callback + * changes which rows return. + * + * @since 3.1.0 + */ + public function test_var_read_only_by_clauses_filter_segments_cache_key() { + $callback = static function ( $clauses, $query ) { + $name = (string) $query->get_query_var( 'only_name' ); + + if ( '' !== $name ) { + global $wpdb; + $clauses[ 'where' ] = $wpdb->prepare( 'WHERE name = %s', $name ); + } + + return $clauses; + }; + + add_filter( 'berlindb_database_widgets_query_clauses', $callback, 10, 2 ); + try { + $plain = $this->cache_key( array( 'number' => 10 ) ); + $filtered = $this->cache_key( + array( + 'number' => 10, + 'only_name' => 'beta', + ) + ); + } finally { + remove_filter( 'berlindb_database_widgets_query_clauses', $callback, 10 ); + } + + $this->assertNotSame( $plain, $filtered ); + } + + /** + * Test that a query-clauses callback which changes nothing does not segment the + * key: hooking the filter alone must not fragment every query's cache entry. + * + * @since 3.1.0 + */ + public function test_clauses_filter_that_changes_nothing_does_not_segment_cache_key() { + $plain = $this->cache_key( array( 'number' => 10 ) ); + + $callback = static function ( $clauses ) { + return $clauses; + }; + + add_filter( 'berlindb_database_widgets_query_clauses', $callback ); + try { + $hooked = $this->cache_key( array( 'number' => 10 ) ); + } finally { + remove_filter( 'berlindb_database_widgets_query_clauses', $callback ); + } + + $this->assertSame( $plain, $hooked ); + } + /** * Test that RESULTS_INVARIANT_VARS matches the reviewed list and contains only * real query vars (no typos, no drift). diff --git a/tests/Database/Kern/Query/QueryCacheTest.php b/tests/Database/Kern/Query/QueryCacheTest.php index 15865157..d0fe245b 100644 --- a/tests/Database/Kern/Query/QueryCacheTest.php +++ b/tests/Database/Kern/Query/QueryCacheTest.php @@ -379,4 +379,139 @@ public function test_generation_is_captured_before_the_database_read(): void { $this->assertCount( 1, $query->query( $args ) ); $this->assertGreaterThan( $queries_before, $wpdb->num_queries ); } + + /** + * A query-clauses callback narrowing on a var with no registered default must + * not be served the unfiltered result, nor fill the entry the unfiltered query + * reads afterward. + * + * @since 3.1.0 + */ + public function test_clauses_filter_on_unregistered_var_gets_its_own_cache_entry(): void { + $beta_id = $this->add_named_widget( 'beta' ); + + $callback = function ( $clauses, $query ) { + return $this->restrict_to_name( $clauses, (string) $query->get_query_var( 'only_name' ) ); + }; + + add_filter( 'berlindb_database_widgets_query_clauses', $callback, 10, 2 ); + try { + $all_ids = ( new TestQuery( array( 'fields' => 'ids' ) ) )->items; + $beta_only = ( new TestQuery( + array( + 'fields' => 'ids', + 'only_name' => 'beta', + ) + ) )->items; + $all_again = ( new TestQuery( array( 'fields' => 'ids' ) ) )->items; + } finally { + remove_filter( 'berlindb_database_widgets_query_clauses', $callback, 10 ); + } + + $this->assertCount( 2, $all_ids ); + $this->assertEquals( array( $beta_id ), $beta_only ); + $this->assertCount( 2, $all_again ); + } + + /** + * A query-clauses callback narrowing on state outside the query vars, such as a + * request superglobal, has no var to register, so only the filtered clauses can + * separate its cache entry. + * + * @since 3.1.0 + */ + public function test_clauses_filter_on_outside_state_gets_its_own_cache_entry(): void { + $beta_id = $this->add_named_widget( 'beta' ); + $restrict_to = ''; + + $callback = function ( $clauses ) use ( &$restrict_to ) { + return $this->restrict_to_name( $clauses, $restrict_to ); + }; + + add_filter( 'berlindb_database_widgets_query_clauses', $callback ); + try { + $all_ids = ( new TestQuery( array( 'fields' => 'ids' ) ) )->items; + + $restrict_to = 'beta'; + $beta_only = ( new TestQuery( array( 'fields' => 'ids' ) ) )->items; + } finally { + remove_filter( 'berlindb_database_widgets_query_clauses', $callback ); + } + + $this->assertCount( 2, $all_ids ); + $this->assertEquals( array( $beta_id ), $beta_only ); + } + + /** + * The aggregate path runs the same query-clauses filter, so a filtered aggregate + * must not be served the unfiltered aggregate's cached result. + * + * @since 3.1.0 + */ + public function test_clauses_filter_on_aggregate_gets_its_own_cache_entry(): void { + $this->add_named_widget( 'beta' ); + $restrict_to = ''; + + $callback = function ( $clauses ) use ( &$restrict_to ) { + return $this->restrict_to_name( $clauses, $restrict_to ); + }; + + add_filter( 'berlindb_database_widgets_query_clauses', $callback ); + try { + $all_count = ( new TestQuery() )->query( array( 'aggregate' => array( 'count' => 'id' ) ) ); + + $restrict_to = 'beta'; + $beta_count = ( new TestQuery() )->query( array( 'aggregate' => array( 'count' => 'id' ) ) ); + } finally { + remove_filter( 'berlindb_database_widgets_query_clauses', $callback ); + } + + $this->assertEquals( 2, $all_count[ 'count' ] ); + $this->assertEquals( 1, $beta_count[ 'count' ] ); + } + + /** + * Add a widget with the given name alongside the setUp() fixture row. + * + * @since 3.1.0 + * + * @param string $name Widget name. + * @return int The new widget's ID. + */ + private function add_named_widget( string $name ): int { + $widget_id = self::$query->add_item( + array( + 'name' => $name, + 'status' => 'active', + ) + ); + + $this->assertIsInt( $widget_id ); + + return $widget_id; + } + + /** + * Narrow query clauses to one widget name, or leave them as-is for an empty name. + * + * @since 3.1.0 + * + * @param array $clauses The query clauses. + * @param string $name Widget name to narrow to. + * @return array + */ + private function restrict_to_name( array $clauses, string $name ): array { + if ( '' === $name ) { + return $clauses; + } + + global $wpdb; + + $condition = $wpdb->prepare( 'name = %s', $name ); + $clauses[ 'where' ] = ( '' === $clauses[ 'where' ] ) + ? "WHERE {$condition}" + : "{$clauses[ 'where' ]} AND {$condition}"; + + return $clauses; + } }