diff --git a/CHANGELOG.md b/CHANGELOG.md index e84d458..ef18179 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,8 +2,11 @@ ## Unreleased -- Bugfix: provider API keys are encrypted in `processDatamap_preProcessFieldArray` instead of only in the post-process hook. On an update, DataHandler captures the `sys_history` diff before the post-process hook runs, so the record history kept the unencrypted value even though the `api_key` column itself was encrypted. Inserts were not affected (#30) +- Bugfix: the Providers overview and Request Log statistics queries no longer select every column alongside their aggregates, which MySQL rejects outright under `sql_mode=ONLY_FULL_GROUP_BY` (#27) +- Bugfix: cost and score columns are declared as `decimal` instead of `double(M,D)`. Doctrine drops precision and scale for float types, so the schema comparison reported the same `ALTER TABLE` on every run without the applied statement ever changing the column, leaving "Analyze Database Structure" stuck with a change list that never went away (#27) +- Bugfix: provider API keys are encrypted in `processDatamap_preProcessFieldArray` instead of only in the post-process hook. On an update, DataHandler captures the `sys_history` diff before the post-process hook runs, so the record history kept the unencrypted value even though the `api_key` column itself was encrypted. Inserts were not affected (#30) + Existing history entries are not rewritten. To drop them for a configuration whose key was changed before this fix: ```sql diff --git a/Classes/Domain/Repository/PagePromptFragmentRepository.php b/Classes/Domain/Repository/PagePromptFragmentRepository.php index 54d5980..7b1f02c 100644 --- a/Classes/Domain/Repository/PagePromptFragmentRepository.php +++ b/Classes/Domain/Repository/PagePromptFragmentRepository.php @@ -93,8 +93,14 @@ public function countByDemand(PagePromptFragmentDemand $demand, ?array $accessib $qb = $this->getQueryBuilderForDemand($demand, $accessiblePageIds); // count() quotes its entire argument as a single identifier, so it // can't express "DISTINCT column", addSelectLiteral() with a raw - // COUNT(DISTINCT ...) expression is this codebase's own established - // way around that (see RequestLogRepository's aggregate queries). + // COUNT(DISTINCT ...) expression is the way around that. Safe here + // specifically because getQueryBuilderForDemand() never calls + // select() itself, so this is the only call that ever populates the + // select list; RequestLogRepository's own aggregate queries instead + // start from a QueryBuilder that already has an explicit + // select('*'), where addSelectLiteral() would append onto that `*` + // rather than define the select list from scratch, that's why those + // use selectLiteral() (replaces) instead. $qb->addSelectLiteral('COUNT(DISTINCT ' . $qb->quoteIdentifier('pages.uid') . ')'); return (int)$qb->executeQuery()->fetchOne(); } diff --git a/Classes/Domain/Repository/RequestLogRepository.php b/Classes/Domain/Repository/RequestLogRepository.php index 2798614..618a910 100644 --- a/Classes/Domain/Repository/RequestLogRepository.php +++ b/Classes/Domain/Repository/RequestLogRepository.php @@ -157,19 +157,7 @@ public function countByDemand(RequestLogDemand $demand): int public function getStatistics(): array { - $qb = $this->getQueryBuilder(); - $result = $qb - ->addSelectLiteral( - $qb->expr()->count('*', 'total_requests'), - 'SUM(cost) AS total_cost', - 'SUM(prompt_tokens) AS total_prompt_tokens', - 'SUM(completion_tokens) AS total_completion_tokens', - 'SUM(cached_tokens) AS total_cached_tokens', - 'SUM(reasoning_tokens) AS total_reasoning_tokens', - 'SUM(total_tokens) AS total_tokens', - 'AVG(duration_ms) AS avg_duration_ms', - 'SUM(success) AS successful_requests', - ) + $result = $this->buildStatisticsQueryBuilder() ->executeQuery() ->fetchAssociative(); @@ -189,11 +177,27 @@ public function getStatistics(): array ]; } + private function buildStatisticsQueryBuilder(): QueryBuilder + { + $qb = $this->getQueryBuilder(); + return $qb->selectLiteral( + $qb->expr()->count('*', 'total_requests'), + 'SUM(cost) AS total_cost', + 'SUM(prompt_tokens) AS total_prompt_tokens', + 'SUM(completion_tokens) AS total_completion_tokens', + 'SUM(cached_tokens) AS total_cached_tokens', + 'SUM(reasoning_tokens) AS total_reasoning_tokens', + 'SUM(total_tokens) AS total_tokens', + 'AVG(duration_ms) AS avg_duration_ms', + 'SUM(success) AS successful_requests', + ); + } + public function getStatisticsByProvider(): array { $qb = $this->getQueryBuilder(); return $qb - ->addSelectLiteral( + ->selectLiteral( 'provider_identifier', $qb->expr()->count('*', 'request_count'), 'SUM(cost) AS total_cost', @@ -211,7 +215,7 @@ public function getStatisticsByExtension(): array { $qb = $this->getQueryBuilder(); return $qb - ->addSelectLiteral( + ->selectLiteral( 'extension_key', $qb->expr()->count('*', 'request_count'), 'SUM(cost) AS total_cost', @@ -233,27 +237,7 @@ public function getStatisticsByExtension(): array */ public function getModelPerformanceProfile(string $requestType = ''): array { - $done = GradeStatus::Done->value; - $qb = $this->getQueryBuilder(); - $qb->addSelectLiteral( - 'model_used', - $qb->expr()->count('*', 'request_count'), - 'AVG(cost) AS avg_cost', - 'AVG(duration_ms) AS avg_duration_ms', - 'SUM(success) AS successful_requests', - 'AVG(total_tokens) AS avg_tokens', - sprintf("SUM(CASE WHEN grade_status = '%s' THEN grade_score ELSE 0 END) AS grade_score_sum", $done), - sprintf("SUM(CASE WHEN grade_status = '%s' THEN 1 ELSE 0 END) AS graded_count", $done), - ); - if ($requestType !== '') { - $qb->where($qb->expr()->eq('request_type', $qb->createNamedParameter($requestType))); - $qb->andWhere($qb->expr()->neq('model_used', $qb->createNamedParameter(''))); - } else { - $qb->where($qb->expr()->neq('model_used', $qb->createNamedParameter(''))); - } - $rows = $qb - ->groupBy('model_used') - ->orderBy('request_count', 'DESC') + $rows = $this->buildModelPerformanceQueryBuilder($requestType) ->executeQuery() ->fetchAllAssociative(); @@ -274,6 +258,31 @@ public function getModelPerformanceProfile(string $requestType = ''): array }, $rows); } + private function buildModelPerformanceQueryBuilder(string $requestType): QueryBuilder + { + $done = GradeStatus::Done->value; + $qb = $this->getQueryBuilder(); + $qb->selectLiteral( + 'model_used', + $qb->expr()->count('*', 'request_count'), + 'AVG(cost) AS avg_cost', + 'AVG(duration_ms) AS avg_duration_ms', + 'SUM(success) AS successful_requests', + 'AVG(total_tokens) AS avg_tokens', + sprintf("SUM(CASE WHEN grade_status = '%s' THEN grade_score ELSE 0 END) AS grade_score_sum", $done), + sprintf("SUM(CASE WHEN grade_status = '%s' THEN 1 ELSE 0 END) AS graded_count", $done), + ); + if ($requestType !== '') { + $qb->where($qb->expr()->eq('request_type', $qb->createNamedParameter($requestType))); + $qb->andWhere($qb->expr()->neq('model_used', $qb->createNamedParameter(''))); + } else { + $qb->where($qb->expr()->neq('model_used', $qb->createNamedParameter(''))); + } + return $qb + ->groupBy('model_used') + ->orderBy('request_count', 'DESC'); + } + public function getDistinctProviders(): array { $qb = $this->getQueryBuilder(); @@ -416,14 +425,7 @@ protected function getQueryBuilderForDemand(RequestLogDemand $demand): QueryBuil */ public function getLastUsedPerConfiguration(): array { - $qb = $this->getQueryBuilder(); - $rows = $qb - ->addSelectLiteral( - 'configuration_uid', - 'MAX(crdate) AS last_used', - ) - ->where($qb->expr()->gt('configuration_uid', $qb->createNamedParameter(0, Connection::PARAM_INT))) - ->groupBy('configuration_uid') + $rows = $this->buildLastUsedPerConfigurationQueryBuilder() ->executeQuery() ->fetchAllAssociative(); @@ -434,6 +436,18 @@ public function getLastUsedPerConfiguration(): array return $result; } + private function buildLastUsedPerConfigurationQueryBuilder(): QueryBuilder + { + $qb = $this->getQueryBuilder(); + return $qb + ->selectLiteral( + 'configuration_uid', + 'MAX(crdate) AS last_used', + ) + ->where($qb->expr()->gt('configuration_uid', $qb->createNamedParameter(0, Connection::PARAM_INT))) + ->groupBy('configuration_uid'); + } + /** * Resolve user IDs to usernames from be_users. * diff --git a/Tests/Functional/Domain/Repository/RequestLogRepositoryTest.php b/Tests/Functional/Domain/Repository/RequestLogRepositoryTest.php index c9a398c..b69639e 100644 --- a/Tests/Functional/Domain/Repository/RequestLogRepositoryTest.php +++ b/Tests/Functional/Domain/Repository/RequestLogRepositoryTest.php @@ -74,6 +74,81 @@ public function countByDemandIsUnaffectedByTheUsernameJoin(): void self::assertCount(2, $logRepo->findByDemand($demand)); } + /** + * getQueryBuilder() always starts from an explicit select('*'). Every + * aggregate/GROUP BY query built on top of it must REPLACE that select + * list (selectLiteral()), not append to it (addSelectLiteral()), or the + * `*` leaks every column of the table into the result alongside the + * aggregates, which MySQL's ONLY_FULL_GROUP_BY rejects outright + * (see https://github.com/b13/aim/issues/27). SQLite tolerates the + * broken query and just returns the extra columns, which is exactly + * what these tests catch. + */ + #[Test] + public function getStatisticsByProviderOnlySelectsTheIntendedColumns(): void + { + $logRepo = $this->get(RequestLogRepository::class); + $logRepo->log(['request_type' => 'TextGenerationRequest', 'provider_identifier' => 'test', 'cost' => 1.0]); + + $rows = $logRepo->getStatisticsByProvider(); + + self::assertCount(1, $rows); + self::assertSame( + ['provider_identifier', 'request_count', 'total_cost', 'total_tokens', 'avg_duration_ms', 'successful_requests'], + array_keys($rows[0]), + ); + } + + #[Test] + public function getStatisticsByExtensionOnlySelectsTheIntendedColumns(): void + { + $logRepo = $this->get(RequestLogRepository::class); + $logRepo->log(['request_type' => 'TextGenerationRequest', 'provider_identifier' => 'test', 'extension_key' => 'some_ext', 'cost' => 1.0]); + + $rows = $logRepo->getStatisticsByExtension(); + + self::assertCount(1, $rows); + self::assertSame( + ['extension_key', 'request_count', 'total_cost', 'total_tokens', 'avg_duration_ms'], + array_keys($rows[0]), + ); + } + + /** + * getStatistics(), getModelPerformanceProfile() and + * getLastUsedPerConfiguration() all re-key their rows into a fixed + * shape before returning, which would silently hide the same `SELECT + * *, ...` regression the two tests above catch directly. Asserted here + * instead on the built query's own SQL, via the private QueryBuilder + * factories those methods were split from for exactly this reason. + */ + #[Test] + public function statisticsQueryHasNoStraySelectStar(): void + { + $logRepo = $this->get(RequestLogRepository::class); + $qb = (new \ReflectionMethod($logRepo, 'buildStatisticsQueryBuilder'))->invoke($logRepo); + + self::assertStringNotContainsString('SELECT *,', $qb->getSQL()); + } + + #[Test] + public function modelPerformanceQueryHasNoStraySelectStar(): void + { + $logRepo = $this->get(RequestLogRepository::class); + $qb = (new \ReflectionMethod($logRepo, 'buildModelPerformanceQueryBuilder'))->invoke($logRepo, ''); + + self::assertStringNotContainsString('SELECT *,', $qb->getSQL()); + } + + #[Test] + public function lastUsedPerConfigurationQueryHasNoStraySelectStar(): void + { + $logRepo = $this->get(RequestLogRepository::class); + $qb = (new \ReflectionMethod($logRepo, 'buildLastUsedPerConfigurationQueryBuilder'))->invoke($logRepo); + + self::assertStringNotContainsString('SELECT *,', $qb->getSQL()); + } + #[Test] public function modelPerformanceProfileAggregatesGradesOverDoneRowsOnly(): void { diff --git a/ext_tables.sql b/ext_tables.sql index 70226e6..4736fd2 100644 --- a/ext_tables.sql +++ b/ext_tables.sql @@ -27,12 +27,12 @@ CREATE TABLE tx_aim_configuration ( `default` tinyint(4) unsigned DEFAULT '0' NOT NULL, api_key text, model varchar(255) DEFAULT '' NOT NULL, - total_cost double(10,6) DEFAULT '0.000000' NOT NULL, + total_cost decimal(10,6) DEFAULT '0.000000' NOT NULL, cost_currency varchar(10) DEFAULT 'USD' NOT NULL, max_tokens int(11) unsigned DEFAULT '0' NOT NULL, - input_token_cost double(10,6) DEFAULT '0.000000' NOT NULL, - output_token_cost double(10,6) DEFAULT '0.000000' NOT NULL, + input_token_cost decimal(10,6) DEFAULT '0.000000' NOT NULL, + output_token_cost decimal(10,6) DEFAULT '0.000000' NOT NULL, be_groups varchar(255) DEFAULT '' NOT NULL, privacy_level varchar(20) DEFAULT 'standard' NOT NULL, rerouting_allowed tinyint(1) unsigned DEFAULT '1' NOT NULL, @@ -49,7 +49,7 @@ CREATE TABLE tx_aim_usage_budget ( period_start int(11) unsigned DEFAULT '0' NOT NULL, period_type varchar(20) DEFAULT 'monthly' NOT NULL, tokens_used int(11) unsigned DEFAULT '0' NOT NULL, - cost_used double(10,6) DEFAULT '0.000000' NOT NULL, + cost_used decimal(10,6) DEFAULT '0.000000' NOT NULL, requests_used int(11) unsigned DEFAULT '0' NOT NULL, PRIMARY KEY (uid), @@ -71,7 +71,7 @@ CREATE TABLE tx_aim_request_log ( cached_tokens int(11) unsigned DEFAULT '0' NOT NULL, reasoning_tokens int(11) unsigned DEFAULT '0' NOT NULL, total_tokens int(11) unsigned DEFAULT '0' NOT NULL, - cost double(10,6) DEFAULT '0.000000' NOT NULL, + cost decimal(10,6) DEFAULT '0.000000' NOT NULL, duration_ms int(11) unsigned DEFAULT '0' NOT NULL, system_fingerprint varchar(255) DEFAULT '' NOT NULL, error_message text, @@ -81,18 +81,18 @@ CREATE TABLE tx_aim_request_log ( request_prompt text, request_system_prompt text, response_content text, - complexity_score double(5,4) DEFAULT '0.0000' NOT NULL, + complexity_score decimal(5,4) DEFAULT '0.0000' NOT NULL, complexity_label varchar(20) DEFAULT '' NOT NULL, complexity_reason text, rerouted tinyint(1) unsigned DEFAULT '0' NOT NULL, reroute_type varchar(20) DEFAULT '' NOT NULL, reroute_reason varchar(255) DEFAULT '' NOT NULL, grade_status varchar(20) DEFAULT 'none' NOT NULL, - grade_score double(5,4) DEFAULT '0.0000' NOT NULL, + grade_score decimal(5,4) DEFAULT '0.0000' NOT NULL, grade_label varchar(20) DEFAULT '' NOT NULL, grade_reason text, judge_model varchar(255) DEFAULT '' NOT NULL, - judge_cost double(10,6) DEFAULT '0.000000' NOT NULL, + judge_cost decimal(10,6) DEFAULT '0.000000' NOT NULL, grade_duration_ms int(11) unsigned DEFAULT '0' NOT NULL, grade_error varchar(500) DEFAULT '' NOT NULL,