From e5b431ec3d2fd0b8c2e61950c8281a0e01cac3e4 Mon Sep 17 00:00:00 2001 From: Oliver Bartsch Date: Fri, 28 Aug 2026 15:14:58 +0200 Subject: [PATCH 1/2] [BUGFIX] Fix ONLY_FULL_GROUP_BY violations in request log statistics queries --- CHANGELOG.md | 4 + .../PagePromptFragmentRepository.php | 10 +- .../Repository/RequestLogRepository.php | 102 ++++++++++-------- .../Repository/RequestLogRepositoryTest.php | 75 +++++++++++++ 4 files changed, 145 insertions(+), 46 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9649abc..7474b0a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # Changelog +## Unreleased + +- 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) + ## 0.4.1 Bugfix release: third-party Symfony AI bridge auto-discovery, and a small footer addition. 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 { From af355ddcd02e0803223b483a86902a8608aa6978 Mon Sep 17 00:00:00 2001 From: Oliver Bartsch Date: Sat, 29 Aug 2026 15:13:55 +0200 Subject: [PATCH 2/2] [BUGFIX] Store cost and score values as decimal The cost and score columns were declared as double(10,6) and double(5,4). Doctrine maps double to its FloatType, which drops precision and scale when rendering a column, so the declared width could never be materialised: the columns end up as plain double in the database. The schema comparator does compare the parsed scale, however, so every "Analyze Database Structure" run reported a change for these columns, generated an ALTER TABLE without a scale, applied nothing, and reported the very same change again on the next run. Declaring the columns as decimal fixes this, as DecimalType honours precision and scale, so the comparison converges after one migration. It is also the correct type for monetary values, avoiding binary float rounding on accumulated cost sums. As a side effect this drops the non-standard double(M,D) syntax, deprecated since MySQL 8.0.17 and removed in newer MySQL versions. Resolves #27 --- CHANGELOG.md | 1 + ext_tables.sql | 16 ++++++++-------- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7474b0a..c0b2dfd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ ## Unreleased - 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) ## 0.4.1 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,