diff --git a/CHANGELOG.md b/CHANGELOG.md index e99426e9a..fe02bf061 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,22 @@ ones are marked like "v1.0.0-fork". ### Fixed +* **One over-long headword aborted a whole dictionary import** (#250): + importing FreeDict German-English failed with "Data too long for column + 'LeTerm' at row 670" and no entries usable. `LeTerm` is `VARCHAR(250)`, and + exactly one of that dictionary's 517,534 entries has a 293-character headword + (the Lord's Prayer, stored as a single entry). Under `STRICT_ALL_TABLES` an + over-long value fails the entire multi-row `INSERT`, so that one entry + discarded its whole 1000-row batch and ended the import — losing the other + 517,533 entries. Entries whose headword cannot be stored are now skipped and + counted instead of aborting the run, and the count is reported alongside the + import total so the result is not silently lossy. 250 characters is also LWT's + own term length (`words.WoText`), so a longer headword could never have become + a usable term. `LeReading` and `LePartOfSpeech` are descriptive metadata and + are truncated rather than costing the entry. This applies to every import path + — curated, uploaded file, and API — not just the curated one. A failed import + now also drops its half-filled dictionary, which previously stayed behind with + however many rows had already been inserted. * **Japanese (MeCab) parsing produced no tokens on Windows**: both MeCab output readers split their lines on `PHP_EOL`, but a subprocess's line terminator comes from MeCab, not from the host PHP runs on. Wherever the two disagree — diff --git a/src/Modules/Dictionary/Application/DictionaryFacade.php b/src/Modules/Dictionary/Application/DictionaryFacade.php index d987e102a..2a2f1e4a6 100644 --- a/src/Modules/Dictionary/Application/DictionaryFacade.php +++ b/src/Modules/Dictionary/Application/DictionaryFacade.php @@ -173,9 +173,10 @@ public function lookupPrefix(int $languageId, string $prefix, int $limit = 10): * @param iterable $entries * Entries to add * - * @return int Number of entries added + * @return array{added: int, skipped: int} Entries inserted, and entries + * skipped for an unstorable headword */ - public function addEntriesBatch(int $dictId, iterable $entries): int + public function addEntriesBatch(int $dictId, iterable $entries): array { return $this->dictionaryService->addEntriesBatch($dictId, $entries); } diff --git a/src/Modules/Dictionary/Application/Services/CuratedDictImportService.php b/src/Modules/Dictionary/Application/Services/CuratedDictImportService.php index 622a1a05f..4707ea8ec 100644 --- a/src/Modules/Dictionary/Application/Services/CuratedDictImportService.php +++ b/src/Modules/Dictionary/Application/Services/CuratedDictImportService.php @@ -63,7 +63,9 @@ public function __construct( * @param string $format Dictionary format (stardict, csv) * @param string $name Dictionary name * - * @return array{success: bool, dictId?: int, imported?: int, vocabCreated?: int, error?: string} + * @return array{success: bool, dictId?: int, imported?: int, skipped?: int, + * vocabCreated?: int, error?: string} `skipped` counts entries + * whose headword was too long to store */ public function importFromUrl( int $languageId, @@ -77,6 +79,7 @@ public function importFromUrl( } $tempFiles = []; + $dictId = null; try { // Increase time limit for large downloads @@ -108,7 +111,8 @@ public function importFromUrl( // Create dictionary record and import entries $dictId = $this->facade->create($languageId, $name, $format); $entries = $importer->parse($importFile); - $count = $this->facade->addEntriesBatch($dictId, $entries); + $result = $this->facade->addEntriesBatch($dictId, $entries); + $count = $result['added']; if ($count === 0) { // Clean up empty dictionary @@ -126,9 +130,21 @@ public function importFromUrl( 'success' => true, 'dictId' => $dictId, 'imported' => $count, + 'skipped' => $result['skipped'], 'vocabCreated' => $vocabCreated, ]; } catch (RuntimeException $e) { + // Drop the half-filled dictionary. Without this a failure part-way + // through leaves the record plus however many entries had already + // been inserted (for a large dictionary, hundreds of thousands of + // rows) with nothing in the UI explaining where they came from. + if ($dictId !== null) { + try { + $this->facade->delete($dictId); + } catch (RuntimeException $cleanupError) { + // Keep the original failure as the reported error. + } + } return ['success' => false, 'error' => $e->getMessage()]; } finally { $this->cleanup(...$tempFiles); diff --git a/src/Modules/Dictionary/Application/Services/LocalDictionaryService.php b/src/Modules/Dictionary/Application/Services/LocalDictionaryService.php index ec2bb6afb..5be2a6034 100644 --- a/src/Modules/Dictionary/Application/Services/LocalDictionaryService.php +++ b/src/Modules/Dictionary/Application/Services/LocalDictionaryService.php @@ -44,6 +44,29 @@ class LocalDictionaryService */ private const BATCH_SIZE = 1000; + /** + * Character capacity of `LeTerm` / `LeTermLc` (both VARCHAR(250)). + * + * Entries with a longer headword are skipped rather than allowed to abort + * the import: under STRICT_ALL_TABLES an over-long value fails the whole + * multi-row INSERT, so a single bad entry used to lose every other entry in + * its batch and end the import (issue #250 — one 293-character headword in + * FreeDict German-English killed all 517,534 entries). 250 is also LWT's + * own term length (`words.WoText`), so a longer headword could not become a + * usable term anyway. + */ + private const MAX_TERM_LENGTH = 250; + + /** + * Character capacity of `LeReading` (VARCHAR(250)). + */ + private const MAX_READING_LENGTH = 250; + + /** + * Character capacity of `LePartOfSpeech` (VARCHAR(50)). + */ + private const MAX_POS_LENGTH = 50; + /** * Create a new local dictionary. * @@ -282,45 +305,55 @@ public function addEntry( /** * Add multiple entries to a dictionary in batches. * + * Entries whose headword exceeds the column's capacity are skipped and + * counted rather than inserted. Under STRICT_ALL_TABLES an over-long value + * aborts the entire multi-row INSERT, so without this a single oversized + * headword discarded its whole batch and ended the import (issue #250). + * `reading` and `pos` are descriptive metadata, so those are truncated + * instead — losing a part-of-speech label beats losing the entry. + * * @param int $dictId Dictionary ID * @param iterable $entries * Entries to add * - * @return int Number of entries added + * @return array{added: int, skipped: int} Entries inserted, and entries + * skipped for an unstorable headword + * + * @since 3.2.2-fork Returns a count pair and skips unstorable headwords */ - public function addEntriesBatch(int $dictId, iterable $entries): int + public function addEntriesBatch(int $dictId, iterable $entries): array { $this->assertOwnsDictionary($dictId); $batch = []; - $count = 0; + $added = 0; + $skipped = 0; foreach ($entries as $entry) { - $batch[] = [ - 'LeLdID' => $dictId, - 'LeTerm' => $entry['term'], - 'LeTermLc' => mb_strtolower($entry['term'], 'UTF-8'), - 'LeDefinition' => $entry['definition'], - 'LeReading' => $entry['reading'] ?? null, - 'LePartOfSpeech' => $entry['pos'] ?? null, - ]; + $row = self::buildRow($dictId, $entry); + if ($row === null) { + $skipped++; + continue; + } + + $batch[] = $row; if (count($batch) >= self::BATCH_SIZE) { $this->insertBatch($batch); - $count += count($batch); + $added += count($batch); $batch = []; } } if (!empty($batch)) { $this->insertBatch($batch); - $count += count($batch); + $added += count($batch); } // Update entry count $this->updateEntryCount($dictId); - return $count; + return ['added' => $added, 'skipped' => $skipped]; } /** @@ -610,6 +643,56 @@ private function hydrateFromRecord(array $record): LocalDictionary ); } + /** + * Build an insertable row from a parsed dictionary entry. + * + * @param int $dictId Dictionary ID + * @param array{term: string, definition: string, reading?: ?string, pos?: ?string} $entry Parsed entry + * + * @return array|null The row, or null when the + * headword cannot be stored and the entry has to be skipped + */ + private static function buildRow(int $dictId, array $entry): ?array + { + $term = $entry['term']; + $termLc = mb_strtolower($term, 'UTF-8'); + + // Lowercasing can lengthen a string (e.g. 'İ' becomes two code points), + // so both columns have to be checked, not just the incoming term. + if ( + mb_strlen($term, 'UTF-8') > self::MAX_TERM_LENGTH + || mb_strlen($termLc, 'UTF-8') > self::MAX_TERM_LENGTH + ) { + return null; + } + + return [ + 'LeLdID' => $dictId, + 'LeTerm' => $term, + 'LeTermLc' => $termLc, + 'LeDefinition' => $entry['definition'], + 'LeReading' => self::clip($entry['reading'] ?? null, self::MAX_READING_LENGTH), + 'LePartOfSpeech' => self::clip($entry['pos'] ?? null, self::MAX_POS_LENGTH), + ]; + } + + /** + * Truncate an optional metadata value to a column's character capacity. + * + * @param string|null $value Value to clip, or null + * @param int $maxLength Maximum characters the column accepts + * + * @return string|null The value, shortened if it was over-long + */ + private static function clip(?string $value, int $maxLength): ?string + { + if ($value === null || mb_strlen($value, 'UTF-8') <= $maxLength) { + return $value; + } + + return mb_substr($value, 0, $maxLength, 'UTF-8'); + } + /** * Insert a batch of entries. * diff --git a/src/Modules/Dictionary/Http/DictionaryApiHandler.php b/src/Modules/Dictionary/Http/DictionaryApiHandler.php index 7d2540214..a719d91c7 100644 --- a/src/Modules/Dictionary/Http/DictionaryApiHandler.php +++ b/src/Modules/Dictionary/Http/DictionaryApiHandler.php @@ -159,7 +159,9 @@ public function createDictionary(array $data): array * - format: string (required, e.g. 'stardict') * - name: string (required) * - * @return array{success: bool, dictId?: int, imported?: int, vocabCreated?: int, error?: string} + * @return array{success: bool, dictId?: int, imported?: int, skipped?: int, + * vocabCreated?: int, error?: string} `skipped` counts entries + * whose headword was too long to store */ public function importCurated(array $data): array { @@ -307,7 +309,8 @@ public function lookupPrefix(int $langId, string $prefix, int $limit = 10): arra * - format: string (csv, json, stardict) * - options: array (format-specific options) * - * @return array{success: bool, imported?: int, error?: string} + * @return array{success: bool, imported?: int, skipped?: int, error?: string} + * `skipped` counts entries whose headword was too long to store */ public function importFile(int $dictId, array $data): array { @@ -339,11 +342,12 @@ public function importFile(int $dictId, array $data): array /** @psalm-suppress UndefinedClass Psalm incorrectly resolves namespace */ $entries = $importer->parse($filePath, $options); - $count = $this->facade->addEntriesBatch($dictId, $entries); + $result = $this->facade->addEntriesBatch($dictId, $entries); return [ 'success' => true, - 'imported' => $count, + 'imported' => $result['added'], + 'skipped' => $result['skipped'], ]; } catch (RuntimeException $e) { return ['success' => false, 'error' => $e->getMessage()]; diff --git a/src/Modules/Dictionary/Http/DictionaryController.php b/src/Modules/Dictionary/Http/DictionaryController.php index 35ed38a3c..71d2c819f 100644 --- a/src/Modules/Dictionary/Http/DictionaryController.php +++ b/src/Modules/Dictionary/Http/DictionaryController.php @@ -184,7 +184,8 @@ public function processImport(array $params): void // Perform import /** @psalm-suppress UndefinedClass Psalm incorrectly resolves namespace */ $entries = $importer->parse($importPath, $options); - $count = $this->dictionaryFacade->addEntriesBatch($dictId, $entries); + $result = $this->dictionaryFacade->addEntriesBatch($dictId, $entries); + $count = $result['added']; $this->redirect("/dictionaries?lang=$langId&message=imported_$count"); } catch (RuntimeException $e) { diff --git a/src/Modules/Vocabulary/Http/TermImportController.php b/src/Modules/Vocabulary/Http/TermImportController.php index 158101699..983b413c4 100644 --- a/src/Modules/Vocabulary/Http/TermImportController.php +++ b/src/Modules/Vocabulary/Http/TermImportController.php @@ -434,7 +434,8 @@ private function handleDictionaryImport(): void $dictId = $this->dictionaryFacade->create($langId, $dictName, $format); $entries = $importer->parse($importPath, $options); - $count = $this->dictionaryFacade->addEntriesBatch($dictId, $entries); + $importResult = $this->dictionaryFacade->addEntriesBatch($dictId, $entries); + $count = $importResult['added']; // Create vocabulary terms (status 1) from dictionary entries $vocabCreated = $this->dictionaryFacade->createVocabularyFromEntries($dictId, $langId); @@ -445,11 +446,17 @@ private function handleDictionaryImport(): void $vocabMsg = $vocabCreated > 0 ? ' and ' . number_format($vocabCreated) . ' vocabulary terms' : ''; + // Report dropped entries rather than let the count silently differ + // from the source dictionary's own total. + $skippedMsg = $importResult['skipped'] > 0 + ? ' ' . number_format($importResult['skipped']) + . ' entries were skipped because their headword was too long.' + : ''; echo '
' . '' . 'Dictionary ' . htmlspecialchars($dictName, ENT_QUOTES, 'UTF-8') . ' created with ' . number_format($count) . ' entries' . - $vocabMsg . '.
'; + $vocabMsg . '.' . $skippedMsg . ''; } catch (RuntimeException $e) { echo '
' . '' . diff --git a/src/frontend/js/modules/vocabulary/pages/starter_vocab.ts b/src/frontend/js/modules/vocabulary/pages/starter_vocab.ts index e761db5b8..54801695b 100644 --- a/src/frontend/js/modules/vocabulary/pages/starter_vocab.ts +++ b/src/frontend/js/modules/vocabulary/pages/starter_vocab.ts @@ -54,6 +54,8 @@ interface CuratedImportResponse { success: boolean; dictId?: number; imported?: number; + /** Entries dropped because their headword exceeded the term column. */ + skipped?: number; error?: string; } @@ -232,9 +234,15 @@ Alpine.data('starterVocab', () => { }; if (result.success) { + const skipped = result.skipped ?? 0; + // Say so when entries were dropped, rather than reporting a count + // that silently differs from the dictionary's own entry total. + const skippedNote = skipped > 0 + ? ` (${skipped} skipped: headword too long)` + : ''; this.dictMessages.push({ success: true, - text: `${source.name}: imported ${result.imported ?? 0} entries.`, + text: `${source.name}: imported ${result.imported ?? 0} entries.${skippedNote}`, }); } else { this.dictMessages.push({ diff --git a/tests/backend/Modules/Dictionary/DictionaryFacadeTest.php b/tests/backend/Modules/Dictionary/DictionaryFacadeTest.php index 2a45285e7..b496558d1 100644 --- a/tests/backend/Modules/Dictionary/DictionaryFacadeTest.php +++ b/tests/backend/Modules/Dictionary/DictionaryFacadeTest.php @@ -555,7 +555,7 @@ public function testAddEntriesBatchReturnsCount(): void $result = $this->facade->addEntriesBatch($dictId, $entries); - $this->assertSame(3, $result); + $this->assertSame(['added' => 3, 'skipped' => 0], $result); // Cleanup $this->facade->delete($dictId); @@ -890,4 +890,62 @@ public function testMultipleDictionariesPriorityOrder(): void $this->facade->delete($dict1Id); $this->facade->delete($dict2Id); } + + // ===== Over-long headwords (issue #250) ===== + + /** + * An entry whose headword exceeds LeTerm must not take its batch-mates + * down with it. Under STRICT_ALL_TABLES the over-long value used to fail + * the whole multi-row INSERT, which aborted the entire import — one + * 293-character headword cost all 517,534 FreeDict entries. + */ + public function testAddEntriesBatchSkipsOverLongTermAndKeepsTheRest(): void + { + $this->skipIfNoLanguage(); + + $dictId = $this->facade->create( + self::$testLanguageId, + 'Test Dict Overlong ' . uniqid() + ); + + $good1 = 'shortterm1' . uniqid(); + $good2 = 'shortterm2' . uniqid(); + + $result = $this->facade->addEntriesBatch($dictId, [ + ['term' => $good1, 'definition' => 'first'], + ['term' => str_repeat('x', 251), 'definition' => 'unstorable headword'], + ['term' => $good2, 'definition' => 'second'], + ]); + + $this->assertSame(2, $result['added'], 'Both storable entries should be inserted'); + $this->assertSame(1, $result['skipped'], 'The over-long headword should be skipped'); + + // The neighbours really are queryable, not merely counted. + $this->assertNotEmpty($this->facade->lookup(self::$testLanguageId, $good1)); + $this->assertNotEmpty($this->facade->lookup(self::$testLanguageId, $good2)); + + // And the over-long headword is genuinely absent from the table. + $this->assertSame(2, $this->service->getEntryCount($dictId)); + + $this->facade->delete($dictId); + } + + public function testAddEntriesBatchReportsZeroSkippedWhenAllEntriesFit(): void + { + $this->skipIfNoLanguage(); + + $dictId = $this->facade->create( + self::$testLanguageId, + 'Test Dict NoSkip ' . uniqid() + ); + + $result = $this->facade->addEntriesBatch($dictId, [ + ['term' => 'fitsfine' . uniqid(), 'definition' => 'ok'], + ]); + + $this->assertSame(1, $result['added']); + $this->assertSame(0, $result['skipped']); + + $this->facade->delete($dictId); + } } diff --git a/tests/backend/Modules/Dictionary/LocalDictionaryRowBuildingTest.php b/tests/backend/Modules/Dictionary/LocalDictionaryRowBuildingTest.php new file mode 100644 index 000000000..6aa96a9cc --- /dev/null +++ b/tests/backend/Modules/Dictionary/LocalDictionaryRowBuildingTest.php @@ -0,0 +1,192 @@ + + * @since 3.2.2-fork + */ + +declare(strict_types=1); + +namespace Lwt\Tests\Modules\Dictionary; + +use Lwt\Modules\Dictionary\Application\Services\LocalDictionaryService; +use PHPUnit\Framework\Attributes\CoversClass; +use PHPUnit\Framework\Attributes\Test; +use PHPUnit\Framework\TestCase; +use ReflectionMethod; + +/** + * Unit tests for LocalDictionaryService::buildRow() and clip(). + * + * @since 3.2.2-fork + */ +#[CoversClass(LocalDictionaryService::class)] +class LocalDictionaryRowBuildingTest extends TestCase +{ + /** + * Character capacity of LeTerm / LeTermLc. + */ + private const TERM_LIMIT = 250; + + /** + * Build a row through the private helper. + * + * @param array $entry Parsed entry + * + * @return array|null + */ + private function buildRow(array $entry): ?array + { + $method = new ReflectionMethod(LocalDictionaryService::class, 'buildRow'); + + /** @var array|null $row */ + $row = $method->invoke(null, 7, $entry); + return $row; + } + + #[Test] + public function buildsARowForAnOrdinaryEntry(): void + { + $row = $this->buildRow(['term' => 'Haus', 'definition' => 'house']); + + $this->assertNotNull($row); + $this->assertSame(7, $row['LeLdID']); + $this->assertSame('Haus', $row['LeTerm']); + $this->assertSame('haus', $row['LeTermLc']); + $this->assertSame('house', $row['LeDefinition']); + $this->assertNull($row['LeReading']); + $this->assertNull($row['LePartOfSpeech']); + } + + #[Test] + public function keepsATermExactlyAtTheLimit(): void + { + $term = str_repeat('a', self::TERM_LIMIT); + + $row = $this->buildRow(['term' => $term, 'definition' => 'x']); + + $this->assertNotNull($row); + $this->assertSame($term, $row['LeTerm']); + } + + /** + * The #250 entry: one headword one character over the limit must be + * reported as unstorable rather than handed to the INSERT, where it would + * take its whole batch down with it. + */ + #[Test] + public function skipsATermOneCharacterOverTheLimit(): void + { + $term = str_repeat('a', self::TERM_LIMIT + 1); + + $this->assertNull($this->buildRow(['term' => $term, 'definition' => 'x'])); + } + + #[Test] + public function skipsTheActualFreeDictOffender(): void + { + // The 293-character headword that aborted the FreeDict German-English + // import: the opening of the Lord's Prayer, stored as one entry. + $term = 'Vater unser im Himmel, geheiligt werde dein Name, dein Reich ' + . 'komme, dein Wille geschehe, wie im Himmel, so auf Erden. Unser ' + . 'täglich Brot gib uns heute und vergib uns unsere Schuld, wie ' + . 'auch wir vergeben unseren Schuldigern, und führe uns nicht in ' + . 'Versuchung, sondern erlöse uns von dem Bösen.'; + + $this->assertGreaterThan(self::TERM_LIMIT, mb_strlen($term, 'UTF-8')); + $this->assertNull($this->buildRow(['term' => $term, 'definition' => 'x'])); + } + + /** + * Multi-byte characters are counted as characters, matching how MySQL + * measures a utf8mb4 VARCHAR — a byte-based check would wrongly reject + * accented terms well inside the limit. + */ + #[Test] + public function measuresLengthInCharactersNotBytes(): void + { + // 200 characters, 400 bytes: comfortably storable. + $term = str_repeat('ä', 200); + + $row = $this->buildRow(['term' => $term, 'definition' => 'x']); + + $this->assertNotNull($row, 'A 200-character term must not be rejected on byte length'); + $this->assertSame($term, $row['LeTerm']); + } + + #[Test] + public function skipsWhenLowercasingPushesTheTermOverTheLimit(): void + { + // 'İ' (U+0130) lowercases to two code points, so a term at the limit + // can overflow LeTermLc while LeTerm still fits. + $term = str_repeat('İ', self::TERM_LIMIT); + $this->assertSame(self::TERM_LIMIT, mb_strlen($term, 'UTF-8')); + $this->assertGreaterThan( + self::TERM_LIMIT, + mb_strlen(mb_strtolower($term, 'UTF-8'), 'UTF-8'), + 'Precondition: lowercasing this term must lengthen it' + ); + + $this->assertNull($this->buildRow(['term' => $term, 'definition' => 'x'])); + } + + #[Test] + public function truncatesOverLongReadingAndPartOfSpeechInsteadOfSkipping(): void + { + $row = $this->buildRow([ + 'term' => 'Haus', + 'definition' => 'house', + 'reading' => str_repeat('r', 400), + 'pos' => str_repeat('p', 80), + ]); + + $this->assertNotNull($row, 'Over-long metadata must not cost the entry'); + $this->assertIsString($row['LeReading']); + $this->assertIsString($row['LePartOfSpeech']); + $this->assertSame(250, mb_strlen($row['LeReading'], 'UTF-8')); + $this->assertSame(50, mb_strlen($row['LePartOfSpeech'], 'UTF-8')); + } + + #[Test] + public function leavesShortMetadataUntouched(): void + { + $row = $this->buildRow([ + 'term' => 'Haus', + 'definition' => 'house', + 'reading' => 'haʊs', + 'pos' => 'noun', + ]); + + $this->assertNotNull($row); + $this->assertSame('haʊs', $row['LeReading']); + $this->assertSame('noun', $row['LePartOfSpeech']); + } + + /** + * A definition of any size is fine — LeDefinition is TEXT, so it must not + * be clipped or cause a skip. FreeDict definitions run to ~14 KB. + */ + #[Test] + public function neverClipsTheDefinition(): void + { + $definition = str_repeat('d', 20000); + + $row = $this->buildRow(['term' => 'Haus', 'definition' => $definition]); + + $this->assertNotNull($row); + $this->assertSame($definition, $row['LeDefinition']); + } +}