[jira] [Created] (OPENNLP-1904) Default-locale case folding corrupts lemmatizer models and output

["Kristian Rickert (Jira)" <ji...@apache.org>]

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)