diff --git a/CHANGELOG.md b/CHANGELOG.md index 984d8c1..2d16fa5 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 b955d2d..c9aed7a 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 2c95673..cafe4a0 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 a4301b5..8d3fa44 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 135b4a8..b37c44a 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 8fc8288..b5f3d8b 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 a4abfcd..4d2edf7 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 1586515..d0fe245 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; + } }