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) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
