Validate l10n ID is non-empty to prevent crash on empty string ID#146
Validate l10n ID is non-empty to prevent crash on empty string ID#146imnasnainaec wants to merge 4 commits into
Conversation
…pdater (#104) An empty string ID propagated silently through the system, producing a malformed .xlf file that crashed the application on next launch. Added ArgumentException at the GetDynamicString/GetDynamicStringOrEnglish entry points and a defensive early-return in XliffTransUnitUpdater.Update mirroring the existing LangId guard.
The CHANGELOG documented GetString as throwing ArgumentException on a null/empty ID, but the guard was only present on GetDynamicString and GetDynamicStringOrEnglish. Add the IsNullOrWhiteSpace guard to both terminal GetString overloads so behavior matches the documented contract. Expand test coverage: parameterize the GetDynamic* empty-ID tests to cover null and whitespace, add GetString guard tests for both overloads, and add a test confirming UpdateLocalizedInfo with an empty ID adds no entry (covering the XliffTransUnitUpdater.Update guard). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
imnasnainaec
force-pushed
the
fix/104-empty-id
branch
from
6be1a22 to
d589560
Compare
andrew-polk
left a comment
andrew-polk
left a comment
There was a problem hiding this comment.
If we're truly treating this as a breaking change, it needs semver:major in the commit message.
Initially, I was going to approve the general idea, but I'm getting more and more hesitant.
I wouldn't be at all surprised if Bloom (or some other program) is accidentally making use of the fallback behavior and sending null or empty sometimes.
Can we solve instead solve the underlying problem while maintaining the immediate behavior of falling back to english?
I'm not sure what can cause the corrupt file, but can we just exit early, returning the English or ID instead of throwing, for example?
@andrew-polk reviewed 4 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on imnasnainaec).
CHANGELOG.md line 35 at r1 (raw file):
- [L10NSharp.Windows.Forms] Removed emailForSubmissions parameter (8th parameter) from LocalizationManagerWinforms.Create. Since the localization dialog was jettisoned, it no longer makes sense to store this information on the localization manager. - [L10NSharp] Replaced the .NET 8.0 target with .NET Standard 2.0 for broader compatibility. - [L10NSharp] BREAKING CHANGE: `LocalizationManager.GetString`, `GetDynamicString`, and `GetDynamicStringOrEnglish` now throw `ArgumentException` when called with a null or empty string ID. Previously, empty IDs were silently accepted and could produce a malformed XLIFF file that crashed on next launch.
This says null or empty, but the actual check is whitespace.
2 participants
Closes #104
Passing an empty string as the
idargument toGetString/GetDynamicStringproduced a malformed or zero-length.xlffile that crashed the application on the next launch.Added an
ArgumentExceptionguard at theGetDynamicStringandGetDynamicStringOrEnglishentry points inLocalizationManagerInternal, and a defensive early-return inXliffTransUnitUpdater.Updatemirroring the existingLangIdcheck.(Technically a breaking change.)
This change is
Devin review: https://app.devin.ai/review/sillsdev/l10nsharp/pull/146