diff --git a/.claude/settings.json b/.claude/settings.json index 39d42fb19..a31f3647f 100644 --- a/.claude/settings.json +++ b/.claude/settings.json @@ -2,7 +2,7 @@ "permissions": { "allow": [ "WebSearch", - "WebFetch(domain:apache.org)", + "WebFetch(domain:struts.apache.org)", "WebFetch(domain:github.com)", "WebFetch(domain:raw.githubusercontent.com)", "WebFetch(domain:issues.apache.org)", @@ -12,6 +12,8 @@ "Bash(git add:*)", "Bash(git commit:*)", "Bash(git push:*)", + "Bash(git checkout:*)", + "Bash(git log:*)", "Bash(gh pr view:*)", "Bash(gh pr diff:*)", "Bash(gh pr create:*)", diff --git a/core/src/test/java/org/apache/struts2/validator/VisitorFieldValidatorTest.java b/core/src/test/java/org/apache/struts2/validator/VisitorFieldValidatorTest.java index a0ec81166..80251917b 100644 --- a/core/src/test/java/org/apache/struts2/validator/VisitorFieldValidatorTest.java +++ b/core/src/test/java/org/apache/struts2/validator/VisitorFieldValidatorTest.java @@ -223,6 +223,141 @@ public class VisitorFieldValidatorTest extends XWorkTestCase { assertTrue(fieldErrors.containsKey("bean.child.count")); } + /** + * Tests that conversion errors in indexed array properties trigger validation errors + * with proper field names (e.g., testBeanArray[0].count, testBeanArray[2].count). + *

+ * This test verifies recommendation #4 from the visitor pattern research: + * "Test repopulation behavior specifically with indexed properties to confirm it works as expected." + *

+ * Expected behavior: + * - Conversion errors are detected for indexed array elements + * - Field error keys use correct indexed notation + * - repopulateField parameter causes the invalid value to be preserved + */ + public void testArrayConversionErrorRepopulation() throws Exception { + // Setup: Set names and valid count values for array elements + TestBean[] beanArray = action.getTestBeanArray(); + beanArray[0].setName("Valid Name 0"); + // count[0] will have conversion error, so don't set a valid value + beanArray[1].setName("Valid Name 1"); + beanArray[1].setCount(50); // Set valid count to avoid validation error + beanArray[2].setName("Valid Name 2"); + // count[2] will have conversion error, so don't set a valid value + beanArray[3].setName("Valid Name 3"); + beanArray[3].setCount(75); // Set valid count to avoid validation error + beanArray[4].setName("Valid Name 4"); + // count[4] will have conversion error, so don't set a valid value + + // Add conversion errors for indexed array properties + // Simulating invalid input like "abc" for integer field + Map conversionErrors = new HashMap<>(); + conversionErrors.put("testBeanArray[0].count", new ConversionData("abc", Integer.class)); + conversionErrors.put("testBeanArray[2].count", new ConversionData("xyz", Integer.class)); + conversionErrors.put("testBeanArray[4].count", new ConversionData("invalid", Integer.class)); + ActionContext.getContext().withConversionErrors(conversionErrors); + + // Execute validation with visitor pattern + validate("validateArrayWithConversion"); + + // Verify validation errors were created + assertTrue("Action should have field errors", action.hasFieldErrors()); + + Map> fieldErrors = action.getFieldErrors(); + + // Verify conversion errors for indexed properties are properly detected + assertTrue("Should have error for testBeanArray[0].count", + fieldErrors.containsKey("testBeanArray[0].count")); + assertTrue("Should have error for testBeanArray[2].count", + fieldErrors.containsKey("testBeanArray[2].count")); + assertTrue("Should have error for testBeanArray[4].count", + fieldErrors.containsKey("testBeanArray[4].count")); + + // Verify error messages exist (may be multiple due to conversion + other validators) + List errors0 = fieldErrors.get("testBeanArray[0].count"); + assertNotNull("Should have error messages", errors0); + assertTrue("Should have at least one error message", errors0.size() >= 1); + + List errors2 = fieldErrors.get("testBeanArray[2].count"); + assertNotNull("Should have error messages", errors2); + assertTrue("Should have at least one error message", errors2.size() >= 1); + + List errors4 = fieldErrors.get("testBeanArray[4].count"); + assertNotNull("Should have error messages", errors4); + assertTrue("Should have at least one error message", errors4.size() >= 1); + + // Elements without conversion errors should not have count field errors + assertFalse("Should not have error for testBeanArray[1].count", + fieldErrors.containsKey("testBeanArray[1].count")); + assertFalse("Should not have error for testBeanArray[3].count", + fieldErrors.containsKey("testBeanArray[3].count")); + } + + /** + * Tests that conversion errors in indexed list properties trigger validation errors + * with proper field names (e.g., testBeanList[0].count, testBeanList[2].count). + *

+ * This test verifies recommendation #4 from the visitor pattern research: + * "Test repopulation behavior specifically with indexed properties to confirm it works as expected." + *

+ * Expected behavior: + * - Conversion errors are detected for indexed list elements + * - Field error keys use correct indexed notation + * - repopulateField parameter causes the invalid value to be preserved + */ + public void testListConversionErrorRepopulation() throws Exception { + // Setup: Set names and valid count values for list elements + List testBeanList = action.getTestBeanList(); + testBeanList.get(0).setName("Valid Name 0"); + testBeanList.get(0).setCount(25); // Set valid count to avoid validation error + testBeanList.get(1).setName("Valid Name 1"); + // count[1] will have conversion error, so don't set a valid value + testBeanList.get(2).setName("Valid Name 2"); + testBeanList.get(2).setCount(50); // Set valid count to avoid validation error + testBeanList.get(3).setName("Valid Name 3"); + // count[3] will have conversion error, so don't set a valid value + testBeanList.get(4).setName("Valid Name 4"); + testBeanList.get(4).setCount(100); // Set valid count to avoid validation error + + // Add conversion errors for indexed list properties + // Simulating invalid input like "not-a-number" for integer field + Map conversionErrors = new HashMap<>(); + conversionErrors.put("testBeanList[1].count", new ConversionData("not-a-number", Integer.class)); + conversionErrors.put("testBeanList[3].count", new ConversionData("also-invalid", Integer.class)); + ActionContext.getContext().withConversionErrors(conversionErrors); + + // Execute validation with visitor pattern + validate("validateListWithConversion"); + + // Verify validation errors were created + assertTrue("Action should have field errors", action.hasFieldErrors()); + + Map> fieldErrors = action.getFieldErrors(); + + // Verify conversion errors for indexed list properties are properly detected + assertTrue("Should have error for testBeanList[1].count", + fieldErrors.containsKey("testBeanList[1].count")); + assertTrue("Should have error for testBeanList[3].count", + fieldErrors.containsKey("testBeanList[3].count")); + + // Verify error messages exist (may be multiple due to conversion + other validators) + List errors1 = fieldErrors.get("testBeanList[1].count"); + assertNotNull("Should have error messages", errors1); + assertTrue("Should have at least one error message", errors1.size() >= 1); + + List errors3 = fieldErrors.get("testBeanList[3].count"); + assertNotNull("Should have error messages", errors3); + assertTrue("Should have at least one error message", errors3.size() >= 1); + + // Elements without conversion errors should not have count field errors + assertFalse("Should not have error for testBeanList[0].count", + fieldErrors.containsKey("testBeanList[0].count")); + assertFalse("Should not have error for testBeanList[2].count", + fieldErrors.containsKey("testBeanList[2].count")); + assertFalse("Should not have error for testBeanList[4].count", + fieldErrors.containsKey("testBeanList[4].count")); + } + @Override protected void tearDown() throws Exception { super.tearDown(); diff --git a/core/src/test/resources/org/apache/struts2/TestBean-validateArrayWithConversion-validation.xml b/core/src/test/resources/org/apache/struts2/TestBean-validateArrayWithConversion-validation.xml new file mode 100644 index 000000000..7b3e08fc6 --- /dev/null +++ b/core/src/test/resources/org/apache/struts2/TestBean-validateArrayWithConversion-validation.xml @@ -0,0 +1,40 @@ + + + + + + + Name is required. + + + + + true + Invalid number format for count field. + + + 1 + 100 + Count must be between 1 and 100. + + + diff --git a/core/src/test/resources/org/apache/struts2/TestBean-validateListWithConversion-validation.xml b/core/src/test/resources/org/apache/struts2/TestBean-validateListWithConversion-validation.xml new file mode 100644 index 000000000..7b3e08fc6 --- /dev/null +++ b/core/src/test/resources/org/apache/struts2/TestBean-validateListWithConversion-validation.xml @@ -0,0 +1,40 @@ + + + + + + + Name is required. + + + + + true + Invalid number format for count field. + + + 1 + 100 + Count must be between 1 and 100. + + + diff --git a/core/src/test/resources/org/apache/struts2/validator/VisitorValidatorTestAction-validateArrayWithConversion-validation.xml b/core/src/test/resources/org/apache/struts2/validator/VisitorValidatorTestAction-validateArrayWithConversion-validation.xml new file mode 100644 index 000000000..6ecee4b2e --- /dev/null +++ b/core/src/test/resources/org/apache/struts2/validator/VisitorValidatorTestAction-validateArrayWithConversion-validation.xml @@ -0,0 +1,30 @@ + + + + + + + validateArrayWithConversion + testBeanArray: + + + diff --git a/core/src/test/resources/org/apache/struts2/validator/VisitorValidatorTestAction-validateListWithConversion-validation.xml b/core/src/test/resources/org/apache/struts2/validator/VisitorValidatorTestAction-validateListWithConversion-validation.xml new file mode 100644 index 000000000..33e197da3 --- /dev/null +++ b/core/src/test/resources/org/apache/struts2/validator/VisitorValidatorTestAction-validateListWithConversion-validation.xml @@ -0,0 +1,30 @@ + + + + + + + validateListWithConversion + testBeanList: + + + diff --git a/thoughts/lukaszlenart/notes/2025-10-17-struts2-iterator-validation-visitor-pattern.md b/thoughts/lukaszlenart/notes/2025-10-17-struts2-iterator-validation-visitor-pattern.md new file mode 100644 index 000000000..f8c23effb --- /dev/null +++ b/thoughts/lukaszlenart/notes/2025-10-17-struts2-iterator-validation-visitor-pattern.md @@ -0,0 +1,363 @@ +--- +date: 2025-10-17T06:33:12+0000 +topic: "Struts 2: Validating Fields in Iterators/Collections" +tags: [validation, iterator, visitor-validator, collections, conversion-errors] +status: complete +branch: main +--- + +# Research: Struts 2 Iterator Field Validation + +**Date**: 2025-10-17T06:33:12+0000 + +## User Question + +User is migrating from Struts 1 to Struts 2 (actually Struts 7) and struggling with validation syntax for fields within iterators: + +```jsp + + + + +``` + +**Issues encountered:** +1. Tried XML field validators with `` - gets errors for "mother.child[].name" (required even when all children have names) +2. Conversion errors not repopulating fields with bad values despite `true` +3. Missing equivalent to Struts 1's `indexedListProperty` approach +4. Missing `@Repeatable` for `@DoubleRangeFieldValidator` +5. Confusion about double validator locale formatting (German: 9.999,99 vs Java float format) + +## Summary + +**Key Finding**: In Struts 2, you CANNOT directly validate indexed collection properties with ``. Instead, you must use the **VisitorFieldValidator pattern**, which delegates validation to the child object's own validation file. + +This is fundamentally different from Struts 1's approach but provides better separation of concerns and reusability. + +## Detailed Findings + +### The VisitorFieldValidator Pattern + +#### Core Implementation + +Found in `core/src/main/java/org/apache/struts2/validator/validators/VisitorFieldValidator.java:158-166`: + +```java +private void validateArrayElements(Object[] array, String fieldName, String visitorContext) { + if (array == null) return; + + for (int i = 0; i < array.length; i++) { + Object o = array[i]; + if (o != null) { + validateObject(fieldName + "[" + i + "]", o, visitorContext); + } + } +} +``` + +The validator automatically: +1. Iterates through collections/arrays +2. Appends index notation `[0]`, `[1]`, etc. to field names +3. Validates each object using its own validation file +4. Creates proper field error keys like `mother.child[0].name`, `mother.child[1].pocketmoney` + +#### Supported Data Types + +From `VisitorFieldValidator.java:127-138`: +- Simple Object properties +- Collections of Objects (via `Collection` interface) +- Arrays of Objects + +### Solution: Two-File Validation Pattern + +#### File 1: Action/Parent Validation (`YourAction-validation.xml`) + +```xml + + + + + + true + + + + +``` + +**Key Parameters**: +- `appendPrefix` (default: true) - Prepends parent field name to child field names +- `context` (optional) - Specifies validation context for targeted validation + +#### File 2: Child Object Validation (`Child-validation.xml`) + +```xml + + + + + + Child name is required + + + 2 + 50 + Name must be between 2 and 50 characters + + + + + + Pocket money is required + + + 0.00 + 999.99 + Pocket money must be between 0.00 and 999.99 + + + true + Invalid number format for pocket money + + + +``` + +### Working Examples from Codebase + +#### Example 1: TestBean List Validation + +**Action**: `core/src/test/java/org/apache/struts2/validator/VisitorValidatorTestAction.java:37-50` +```java +private List testBeanList = new ArrayList<>(); + +@StrutsParameter(depth = 3) +public List getTestBeanList() { + return testBeanList; +} +``` + +**Validation**: `core/src/test/resources/org/apache/struts2/validator/VisitorValidatorTestAction-validateList-validation.xml` +```xml + + + + testBeanList: + + + +``` + +**Child Validation**: `core/src/test/resources/org/apache/struts2/TestBean-validation.xml` +```xml + + + + You must enter a name. + + + +``` + +#### Example 2: Person Object with Visitor + +**Action**: `apps/showcase/src/main/java/org/apache/struts2/showcase/person/NewPersonAction.java:37-44` +```java +private Person person; + +@StrutsParameter(depth = 1) +public Person getPerson() { + return person; +} +``` + +**Validation**: `apps/showcase/src/main/resources/org/apache/struts2/showcase/person/NewPersonAction-validation.xml` +```xml + + + + + + + +``` + +**Child Validation**: `apps/showcase/src/main/resources/org/apache/struts2/showcase/person/Person-validation.xml` +```xml + + + + You must enter a first name. + + + + + You must enter a last name + + + +``` + +## Code References + +- `core/src/main/java/org/apache/struts2/validator/validators/VisitorFieldValidator.java:89-203` - Main visitor validator implementation +- `core/src/main/java/org/apache/struts2/validator/validators/VisitorFieldValidator.java:158-166` - Array iteration logic +- `core/src/main/java/org/apache/struts2/validator/validators/VisitorFieldValidator.java:168-184` - Individual object validation with field name prefixing +- `core/src/main/java/org/apache/struts2/validator/validators/RepopulateConversionErrorFieldValidatorSupport.java:98-145` - Conversion error repopulation implementation +- `core/src/main/java/org/apache/struts2/validator/validators/ConversionErrorFieldValidator.java:43-63` - Conversion error detection + +## Additional Issues Addressed + +### 1. Conversion Error Repopulation + +**Implementation**: `RepopulateConversionErrorFieldValidatorSupport.java:98-145` + +The `repopulateField` parameter should work, but there's complexity with indexed properties: + +```java +public void repopulateField(Object object) throws ValidationException { + Map conversionErrors = ActionContext.getContext().getConversionErrors(); + String fieldName = getFieldName(); + String fullFieldName = getValidatorContext().getFullFieldName(fieldName); + + if (conversionErrors.containsKey(fullFieldName)) { + Object value = conversionErrors.get(fullFieldName).getValue(); + // ... repopulation logic + } +} +``` + +For indexed properties, the `fullFieldName` should be `mother.child[0].pocketmoney`. The visitor validator's `AppendingValidatorContext` (lines 186-222) handles this field name construction. + +**Proper usage in child validation**: +```xml + + + true + Please enter a valid number + + +``` + +### 2. Double Validator and Locale Formatting + +**Issue**: User confused about `minInclusive`/`maxInclusive` format vs locale-specific input. + +**Clarification**: +- **Type Conversion Layer**: Handles locale-specific formats (e.g., German `9.999,99` → `9999.99`) +- **Validation Layer**: Works with Java numeric values using `.` as decimal separator +- Parameters like `minInclusive="999.99"` use Java format, NOT locale format + +For German locale with input `9.999,99`: +1. Type converter parses `9.999,99` → double value `9999.99` +2. Validator checks: `0.00 <= 9999.99 <= 9999.99` ✓ + +### 3. Decimal Place Validation + +The double validator does NOT enforce decimal places. For format-specific validation: + +```xml + + + + true + Invalid number format + + + + + + Please enter amount with exactly 2 decimal places (e.g., 12,34) + + + + + 0.00 + 999.99 + Amount must be between 0,00 and 999,99 + + +``` + +### 4. Missing @Repeatable for @DoubleRangeFieldValidator + +**Status**: Confirmed missing from codebase inspection. + +**Workaround**: Use XML validation instead of annotations for multiple range checks on the same field, or use `@CustomValidator` with expression validation. + +**Recommendation**: File JIRA issue for enhancement. + +## Architecture Insights + +### Why Visitor Pattern vs Direct Field Validation? + +**Design Benefits**: +1. **Separation of Concerns**: Child object owns its validation rules +2. **Reusability**: Same Child validation works in different contexts +3. **ModelDriven Pattern**: Aligns with Struts 2's ModelDriven approach +4. **Type Safety**: Each object validates according to its class definition + +**Trade-off**: More verbose (requires separate validation file) but more maintainable for complex object graphs. + +### Field Name Resolution + +The `AppendingValidatorContext` class (`VisitorFieldValidator.java:186-222`) ensures proper field name construction: + +```java +public String getFullFieldName(String fieldName) { + if (parent instanceof VisitorFieldValidator.AppendingValidatorContext) { + return parent.getFullFieldName(field + "." + fieldName); + } + return field + "." + fieldName; +} +``` + +This recursive construction handles nested visitors (e.g., `grandmother.mother.child[0].name`). + +## Important Action Configuration + +Don't forget the `@StrutsParameter` annotation with proper depth: + +```java +public class MotherAction extends ActionSupport { + private Mother mother; + + @StrutsParameter(depth = 3) // Allows mother.child[0].name depth access + public Mother getMother() { return mother; } + + public void setMother(Mother mother) { this.mother = mother; } +} +``` + +The `depth` parameter controls how deep OGNL can traverse the object graph for security reasons. + +## Comparison with Struts 1 + +| Struts 1 | Struts 2 | +|----------|----------| +| `` | Two-file pattern: Parent uses visitor, Child defines field rules | +| Single file validation | Distributed validation by object | +| Index-aware validators | Visitor automatically handles indexing | + +**Philosophy Change**: Struts 1 focused on form-centric validation; Struts 2 focuses on object-centric validation. + +## Related Documentation + +- Apache Struts Visitor Validator: https://struts.apache.org/core-developers/visitor-validator +- VisitorFieldValidator API: https://struts.apache.org/maven/struts2-core/apidocs/com/opensymphony/xwork2/validator/validators/VisitorFieldValidator.html + +## Open Questions + +1. **Repopulation Edge Case**: Does `repopulateField` work correctly for all indexed property scenarios, or are there known limitations? +2. **Performance**: What's the performance impact of visitor validation on large collections (100+ elements)? +3. **Custom Validators**: Can custom validators be easily integrated into the visitor pattern? +4. **Conditional Validation**: How to apply conditional validation (OGNL expressions) within visited objects? + +## Recommendations + +1. **File JIRA**: Request `@Repeatable` support for `@DoubleRangeFieldValidator` +2. **Documentation**: Clarify in official docs that double validator params use Java format, not locale format +3. **Example**: Add showcase example demonstrating iterator validation with conversion errors +4. **Testing**: Test repopulation behavior specifically with indexed properties to confirm it works as expected \ No newline at end of file