Kristian Rickert created OPENNLP-1904:
-----------------------------------------
Summary: Default-locale case folding corrupts lemmatizer models
and output
Key: OPENNLP-1904
URL: https://issues.apache.org/jira/browse/OPENNLP-1904
Project: OpenNLP
Issue Type: Bug
Reporter: Kristian Rickert
Assignee: Kristian Rickert
h2. Summary
Several code paths fold case with the JVM default locale. In the lemmatizer
this is
not cosmetic: the folded strings become outcome labels that are *written into
the
trained model*, and the inference path folds again with whatever locale the
consumer
happens to run under. A model trained on a Turkish JVM therefore carries labels
no
other machine reproduces, and an existing English-trained model emits corrupted
lemmas when served on one. Nothing logs, nothing throws; the output is simply
wrong.
h2. Reproduction
Each of these is an actual assertion failure observed against unfixed code,
under
{{Locale.setDefault}} of {{tr}} (or {{de_DE}} for the report cases).
{noformat}
LemmatizerME.decodeLemmas expected <mouse> but was <mıuse>
StringUtil.getShortestEditScript expected <D0s> but was <R6ıiD0s>
LemmatizerME.encodeLemmas expected <R2ioR1cuI1s> but was <R2ıoR1cuI1s>
DictionaryLemmatizer expected <Illinois> but was <O>
TrainingParameters.setParams expected <50> but was <100>
{noformat}
The Turkish dotless i is the mechanism: {{"Illinois".toLowerCase()}} under
{{tr}}
yields {{{}ıllinois{}}}, which never matches an ASCII-lowercase dictionary
entry, so
{{DictionaryLemmatizer}} silently degrades to its {{"O"}} sentinel.
h2. Affected sites
||Severity||Location||Expression||
|Critical|{{opennlp-api/.../util/StringUtil.java:488}}|{{wordForm.toLowerCase()}}
in {{getShortestEditScript}}|
|Critical|{{opennlp-api/.../util/StringUtil.java:489}}|{{{}lemma.toLowerCase(){}}},
same method|
|Critical|{{opennlp-core/opennlp-runtime/.../lemmatizer/LemmatizerME.java:171}}|{{toks[i].toLowerCase()}}
in {{decodeLemmas}}|
|Critical|{{opennlp-core/opennlp-runtime/.../lemmatizer/DictionaryLemmatizer.java:164}}|{{word.toLowerCase()}}
in {{getDictKeys}}|
|Moderate|{{opennlp-ml-commons/.../util/TrainingParameters.java:332-337}}|{{{}ITERATIONS_PARAM.toLowerCase(){}}},
{{CUTOFF_PARAM.toLowerCase()}}|
|Moderate|{{opennlp-ml-commons/.../ml/model/AbstractModel.java:34}}|{{static
final DecimalFormat}} without locale, used by {{getAllOutcomes()}}|
|Moderate|{{opennlp-core/opennlp-cli/.../cmdline/DetailedFMeasureListener.java:119}}|{{createReport(Locale.getDefault())}}|
|Moderate|{{opennlp-core/opennlp-cli/.../cmdline/FineGrainedReportListener.java}}|9
{{MessageFormat.format}} sites|
Sites 1 to 3 are one defect in two halves. {{getShortestEditScript}} produces
the
labels via {{LemmatizerME.encodeLemmas}} and {{{}LemmaSampleEventStream{}}};
{{decodeLemmas}}
consumes them. They must change together or train/serve skew is reintroduced.
h2. Why {{Locale.ROOT}} and not {{StringUtil.toLowerCase}}
This is the load-bearing decision in the patch and a reviewer will likely raise
it.
The project's own {{StringUtil.toLowerCase}} maps code points through
{{{}Character.toLowerCase{}}}. That is context-free, and it *disagrees* with
{{String.toLowerCase(Locale.ROOT)}} on context-sensitive mappings (Greek final
sigma:
{{ΟΔΟΣ}} folds to {{οδοσ}} rather than {{{}οδος{}}}) and on one-to-many
mappings such as
{{{}ß{}}}.
Every shipped model and dictionary was produced on an English-locale JVM, whose
fold
is byte-identical to {{{}Locale.ROOT{}}}. So {{Locale.ROOT}} keeps those
artifacts working
unchanged, while the code-point fold would silently alter behaviour for every
existing
model.
*No model regeneration is required.* That is the point of the choice. A guard
test
({{{}StringUtilLocaleTest.testGetShortestEditScriptUsesRootFoldingNotCodePointFolding{}}})
exists specifically to fail if someone later "simplifies" this to
{{StringUtil.toLowerCase}} for consistency.
h2. Deliberately not changed
Reporting these would waste review time; all were checked and are already safe:
{{{}StringUtil.toLowerCase{}}}/{{{}toUpperCase{}}} (code-point based),
{{{}String.CASE_INSENSITIVE_ORDER{}}},
all {{equalsIgnoreCase}} uses, {{String.format("U+%04X")}} (the
{{{}x{}}}/{{{}X{}}}/{{{}o{}}}
conversions are never localized per the {{Formatter}} spec),
{{{}NameFinderDL{}}}'s own
fold, and all model serialization ({{{}Double.toString{}}},
{{{}DataOutputStream{}}}).
Two residual {{toUpperCase()}} calls in main sources are benign and were left
alone:
{{EmojiFlags.java:171}} (hex digits in an exception message) and
{{GenerateManualTool.java:227}} (DocBook titles in a build-time doc generator).
h2. Behaviour change to note in release notes
{{DetailedFMeasureListener.createReport()}} and {{FineGrainedReportListener}}
now
always render with {{{}Locale.ROOT{}}}. Eval report output is therefore no
longer
localized. This was chosen so reports are reproducible, diffable and
machine-parseable
across machines. The explicit {{createReport(Locale)}} overload is untouched,
so a
caller who wants localized output can still ask for it.
This is the one judgement call in the patch and deserves a maintainer sanity
check.
h2. Test coverage
There was *no locale test coverage anywhere in the tree* before this branch.
There is
no {{Locale.setDefault}} in main or test sources; the only Turkish-aware tests
are
Snowball stemmer data tests, which exercise Turkish {*}input{*}, not a Turkish
{*}JVM locale{*}.
Nine new tests, each verified to fail before the fix and pass after (the fix was
stashed, the build rerun, the failure observed, then restored). Failure
messages are
in the Reproduction section above.
Two gaps in that coverage:
* {{AbstractModel.DECIMAL_FORMAT}} is fixed *without* a red/green test. The
formatter is
a {{static final}} resolved at class initialization, so any
{{Locale.setDefault}} in a
test runs after the class is loaded and such a test passes both before and
after. A
fresh-classloader harness would need to reload an abstract class plus
{{Context}} and
{{EvalParameters}} and reflectively build a concrete subclass; adding
{{-Duser.language=tr}} to the surefire {{argLine}} would flip the locale for
every
other test in the module. Neither seemed worth it for a formatter used only by
the
diagnostic {{{}getAllOutcomes(){}}}.
*
{{StringUtilLocaleTest.testGetShortestEditScriptUsesRootFoldingNotCodePointFolding}}
is a {*}guard{*}, not a regression test. It passes before and after, because
under an
English default the two folds agree. Its purpose is to break on a future
"simplification". Flagged because it does not meet the fails-before bar.
One drafted test was discarded after it passed unfixed: an encode/decode round
trip is
self-consistent within a single locale and so could never fail. It is not in
the commits.
Note for anyone adding more: the root {{pom.xml}} surefire \{{argLine}}s do
*not*
forward {{{}-Duser.language{}}}, so {{mvn -Duser.language=tr test}} does not
reach the
forked test JVM.
h2. Build
{{mvn -o clean install}} over the full 23-module reactor: BUILD SUCCESS, zero
failures
or errors, with checkstyle, rat and the existing forbiddenapis configuration
active.
{{mvn javadoc:javadoc}} clean on the five touched modules (three pre-existing
unknown-tag warnings, none on a changed line).
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
