From c8946ab379745c2061acb27cf5beaa9ec79df3cd Mon Sep 17 00:00:00 2001 From: Marko Glaubitz Date: Fri, 24 Jul 2026 13:34:06 +0200 Subject: [PATCH 1/6] 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/6] 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/6] 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/6] 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 From ee40e97d8ddb525eea02372756b30b856c50066d Mon Sep 17 00:00:00 2001 From: Marko Glaubitz Date: Thu, 30 Jul 2026 11:24:06 +0200 Subject: [PATCH 5/6] Remove PERFORMANCE_PLAN.md from repo root (breaks RootFolderTest) --- PERFORMANCE_PLAN.md | 207 -------------------------------------------- 1 file changed, 207 deletions(-) delete mode 100644 PERFORMANCE_PLAN.md diff --git a/PERFORMANCE_PLAN.md b/PERFORMANCE_PLAN.md deleted file mode 100644 index a0d0aaf527a4..000000000000 --- a/PERFORMANCE_PLAN.md +++ /dev/null @@ -1,207 +0,0 @@ -# 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 7f9e7ab16f73dc1fb8ee8435648bb5d86fa22e38 Mon Sep 17 00:00:00 2001 From: Marko Glaubitz Date: Fri, 31 Jul 2026 00:17:41 +0200 Subject: [PATCH 6/6] Performance(TestQuestionPool): granular computed fields, table constants, review fixes - Replace blanket $with_computed boolean with per-field set: only the computed columns (feedback/hints/taxonomies) actually needed for HAVING filtering or ORDER BY are included in step 1; step 2 enriches only the remaining ones. - Add class constants for all table names (QUESTION_TABLE_NAME, etc.) and use them consistently throughout the query builders. - Fix truthiness checks: use !== null instead of implicit bool cast for nullable int properties (parentObjId, answerStatusActiveId). - Merge qualifyField()+backtickField() into qualifyAndBacktickField() to avoid intermediate variable reassignment. - Update tests for the new granular structure (22 tests). --- .../classes/class.ilAssQuestionList.php | 402 +++++++++++------- .../tests/ilAssQuestionListTwoPhaseTest.php | 88 ++-- 2 files changed, 310 insertions(+), 180 deletions(-) diff --git a/components/ILIAS/TestQuestionPool/classes/class.ilAssQuestionList.php b/components/ILIAS/TestQuestionPool/classes/class.ilAssQuestionList.php index d629b47af94a..40353cf323b5 100755 --- a/components/ILIAS/TestQuestionPool/classes/class.ilAssQuestionList.php +++ b/components/ILIAS/TestQuestionPool/classes/class.ilAssQuestionList.php @@ -29,6 +29,27 @@ */ class ilAssQuestionList implements ilTaxAssignedItemInfo { + private const QUESTION_TABLE_NAME = 'qpl_questions'; + private const QUESTION_TYPE_TABLE_NAME = 'qpl_qst_type'; + private const OBJECT_DATA_TABLE_NAME = 'object_data'; + private const TEST_QUESTION_TABLE_NAME = 'tst_test_question'; + private const TEST_RESULT_TABLE_NAME = 'tst_test_result'; + private const FEEDBACK_GENERIC_TABLE_NAME = 'qpl_fb_generic'; + private const FEEDBACK_SPECIFIC_TABLE_NAME = 'qpl_fb_specific'; + private const HINTS_TABLE_NAME = 'qpl_hints'; + private const PAGE_OBJECT_TABLE_NAME = 'page_object'; + private const TAX_NODE_ASSIGNMENT_TABLE_NAME = 'tax_node_assignment'; + private const TAX_NODE_TABLE_NAME = 'tax_node'; + + private const COMPUTED_FIELD_FEEDBACK = 'feedback'; + private const COMPUTED_FIELD_HINTS = 'hints'; + private const COMPUTED_FIELD_TAXONOMIES = 'taxonomies'; + private const ALL_COMPUTED_FIELDS = [ + self::COMPUTED_FIELD_FEEDBACK, + self::COMPUTED_FIELD_HINTS, + self::COMPUTED_FIELD_TAXONOMIES, + ]; + private array $parentObjIdsFilter = []; private ?int $parentObjId = null; private string $parentObjType = 'qpl'; @@ -174,12 +195,15 @@ public function setJoinObjectData(bool $a_val): void private function getParentObjFilterExpression(): ?string { - if ($this->parentObjId) { - return "qpl_questions.obj_fi = {$this->db->quote($this->parentObjId, ilDBConstants::T_INTEGER)}"; + $questions_table = self::QUESTION_TABLE_NAME; + if ($this->parentObjId !== null) { + $quoted = $this->db->quote($this->parentObjId, ilDBConstants::T_INTEGER); + return "{$questions_table}.obj_fi = {$quoted}"; } if (!empty($this->parentObjIdsFilter)) { - return $this->db->in('qpl_questions.obj_fi', $this->parentObjIdsFilter, false, ilDBConstants::T_INTEGER); + $field = "{$questions_table}.obj_fi"; + return $this->db->in($field, $this->parentObjIdsFilter, false, ilDBConstants::T_INTEGER); } return null; @@ -188,6 +212,9 @@ private function getParentObjFilterExpression(): ?string private function getFieldFilterExpressions(): array { $expressions = []; + $questions_table = self::QUESTION_TABLE_NAME; + $qst_type_table = self::QUESTION_TYPE_TABLE_NAME; + $object_data_table = self::OBJECT_DATA_TABLE_NAME; foreach ($this->fieldFilters as $fieldName => $fieldValue) { switch ($fieldName) { @@ -195,20 +222,24 @@ private function getFieldFilterExpressions(): array case 'description': case 'author': case 'lifecycle': - $expressions[] = $this->db->like("qpl_questions.$fieldName", ilDBConstants::T_TEXT, "%%$fieldValue%%"); + $field = "{$questions_table}.{$fieldName}"; + $expressions[] = $this->db->like($field, ilDBConstants::T_TEXT, "%%{$fieldValue}%%"); break; case 'type': - $expressions[] = "qpl_qst_type.type_tag = {$this->db->quote($fieldValue, ilDBConstants::T_TEXT)}"; + $quoted = $this->db->quote($fieldValue, ilDBConstants::T_TEXT); + $expressions[] = "{$qst_type_table}.type_tag = {$quoted}"; break; case 'question_id': if ($fieldValue !== '' && !is_array($fieldValue)) { $fieldValue = [$fieldValue]; } - $expressions[] = $this->db->in('qpl_questions.question_id', $fieldValue, false, ilDBConstants::T_INTEGER); + $field = "{$questions_table}.question_id"; + $expressions[] = $this->db->in($field, $fieldValue, false, ilDBConstants::T_INTEGER); break; case 'parent_title': if ($this->join_obj_data) { - $expressions[] = $this->db->like('object_data.title', ilDBConstants::T_TEXT, "%%$fieldValue%%"); + $field = "{$object_data_table}.title"; + $expressions[] = $this->db->like($field, ilDBConstants::T_TEXT, "%%{$fieldValue}%%"); } break; } @@ -226,7 +257,9 @@ private function handleFeedbackJoin(string $tableJoin): string }; if (isset($feedback_join)) { - $SQL = "$feedback_join JOIN qpl_fb_generic ON qpl_fb_generic.question_fi = qpl_questions.question_id "; + $fb_table = self::FEEDBACK_GENERIC_TABLE_NAME; + $q_table = self::QUESTION_TABLE_NAME; + $SQL = "{$feedback_join} JOIN {$fb_table} ON {$fb_table}.question_fi = {$q_table}.question_id "; $tableJoin .= !str_contains($tableJoin, $SQL) ? $SQL : ''; } @@ -235,14 +268,16 @@ private function handleFeedbackJoin(string $tableJoin): string private function handleHintJoin(string $tableJoin): string { - $feedback_join = match ($this->fieldFilters['hints'] ?? null) { + $hint_join = match ($this->fieldFilters['hints'] ?? null) { 'true' => 'INNER', 'false' => 'LEFT', default => null }; - if (isset($feedback_join)) { - $SQL = "$feedback_join JOIN qpl_hints ON qpl_hints.qht_question_fi = qpl_questions.question_id "; + if (isset($hint_join)) { + $hints_table = self::HINTS_TABLE_NAME; + $q_table = self::QUESTION_TABLE_NAME; + $SQL = "{$hint_join} JOIN {$hints_table} ON {$hints_table}.qht_question_fi = {$q_table}.question_id "; $tableJoin .= !str_contains($tableJoin, $SQL) ? $SQL : ''; } @@ -260,27 +295,37 @@ private function getTaxonomyFilterExpressions(): array return $expressions; } - $base = 'SELECT DISTINCT item_id FROM tax_node_assignment'; + $tax_assignment_table = self::TAX_NODE_ASSIGNMENT_TABLE_NAME; + $object_data_table = self::OBJECT_DATA_TABLE_NAME; + $tax_node_table = self::TAX_NODE_TABLE_NAME; + $questions_table = self::QUESTION_TABLE_NAME; + + $base = "SELECT DISTINCT item_id FROM {$tax_assignment_table}"; + $object_data_title_field = "{$object_data_table}.title"; $like_taxonomy_title = $taxonomy_title !== '' - ? "AND {$this->db->like('object_data.title', ilDBConstants::T_TEXT, "%$taxonomy_title%", false)}" + ? "AND " . $this->db->like($object_data_title_field, ilDBConstants::T_TEXT, "%{$taxonomy_title}%", false) : ''; + + $tax_node_title_field = "{$tax_node_table}.title"; $like_taxonomy_node_title = $taxonomy_node_title !== '' - ? "AND {$this->db->like('tax_node.title', ilDBConstants::T_TEXT, "%$taxonomy_node_title%", false)}" + ? "AND " . $this->db->like($tax_node_title_field, ilDBConstants::T_TEXT, "%{$taxonomy_node_title}%", false) : ''; - $inner_join_object_data = "INNER JOIN object_data ON (object_data.obj_id = tax_node_assignment.tax_id AND object_data.type = 'tax' $like_taxonomy_title)"; - $inner_join_tax_node = "INNER JOIN tax_node ON (tax_node.tax_id = tax_node_assignment.tax_id AND tax_node.type = 'taxn' AND tax_node_assignment.node_id = tax_node.obj_id $like_taxonomy_node_title)"; + $inner_join_object_data = "INNER JOIN {$object_data_table} ON ({$object_data_table}.obj_id = {$tax_assignment_table}.tax_id AND {$object_data_table}.type = 'tax' {$like_taxonomy_title})"; + $inner_join_tax_node = "INNER JOIN {$tax_node_table} ON ({$tax_node_table}.tax_id = {$tax_assignment_table}.tax_id AND {$tax_node_table}.type = 'taxn' AND {$tax_assignment_table}.node_id = {$tax_node_table}.obj_id {$like_taxonomy_node_title})"; - $expressions[] = "qpl_questions.question_id IN ($base $inner_join_object_data $inner_join_tax_node)"; + $expressions[] = "{$questions_table}.question_id IN ({$base} {$inner_join_object_data} {$inner_join_tax_node})"; return $expressions; } private function getFilterByAssignedTaxonomyIdsExpression(): array { + $tax_assignment_table = self::TAX_NODE_ASSIGNMENT_TABLE_NAME; if ($this->taxFiltersExcludeAnyObjectsWithTaxonomies) { - return ['question_id NOT IN (SELECT DISTINCT item_id FROM tax_node_assignment)']; + $subquery = "SELECT DISTINCT item_id FROM {$tax_assignment_table}"; + return ["question_id NOT IN ({$subquery})"]; } $expressions = []; @@ -337,9 +382,10 @@ protected function getTaxItems(string $parentType, int $parentObjId, int $taxId, private function getQuestionInstanceTypeFilterExpression(): ?string { + $questions_table = self::QUESTION_TABLE_NAME; return match ($this->questionInstanceTypeFilter) { - self::QUESTION_INSTANCE_TYPE_ORIGINALS => 'qpl_questions.original_id IS NULL', - self::QUESTION_INSTANCE_TYPE_DUPLICATES => 'qpl_questions.original_id IS NOT NULL', + self::QUESTION_INSTANCE_TYPE_ORIGINALS => "{$questions_table}.original_id IS NULL", + self::QUESTION_INSTANCE_TYPE_DUPLICATES => "{$questions_table}.original_id IS NOT NULL", default => null }; } @@ -347,10 +393,12 @@ private function getQuestionInstanceTypeFilterExpression(): ?string private function getQuestionIdsFilterExpressions(): array { $expressions = []; + $questions_table = self::QUESTION_TABLE_NAME; if (!empty($this->includeQuestionIdsFilter)) { + $field = "{$questions_table}.question_id"; $expressions[] = $this->db->in( - 'qpl_questions.question_id', + $field, $this->includeQuestionIdsFilter, false, ilDBConstants::T_INTEGER @@ -358,14 +406,15 @@ private function getQuestionIdsFilterExpressions(): array } if (!empty($this->excludeQuestionIdsFilter)) { + $field = "{$questions_table}.question_id"; $IN = $this->db->in( - 'qpl_questions.question_id', + $field, $this->excludeQuestionIdsFilter, true, ilDBConstants::T_INTEGER ); - $expressions[] = $IN === ' 1=2 ' ? ' 1=1 ' : $IN; // required for ILIAS < 5.0 + $expressions[] = $IN === ' 1=2 ' ? ' 1=1 ' : $IN; } return $expressions; @@ -373,14 +422,16 @@ private function getQuestionIdsFilterExpressions(): array private function getAnswerStatusFilterExpressions(): array { + $test_result_table = self::TEST_RESULT_TABLE_NAME; + $questions_table = self::QUESTION_TABLE_NAME; return match ($this->answerStatusFilter) { - self::ANSWER_STATUS_FILTER_ALL_NON_CORRECT => [' - (tst_test_result.question_fi IS NULL OR tst_test_result.points < qpl_questions.points) - '], - self::ANSWER_STATUS_FILTER_NON_ANSWERED_ONLY => ['tst_test_result.question_fi IS NULL'], + self::ANSWER_STATUS_FILTER_ALL_NON_CORRECT => [" + ({$test_result_table}.question_fi IS NULL OR {$test_result_table}.points < {$questions_table}.points) + "], + self::ANSWER_STATUS_FILTER_NON_ANSWERED_ONLY => ["{$test_result_table}.question_fi IS NULL"], self::ANSWER_STATUS_FILTER_WRONG_ANSWERED_ONLY => [ - 'tst_test_result.question_fi IS NOT NULL', - 'tst_test_result.points < qpl_questions.points' + "{$test_result_table}.question_fi IS NOT NULL", + "{$test_result_table}.points < {$questions_table}.points" ], default => [], }; @@ -388,33 +439,40 @@ private function getAnswerStatusFilterExpressions(): array private function getTableJoinExpression(): string { - $tableJoin = ' - INNER JOIN qpl_qst_type - ON qpl_qst_type.question_type_id = qpl_questions.question_type_fi - '; + $qst_type_table = self::QUESTION_TYPE_TABLE_NAME; + $questions_table = self::QUESTION_TABLE_NAME; + $object_data_table = self::OBJECT_DATA_TABLE_NAME; + $test_question_table = self::TEST_QUESTION_TABLE_NAME; + $test_result_table = self::TEST_RESULT_TABLE_NAME; + + $tableJoin = " + INNER JOIN {$qst_type_table} + ON {$qst_type_table}.question_type_id = {$questions_table}.question_type_fi + "; if ($this->join_obj_data) { - $tableJoin .= ' - INNER JOIN object_data - ON object_data.obj_id = qpl_questions.obj_fi - '; + $tableJoin .= " + INNER JOIN {$object_data_table} + ON {$object_data_table}.obj_id = {$questions_table}.obj_fi + "; } if ( $this->parentObjType === 'tst' && $this->questionInstanceTypeFilter === self::QUESTION_INSTANCE_TYPE_ALL ) { - $tableJoin .= 'INNER JOIN tst_test_question tstquest ON tstquest.question_fi = qpl_questions.question_id'; + $tableJoin .= "INNER JOIN {$test_question_table} tstquest ON tstquest.question_fi = {$questions_table}.question_id"; } $tableJoin = $this->handleFeedbackJoin($tableJoin); $tableJoin = $this->handleHintJoin($tableJoin); - if ($this->answerStatusActiveId) { + if ($this->answerStatusActiveId !== null) { + $quoted = $this->db->quote($this->answerStatusActiveId, ilDBConstants::T_INTEGER); $tableJoin .= " - 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)} + LEFT JOIN {$test_result_table} + ON {$test_result_table}.question_fi = {$questions_table}.question_id + AND {$test_result_table}.active_fi = {$quoted} "; } @@ -425,12 +483,14 @@ private function getConditionalFilterExpression(): string { $conditions = []; - if ($this->getQuestionInstanceTypeFilterExpression() !== null) { - $conditions[] = $this->getQuestionInstanceTypeFilterExpression(); + $instance_type_filter = $this->getQuestionInstanceTypeFilterExpression(); + if ($instance_type_filter !== null) { + $conditions[] = $instance_type_filter; } - if ($this->getParentObjFilterExpression() !== null) { - $conditions[] = $this->getParentObjFilterExpression(); + $parent_obj_filter = $this->getParentObjFilterExpression(); + if ($parent_obj_filter !== null) { + $conditions[] = $parent_obj_filter; } $conditions = array_merge( @@ -441,98 +501,123 @@ private function getConditionalFilterExpression(): string $this->getAnswerStatusFilterExpressions() ); - $conditions = implode(' AND ', $conditions); - return $conditions !== '' ? "AND $conditions" : ''; + $merged = implode(' AND ', $conditions); + return $merged !== '' ? "AND {$merged}" : ''; } - private function getSelectFieldsExpression(bool $with_computed = true): string + private function getSelectFieldsExpression(array $computed_fields = []): string { + $questions_table = self::QUESTION_TABLE_NAME; + $qst_type_table = self::QUESTION_TYPE_TABLE_NAME; + $object_data_table = self::OBJECT_DATA_TABLE_NAME; + $test_result_table = self::TEST_RESULT_TABLE_NAME; + $select_fields = [ - 'qpl_questions.*', - 'qpl_qst_type.type_tag', - 'qpl_qst_type.plugin', - 'qpl_qst_type.plugin_name', - 'qpl_questions.points max_points' + "{$questions_table}.*", + "{$qst_type_table}.type_tag", + "{$qst_type_table}.plugin", + "{$qst_type_table}.plugin_name", + "{$questions_table}.points max_points" ]; if ($this->join_obj_data) { - $select_fields[] = 'object_data.title parent_title'; + $select_fields[] = "{$object_data_table}.title parent_title"; } - if ($this->answerStatusActiveId) { - $select_fields[] = 'tst_test_result.points reached_points'; + if ($this->answerStatusActiveId !== null) { + $select_fields[] = "{$test_result_table}.points reached_points"; $select_fields[] = "CASE - WHEN tst_test_result.points IS NULL THEN '" . self::QUESTION_ANSWER_STATUS_NON_ANSWERED . "' - WHEN tst_test_result.points < qpl_questions.points THEN '" . self::QUESTION_ANSWER_STATUS_WRONG_ANSWERED . "' + WHEN {$test_result_table}.points IS NULL THEN '" . self::QUESTION_ANSWER_STATUS_NON_ANSWERED . "' + WHEN {$test_result_table}.points < {$questions_table}.points THEN '" . self::QUESTION_ANSWER_STATUS_WRONG_ANSWERED . "' ELSE '" . self::QUESTION_ANSWER_STATUS_CORRECT_ANSWERED . "' END question_answer_status "; } - if ($with_computed) { - $select_fields[] = $this->generateFeedbackSubquery(); - $select_fields[] = $this->generateHintSubquery(); - $select_fields[] = $this->generateTaxonomySubquery(); + foreach ($this->computedSubqueriesForFields($computed_fields) as $subquery) { + $select_fields[] = $subquery; } - $select_fields = implode(', ', $select_fields); - return "SELECT DISTINCT {$select_fields}"; + $merged = implode(', ', $select_fields); + return "SELECT DISTINCT {$merged}"; } - private function getComputedFieldsExpression(): string + private function getComputedFieldsExpression(array $computed_fields): string { - $fields = [ - 'qpl_questions.question_id', - $this->generateFeedbackSubquery(), - $this->generateHintSubquery(), - $this->generateTaxonomySubquery(), - ]; - $fields = implode(', ', $fields); - return "SELECT DISTINCT {$fields}"; + $questions_table = self::QUESTION_TABLE_NAME; + $fields = ["{$questions_table}.question_id"]; + foreach ($this->computedSubqueriesForFields($computed_fields) as $subquery) { + $fields[] = $subquery; + } + $merged = implode(', ', $fields); + return "SELECT DISTINCT {$merged}"; + } + + private function computedSubqueriesForFields(array $computed_fields): array + { + $subqueries = []; + if (in_array(self::COMPUTED_FIELD_FEEDBACK, $computed_fields, true)) { + $subqueries[] = $this->generateFeedbackSubquery(); + } + if (in_array(self::COMPUTED_FIELD_HINTS, $computed_fields, true)) { + $subqueries[] = $this->generateHintSubquery(); + } + if (in_array(self::COMPUTED_FIELD_TAXONOMIES, $computed_fields, true)) { + $subqueries[] = $this->generateTaxonomySubquery(); + } + return $subqueries; } private function generateFeedbackSubquery(): string { + $questions_table = self::QUESTION_TABLE_NAME; + $fb_generic_table = self::FEEDBACK_GENERIC_TABLE_NAME; + $fb_specific_table = self::FEEDBACK_SPECIFIC_TABLE_NAME; + $page_object_table = self::PAGE_OBJECT_TABLE_NAME; + $cases = []; - $tables = ['qpl_fb_generic', 'qpl_fb_specific']; + $tables = [$fb_generic_table, $fb_specific_table]; foreach ($tables as $table) { - $subquery = "SELECT 1 FROM $table WHERE $table.question_fi = qpl_questions.question_id AND $table.feedback <> ''"; - $cases[] = "WHEN EXISTS ($subquery) THEN TRUE"; + $subquery = "SELECT 1 FROM {$table} WHERE {$table}.question_fi = {$questions_table}.question_id AND {$table}.feedback <> ''"; + $cases[] = "WHEN EXISTS ({$subquery}) THEN TRUE"; } - $page_object_table = 'page_object'; foreach ($tables as $table) { $subquery = sprintf( - "SELECT 1 FROM $table JOIN $page_object_table ON $page_object_table.page_id = $table.feedback_id WHERE $page_object_table.parent_type IN ('%s', '%s') AND $page_object_table.is_empty <> 1 AND $table.question_fi = qpl_questions.question_id", + "SELECT 1 FROM {$table} JOIN {$page_object_table} ON {$page_object_table}.page_id = {$table}.feedback_id WHERE {$page_object_table}.parent_type IN ('%s', '%s') AND {$page_object_table}.is_empty <> 1 AND {$table}.question_fi = {$questions_table}.question_id", \ilAssQuestionFeedback::PAGE_OBJECT_TYPE_GENERIC_FEEDBACK, \ilAssQuestionFeedback::PAGE_OBJECT_TYPE_SPECIFIC_FEEDBACK, ); - $cases[] = "WHEN EXISTS ($subquery) THEN TRUE"; + $cases[] = "WHEN EXISTS ({$subquery}) THEN TRUE"; } $feedback_case_subquery = implode(' ', $cases); - return "CASE $feedback_case_subquery ELSE FALSE END AS feedback"; + return "CASE {$feedback_case_subquery} ELSE FALSE END AS feedback"; } private function generateHintSubquery(): string { - $hint_subquery = 'SELECT 1 FROM qpl_hints WHERE qpl_hints.qht_question_fi = qpl_questions.question_id'; - return "CASE WHEN EXISTS ($hint_subquery) THEN TRUE ELSE FALSE END AS hints"; + $questions_table = self::QUESTION_TABLE_NAME; + $hints_table = self::HINTS_TABLE_NAME; + $hint_subquery = "SELECT 1 FROM {$hints_table} WHERE {$hints_table}.qht_question_fi = {$questions_table}.question_id"; + return "CASE WHEN EXISTS ({$hint_subquery}) THEN TRUE ELSE FALSE END AS hints"; } private function generateTaxonomySubquery(): string { - $tax_node_assignment_table = 'tax_node_assignment'; - $tax_subquery = "SELECT 1 FROM $tax_node_assignment_table WHERE $tax_node_assignment_table.item_id = qpl_questions.question_id AND $tax_node_assignment_table.item_type = 'quest'"; - return "CASE WHEN EXISTS ($tax_subquery) THEN TRUE ELSE FALSE END AS taxonomies"; + $questions_table = self::QUESTION_TABLE_NAME; + $tax_assignment_table = self::TAX_NODE_ASSIGNMENT_TABLE_NAME; + $tax_subquery = "SELECT 1 FROM {$tax_assignment_table} WHERE {$tax_assignment_table}.item_id = {$questions_table}.question_id AND {$tax_assignment_table}.item_type = 'quest'"; + return "CASE WHEN EXISTS ({$tax_subquery}) THEN TRUE ELSE FALSE END AS taxonomies"; } - private function buildBasicQuery(bool $with_computed = true): string + private function buildBasicQuery(array $computed_fields = []): string { - $select = $this->getSelectFieldsExpression($with_computed); + $select = $this->getSelectFieldsExpression($computed_fields); $joins = $this->getTableJoinExpression(); - return "{$select} FROM qpl_questions {$joins} WHERE qpl_questions.tstamp > 0"; + $questions_table = self::QUESTION_TABLE_NAME; + return "{$select} FROM {$questions_table} {$joins} WHERE {$questions_table}.tstamp > 0"; } private function getHavingFilterExpression(): string @@ -540,25 +625,24 @@ private function getHavingFilterExpression(): string $expressions = []; foreach ($this->fieldFilters as $fieldName => $fieldValue) { - if ($fieldName === 'feedback') { - $fieldValue = strtoupper($fieldValue); - if (in_array($fieldValue, ['TRUE', 'FALSE'], true)) { - $expressions[] = "feedback IS $fieldValue"; + if ($fieldName === self::COMPUTED_FIELD_FEEDBACK) { + $upper = strtoupper((string) $fieldValue); + if (in_array($upper, ['TRUE', 'FALSE'], true)) { + $expressions[] = "feedback IS {$upper}"; } continue; - } - if ($fieldName === 'hints') { - $fieldValue = strtoupper($fieldValue); - if (in_array($fieldValue, ['TRUE', 'FALSE'], true)) { - $expressions[] = "hints IS $fieldValue"; + if ($fieldName === self::COMPUTED_FIELD_HINTS) { + $upper = strtoupper((string) $fieldValue); + if (in_array($upper, ['TRUE', 'FALSE'], true)) { + $expressions[] = "hints IS {$upper}"; } } } $having = implode(' AND ', $expressions); - return $having !== '' ? "HAVING $having" : ''; + return $having !== '' ? "HAVING {$having}" : ''; } private function buildOrderQueryExpression(): string @@ -573,20 +657,20 @@ private function buildOrderQueryExpression(): string static fn(string $index, string $key, string $value): array => [$key, $value] ); - $order_direction = strtoupper($order_direction); - if (!in_array($order_direction, [Order::ASC, Order::DESC], true)) { - $order_direction = Order::ASC; + $upper_direction = strtoupper($order_direction); + if (!in_array($upper_direction, [Order::ASC, Order::DESC], true)) { + $upper_direction = Order::ASC; } - $order_field = $this->qualifyField($order_field); - $order_field = $this->backtickField($order_field); + $quoted_order_field = $this->qualifyAndBacktickField($order_field); - return " ORDER BY {$order_field} {$order_direction}"; + return " ORDER BY {$quoted_order_field} {$upper_direction}"; } - private function backtickField(string $field): string + private function qualifyAndBacktickField(string $field): string { - $segments = explode('.', $field); + $qualified = $this->qualifyField($field); + $segments = explode('.', $qualified); $quoted = array_map( static fn(string $segment): string => "`{$segment}`", $segments @@ -596,12 +680,15 @@ private function backtickField(string $field): string private function qualifyField(string $field): string { + $questions_table = self::QUESTION_TABLE_NAME; + $qst_type_table = self::QUESTION_TYPE_TABLE_NAME; + $object_data_table = self::OBJECT_DATA_TABLE_NAME; 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', + 'created', 'tstamp', 'complete', 'question_id', 'original_id' => "{$questions_table}.{$field}", + 'max_points' => "{$questions_table}.points", + 'type_tag' => "{$qst_type_table}.type_tag", + 'parent_title' => "{$object_data_table}.title", default => $field, }; } @@ -621,9 +708,9 @@ private function buildLimitQueryExpression(): string private function buildQuery(): string { - $with_computed = $this->computedColumnsRequired(); + $required_computed = $this->requiredComputedFields(); return implode(PHP_EOL, array_filter([ - $this->buildBasicQuery($with_computed), + $this->buildBasicQuery($required_computed), $this->getConditionalFilterExpression(), $this->getHavingFilterExpression(), $this->buildOrderQueryExpression(), @@ -631,59 +718,72 @@ private function buildQuery(): string ])); } - private function computedColumnsRequired(): bool + private function requiredComputedFields(): array { - return $this->getHavingFilterExpression() !== '' - || $this->isOrderByComputedField(); - } + $fields = []; - private function isOrderByComputedField(): bool - { - if ($this->order === null) { - return false; + if (isset($this->fieldFilters[self::COMPUTED_FIELD_FEEDBACK])) { + $fields[] = self::COMPUTED_FIELD_FEEDBACK; } - [$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); + if (isset($this->fieldFilters[self::COMPUTED_FIELD_HINTS])) { + $fields[] = self::COMPUTED_FIELD_HINTS; + } + + if ($this->order !== null) { + [$order_field] = $this->order->join( + '', + static fn(string $index, string $key, string $value): array => [$key, $value] + ); + if (in_array($order_field, self::ALL_COMPUTED_FIELDS, true) && !in_array($order_field, $fields, true)) { + $fields[] = $order_field; + } + } + + return $fields; } - private function buildEnrichmentQuery(array $question_ids): string + private function buildEnrichmentQuery(array $question_ids, array $computed_fields): string { - $in = $this->db->in('qpl_questions.question_id', $question_ids, false, ilDBConstants::T_INTEGER); - $select_fields = $this->getComputedFieldsExpression(); + $questions_table = self::QUESTION_TABLE_NAME; + $in = $this->db->in("{$questions_table}.question_id", $question_ids, false, ilDBConstants::T_INTEGER); + $select_fields = $this->getComputedFieldsExpression($computed_fields); $joins = $this->getBaseTableJoinExpression(); - return "{$select_fields} FROM qpl_questions {$joins} WHERE qpl_questions.tstamp > 0 AND {$in}"; + return "{$select_fields} FROM {$questions_table} {$joins} WHERE {$questions_table}.tstamp > 0 AND {$in}"; } private function getBaseTableJoinExpression(): string { - $table_join = ' - INNER JOIN qpl_qst_type - ON qpl_qst_type.question_type_id = qpl_questions.question_type_fi - '; + $qst_type_table = self::QUESTION_TYPE_TABLE_NAME; + $questions_table = self::QUESTION_TABLE_NAME; + $object_data_table = self::OBJECT_DATA_TABLE_NAME; + $test_question_table = self::TEST_QUESTION_TABLE_NAME; + $test_result_table = self::TEST_RESULT_TABLE_NAME; + + $table_join = " + INNER JOIN {$qst_type_table} + ON {$qst_type_table}.question_type_id = {$questions_table}.question_type_fi + "; if ($this->join_obj_data) { - $table_join .= ' - INNER JOIN object_data - ON object_data.obj_id = qpl_questions.obj_fi - '; + $table_join .= " + INNER JOIN {$object_data_table} + ON {$object_data_table}.obj_id = {$questions_table}.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'; + $table_join .= "INNER JOIN {$test_question_table} tstquest ON tstquest.question_fi = {$questions_table}.question_id"; } - if ($this->answerStatusActiveId) { + if ($this->answerStatusActiveId !== null) { $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 = {$quoted} + LEFT JOIN {$test_result_table} + ON {$test_result_table}.question_fi = {$questions_table}.question_id + AND {$test_result_table}.active_fi = {$quoted} "; } @@ -695,7 +795,8 @@ public function load(): void $this->checkFilters(); $tags_trafo = $this->refinery->encode()->htmlSpecialCharsAsEntities(); - $with_computed = $this->computedColumnsRequired(); + $required_computed = $this->requiredComputedFields(); + $enrichment_fields = array_diff(self::ALL_COMPUTED_FIELDS, $required_computed); $res = $this->db->query($this->buildQuery()); $rows_by_id = []; @@ -706,14 +807,14 @@ public function load(): void $ordered_ids[] = $qid; } - if (!$with_computed && $ordered_ids !== []) { - $enrichment_res = $this->db->query($this->buildEnrichmentQuery($ordered_ids)); + if ($enrichment_fields !== [] && $ordered_ids !== []) { + $enrichment_res = $this->db->query($this->buildEnrichmentQuery($ordered_ids, $enrichment_fields)); 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 ($enrichment_fields as $field) { + $rows_by_id[$eid][$field] = $enrichment_row[$field]; + } } } } @@ -750,10 +851,11 @@ public function getTotalRowCount(?array $filter_data, ?array $additional_paramet { $this->checkFilters(); + $questions_table = self::QUESTION_TABLE_NAME; $count = 'COUNT(*)'; $joins = $this->getTableJoinExpression(); $filters = $this->getConditionalFilterExpression(); - $query = "SELECT {$count} FROM qpl_questions {$joins} WHERE qpl_questions.tstamp > 0 {$filters}"; + $query = "SELECT {$count} FROM {$questions_table} {$joins} WHERE {$questions_table}.tstamp > 0 {$filters}"; $result = $this->db->query($query); $fetch = $this->db->fetchAssoc($result); diff --git a/components/ILIAS/TestQuestionPool/tests/ilAssQuestionListTwoPhaseTest.php b/components/ILIAS/TestQuestionPool/tests/ilAssQuestionListTwoPhaseTest.php index f4b9c18631a5..a830cb2cfedd 100644 --- a/components/ILIAS/TestQuestionPool/tests/ilAssQuestionListTwoPhaseTest.php +++ b/components/ILIAS/TestQuestionPool/tests/ilAssQuestionListTwoPhaseTest.php @@ -62,54 +62,68 @@ private function invokePrivate(string $name, array $args = []): mixed return $method->invokeArgs($this->object, $args); } - public function testComputedColumnsRequiredReturnsFalseWithoutHavingOrComputedOrder(): void + public function testRequiredComputedFieldsReturnsEmptyWithoutFiltersOrComputedOrder(): void { $this->setPrivateProperty('order', new Order('title', Order::ASC)); - $this->assertFalse($this->invokePrivate('computedColumnsRequired')); + $this->assertSame([], $this->invokePrivate('requiredComputedFields')); } - public function testComputedColumnsRequiredReturnsTrueForOrderByFeedback(): void + public function testRequiredComputedFieldsReturnsFeedbackForOrderByFeedback(): void { $this->setPrivateProperty('order', new Order('feedback', Order::ASC)); - $this->assertTrue($this->invokePrivate('computedColumnsRequired')); + $result = $this->invokePrivate('requiredComputedFields'); + $this->assertContains('feedback', $result); + $this->assertCount(1, $result); } - public function testComputedColumnsRequiredReturnsTrueForOrderByHints(): void + public function testRequiredComputedFieldsReturnsHintsForOrderByHints(): void { $this->setPrivateProperty('order', new Order('hints', Order::ASC)); - $this->assertTrue($this->invokePrivate('computedColumnsRequired')); + $result = $this->invokePrivate('requiredComputedFields'); + $this->assertContains('hints', $result); + $this->assertCount(1, $result); } - public function testComputedColumnsRequiredReturnsTrueForOrderByTaxonomies(): void + public function testRequiredComputedFieldsReturnsTaxonomiesForOrderByTaxonomies(): void { $this->setPrivateProperty('order', new Order('taxonomies', Order::ASC)); - $this->assertTrue($this->invokePrivate('computedColumnsRequired')); + $result = $this->invokePrivate('requiredComputedFields'); + $this->assertContains('taxonomies', $result); + $this->assertCount(1, $result); } - public function testComputedColumnsRequiredReturnsTrueWithFeedbackFalseFilter(): void + public function testRequiredComputedFieldsReturnsFeedbackForFeedbackFilter(): void { $this->setPrivateProperty('order', new Order('title', Order::ASC)); $this->setPrivateProperty('fieldFilters', ['feedback' => 'false']); - $this->assertTrue($this->invokePrivate('computedColumnsRequired')); + $result = $this->invokePrivate('requiredComputedFields'); + $this->assertContains('feedback', $result); + $this->assertCount(1, $result); } - public function testComputedColumnsRequiredReturnsTrueWithFeedbackTrueFilter(): void + public function testRequiredComputedFieldsReturnsHintsForHintsFilter(): void { $this->setPrivateProperty('order', new Order('title', Order::ASC)); - $this->setPrivateProperty('fieldFilters', ['feedback' => 'true']); - $this->assertTrue($this->invokePrivate('computedColumnsRequired')); + $this->setPrivateProperty('fieldFilters', ['hints' => 'false']); + $result = $this->invokePrivate('requiredComputedFields'); + $this->assertContains('hints', $result); + $this->assertCount(1, $result); } - public function testIsOrderByComputedFieldReturnsFalseWithoutOrder(): void + public function testRequiredComputedFieldsReturnsMultipleForFeedbackFilterAndHintsOrder(): void { - $this->setPrivateProperty('order', null); - $this->assertFalse($this->invokePrivate('isOrderByComputedField')); + $this->setPrivateProperty('order', new Order('hints', Order::ASC)); + $this->setPrivateProperty('fieldFilters', ['feedback' => 'true']); + $result = $this->invokePrivate('requiredComputedFields'); + $this->assertContains('feedback', $result); + $this->assertContains('hints', $result); + $this->assertCount(2, $result); } - public function testIsOrderByComputedFieldReturnsFalseForRegularField(): void + public function testRequiredComputedFieldsReturnsEmptyWithoutOrder(): void { - $this->setPrivateProperty('order', new Order('title', Order::ASC)); - $this->assertFalse($this->invokePrivate('isOrderByComputedField')); + $this->setPrivateProperty('order', null); + $this->assertSame([], $this->invokePrivate('requiredComputedFields')); } public function testQualifyFieldMapsKnownFields(): void @@ -132,14 +146,15 @@ public function testQualifyFieldLeavesComputedFieldsUnchanged(): void $this->assertSame('taxonomies', $this->invokePrivate('qualifyField', ['taxonomies'])); } - public function testBacktickFieldWrapsSimpleField(): void + public function testQualifyAndBacktickFieldWrapsSimpleField(): void { - $this->assertSame('`title`', $this->invokePrivate('backtickField', ['title'])); + $this->assertSame('`qpl_questions`.`title`', $this->invokePrivate('qualifyAndBacktickField', ['title'])); } - public function testBacktickFieldWrapsEachSegmentOfQualifiedName(): void + public function testQualifyAndBacktickFieldWrapsAlreadyQualifiedField(): void { - $this->assertSame('`qpl_questions`.`title`', $this->invokePrivate('backtickField', ['qpl_questions.title'])); + $result = $this->invokePrivate('qualifyAndBacktickField', ['type_tag']); + $this->assertSame('`qpl_qst_type`.`type_tag`', $result); } public function testBuildOrderQueryExpressionQualifiesAndBackticksField(): void @@ -159,7 +174,7 @@ public function testBuildOrderQueryExpressionReturnsEmptyWithoutOrder(): void public function testBuildBasicQueryWithoutComputedHasNoExistsSubqueries(): void { $this->setPrivateProperty('join_obj_data', true); - $sql = $this->invokePrivate('buildBasicQuery', [false]); + $sql = $this->invokePrivate('buildBasicQuery', [[]]); $this->assertStringContainsString('qpl_questions.*', $sql); $this->assertStringNotContainsString('EXISTS', $sql); $this->assertStringNotContainsString('qpl_fb_generic', $sql); @@ -167,22 +182,32 @@ public function testBuildBasicQueryWithoutComputedHasNoExistsSubqueries(): void $this->assertStringNotContainsString('tax_node_assignment', $sql); } - public function testBuildBasicQueryWithComputedContainsExistsSubqueries(): void + public function testBuildBasicQueryWithAllComputedContainsAllExistsSubqueries(): void { $this->setPrivateProperty('join_obj_data', true); - $sql = $this->invokePrivate('buildBasicQuery', [true]); + $sql = $this->invokePrivate('buildBasicQuery', [['feedback', 'hints', 'taxonomies']]); $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 + public function testBuildBasicQueryWithOnlyFeedbackContainsOnlyFeedbackSubquery(): void { $this->setPrivateProperty('join_obj_data', true); - $sql = $this->invokePrivate('buildEnrichmentQuery', [[10, 20, 30]]); + $sql = $this->invokePrivate('buildBasicQuery', [['feedback']]); $this->assertStringContainsString('EXISTS', $sql); $this->assertStringContainsString('qpl_fb_generic', $sql); + $this->assertStringNotContainsString('qpl_hints', $sql); + $this->assertStringNotContainsString('tax_node_assignment', $sql); + } + + public function testBuildEnrichmentQueryContainsOnlyRequestedExistsSubqueries(): void + { + $this->setPrivateProperty('join_obj_data', true); + $sql = $this->invokePrivate('buildEnrichmentQuery', [[10, 20, 30], ['hints', 'taxonomies']]); + $this->assertStringContainsString('EXISTS', $sql); + $this->assertStringNotContainsString('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); @@ -192,7 +217,7 @@ public function testBuildEnrichmentQueryContainsExistsSubqueriesAndInClause(): v public function testBuildEnrichmentQueryHandlesEmptyIdList(): void { $this->setPrivateProperty('join_obj_data', true); - $sql = $this->invokePrivate('buildEnrichmentQuery', [[]]); + $sql = $this->invokePrivate('buildEnrichmentQuery', [[], ['feedback']]); $this->assertStringContainsString('1=2', $sql); } @@ -207,13 +232,16 @@ public function testBuildQueryWithoutComputedAppliesRangeAndOrder(): void $this->assertStringContainsString('LIMIT 800 OFFSET 0', $sql); } - public function testBuildQueryWithComputedIncludesExistsSubqueries(): void + public function testBuildQueryWithComputedOrderIncludesOnlyRelevantSubquery(): void { $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('qpl_fb_generic', $sql); + $this->assertStringNotContainsString('qpl_hints', $sql); + $this->assertStringNotContainsString('tax_node_assignment', $sql); $this->assertStringContainsString('LIMIT 800 OFFSET 0', $sql); } }