[
https://issues.apache.org/jira/browse/WW-5685?focusedWorklogId=1037288&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1037288
]
ASF GitHub Bot logged work on WW-5685:
--------------------------------------
Author: ASF GitHub Bot
Created on: 23/Aug/26 17:45
Start Date: 23/Aug/26 17:45
Worklog Time Spent: 10m
Work Description: lukaszlenart opened a new pull request, #1860:
URL: https://github.com/apache/struts/pull/1860
Fixes [WW-5685](https://issues.apache.org/jira/browse/WW-5685)
`DefaultConversionFileProcessor.process` skipped keys already present in the
converter mapping with a `break` rather than a `continue`, so the first
already-mapped entry ended the loop and **every remaining entry in that
`-conversion.properties` file was silently dropped** — no warning, no error,
the affected properties simply fell back to default conversion.
```diff
if (mapping.containsKey(key)) {
- break;
+ continue;
}
```
## Why it triggers in practice
`XWorkConverter.buildConverterMapping` walks the class, then its interfaces,
then its superclass, passing *one accumulating* mapping into each
`addConverterMapping` call. So a key claimed earlier in that walk aborts a
later file outright:
- subclass `Foo-conversion.properties` declares `bar`
- superclass `FooBase-conversion.properties` declares `bar`, `baz`, `qux`
- when the superclass file is read, `bar` is already mapped → `baz` and
`qux` are never registered
Annotation-derived entries land in the same map, so an `@TypeConversion` on
the class could abort its properties file the same way.
`Properties` extends `Hashtable` and `entrySet()` has no defined order, so
*which* entries survived depended on hash order rather than file order — which
is what makes this hard to diagnose from the symptom.
WW-3871 fixed the identical defect in the annotation path (`XWorkConverter`,
[#1812](https://github.com/apache/struts/pull/1812), shipped 7.3.0). This is
the properties-file path that change did not touch. The remaining `break` at
`XWorkConverter:761` is unrelated and correct — it exits an interface search
after a hit.
## Testing — and a vacuous test caught by mutation
**`DefaultConversionFileProcessorTest`** (new). The ticket suggested a
fixture "whose first key is already mapped", but `Hashtable` ordering means no
fixture can pin down which key is read first. So the test **derives** the
colliding key from the actual iteration order at run time and pre-maps that one:
```java
String firstKey = (String) fixture.entrySet().iterator().next().getKey();
mapping.put(firstKey, SENTINEL);
processor.process(mapping, PropertiesCollisionBaseAction.class, FILENAME);
// every key in the file must now be present
```
With `break`, the loop stops on that first entry and registers nothing at
all — discriminating on any JDK, in any hash order, with no reliance on
`Hashtable` layout. A second test asserts every entry registers when nothing is
pre-mapped, guarding against a fix that skips too much.
**`XWorkConverterTest.testPropertiesEntriesAfterAKeyCollisionAreStillRegistered`**
covers the realistic hierarchy trigger. Worth flagging: **the first version of
this test was vacuous.** Its shared key hashed to the last position, so `break`
dropped nothing and it passed against the unfixed code — only the mutation
check exposed it. The key is renamed so it is read first (verified identical on
Temurin 17.0.14, 21.0.7 and 25.0.1), and the test now asserts that precondition
explicitly:
```java
assertEquals("the fixture only discriminates while the shared key is read
first; "
+ "Properties iteration order has changed and it must be
renamed again",
"CreateIfNull_overridden",
firstKeyOf(PROPERTIES_COLLISION_BASE_FILE));
```
so a future reordering fails loudly instead of going quiet.
Both tests were mutation-checked by reverting `continue` to `break`; both
fail.
`mvn test -DskipAssembly -pl core` — 3193 tests, 0 failures, 0 errors.
## Compatibility
Behaviour changes as intended: entries previously dropped now register. That
matches what WW-3871 shipped for the annotation path in 7.3.0. The
`containsKey` precedence rule itself is untouched — first source to claim a key
still wins, so nothing that already worked is overridden. Worth a Version Notes
line; no Migration Guide entry needed.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Issue Time Tracking
-------------------
Worklog Id: (was: 1037288)
Remaining Estimate: 0h
Time Spent: 10m
> 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)
