From c8946ab379745c2061acb27cf5beaa9ec79df3cd Mon Sep 17 00:00:00 2001 From: Marko Glaubitz Date: Fri, 24 Jul 2026 13:34:06 +0200 Subject: [PATCH 1/4] Performance(TestQuestionPool): two-phase query for question browser + index The 'Add from pool' question browser (ilObjTestGUI -> ilTestQuestionBrowserTableGUI -> ilAssQuestionList::load) built one large query with 7 correlated EXISTS subqueries (feedback x4, hints, taxonomies) as SELECT fields. These were evaluated for every candidate row BEFORE ORDER BY/LIMIT, so on large instances (bug report: ~199k rows) the query ran 30-40 min and blocked other queries. Fix: when a Range is set and neither a HAVING filter nor an ORDER BY on a computed column (feedback/hints/taxonomies) is active, load in two phases: Phase A: SELECT question_id + required JOINs/filters + GROUP BY + ORDER BY + LIMIT (no EXISTS subqueries) -> small paginated id set Phase B: full SELECT incl. feedback/hints/taxonomies EXISTS, restricted to the paginated ids via IN (...) -> flags computed only for the ~800 visible rows instead of the full candidate set. Fallback to the original single-phase query for: no range, HAVING filter (feedback/hints = true|false), ORDER BY feedback/hints/taxonomies. buildOrderQueryExpression now qualifies columns for phase A (qpl_questions.* not selected -> ambiguous title) and backticks qualified names per segment (`qpl_questions`.`title`). Adds composite index i6 (obj_fi, original_id, title) on qpl_questions via ilTestQuestionPool10DBUpdateSteps::step_3() to support the phase A filter (obj_fi IN ... AND original_id IS NULL) + default title ordering. Adds ilAssQuestionListTwoPhaseTest (15 tests) covering the two-phase path, all fallback conditions, column qualification and SQL shape. Verified with EXPLAIN against the local docker DB: phase A has no DEPENDENT SUBQUERY (3 simple JOINs only), phase B's subqueries are evaluated over rows=2 (paginated set) instead of the full candidate set. --- ...lass.ilTestQuestionPool10DBUpdateSteps.php | 19 ++ .../classes/class.ilAssQuestionList.php | 211 +++++++++++++++++- .../tests/ilAssQuestionListTwoPhaseTest.php | 200 +++++++++++++++++ 3 files changed, 428 insertions(+), 2 deletions(-) create mode 100644 components/ILIAS/TestQuestionPool/tests/ilAssQuestionListTwoPhaseTest.php diff --git a/components/ILIAS/TestQuestionPool/classes/Setup/class.ilTestQuestionPool10DBUpdateSteps.php b/components/ILIAS/TestQuestionPool/classes/Setup/class.ilTestQuestionPool10DBUpdateSteps.php index 7b9b56281927..3d3827969f7e 100755 --- a/components/ILIAS/TestQuestionPool/classes/Setup/class.ilTestQuestionPool10DBUpdateSteps.php +++ b/components/ILIAS/TestQuestionPool/classes/Setup/class.ilTestQuestionPool10DBUpdateSteps.php @@ -47,4 +47,23 @@ public function step_2(): void $this->db->dropTableColumn('qpl_questionpool', 'show_taxonomies'); } } + + /** + * Composite index supporting the paginated question browser query + * (ilAssQuestionList two-phase loading, phase A) which filters by + * obj_fi (eq / IN) and original_id IS NULL and commonly orders by + * title. Covers both the pool-internal view (obj_fi = X) and the + * "add from pool" browser (obj_fi IN (...)) of ilObjTestGUI. + * + * Note: ILIAS' ilDBPdoFieldDefinition::checkIndexName limits index + * names to 3 characters, hence the short name "i6" (the existing + * i1_idx..i5_idx on this table predate that constraint). + */ + public function step_3(): void + { + $fields = ['obj_fi', 'original_id', 'title']; + if (!$this->db->indexExistsByFields('qpl_questions', $fields)) { + $this->db->addIndex('qpl_questions', $fields, 'i6'); + } + } } diff --git a/components/ILIAS/TestQuestionPool/classes/class.ilAssQuestionList.php b/components/ILIAS/TestQuestionPool/classes/class.ilAssQuestionList.php index d7046c9e6df0..fe295adf1fa2 100755 --- a/components/ILIAS/TestQuestionPool/classes/class.ilAssQuestionList.php +++ b/components/ILIAS/TestQuestionPool/classes/class.ilAssQuestionList.php @@ -545,7 +545,13 @@ private function getHavingFilterExpression(): string return $having !== '' ? "HAVING $having" : ''; } - private function buildOrderQueryExpression(): string + /** + * @param bool $qualify In the two-phase ID query (phase A) qpl_questions.* + * is not selected, so ambiguous order columns like + * `title` (exists on qpl_questions and object_data) + * must be qualified to their table. + */ + private function buildOrderQueryExpression(bool $qualify = false): string { $order = $this->order; if ($order === null) { @@ -562,7 +568,37 @@ private function buildOrderQueryExpression(): string $order_direction = Order::ASC; } - return " ORDER BY `$order_field` $order_direction"; + if ($qualify) { + $order_field = $this->qualifyOrderField($order_field); + } + + // Backtick each segment separately so qualified names like + // qpl_questions.title become `qpl_questions`.`title` (a single + // pair of backticks around the dotted name would be parsed as one + // oddly-named column). + $order_field = implode('.', array_map( + static fn(string $seg): string => '`' . $seg . '`', + explode('.', $order_field) + )); + + return " ORDER BY $order_field $order_direction"; + } + + /** + * Map an order field to its fully qualified column. Needed in phase A + * where qpl_questions.* is not selected and columns like `title` would + * be ambiguous between qpl_questions and object_data. + */ + private function qualifyOrderField(string $field): string + { + return match ($field) { + 'title', 'description', 'author', 'lifecycle', 'points', + 'created', 'tstamp', 'complete', 'question_id', 'original_id' => "qpl_questions.$field", + 'max_points' => 'qpl_questions.points', + 'type_tag' => 'qpl_qst_type.type_tag', + 'parent_title' => 'object_data.title', + default => $field, + }; } private function buildLimitQueryExpression(): string @@ -589,10 +625,181 @@ private function buildQuery(): string ])); } + /** + * Two-phase query loading can be used when the result is paginated + * (range set) and neither a HAVING filter nor an ORDER BY on a computed + * column (feedback/hints/taxonomies) is active. In that case the + * expensive correlated EXISTS subqueries are evaluated only for the + * small paginated set of question ids instead of the full candidate + * set before LIMIT/OFFSET is applied. + */ + private function canUseTwoPhaseQuery(): bool + { + if ($this->range === null) { + return false; + } + if ($this->getHavingFilterExpression() !== '') { + return false; + } + if ($this->isOrderByComputedField()) { + return false; + } + return true; + } + + /** + * ORDER BY targets one of the computed columns (feedback/hints/taxonomies) + * that only exist in the single-phase query. Two-phase loading cannot + * sort by these and must fall back to the single-phase query. + */ + private function isOrderByComputedField(): bool + { + if ($this->order === null) { + return false; + } + [$order_field] = $this->order->join( + '', + static fn(string $index, string $key, string $value): array => [$key, $value] + ); + return in_array($order_field, ['feedback', 'hints', 'taxonomies'], true); + } + + /** + * Phase A: determine the paginated, ordered set of question ids using + * only the required JOINs/filters — without the expensive correlated + * EXISTS subqueries for feedback/hints/taxonomies. + * + * GROUP BY question_id (instead of DISTINCT) is used so that ORDER BY + * may reference any column functionally dependent on question_id (e.g. + * qpl_questions.title) without having to add it to the SELECT list — + * MySQL rejects `SELECT DISTINCT question_id ... ORDER BY title`. + * GROUP BY also deduplicates rows that the tst_test_question / + * feedback / hints filter JOINs may produce. + */ + private function buildPaginatedIdsQuery(): string + { + return implode(PHP_EOL, array_filter([ + "SELECT qpl_questions.question_id FROM qpl_questions {$this->getTableJoinExpression()}", + "WHERE qpl_questions.tstamp > 0", + $this->getConditionalFilterExpression(), + "GROUP BY qpl_questions.question_id", + $this->buildOrderQueryExpression(true), + $this->buildLimitQueryExpression(), + ])); + } + + /** + * Phase B: enrich the small paginated set of question ids with the full + * select fields including the computed feedback/hints/taxonomies flags. + * Filter JOINs (feedback/hints) are NOT applied here — they were already + * honoured in phase A. + */ + private function buildEnrichmentQuery(array $question_ids): string + { + $in = $this->db->in('qpl_questions.question_id', $question_ids, false, ilDBConstants::T_INTEGER); + return implode(PHP_EOL, array_filter([ + "{$this->getSelectFieldsExpression()} FROM qpl_questions {$this->getBaseTableJoinExpression()}", + "WHERE qpl_questions.tstamp > 0 AND $in", + ])); + } + + /** + * Base table joins without the feedback/hints filter JOINs added by + * handleFeedbackJoin()/handleHintJoin(). Used in phase B where those + * filter JOINs are not needed (filtering happened in phase A). + */ + private function getBaseTableJoinExpression(): string + { + $table_join = ' + INNER JOIN qpl_qst_type + ON qpl_qst_type.question_type_id = qpl_questions.question_type_fi + '; + + if ($this->join_obj_data) { + $table_join .= ' + INNER JOIN object_data + ON object_data.obj_id = qpl_questions.obj_fi + '; + } + + if ( + $this->parentObjType === 'tst' + && $this->questionInstanceTypeFilter === self::QUESTION_INSTANCE_TYPE_ALL + ) { + $table_join .= 'INNER JOIN tst_test_question tstquest ON tstquest.question_fi = qpl_questions.question_id'; + } + + if ($this->answerStatusActiveId) { + $table_join .= " + LEFT JOIN tst_test_result + ON tst_test_result.question_fi = qpl_questions.question_id + AND tst_test_result.active_fi = {$this->db->quote($this->answerStatusActiveId, ilDBConstants::T_INTEGER)} + "; + } + + return $table_join; + } + + private function loadTwoPhase(): void + { + $tags_trafo = $this->refinery->encode()->htmlSpecialCharsAsEntities(); + + $ids_res = $this->db->query($this->buildPaginatedIdsQuery()); + $ordered_ids = []; + while ($row = $this->db->fetchAssoc($ids_res)) { + $ordered_ids[] = (int) $row['question_id']; + } + + if ($ordered_ids === []) { + return; + } + + $rows_by_id = []; + $res = $this->db->query($this->buildEnrichmentQuery($ordered_ids)); + while ($row = $this->db->fetchAssoc($res)) { + $rows_by_id[(int) $row['question_id']] = $row; + } + + foreach ($ordered_ids as $question_id) { + if (!isset($rows_by_id[$question_id])) { + continue; + } + $row = $rows_by_id[$question_id]; + $row = ilAssQuestionType::completeMissingPluginName($row); + + if (!$this->isActiveQuestionType($row)) { + continue; + } + + $row['title'] = $tags_trafo->transform($row['title'] ?? ' '); + $row['description'] = $tags_trafo->transform($row['description'] ?? ''); + $row['author'] = $tags_trafo->transform($row['author']); + $row['taxonomies'] = $this->loadTaxonomyAssignmentData($row['obj_fi'], $row['question_id']); + $row['ttype'] = $this->lng->txt($row['type_tag']); + $row['feedback'] = $row['feedback'] === 1; + $row['hints'] = $row['hints'] === 1; + $row['comments'] = $this->getNumberOfCommentsForQuestion($row['question_id']); + + if ( + $this->filter_comments === self::QUESTION_COMMENTED_ONLY && $row['comments'] === 0 + || $this->filter_comments === self::QUESTION_COMMENTED_EXCLUDED && $row['comments'] > 0 + ) { + continue; + } + + $this->questions[$row['question_id']] = $row; + } + } + public function load(): void { $this->checkFilters(); + if ($this->canUseTwoPhaseQuery()) { + $this->loadTwoPhase(); + return; + } + $tags_trafo = $this->refinery->encode()->htmlSpecialCharsAsEntities(); $res = $this->db->query($this->buildQuery()); diff --git a/components/ILIAS/TestQuestionPool/tests/ilAssQuestionListTwoPhaseTest.php b/components/ILIAS/TestQuestionPool/tests/ilAssQuestionListTwoPhaseTest.php new file mode 100644 index 000000000000..96c8750b5f0e --- /dev/null +++ b/components/ILIAS/TestQuestionPool/tests/ilAssQuestionListTwoPhaseTest.php @@ -0,0 +1,200 @@ +createMock(ilDBInterface::class); + $db->method('like')->willReturn('1'); + $db->method('in')->willReturnCallback(function (string $field, array $values, bool $negate = false): string { + if ($values === []) { + return $negate ? ' 1=1 ' : ' 1=2 '; + } + $list = implode(',', array_map('intval', $values)); + return ($negate ? 'NOT ' : '') . "$field IN ($list)"; + }); + $db->method('quote')->willReturnCallback(fn($v) => is_numeric($v) ? (string) $v : "'$v'"); + + $lng = $this->createMock(ilLanguage::class); + $refinery = $this->createMock(ILIAS\Refinery\Factory::class); + $component_repository = $this->createMock(ilComponentRepository::class); + $notes_service = $this->createMock(ILIAS\Notes\Service::class); + + $this->object = new ilAssQuestionList($db, $lng, $refinery, $component_repository, $notes_service); + } + + private function setPrivateProperty(string $name, mixed $value): void + { + $prop = new ReflectionProperty(ilAssQuestionList::class, $name); + $prop->setAccessible(true); + $prop->setValue($this->object, $value); + } + + private function invokePrivate(string $name, array $args = []): mixed + { + $method = new ReflectionMethod(ilAssQuestionList::class, $name); + $method->setAccessible(true); + return $method->invokeArgs($this->object, $args); + } + + public function testCanUseTwoPhaseQueryReturnsFalseWithoutRange(): void + { + $this->setPrivateProperty('range', null); + $this->assertFalse($this->invokePrivate('canUseTwoPhaseQuery')); + } + + public function testCanUseTwoPhaseQueryReturnsTrueWithRangeAndSimpleOrder(): void + { + $this->setPrivateProperty('range', new Range(0, 800)); + $this->setPrivateProperty('order', new Order('title', Order::ASC)); + $this->assertTrue($this->invokePrivate('canUseTwoPhaseQuery')); + } + + public function testCanUseTwoPhaseQueryReturnsFalseForOrderByFeedback(): void + { + $this->setPrivateProperty('range', new Range(0, 800)); + $this->setPrivateProperty('order', new Order('feedback', Order::ASC)); + $this->assertFalse($this->invokePrivate('canUseTwoPhaseQuery')); + } + + public function testCanUseTwoPhaseQueryReturnsFalseForOrderByHints(): void + { + $this->setPrivateProperty('range', new Range(0, 800)); + $this->setPrivateProperty('order', new Order('hints', Order::ASC)); + $this->assertFalse($this->invokePrivate('canUseTwoPhaseQuery')); + } + + public function testCanUseTwoPhaseQueryReturnsFalseForOrderByTaxonomies(): void + { + $this->setPrivateProperty('range', new Range(0, 800)); + $this->setPrivateProperty('order', new Order('taxonomies', Order::ASC)); + $this->assertFalse($this->invokePrivate('canUseTwoPhaseQuery')); + } + + public function testCanUseTwoPhaseQueryReturnsFalseWithFeedbackFalseFilter(): void + { + $this->setPrivateProperty('range', new Range(0, 800)); + $this->setPrivateProperty('order', new Order('title', Order::ASC)); + $this->setPrivateProperty('fieldFilters', ['feedback' => 'false']); + $this->assertFalse($this->invokePrivate('canUseTwoPhaseQuery')); + } + + public function testCanUseTwoPhaseQueryReturnsFalseWithFeedbackTrueFilter(): void + { + // feedback=true uses an INNER JOIN but ALSO a HAVING clause -> fallback + $this->setPrivateProperty('range', new Range(0, 800)); + $this->setPrivateProperty('order', new Order('title', Order::ASC)); + $this->setPrivateProperty('fieldFilters', ['feedback' => 'true']); + $this->assertFalse($this->invokePrivate('canUseTwoPhaseQuery')); + } + + public function testQualifyOrderFieldMapsKnownFields(): void + { + $this->assertSame('qpl_questions.title', $this->invokePrivate('qualifyOrderField', ['title'])); + $this->assertSame('qpl_questions.description', $this->invokePrivate('qualifyOrderField', ['description'])); + $this->assertSame('qpl_questions.author', $this->invokePrivate('qualifyOrderField', ['author'])); + $this->assertSame('qpl_questions.points', $this->invokePrivate('qualifyOrderField', ['points'])); + $this->assertSame('qpl_questions.points', $this->invokePrivate('qualifyOrderField', ['max_points'])); + $this->assertSame('qpl_questions.created', $this->invokePrivate('qualifyOrderField', ['created'])); + $this->assertSame('qpl_questions.tstamp', $this->invokePrivate('qualifyOrderField', ['tstamp'])); + $this->assertSame('qpl_qst_type.type_tag', $this->invokePrivate('qualifyOrderField', ['type_tag'])); + $this->assertSame('object_data.title', $this->invokePrivate('qualifyOrderField', ['parent_title'])); + } + + public function testQualifyOrderFieldLeavesUnknownFieldsUnchanged(): void + { + $this->assertSame('feedback', $this->invokePrivate('qualifyOrderField', ['feedback'])); + $this->assertSame('hints', $this->invokePrivate('qualifyOrderField', ['hints'])); + $this->assertSame('taxonomies', $this->invokePrivate('qualifyOrderField', ['taxonomies'])); + } + + public function testBuildPaginatedIdsQueryDoesNotContainExistsSubqueries(): void + { + $this->setPrivateProperty('range', new Range(0, 800)); + $this->setPrivateProperty('order', new Order('title', Order::ASC)); + $this->setPrivateProperty('join_obj_data', true); + + $sql = $this->invokePrivate('buildPaginatedIdsQuery'); + + $this->assertStringContainsString('SELECT qpl_questions.question_id', $sql); + $this->assertStringNotContainsString('EXISTS', $sql); + $this->assertStringNotContainsString('qpl_fb_generic', $sql); + $this->assertStringNotContainsString('qpl_hints', $sql); + $this->assertStringNotContainsString('tax_node_assignment', $sql); + $this->assertStringContainsString('`qpl_questions`.`title`', $sql); + $this->assertStringContainsString('GROUP BY qpl_questions.question_id', $sql); + $this->assertStringContainsString('LIMIT 800 OFFSET 0', $sql); + } + + public function testBuildEnrichmentQueryContainsExistsSubqueriesAndInClause(): void + { + $this->setPrivateProperty('join_obj_data', true); + + $sql = $this->invokePrivate('buildEnrichmentQuery', [[10, 20, 30]]); + + $this->assertStringContainsString('EXISTS', $sql); + $this->assertStringContainsString('qpl_fb_generic', $sql); + $this->assertStringContainsString('qpl_hints', $sql); + $this->assertStringContainsString('tax_node_assignment', $sql); + $this->assertStringContainsString('qpl_questions.question_id IN (10,20,30)', $sql); + $this->assertStringNotContainsString('LIMIT', $sql); + } + + public function testBuildEnrichmentQueryHandlesEmptyIdList(): void + { + $this->setPrivateProperty('join_obj_data', true); + $sql = $this->invokePrivate('buildEnrichmentQuery', [[]]); + $this->assertStringContainsString('1=2', $sql); + } + + public function testBuildOrderQueryExpressionQualifiesWhenRequested(): void + { + $this->setPrivateProperty('order', new Order('title', Order::ASC)); + $sql = $this->invokePrivate('buildOrderQueryExpression', [true]); + $this->assertStringContainsString('`qpl_questions`.`title`', $sql); + } + + public function testBuildOrderQueryExpressionDoesNotQualifyByDefault(): void + { + $this->setPrivateProperty('order', new Order('title', Order::ASC)); + $sql = $this->invokePrivate('buildOrderQueryExpression'); + $this->assertStringNotContainsString('qpl_questions', $sql); + $this->assertStringContainsString('`title`', $sql); + } + + public function testIsOrderByComputedFieldReturnsFalseWithoutOrder(): void + { + $this->setPrivateProperty('order', null); + $this->assertFalse($this->invokePrivate('isOrderByComputedField')); + } +} From 7f64aea23503ff69e78a7c25e9acf86658968599 Mon Sep 17 00:00:00 2001 From: Marko Glaubitz Date: Fri, 24 Jul 2026 13:34:09 +0200 Subject: [PATCH 2/4] Docs(TestQuestionPool): performance plan for question browser two-phase query --- PERFORMANCE_PLAN.md | 207 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 207 insertions(+) create mode 100644 PERFORMANCE_PLAN.md diff --git a/PERFORMANCE_PLAN.md b/PERFORMANCE_PLAN.md new file mode 100644 index 000000000000..a0d0aaf527a4 --- /dev/null +++ b/PERFORMANCE_PLAN.md @@ -0,0 +1,207 @@ +# Performance: Test "Add from pool" — Fragenbrowser + +## Problem + +`ilObjTestGUI::showQuestions` → Button "Add from pool" → +`ilTestQuestionBrowserTableGUI::browseQuestionsCmd` baut eine +`ilAssQuestionList` und ruft `load()` mit gesetzter Range/Order auf. + +`ilAssQuestionList::buildQuery()` (`class.ilAssQuestionList.php:577`) +setzt über `getSelectFieldsExpression()` (`:444`) **korrelierte +EXISTS-Subqueries** als SELECT-Felder: + +- `generateFeedbackSubquery()` (`:476`) — 4 abhängige Subqueries + (qpl_fb_generic, qpl_fb_specific, jeweils + page_object) +- `generateHintSubquery()` (`:500`) — qpl_hints (EXPLAIN: `ALL`-Scan) +- `generateTaxonomySubquery()` (`:506`) — tax_node_assignment + +Diese werden für **jede Kandidatenzeile** berechnet, **bevor** +`ORDER BY`/`LIMIT` greifen (EXPLAIN: `Using temporary; Using filesort` +über ~199k Zeilen). Auf großen Instanzen → 30–40 min Laufzeit, +Blockieren anderer Queries. Lokal (1 Pool, 2 Fragen) nicht reproduzierbar. + +Bug-Report schlägt vor: erst paginierte question_id-Menge bestimmen, +dann Flags nur für diese IDs berechnen. + +## Lösungsansatz: Zweiphasige Abfrage + +**Aktiv nur wenn `$this->range !== null`** (Pagination aktiv — der +Langsamm-Pfad). Alle anderen Caller setzen keine Range → unverändert. + +### Phase A — ID-Auswahl (`buildPaginatedIdsQuery()`) + +``` +SELECT DISTINCT qpl_questions.question_id +FROM qpl_questions + {getTableJoinExpression()} -- inkl. Filter-Joins (handleFeedbackJoin/handleHintJoin) +WHERE qpl_questions.tstamp > 0 + {getConditionalFilterExpression()} -- alle WHERE-Filter + {buildOrderQueryExpression()} -- Spalten qualifiziert! + {buildLimitQueryExpression()} -- Pagination hier +``` + +- **kein** `getSelectFieldsExpression()` (keine EXISTS-Subqueries) +- **kein** `getHavingFilterExpression()` +- liefert geordnete Liste von `question_id`s + +### Phase B — Anreicherung (`buildEnrichmentQuery(array $ids)`) + +``` +{getSelectFieldsExpression()} -- inkl. feedback/hints/taxonomies (nur f. wenige IDs!) +FROM qpl_questions + {getBaseTableJoinExpression()} -- qpl_qst_type, object_data, ggf. tst_test_result + -- OHNE handleFeedbackJoin/handleHintJoin +WHERE qpl_questions.tstamp > 0 + AND qpl_questions.question_id IN () +``` + +- Ergebnis in PHP nach Phase-A-Reihenfolge ordnen. + +### Fallback auf alte Einzelabfrage + +Wenn **eine** Bedingung zutrifft: + +1. `getHavingFilterExpression() !== ''` + (Filter `feedback=false`/`hints=false` nutzt HAVING) +2. Order-Feld ∈ `{feedback, hints, taxonomies}` + (Sortierung nach berechneter Spalte → Phase A kann nicht danach sortieren) + +→ alte `buildQuery()` (unverändert). + +## Detailänderungen in `class.ilAssQuestionList.php` + +### 1. `load()` (`:588`) + +```php +public function load(): void +{ + $this->checkFilters(); + + if ($this->canUseTwoPhaseQuery()) { + $this->loadTwoPhase(); + return; + } + + $this->loadSinglePhase(); // bisherige Implementierung +} +``` + +### 2. Neu: `canUseTwoPhaseQuery(): bool` + +```php +private function canUseTwoPhaseQuery(): bool +{ + if ($this->range === null) { + return false; + } + if ($this->getHavingFilterExpression() !== '') { + return false; + } + if ($this->order !== null + && $this->isOrderByComputedField() + ) { + return false; + } + return true; +} +``` + +### 3. Neu: `buildPaginatedIdsQuery(): string` + +SELECT DISTINCT question_id + Joins + Filter + Order(qualifiziert) + Limit. + +### 4. Neu: `buildEnrichmentQuery(array $ids): string` + +getSelectFieldsExpression() + getBaseTableJoinExpression() + WHERE IN. + +### 5. Neu: `getBaseTableJoinExpression(): string` + +Wie `getTableJoinExpression()`, aber **ohne** `handleFeedbackJoin`/ +`handleHintJoin` (die waren nur Filter-Joins, in Phase A bereits +angewendet; in Phase B stören sie nur). + +### 6. `buildOrderQueryExpression()` (`:544`) erweitern + +Neuer Parameter `bool $qualify = false`. Bei `$qualify=true`: +Spalten qualifizieren, da `qpl_questions.*` in Phase A fehlt → sonst +"Column 'title' is ambiguous": + +| Order-Feld | qualifiziert zu | +|---------------|---------------------------| +| title | qpl_questions.title | +| description | qpl_questions.description | +| author | qpl_questions.author | +| lifecycle | qpl_questions.lifecycle | +| points | qpl_questions.points | +| max_points | qpl_questions.points | +| created | qpl_questions.created | +| tstamp | qpl_questions.tstamp | +| type_tag | qpl_qst_type.type_tag | +| parent_title | object_data.title | +| feedback/hints/taxonomies | (Fallback — kommt hier nie an) | + +### 7. Neu: `isOrderByComputedField(): bool` + +Prüft ob Order-Feld ∈ {feedback, hints, taxonomies}. + +### 8. `getTotalRowCount()` (`:622`) + +Unverändert (bereits ohne EXISTS-Subqueries → schnell). + +**Vorhandener Bug (nur dokumentieren):** wendet `getHavingFilterExpression()` +nicht an → Count bei `feedback=false`-Filter falsch. Nicht Teil dieses +Tickets. + +## DB-Index — `class.ilTestQuestionPool10DBUpdateSteps.php` + +Neue `step_3()`: Composite-Index `(obj_fi, original_id, title)` auf +`qpl_questions` als `i6` (ILIAS' `checkIndexName` limitiert Indexnamen +auf 3 Zeichen; die bestehenden `i1_idx`…`i5_idx` predaten diese Regel). + +- Deckt Phase A für beide Hauptpfade: + - Pool-intern: `obj_fi = X` + `original_id IS NULL` + - Browse-from-pools: `obj_fi IN (...)` + `original_id IS NULL` +- `title` als dritte Spalte unterstützt Default-Sortierung. +- `addIndex()` ist idempotent (vorhandene Single-Column-Indexes + `i3_idx`(obj_fi), `i2_idx`(original_id), `i4_idx`(title) bleiben). +- Trade-off: höhere Schreibkosten auf `qpl_questions` bei INSERT/UPDATE + (große Tabelle) — akzeptiert zugunsten der Lese-Performance. + +## Kompatibilität + +- `QuestionTable extends ilAssQuestionList`: überschreibt `load()` nicht; + `getData()` setzt **kein** `setRange` auf der Liste (paginiert in PHP) + → `canUseTwoPhaseQuery()` liefert `false` → unverändert. +- `getTotalRowCount()` dort ruft `load()`+`count()` auf (lädt alles — + separates Problem, nicht dieses Ticket). +- Alle anderen Caller (`ilTestResultsGUI`, `ilTestSkillAdministrationGUI`, + `ilLMTracker`, `ilAsqFactory`, `ilCopySelfAssQuestionTableGUI`, + `ilObjQuestionPoolTaxonomyEditingCommandForwarder`, + `ilQuestionPoolSkillAdministrationGUI`, `ilTestPlayerAbstractGUI`) + setzen keine Range → unverändert. + +## Validierung + +1. `ilAssQuestionListTest` ergänzen: + - Phase-A/B-Pfad mit Mock-DB + - Fallback bei Sortierung nach `feedback` + - Fallback bei Filter `feedback=false` (HAVING) + - Fallback bei fehlender Range +2. Docker: Setup-Step ausführen, Index prüfen + (`SHOW INDEX FROM qpl_questions`). +3. Docker: Query-Log vor/nachher (ILIAS-Debug-Toolbar) — + 2 Queries statt 1, dafür ohne korrelierte Subqueries über die + Kandidatenmenge. +4. Edge Cases manuell: + - Sortierung nach `feedback`/`hints`/`taxonomies` → Fallback + - Filter `feedback=false` → Fallback + - Filter `feedback=true` → Fallback (INNER JOIN + HAVING) + - Sortierung nach `title`/`parent_title`/`type_tag` → Phase A +5. Auf großer Instanz: EXPLAIN der Phase-A-Query (sollte nur noch + `qpl_questions` + billige JOINs, keine `DEPENDENT SUBQUERY`), + Laufzeit messen. + +## Out of scope + +- `QuestionTable` (PHP-Pagination, lädt alle) — separates Ticket. +- `getTotalRowCount` HAVING-Korrektur — vorhandener Bug, nur dokumentieren. From 25548320283aa80e8250ffc849ab947005120781 Mon Sep 17 00:00:00 2001 From: Marko Glaubitz Date: Wed, 29 Jul 2026 14:10:36 +0200 Subject: [PATCH 3/4] Performance(TestQuestionPool): unified two-phase query path per core review - Single load() path: step 1 always fetches all readily-available columns + filter/order/limit; step 2 enriches only the small paginated id set with the expensive EXISTS subqueries (feedback/ hints/taxonomies) when they are not already needed for filtering or ordering. - Remove separate canUseTwoPhaseQuery()/loadTwoPhase() execution path; range === null is no longer disallowed. - qualifyField() qualifies all fields (not only order fields); backtickField() extracted as separate helper. - No nested function calls on one line; variables in strings always wrapped in curly braces; reduced complexity. - Rewrite ilAssQuestionListTwoPhaseTest for the new structure. --- .../classes/class.ilAssQuestionList.php | 242 ++++++------------ .../tests/ilAssQuestionListTwoPhaseTest.php | 159 +++++++----- 2 files changed, 169 insertions(+), 232 deletions(-) diff --git a/components/ILIAS/TestQuestionPool/classes/class.ilAssQuestionList.php b/components/ILIAS/TestQuestionPool/classes/class.ilAssQuestionList.php index fe295adf1fa2..2e6607fa949e 100755 --- a/components/ILIAS/TestQuestionPool/classes/class.ilAssQuestionList.php +++ b/components/ILIAS/TestQuestionPool/classes/class.ilAssQuestionList.php @@ -445,7 +445,7 @@ private function getConditionalFilterExpression(): string return $conditions !== '' ? "AND $conditions" : ''; } - private function getSelectFieldsExpression(): string + private function getSelectFieldsExpression(bool $with_computed = true): string { $select_fields = [ 'qpl_questions.*', @@ -469,12 +469,26 @@ private function getSelectFieldsExpression(): string "; } - $select_fields[] = $this->generateFeedbackSubquery(); - $select_fields[] = $this->generateHintSubquery(); - $select_fields[] = $this->generateTaxonomySubquery(); + if ($with_computed) { + $select_fields[] = $this->generateFeedbackSubquery(); + $select_fields[] = $this->generateHintSubquery(); + $select_fields[] = $this->generateTaxonomySubquery(); + } $select_fields = implode(', ', $select_fields); - return "SELECT DISTINCT $select_fields"; + return "SELECT DISTINCT {$select_fields}"; + } + + private function getComputedFieldsExpression(): string + { + $fields = [ + 'qpl_questions.question_id', + $this->generateFeedbackSubquery(), + $this->generateHintSubquery(), + $this->generateTaxonomySubquery(), + ]; + $fields = implode(', ', $fields); + return "SELECT DISTINCT {$fields}"; } private function generateFeedbackSubquery(): string @@ -514,9 +528,11 @@ private function generateTaxonomySubquery(): string return "CASE WHEN EXISTS ($tax_subquery) THEN TRUE ELSE FALSE END AS taxonomies"; } - private function buildBasicQuery(): string + private function buildBasicQuery(bool $with_computed = true): string { - return "{$this->getSelectFieldsExpression()} FROM qpl_questions {$this->getTableJoinExpression()} WHERE qpl_questions.tstamp > 0"; + $select = $this->getSelectFieldsExpression($with_computed); + $joins = $this->getTableJoinExpression(); + return "{$select} FROM qpl_questions {$joins} WHERE qpl_questions.tstamp > 0"; } private function getHavingFilterExpression(): string @@ -545,13 +561,7 @@ private function getHavingFilterExpression(): string return $having !== '' ? "HAVING $having" : ''; } - /** - * @param bool $qualify In the two-phase ID query (phase A) qpl_questions.* - * is not selected, so ambiguous order columns like - * `title` (exists on qpl_questions and object_data) - * must be qualified to their table. - */ - private function buildOrderQueryExpression(bool $qualify = false): string + private function buildOrderQueryExpression(): string { $order = $this->order; if ($order === null) { @@ -568,32 +578,27 @@ private function buildOrderQueryExpression(bool $qualify = false): string $order_direction = Order::ASC; } - if ($qualify) { - $order_field = $this->qualifyOrderField($order_field); - } + $order_field = $this->qualifyField($order_field); + $order_field = $this->backtickField($order_field); - // Backtick each segment separately so qualified names like - // qpl_questions.title become `qpl_questions`.`title` (a single - // pair of backticks around the dotted name would be parsed as one - // oddly-named column). - $order_field = implode('.', array_map( - static fn(string $seg): string => '`' . $seg . '`', - explode('.', $order_field) - )); + return " ORDER BY {$order_field} {$order_direction}"; + } - return " ORDER BY $order_field $order_direction"; + private function backtickField(string $field): string + { + $segments = explode('.', $field); + $quoted = array_map( + static fn(string $segment): string => "`{$segment}`", + $segments + ); + return implode('.', $quoted); } - /** - * Map an order field to its fully qualified column. Needed in phase A - * where qpl_questions.* is not selected and columns like `title` would - * be ambiguous between qpl_questions and object_data. - */ - private function qualifyOrderField(string $field): string + private function qualifyField(string $field): string { return match ($field) { 'title', 'description', 'author', 'lifecycle', 'points', - 'created', 'tstamp', 'complete', 'question_id', 'original_id' => "qpl_questions.$field", + 'created', 'tstamp', 'complete', 'question_id', 'original_id' => "qpl_questions.{$field}", 'max_points' => 'qpl_questions.points', 'type_tag' => 'qpl_qst_type.type_tag', 'parent_title' => 'object_data.title', @@ -611,47 +616,25 @@ private function buildLimitQueryExpression(): string $limit = max($range->getLength(), 0); $offset = max($range->getStart(), 0); - return " LIMIT $limit OFFSET $offset"; + return " LIMIT {$limit} OFFSET {$offset}"; } private function buildQuery(): string { - return implode(PHP_EOL, array_filter([ - $this->buildBasicQuery(), - $this->getConditionalFilterExpression(), - $this->getHavingFilterExpression(), - $this->buildOrderQueryExpression(), - $this->buildLimitQueryExpression(), - ])); + $with_computed = $this->computedColumnsRequired(); + return $this->buildBasicQuery($with_computed) + . $this->getConditionalFilterExpression() + . $this->getHavingFilterExpression() + . $this->buildOrderQueryExpression() + . $this->buildLimitQueryExpression(); } - /** - * Two-phase query loading can be used when the result is paginated - * (range set) and neither a HAVING filter nor an ORDER BY on a computed - * column (feedback/hints/taxonomies) is active. In that case the - * expensive correlated EXISTS subqueries are evaluated only for the - * small paginated set of question ids instead of the full candidate - * set before LIMIT/OFFSET is applied. - */ - private function canUseTwoPhaseQuery(): bool + private function computedColumnsRequired(): bool { - if ($this->range === null) { - return false; - } - if ($this->getHavingFilterExpression() !== '') { - return false; - } - if ($this->isOrderByComputedField()) { - return false; - } - return true; + return $this->getHavingFilterExpression() !== '' + || $this->isOrderByComputedField(); } - /** - * ORDER BY targets one of the computed columns (feedback/hints/taxonomies) - * that only exist in the single-phase query. Two-phase loading cannot - * sort by these and must fall back to the single-phase query. - */ private function isOrderByComputedField(): bool { if ($this->order === null) { @@ -664,50 +647,14 @@ private function isOrderByComputedField(): bool return in_array($order_field, ['feedback', 'hints', 'taxonomies'], true); } - /** - * Phase A: determine the paginated, ordered set of question ids using - * only the required JOINs/filters — without the expensive correlated - * EXISTS subqueries for feedback/hints/taxonomies. - * - * GROUP BY question_id (instead of DISTINCT) is used so that ORDER BY - * may reference any column functionally dependent on question_id (e.g. - * qpl_questions.title) without having to add it to the SELECT list — - * MySQL rejects `SELECT DISTINCT question_id ... ORDER BY title`. - * GROUP BY also deduplicates rows that the tst_test_question / - * feedback / hints filter JOINs may produce. - */ - private function buildPaginatedIdsQuery(): string - { - return implode(PHP_EOL, array_filter([ - "SELECT qpl_questions.question_id FROM qpl_questions {$this->getTableJoinExpression()}", - "WHERE qpl_questions.tstamp > 0", - $this->getConditionalFilterExpression(), - "GROUP BY qpl_questions.question_id", - $this->buildOrderQueryExpression(true), - $this->buildLimitQueryExpression(), - ])); - } - - /** - * Phase B: enrich the small paginated set of question ids with the full - * select fields including the computed feedback/hints/taxonomies flags. - * Filter JOINs (feedback/hints) are NOT applied here — they were already - * honoured in phase A. - */ private function buildEnrichmentQuery(array $question_ids): string { $in = $this->db->in('qpl_questions.question_id', $question_ids, false, ilDBConstants::T_INTEGER); - return implode(PHP_EOL, array_filter([ - "{$this->getSelectFieldsExpression()} FROM qpl_questions {$this->getBaseTableJoinExpression()}", - "WHERE qpl_questions.tstamp > 0 AND $in", - ])); + $select_fields = $this->getComputedFieldsExpression(); + $joins = $this->getBaseTableJoinExpression(); + return "{$select_fields} FROM qpl_questions {$joins} WHERE qpl_questions.tstamp > 0 AND {$in}"; } - /** - * Base table joins without the feedback/hints filter JOINs added by - * handleFeedbackJoin()/handleHintJoin(). Used in phase B where those - * filter JOINs are not needed (filtering happened in phase A). - */ private function getBaseTableJoinExpression(): string { $table_join = ' @@ -730,40 +677,46 @@ private function getBaseTableJoinExpression(): string } if ($this->answerStatusActiveId) { + $quoted = $this->db->quote($this->answerStatusActiveId, ilDBConstants::T_INTEGER); $table_join .= " LEFT JOIN tst_test_result ON tst_test_result.question_fi = qpl_questions.question_id - AND tst_test_result.active_fi = {$this->db->quote($this->answerStatusActiveId, ilDBConstants::T_INTEGER)} + AND tst_test_result.active_fi = {$quoted} "; } return $table_join; } - private function loadTwoPhase(): void + public function load(): void { - $tags_trafo = $this->refinery->encode()->htmlSpecialCharsAsEntities(); - - $ids_res = $this->db->query($this->buildPaginatedIdsQuery()); - $ordered_ids = []; - while ($row = $this->db->fetchAssoc($ids_res)) { - $ordered_ids[] = (int) $row['question_id']; - } + $this->checkFilters(); - if ($ordered_ids === []) { - return; - } + $tags_trafo = $this->refinery->encode()->htmlSpecialCharsAsEntities(); + $with_computed = $this->computedColumnsRequired(); + $res = $this->db->query($this->buildQuery()); $rows_by_id = []; - $res = $this->db->query($this->buildEnrichmentQuery($ordered_ids)); + $ordered_ids = []; while ($row = $this->db->fetchAssoc($res)) { - $rows_by_id[(int) $row['question_id']] = $row; + $qid = (int) $row['question_id']; + $rows_by_id[$qid] = $row; + $ordered_ids[] = $qid; + } + + if (!$with_computed && $ordered_ids !== []) { + $enrichment_res = $this->db->query($this->buildEnrichmentQuery($ordered_ids)); + while ($enrichment_row = $this->db->fetchAssoc($enrichment_res)) { + $eid = (int) $enrichment_row['question_id']; + if (isset($rows_by_id[$eid])) { + $rows_by_id[$eid]['feedback'] = $enrichment_row['feedback']; + $rows_by_id[$eid]['hints'] = $enrichment_row['hints']; + $rows_by_id[$eid]['taxonomies'] = $enrichment_row['taxonomies']; + } + } } foreach ($ordered_ids as $question_id) { - if (!isset($rows_by_id[$question_id])) { - continue; - } $row = $rows_by_id[$question_id]; $row = ilAssQuestionType::completeMissingPluginName($row); @@ -771,45 +724,6 @@ private function loadTwoPhase(): void continue; } - $row['title'] = $tags_trafo->transform($row['title'] ?? ' '); - $row['description'] = $tags_trafo->transform($row['description'] ?? ''); - $row['author'] = $tags_trafo->transform($row['author']); - $row['taxonomies'] = $this->loadTaxonomyAssignmentData($row['obj_fi'], $row['question_id']); - $row['ttype'] = $this->lng->txt($row['type_tag']); - $row['feedback'] = $row['feedback'] === 1; - $row['hints'] = $row['hints'] === 1; - $row['comments'] = $this->getNumberOfCommentsForQuestion($row['question_id']); - - if ( - $this->filter_comments === self::QUESTION_COMMENTED_ONLY && $row['comments'] === 0 - || $this->filter_comments === self::QUESTION_COMMENTED_EXCLUDED && $row['comments'] > 0 - ) { - continue; - } - - $this->questions[$row['question_id']] = $row; - } - } - - public function load(): void - { - $this->checkFilters(); - - if ($this->canUseTwoPhaseQuery()) { - $this->loadTwoPhase(); - return; - } - - $tags_trafo = $this->refinery->encode()->htmlSpecialCharsAsEntities(); - - $res = $this->db->query($this->buildQuery()); - while ($row = $this->db->fetchAssoc($res)) { - $row = ilAssQuestionType::completeMissingPluginName($row); - - if (!$this->isActiveQuestionType($row)) { - continue; - } - $row['title'] = $tags_trafo->transform($row['title'] ?? ' '); $row['description'] = $tags_trafo->transform($row['description'] ?? ''); $row['author'] = $tags_trafo->transform($row['author']); @@ -835,9 +749,13 @@ public function getTotalRowCount(?array $filter_data, ?array $additional_paramet $this->checkFilters(); $count = 'COUNT(*)'; - $query = "SELECT $count FROM qpl_questions {$this->getTableJoinExpression()} WHERE qpl_questions.tstamp > 0 {$this->getConditionalFilterExpression()}"; + $joins = $this->getTableJoinExpression(); + $filters = $this->getConditionalFilterExpression(); + $query = "SELECT {$count} FROM qpl_questions {$joins} WHERE qpl_questions.tstamp > 0 {$filters}"; - return (int) ($this->db->query($query)->fetch()[$count] ?? 0); + $result = $this->db->query($query); + $fetch = $this->db->fetchAssoc($result); + return (int) ($fetch[$count] ?? 0); } protected function getNumberOfCommentsForQuestion(int $question_id): int diff --git a/components/ILIAS/TestQuestionPool/tests/ilAssQuestionListTwoPhaseTest.php b/components/ILIAS/TestQuestionPool/tests/ilAssQuestionListTwoPhaseTest.php index 96c8750b5f0e..f4b9c18631a5 100644 --- a/components/ILIAS/TestQuestionPool/tests/ilAssQuestionListTwoPhaseTest.php +++ b/components/ILIAS/TestQuestionPool/tests/ilAssQuestionListTwoPhaseTest.php @@ -9,7 +9,7 @@ * You should have received a copy of said license along with the * source code, too. * - * If this is not the case or you just want to try ILIAS, you'll find + * If you are not the case or you just want to try ILIAS, you'll find * us at: * https://www.ilias.de * https://github.com/ILIAS-eLearning @@ -19,11 +19,6 @@ use ILIAS\Data\Order; use ILIAS\Data\Range; -/** - * Unit tests for the two-phase query loading optimisation in - * ilAssQuestionList (performance fix for the test "add from pool" - * question browser). - */ class ilAssQuestionListTwoPhaseTest extends assBaseTestCase { protected $backupGlobals = false; @@ -41,9 +36,9 @@ protected function setUp(): void return $negate ? ' 1=1 ' : ' 1=2 '; } $list = implode(',', array_map('intval', $values)); - return ($negate ? 'NOT ' : '') . "$field IN ($list)"; + return ($negate ? 'NOT ' : '') . "{$field} IN ({$list})"; }); - $db->method('quote')->willReturnCallback(fn($v) => is_numeric($v) ? (string) $v : "'$v'"); + $db->method('quote')->willReturnCallback(fn($v) => is_numeric($v) ? (string) $v : "'{$v}'"); $lng = $this->createMock(ilLanguage::class); $refinery = $this->createMock(ILIAS\Refinery\Factory::class); @@ -67,101 +62,125 @@ private function invokePrivate(string $name, array $args = []): mixed return $method->invokeArgs($this->object, $args); } - public function testCanUseTwoPhaseQueryReturnsFalseWithoutRange(): void + public function testComputedColumnsRequiredReturnsFalseWithoutHavingOrComputedOrder(): void { - $this->setPrivateProperty('range', null); - $this->assertFalse($this->invokePrivate('canUseTwoPhaseQuery')); - } - - public function testCanUseTwoPhaseQueryReturnsTrueWithRangeAndSimpleOrder(): void - { - $this->setPrivateProperty('range', new Range(0, 800)); $this->setPrivateProperty('order', new Order('title', Order::ASC)); - $this->assertTrue($this->invokePrivate('canUseTwoPhaseQuery')); + $this->assertFalse($this->invokePrivate('computedColumnsRequired')); } - public function testCanUseTwoPhaseQueryReturnsFalseForOrderByFeedback(): void + public function testComputedColumnsRequiredReturnsTrueForOrderByFeedback(): void { - $this->setPrivateProperty('range', new Range(0, 800)); $this->setPrivateProperty('order', new Order('feedback', Order::ASC)); - $this->assertFalse($this->invokePrivate('canUseTwoPhaseQuery')); + $this->assertTrue($this->invokePrivate('computedColumnsRequired')); } - public function testCanUseTwoPhaseQueryReturnsFalseForOrderByHints(): void + public function testComputedColumnsRequiredReturnsTrueForOrderByHints(): void { - $this->setPrivateProperty('range', new Range(0, 800)); $this->setPrivateProperty('order', new Order('hints', Order::ASC)); - $this->assertFalse($this->invokePrivate('canUseTwoPhaseQuery')); + $this->assertTrue($this->invokePrivate('computedColumnsRequired')); } - public function testCanUseTwoPhaseQueryReturnsFalseForOrderByTaxonomies(): void + public function testComputedColumnsRequiredReturnsTrueForOrderByTaxonomies(): void { - $this->setPrivateProperty('range', new Range(0, 800)); $this->setPrivateProperty('order', new Order('taxonomies', Order::ASC)); - $this->assertFalse($this->invokePrivate('canUseTwoPhaseQuery')); + $this->assertTrue($this->invokePrivate('computedColumnsRequired')); } - public function testCanUseTwoPhaseQueryReturnsFalseWithFeedbackFalseFilter(): void + public function testComputedColumnsRequiredReturnsTrueWithFeedbackFalseFilter(): void { - $this->setPrivateProperty('range', new Range(0, 800)); $this->setPrivateProperty('order', new Order('title', Order::ASC)); $this->setPrivateProperty('fieldFilters', ['feedback' => 'false']); - $this->assertFalse($this->invokePrivate('canUseTwoPhaseQuery')); + $this->assertTrue($this->invokePrivate('computedColumnsRequired')); } - public function testCanUseTwoPhaseQueryReturnsFalseWithFeedbackTrueFilter(): void + public function testComputedColumnsRequiredReturnsTrueWithFeedbackTrueFilter(): void { - // feedback=true uses an INNER JOIN but ALSO a HAVING clause -> fallback - $this->setPrivateProperty('range', new Range(0, 800)); $this->setPrivateProperty('order', new Order('title', Order::ASC)); $this->setPrivateProperty('fieldFilters', ['feedback' => 'true']); - $this->assertFalse($this->invokePrivate('canUseTwoPhaseQuery')); + $this->assertTrue($this->invokePrivate('computedColumnsRequired')); } - public function testQualifyOrderFieldMapsKnownFields(): void + public function testIsOrderByComputedFieldReturnsFalseWithoutOrder(): void { - $this->assertSame('qpl_questions.title', $this->invokePrivate('qualifyOrderField', ['title'])); - $this->assertSame('qpl_questions.description', $this->invokePrivate('qualifyOrderField', ['description'])); - $this->assertSame('qpl_questions.author', $this->invokePrivate('qualifyOrderField', ['author'])); - $this->assertSame('qpl_questions.points', $this->invokePrivate('qualifyOrderField', ['points'])); - $this->assertSame('qpl_questions.points', $this->invokePrivate('qualifyOrderField', ['max_points'])); - $this->assertSame('qpl_questions.created', $this->invokePrivate('qualifyOrderField', ['created'])); - $this->assertSame('qpl_questions.tstamp', $this->invokePrivate('qualifyOrderField', ['tstamp'])); - $this->assertSame('qpl_qst_type.type_tag', $this->invokePrivate('qualifyOrderField', ['type_tag'])); - $this->assertSame('object_data.title', $this->invokePrivate('qualifyOrderField', ['parent_title'])); + $this->setPrivateProperty('order', null); + $this->assertFalse($this->invokePrivate('isOrderByComputedField')); } - public function testQualifyOrderFieldLeavesUnknownFieldsUnchanged(): void + public function testIsOrderByComputedFieldReturnsFalseForRegularField(): void { - $this->assertSame('feedback', $this->invokePrivate('qualifyOrderField', ['feedback'])); - $this->assertSame('hints', $this->invokePrivate('qualifyOrderField', ['hints'])); - $this->assertSame('taxonomies', $this->invokePrivate('qualifyOrderField', ['taxonomies'])); + $this->setPrivateProperty('order', new Order('title', Order::ASC)); + $this->assertFalse($this->invokePrivate('isOrderByComputedField')); } - public function testBuildPaginatedIdsQueryDoesNotContainExistsSubqueries(): void + public function testQualifyFieldMapsKnownFields(): void + { + $this->assertSame('qpl_questions.title', $this->invokePrivate('qualifyField', ['title'])); + $this->assertSame('qpl_questions.description', $this->invokePrivate('qualifyField', ['description'])); + $this->assertSame('qpl_questions.author', $this->invokePrivate('qualifyField', ['author'])); + $this->assertSame('qpl_questions.points', $this->invokePrivate('qualifyField', ['points'])); + $this->assertSame('qpl_questions.points', $this->invokePrivate('qualifyField', ['max_points'])); + $this->assertSame('qpl_questions.created', $this->invokePrivate('qualifyField', ['created'])); + $this->assertSame('qpl_questions.tstamp', $this->invokePrivate('qualifyField', ['tstamp'])); + $this->assertSame('qpl_qst_type.type_tag', $this->invokePrivate('qualifyField', ['type_tag'])); + $this->assertSame('object_data.title', $this->invokePrivate('qualifyField', ['parent_title'])); + } + + public function testQualifyFieldLeavesComputedFieldsUnchanged(): void + { + $this->assertSame('feedback', $this->invokePrivate('qualifyField', ['feedback'])); + $this->assertSame('hints', $this->invokePrivate('qualifyField', ['hints'])); + $this->assertSame('taxonomies', $this->invokePrivate('qualifyField', ['taxonomies'])); + } + + public function testBacktickFieldWrapsSimpleField(): void + { + $this->assertSame('`title`', $this->invokePrivate('backtickField', ['title'])); + } + + public function testBacktickFieldWrapsEachSegmentOfQualifiedName(): void + { + $this->assertSame('`qpl_questions`.`title`', $this->invokePrivate('backtickField', ['qpl_questions.title'])); + } + + public function testBuildOrderQueryExpressionQualifiesAndBackticksField(): void { - $this->setPrivateProperty('range', new Range(0, 800)); $this->setPrivateProperty('order', new Order('title', Order::ASC)); - $this->setPrivateProperty('join_obj_data', true); + $sql = $this->invokePrivate('buildOrderQueryExpression'); + $this->assertStringContainsString('`qpl_questions`.`title`', $sql); + $this->assertStringContainsString('ASC', $sql); + } - $sql = $this->invokePrivate('buildPaginatedIdsQuery'); + public function testBuildOrderQueryExpressionReturnsEmptyWithoutOrder(): void + { + $this->setPrivateProperty('order', null); + $this->assertSame('', $this->invokePrivate('buildOrderQueryExpression')); + } - $this->assertStringContainsString('SELECT qpl_questions.question_id', $sql); + public function testBuildBasicQueryWithoutComputedHasNoExistsSubqueries(): void + { + $this->setPrivateProperty('join_obj_data', true); + $sql = $this->invokePrivate('buildBasicQuery', [false]); + $this->assertStringContainsString('qpl_questions.*', $sql); $this->assertStringNotContainsString('EXISTS', $sql); $this->assertStringNotContainsString('qpl_fb_generic', $sql); $this->assertStringNotContainsString('qpl_hints', $sql); $this->assertStringNotContainsString('tax_node_assignment', $sql); - $this->assertStringContainsString('`qpl_questions`.`title`', $sql); - $this->assertStringContainsString('GROUP BY qpl_questions.question_id', $sql); - $this->assertStringContainsString('LIMIT 800 OFFSET 0', $sql); } - public function testBuildEnrichmentQueryContainsExistsSubqueriesAndInClause(): void + public function testBuildBasicQueryWithComputedContainsExistsSubqueries(): void { $this->setPrivateProperty('join_obj_data', true); + $sql = $this->invokePrivate('buildBasicQuery', [true]); + $this->assertStringContainsString('EXISTS', $sql); + $this->assertStringContainsString('qpl_fb_generic', $sql); + $this->assertStringContainsString('qpl_hints', $sql); + $this->assertStringContainsString('tax_node_assignment', $sql); + } + public function testBuildEnrichmentQueryContainsExistsSubqueriesAndInClause(): void + { + $this->setPrivateProperty('join_obj_data', true); $sql = $this->invokePrivate('buildEnrichmentQuery', [[10, 20, 30]]); - $this->assertStringContainsString('EXISTS', $sql); $this->assertStringContainsString('qpl_fb_generic', $sql); $this->assertStringContainsString('qpl_hints', $sql); @@ -177,24 +196,24 @@ public function testBuildEnrichmentQueryHandlesEmptyIdList(): void $this->assertStringContainsString('1=2', $sql); } - public function testBuildOrderQueryExpressionQualifiesWhenRequested(): void + public function testBuildQueryWithoutComputedAppliesRangeAndOrder(): void { + $this->setPrivateProperty('range', new Range(0, 800)); $this->setPrivateProperty('order', new Order('title', Order::ASC)); - $sql = $this->invokePrivate('buildOrderQueryExpression', [true]); + $this->setPrivateProperty('join_obj_data', true); + $sql = $this->invokePrivate('buildQuery'); + $this->assertStringNotContainsString('EXISTS', $sql); $this->assertStringContainsString('`qpl_questions`.`title`', $sql); + $this->assertStringContainsString('LIMIT 800 OFFSET 0', $sql); } - public function testBuildOrderQueryExpressionDoesNotQualifyByDefault(): void - { - $this->setPrivateProperty('order', new Order('title', Order::ASC)); - $sql = $this->invokePrivate('buildOrderQueryExpression'); - $this->assertStringNotContainsString('qpl_questions', $sql); - $this->assertStringContainsString('`title`', $sql); - } - - public function testIsOrderByComputedFieldReturnsFalseWithoutOrder(): void + public function testBuildQueryWithComputedIncludesExistsSubqueries(): void { - $this->setPrivateProperty('order', null); - $this->assertFalse($this->invokePrivate('isOrderByComputedField')); + $this->setPrivateProperty('range', new Range(0, 800)); + $this->setPrivateProperty('order', new Order('feedback', Order::ASC)); + $this->setPrivateProperty('join_obj_data', true); + $sql = $this->invokePrivate('buildQuery'); + $this->assertStringContainsString('EXISTS', $sql); + $this->assertStringContainsString('LIMIT 800 OFFSET 0', $sql); } } From 0a00072b214c8e5fe1f98975c3f316a0bb873efd Mon Sep 17 00:00:00 2001 From: Marko Glaubitz Date: Wed, 29 Jul 2026 14:18:53 +0200 Subject: [PATCH 4/4] Fix(TestQuestionPool): restore newline-separated query parts in buildQuery() Direct string concatenation dropped the separator between 'WHERE qpl_questions.tstamp > 0' and the conditional filter expression ('AND ...'), producing invalid SQL like '> 0AND'. Revert to implode(PHP_EOL, array_filter([...])). --- .../classes/class.ilAssQuestionList.php | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/components/ILIAS/TestQuestionPool/classes/class.ilAssQuestionList.php b/components/ILIAS/TestQuestionPool/classes/class.ilAssQuestionList.php index 2e6607fa949e..d629b47af94a 100755 --- a/components/ILIAS/TestQuestionPool/classes/class.ilAssQuestionList.php +++ b/components/ILIAS/TestQuestionPool/classes/class.ilAssQuestionList.php @@ -622,11 +622,13 @@ private function buildLimitQueryExpression(): string private function buildQuery(): string { $with_computed = $this->computedColumnsRequired(); - return $this->buildBasicQuery($with_computed) - . $this->getConditionalFilterExpression() - . $this->getHavingFilterExpression() - . $this->buildOrderQueryExpression() - . $this->buildLimitQueryExpression(); + return implode(PHP_EOL, array_filter([ + $this->buildBasicQuery($with_computed), + $this->getConditionalFilterExpression(), + $this->getHavingFilterExpression(), + $this->buildOrderQueryExpression(), + $this->buildLimitQueryExpression(), + ])); } private function computedColumnsRequired(): bool