fix: Review 2.1245

[hduchesne]

Description

2.1 FormAction.execute — RenderContext is always null : done

2.2 HttpServletRequest as a back door for parsed files : done

2.3 No pre/post pipeline extension points : keep for later

2.4 ConditionalLogicEvaluator — O(n²) lookups and monolithic design :
I don’t think this review is valid as written.
The performance concern appears stale relative to the current code. ConditionalLogicEvaluator already performs an O(1) lookup through logicIdToFieldName.get(logicId), and that map is precomputed in FormFieldMetadataCollector. So the claimed O(n²) reverse-lookup cost does not match the implementation that is currently in the branch.
The broader design comment is more subjective. Yes, the evaluator centralizes operator semantics in one switch, and a registry-based operator model could be a reasonable future refactor if this area grows. But at the current scale (about 10 simple operators), that is an architectural preference, not a correctness issue, and not something I would block on in this PR.
My conclusion:

• the performance finding is not applicable to the current code
• the registry suggestion is an optional refactor, not a required fix

2.5 FieldMetadata would be cleaner as a single map : done

Checklist

Source code

• I've shared and documented any breaking change
• I've reviewed and updated the jahia-depends

Tests

• I've provided Unit and/or Integration Tests
• I've updated the parent issue with required manual validations

Tip

Documentation to guide the reviews: How to do a code review

[github-actions]

📝 Documentation Guidelines

Thank you for contributing to our documentation! To ensure your contributions meet our standards, please review these resources:

• How to push markdown documents to the Academy - Learn the process for publishing documentation
• Documentation style guide and writing guidelines - Follow our style conventions

This comment is posted automatically when changes are detected in the docs/ folder.

[Copilot]

Pull request overview

This PR updates the Formidable submission pipeline and action SPI to pass validated uploaded files explicitly, while also simplifying field metadata into a single per-field model and consolidating action-authoring documentation.

Changes:

• Replaces the request-attribute file handoff with List<SubmittedFile> on FormAction.execute(...).
• Collapses parser field metadata into FieldInfo / FieldMetadata and updates validation/collection logic.
• Updates in-tree actions, tests, and documentation for the new SPI.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
formidable-engine/src/main/java/org/jahia/modules/formidable/engine/actions/FormAction.java	Updates the action SPI signature to include uploaded files and remove RenderContext.
formidable-engine/src/main/java/org/jahia/modules/formidable/engine/actions/SubmittedFile.java	Adds the SPI-level uploaded-file representation.
formidable-engine/src/main/java/org/jahia/modules/formidable/engine/actions/FormDataParser.java	Introduces consolidated field metadata accessors and updates parser validation usage.
formidable-engine/src/main/java/org/jahia/modules/formidable/engine/servlet/FormSubmissionPipeline.java	Converts parsed files to submitted files and passes them to actions.
formidable-engine/src/main/java/org/jahia/modules/formidable/engine/servlet/FormFieldMetadataCollector.java	Builds the new FieldInfo metadata map.
formidable-engine/src/main/java/org/jahia/modules/formidable/engine/actions/storage/SaveToJcrFormAction.java	Uses the new files argument for persisted uploads.
formidable-engine/src/main/java/org/jahia/modules/formidable/engine/actions/forward/ForwardSubmissionFormAction.java	Uses the new files argument when rebuilding forwarded multipart requests.
formidable-engine/src/main/java/org/jahia/modules/formidable/engine/actions/email/SendEmailContentFormAction.java	Uses the new files argument for email attachments.
formidable-engine/src/main/java/org/jahia/modules/formidable/engine/actions/email/SendEmailNotificationFormAction.java	Updates the method signature for the new SPI.
formidable-engine/src/test/java/org/jahia/modules/formidable/engine/actions/FormDataParserFieldMetadataTest.java	Adds tests for the new field metadata model.
formidable-engine/src/test/java/org/jahia/modules/formidable/engine/actions/email/EmailActionRecipientTest.java	Updates test calls to the new action signature.
docs/how-to-create-form-action.md	Consolidates action-authoring guidance and documents the new file argument.
docs/custom-form-action-from-external-module.md	Removes the separate external-module action guide after consolidation.
docs/save-to-jcr.md	Updates runtime steps to reference the new files argument.
docs/form-submission-flow.md	Updates submission-flow docs for the new uploaded-file handoff.

[jkevan]

Scope

This PR targets items 2.1, 2.2, and 2.5 from the master review. Items 2.3 and 2.4 are explicitly deferred or disputed in the PR description. An unrelated pom.xml cleanup (drop ${revision} / flatten-maven-plugin) is included as a separate commit.

Commits reviewed

