[
https://issues.apache.org/jira/browse/WW-5685?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18107167#comment-18107167
]
Lukasz Lenart commented on WW-5685:
-----------------------------------
PR [#1860|https://github.com/apache/struts/pull/1860] opened against main.
Confirmed the defect is still live on {{main}} and fixed it as suggested —
{{break}} to {{continue}}, plus a debug log line so a skipped key is at least
traceable.
h3. A correction to the suggested test
This ticket proposed "a {{-conversion.properties}} file whose first key is
already mapped". That cannot be written as a fixture: {{Properties}} extends
{{Hashtable}}, so there is no first key you can control from the file. A
fixture whose colliding key happens to hash to the last position passes against
the unfixed code, because {{break}} then drops nothing.
That is not hypothetical — it is what happened here. The hierarchy test was
written first, its shared key landed last in iteration order, and it passed
with {{break}} still in place. Only reverting the fix and re-running exposed it.
Two tests resulted, both mutation-checked:
* {{DefaultConversionFileProcessorTest}} derives the colliding key from the
actual iteration order at run time and pre-maps whichever key comes first.
{{break}} then registers nothing at all, so the test discriminates on any JDK
in any hash order, with no dependence on {{Hashtable}} layout. A second case
asserts every entry registers when nothing is pre-mapped, so a fix that skips
too much would also fail.
*
{{XWorkConverterTest.testPropertiesEntriesAfterAKeyCollisionAreStillRegistered}}
covers the realistic hierarchy trigger from this ticket, a subclass and
superclass file sharing a key. It does depend on fixture ordering, so the
shared key is now named to be read first — verified identical on Temurin
17.0.14, 21.0.7 and 25.0.1 — and the test asserts that precondition, failing
loudly with an explanatory message if a future reordering would make it vacuous
again.
h3. Scope
The {{mapping.containsKey}} precedence rule is untouched: the first source to
claim a key still wins, matching the annotation path. Only the loop exit
changed.
The remaining {{break}} at {{XWorkConverter:761}} was checked and is correct —
it ends an interface search after a hit, unrelated to this defect.
{{mvn test -DskipAssembly -pl core}}: 3193 tests, 0 failures, 0 errors.
Behaviour does change, in that previously dropped entries now register, so this
is worth a Version Notes line; no Migration Guide entry is needed.
> DefaultConversionFileProcessor silently drops the rest of a
> -conversion.properties file after the first already-mapped key
> --------------------------------------------------------------------------------------------------------------------------
>
> Key: WW-5685
> URL: https://issues.apache.org/jira/browse/WW-5685
> Project: Struts 2
> Issue Type: Bug
> Components: Core Actions
> Reporter: Lukasz Lenart
> Priority: Major
> Fix For: 7.4.0
>
> Time Spent: 10m
> Remaining Estimate: 0h
>
> h3. Problem
> {{DefaultConversionFileProcessor.process}} iterates the entries of a
> {{-conversion.properties}} file and skips keys that are already present in
> the converter mapping. The skip is written as a {{break}} rather than a
> {{continue}}, so instead of skipping that one entry it abandons the whole
> file:
> {code:java}
> for (Map.Entry entry : prop.entrySet()) {
> String key = (String) entry.getKey();
> if (mapping.containsKey(key)) {
> break; // <-- should be continue
> }
> ...
> }
> {code}
> Every remaining entry in that properties file is silently dropped. There is
> no warning and no error; the affected properties simply fall back to default
> conversion at runtime.
> h3. Why it triggers in practice
> {{XWorkConverter.buildConverterMapping}} walks the class hierarchy — the
> class itself, then its interfaces, then its superclass — and passes _one
> shared, accumulating_ mapping into each {{addConverterMapping}} call. So a
> key claimed earlier in that walk aborts a later file entirely:
> * A subclass {{Foo-conversion.properties}} declares {{bar}}.
> * The superclass {{FooBase-conversion.properties}} declares {{bar}}, {{baz}}
> and {{qux}}.
> * When the superclass file is processed, {{bar}} is already mapped, so
> {{baz}} and {{qux}} are never registered.
> Annotation-derived entries land in the same map, so an {{@TypeConversion}} on
> the class can abort its properties file the same way.
> h3. Nondeterminism
> {{Properties}} extends {{Hashtable}} and {{entrySet()}} has no defined
> iteration order, so _which_ entries survive depends on hash order rather than
> file order. The same file can behave differently across JDK versions or after
> an unrelated key is added, which makes this hard to diagnose from the symptom.
> h3. Related
> WW-3871 fixed the identical {{break}} instead of {{continue}} defect in the
> annotation path ({{DefaultConversionAnnotationProcessor}}), merged as PR
> #1812 and shipped in 7.3.0. This is the same bug in the properties-file path,
> which that change did not touch. It was noted during that work but
> deliberately left out of scope.
> h3. Suggested fix
> Change the {{break}} to {{continue}}, and add a regression test with a
> {{-conversion.properties}} file whose first key is already mapped, asserting
> the later keys still register. A hierarchy case (subclass and superclass
> files sharing a key) covers the realistic trigger.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
