Lukasz Lenart created WW-5702:
---------------------------------
Summary: 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
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)
