[jira] [Updated] (WW-5702) Polish HTML5 constraint derivation: scope gaps and attribute hygiene found reviewing WW-5695

["Lukasz Lenart (Jira)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/WW-5702?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Lukasz Lenart updated WW-5702:
------------------------------
    Fix Version/s: 7.4.0

> Polish HTML5 constraint derivation: scope gaps and attribute hygiene found reviewing WW-5695
> --------------------------------------------------------------------------------------------
>
>                 Key: WW-5702
>                 URL: https://issues.apache.org/jira/browse/WW-5702
>             Project: Struts 2
>          Issue Type: Task
>            Reporter: Lukasz Lenart
>            Priority: Major
>             Fix For: 7.4.0
>
>
> Follow-up to WW-5695 (PR #1865), collecting review findings that are real but not blocking. None of these is a false-reject; the false-reject class was fixed on the WW-5695 branch itself.
> The governing rule for the feature is unchanged: emit a constraint only when the browser cannot reject input the server would accept, and never set or change an input's {{type}}.
> h3. 1. Derivation is not gated on the theme
> {{UIBean.addConstraintAttributes}} guards on {{struts.ui.html5.constraints}} but not on the resolved theme, while only {{template/html5/common-attributes.ftl}} renders the map. Enabling the constant for a few html5 forms makes every xhtml/simple/css\_xhtml form in the application resolve its action's validators and call {{getMessage()}} per field to build a map nothing renders. Gate on the resolved theme as well as the constant. Note {{ConstraintAttributesTest}} never calls {{setTheme}}, so it currently asserts derivation under the default xhtml theme.
> h3. 2. data-msg-* is emitted regardless of control type
> {{StrutsHtmlConstraintProvider.constraintsFor}} calls {{addMessage}} for every validator independently of whether {{addConstraints}} derived anything and independently of the control. A checkbox whose {{required}} constraint was deliberately suppressed still renders {{data-msg-required}}, and {{s:label}} renders validation text onto an element that never submits. Gate {{addMessage}} on the control supporting at least one constraint.
> The test that should catch this is vacuous: {{StrutsHtmlConstraintProviderTest}}'s helper passes a null action, and {{addMessage}} returns immediately on null, so no assertion in that class exercises the message path at all. Fix the helper to pass a real action.
> h3. 3. Forms with no action attribute resolve validators under an empty context
> {{Form.resolveActionValidators}} calls {{findString(action)}}, but {{action}} is frequently null: {{ServletUrlRenderer.renderFormUrl}} resolves the name from the current invocation into {{attributes.actionName}} without writing it back to the field. {{findString(null)}} does not throw - {{OgnlTextParser.evaluate}} coerces null to the empty string - and {{DefaultActionMapper.getMappingFromActionName("")}} returns a non-null mapping, so the {{attributes.actionName}} fallback is unreachable and the context becomes the empty string. Alias-scoped {{ActionClass-alias-validation.xml}} validators are then silently skipped, for the most common form usage.
> This defect is inherited from the deprecated {{getValidators(String)}}, not introduced by WW-5695; what is new is that the path now runs for every field of every form. Fall back to {{attributes.actionName}} when {{findString(action)}} is blank. No test covers an action-less form.
> h3. 4. Visitor-validated nested fields get a message and never a constraint
> {{Form.findFieldValidators}} wraps prefixed-field validators in {{Form.FieldVisitorValidatorWrapper}}, which implements only {{FieldValidator}}. {{addConstraints}} dispatches on concrete validator types, so for the common nested-bean case - a textfield named {{user.name}} behind a {{visitor}} validator on {{user}} - no constraint is ever derived, while the wrapper's {{getValidatorType()}} returns the literal {{field-visitor}} and {{getMessage()}} delegates, so the field renders {{data-msg-field-visitor}} and nothing else. Unwrap via {{getFieldValidator()}} before dispatching, or skip wrapped validators entirely. Same visitor blind spot recorded against WW-3530.
> h3. 5. max is emitted without the type guard applied to min
> {{addRange}} guards {{min}} through {{isIntegral}} but passes {{max}} straight to {{putIfPresent}}, which stringifies whatever it is given. A date-typed or string-typed range validator on a control the developer declared numeric emits a nonsense {{max}}. {{addDoubleRange}} has the same asymmetry for NaN and infinite upper bounds. Browsers ignore an unparseable bound, so this is invalid markup rather than a false reject.
> h3. 6. isIntegral rounds through double
> {{isIntegral}} tests {{doubleValue()}} while {{putIfPresent}} renders the original object, so a custom range validator carrying a BigDecimal such as 1.0000000000000000001 passes the guard and then renders that fractional value as {{min}}, shifting the HTML step base. No shipped range validator is affected - only Short, Int, Long and Date subclasses exist - so this needs a custom validator to reach. Determine integrality from the decimal representation instead.
> h3. 7. CHECKBOX and HIDDEN are unreachable control types
> The enum declares both but only six components override {{getControlType()}}, so {{Checkbox}} and {{Hidden}} inherit {{OTHER}} and a replacement provider cannot distinguish them. Returning their real types is behaviourally inert with the default provider, which ignores both, so this is purely about the extension contract being honest.
> h3. 8. The provider javadoc advertises a type override that cannot work
> {{HtmlConstraintProvider}}'s javadoc suggests a replacement provider could map an email validator to {{type="email"}}. It cannot: {{TextField}} only puts {{type}} into the attribute map when the developer set it, so {{isAlreadyRendered}} does not filter it, and {{html5/text.ftl}} has already emitted a hardcoded {{type="text"}} by the time the constraint map renders - producing a duplicate attribute of which the browser keeps the first. The example also contradicts the never-change-type rule stated in {{StrutsHtmlConstraintProvider}}. Correct the javadoc, and consider dropping {{type}} from the derived map outright.
> h3. 9. Dynamic attribute names are compared case-sensitively
> {{isAlreadyRendered}} does an exact-key lookup in {{dynamicAttributes}}, but HTML attribute names are ASCII case-insensitive. A dynamic {{MAXLENGTH}} misses the check and renders after the derived {{maxlength}}, so the browser keeps the derived one - the opposite of the documented "the developer's own value always wins". Compare dynamic keys case-insensitively.
> h3. 10. data-msg attribute names are not sanitised
> The name is {{data-msg-}} concatenated with the validator type. FreeMarker's HTML auto-escaping protects the value, not the name, and validator types are freely named in validators.xml, so a type containing a space or an equals sign splits into a second attribute. Config-controlled, so low risk; a character-class guard is one line.
> h3. 11. The allowlist admits patterns the browser refuses to compile
> HTML compiles {{pattern}} with the Unicode flag, under which an identity escape outside a character class is restricted to syntax characters plus the solidus. An escaped hyphen outside a class is a SyntaxError and the whole attribute is then ignored, so a pattern that passes {{EcmaScriptSafeRegex.isSafe}} can be emitted and silently do nothing. Safe direction - no false reject - but the allowlist should be honest and permit the hyphen escape only inside a character class. Related and very low: {{isSafe}} clears its in-class state at the first unescaped closing bracket, so a literal-bracket class is modelled as class-then-literal, matching neither engine.
> h3. 12. Six html5 templates can never render constraints
> {{constraints.ftl}} is reachable only through {{html5/common-attributes.ftl}}, which combobox, datetextfield, doubleselect, updownselect, optiontransferselect and inputtransferselect do not include. datetextfield is the notable one given the enum already models the temporal control types. Coverage gap rather than a bug; worth recording alongside the deliberately deferred temporal min/max formatting.
> h3. 13. Visitor validators are re-resolved once per field
> {{resolveActionValidators()}} caches the top-level list once per form, but the visitor branch inside {{findFieldValidators}} calls the validator manager once per field, and the manager caches only the config list - it constructs fresh validator instances on every call. An N-field form with one visitor validator does N full instantiation passes per render. {{FormFieldValidatorsTest}} pins the top-level call count only.
> h3. Suggested test additions
> * a form with no action attribute (item 3 has no coverage today)
> * a real action passed through the provider test helper, so the message path stops being invisible (item 2)
> * end-to-end template rendering of pattern, and of required on radio and file; only minlength is currently asserted through a template



--
This message was sent by Atlassian Jira
(v8.20.10#820010)