SHA	Message
a8f3615	fix: review 2.5 FieldMetadata would be cleaner as a single map
552068e	fix: review 2.1-2 remove null RenderContext and pass submitted files explicitly
f7025be	Potential fix for pull request finding
01f7c82	Potential fix for pull request finding
c597d54	Potential fix for pull request finding
2705982	update lessons-learned.md to clarify
0a0ba41	refactor: drop ${revision} and remove flatten-maven-plugin (#85)

---

Status summary

Item	Title	Status
2.1	FormAction.execute — RenderContext always null	✅ Addressed
2.2	HttpServletRequest as backdoor for files	✅ Addressed — ⚠️ req now unused in SPI (see below)
2.3	No pre/post pipeline extension points	ℹ️ Deferred — explicitly acknowledged
2.4	ConditionalLogicEvaluator O(n²) + monolithic design	ℹ️ Disputed — accepted (see below)
2.5	FieldMetadata as parallel maps	✅ Addressed

---

Item-by-item analysis

2.1 — RenderContext removed from FormAction SPI ✅

RenderContext renderContext is removed from FormAction.execute(...) across the interface and all four implementations (SendEmailNotificationFormAction, SendEmailContentFormAction, ForwardSubmissionFormAction, SaveToJcrFormAction).

docs/custom-form-action-from-external-module.md (which advertised the old signature) is deleted. Its content is merged into the revised docs/how-to-create-form-action.md, which now correctly describes the SPI without RenderContext and covers both internal and external module authors.

EmailActionRecipientTest call sites updated accordingly.

---

2.2 — SubmittedFile SPI type; PARSED_FILES_ATTR removed ✅

What was done:

• New SubmittedFile public record introduced as the SPI boundary type for uploaded files.
• FormAction.execute now receives List<SubmittedFile> files as an explicit parameter.
• FormDataParser.PARSED_FILES_ATTR constant removed.
• req.setAttribute(PARSED_FILES_ATTR, ...) removed from FormSubmissionPipeline.run.
• All three actions that previously did an unchecked req.getAttribute cast now receive files directly.
• FormSubmissionPipeline.toSubmittedFiles() converts the internal List<FormFile> to List<SubmittedFile> before dispatching actions.

The @SuppressWarnings("unchecked") casts are gone. The magic string attribute is gone. The separation between internal FormFile (parser output) and SubmittedFile (SPI boundary) is correct — it avoids leaking internal parser types into the public API.

Issue 1 — SubmittedFile is a record with a byte[] field — broken equals()/hashCode() 🔴

Java records generate equals() and hashCode() using the component values. For array components, the compiler uses == (identity) for equals() and System.identityHashCode() for hashCode(), not Arrays.equals/Arrays.hashCode. This means:

byte[] content = {1, 2, 3};
new SubmittedFile("f", "f.txt", "image/png", content)
    .equals(new SubmittedFile("f", "f.txt", "image/png", content.clone()))
// → false  (even though data is identical)

This is the well-known Java SonarQube rule java:S6218 ("Mutable members should not be used in equals()/hashCode() without proper overriding in records"). This is almost certainly the root cause of the E Reliability Rating and 9 new reliability issues reported by Sonar on this PR (3/12 CI checks failing).

Fix options:

1. Override equals() and hashCode() manually using Arrays.equals/Arrays.deepHashCode.
2. Replace the byte[] data component with an Object wrapper (e.g. java.nio.ByteBuffer.wrap(data).asReadOnlyBuffer()) — but this changes the accessor API.
3. Drop the record syntax and use a plain final class. This is the most explicit option for a stable SPI type with a mutable array field.

This must be resolved before merge — Sonar CI gate is failing.

Issue 2 — HttpServletRequest req is now unused in the SPI 🟠

After removing the req.getAttribute(PARSED_FILES_ATTR) calls, req is no longer used by any of the four action implementations. Verified: ForwardSubmissionFormAction, SaveToJcrFormAction, SendEmailContentFormAction, SendEmailNotificationFormAction — none call any method on req.

The original review proposed keeping req in the signature (as a potential hook for future needs). But with 0 current usages, this is dead weight in a public SPI. Removing it now costs nothing; removing it later is a breaking change. Recommend removing req from the signature while the surface is still young.

---

2.3 — Pre/post pipeline extension points ℹ️ Deferred

Explicitly deferred in the PR description. No action needed.

---

2.4 — ConditionalLogicEvaluator O(n²) dispute ℹ️

The PR author disputes the finding: logicIdToFieldName is already precomputed in FormFieldMetadataCollector and the evaluator performs an O(1) lookup. The review's O(n²) claim was based on the original code pattern; if the reverse map was added in an earlier PR, the performance concern no longer applies.

The registry-based operator architecture remains a subjective design suggestion, not a blocking concern. The author's conclusion is accepted.

---

2.5 — FieldMetadata unified model ✅

What was done:

• New FormDataParser.FieldInfo record consolidates nodeType, allowedChoices, acceptTypes, constraints per field.
• FormDataParser.FieldMetadata is refactored to wrap Map<String, FieldInfo> with helper accessors (field(), fieldType(), allowedChoices(), acceptTypes(), constraints(), allowedNames()).
• FormFieldMetadataCollector.CollectorContext loses 4 parallel maps; now holds a single Map<String, FieldInfo> built bottom-up in registerField.
• FormFieldMetadataCollector.Result simplified to fieldInfos + logic-related maps.
• FormFieldMetadataCollector.collectChoices() and new collectAcceptTypes() now return values instead of mutating context — a cleaner functional approach.
• FormSubmissionPipeline.validateRequired() updated to use the new API.
• FormDataParserFieldMetadataTest added with 3 test cases covering the happy path, absent-field defaults, and null-collection normalization.

Both the constructor defensive copies (Set.copyOf, Map.copyOf) and the null normalization are correctly implemented. The refactor is clean.

---

Docs and tooling changes

how-to-create-form-action.md — example code uses System.out.println 🟠

---

Summary of new issues surfaced

Severity	Location	Description
🔴 Blocking	SubmittedFile.java	Record with byte[] data — auto-generated equals()/hashCode() use array identity, not content; root cause of Sonar E Reliability rating and 9 new issues — must fix before merge
🟠 Medium	FormAction.java	req parameter is now unused across all 4 implementations — dead SPI weight; remove while still cheap
🟠 Medium	docs/how-to-create-form-action.md	Example code uses System.out.println — should use SLF4J logger; teaches bad practice to external module authors

[jahia-sonarqube]

Quality Gate failed

Failed conditions
B Security Rating on New Code (required ≥ A)
E Reliability Rating on New Code (required ≥ A)
9 New Reliability Issues (required ≤ 0)
891 Added Technical Debt (required ≤ 0)
60 New Maintainability Issues (required ≤ 0)
13 New Security Issues (required ≤ 0)

See analysis details on SonarQube