Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 —
Expand Down
5 changes: 3 additions & 2 deletions src/Modules/Dictionary/Application/DictionaryFacade.php
Original file line number Diff line number Diff line change
Expand Up @@ -173,9 +173,10 @@ public function lookupPrefix(int $languageId, string $prefix, int $limit = 10):
* @param iterable<array{term: string, definition: string, reading?: ?string, pos?: ?string}> $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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -77,6 +79,7 @@ public function importFromUrl(
}

$tempFiles = [];
$dictId = null;

try {
// Increase time limit for large downloads
Expand Down Expand Up @@ -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
Expand All @@ -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);
Expand Down
111 changes: 97 additions & 14 deletions src/Modules/Dictionary/Application/Services/LocalDictionaryService.php
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand Down Expand Up @@ -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<array{term: string, definition: string, reading?: ?string, pos?: ?string}> $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];
}

/**
Expand Down Expand Up @@ -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<string, string|int|null>|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.
*
Expand Down
12 changes: 8 additions & 4 deletions src/Modules/Dictionary/Http/DictionaryApiHandler.php
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand Down Expand Up @@ -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
{
Expand Down Expand Up @@ -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()];
Expand Down
3 changes: 2 additions & 1 deletion src/Modules/Dictionary/Http/DictionaryController.php
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
11 changes: 9 additions & 2 deletions src/Modules/Vocabulary/Http/TermImportController.php
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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 '<div class="notification is-success">' .
'<button class="delete" aria-label="close"></button>' .
'Dictionary <strong>' . htmlspecialchars($dictName, ENT_QUOTES, 'UTF-8') .
'</strong> created with ' . number_format($count) . ' entries' .
$vocabMsg . '.</div>';
$vocabMsg . '.' . $skippedMsg . '</div>';
} catch (RuntimeException $e) {
echo '<div class="notification is-danger">' .
'<button class="delete" aria-label="close"></button>' .
Expand Down
10 changes: 9 additions & 1 deletion src/frontend/js/modules/vocabulary/pages/starter_vocab.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down Expand Up @@ -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({
Expand Down
Loading