[
https://issues.apache.org/jira/browse/WW-5655?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Lukasz Lenart updated WW-5655:
------------------------------
Description:
h2. Summary
Replace the {{null}}-overloaded control flow in
{{AbstractLocalizedTextProvider}} / {{StrutsLocalizedTextProvider}} message
resolution with an explicit result type, so that "not found" and "found but the
value degraded to the literal {{"null"}}" are represented distinctly instead of
both collapsing to a Java {{null}} that callers must branch on.
h2. Background
Message lookup uses {{null}} as an overloaded signal:
* {{formatWithNullDetection}} returns {{null}} when a formatted message equals
the literal string {{"null"}} (e.g. a {{ {0\} }} pattern rendered with a
{{null}} argument).
* Callers ({{findText}}'s tiers, the deprecated {{findMessage}}) treat that
{{null}} identically to a genuine "key not found" and fall through to the next
source: {{if (msg != null) return msg;}}.
Both cases legitimately mean "keep searching", so the overloading is not a bug
— but it is a readability/robustness wart. The codebase already has a wrapper
for the analogous default-message path — {{GetDefaultMessageReturnArg}}
(holding {{String message}} and {{boolean foundInBundle}}). Extending a similar
explicit result to the hierarchy path would make the intent self-documenting
and consistent.
Context: this was surfaced while implementing WW-5540 (hierarchy-traversal
caching). WW-5540 deliberately kept the existing {{null}} convention and only
added a cache-boundary marker ({{NOT_FOUND}} sentinel + {{isNotFound(...)}})
plus a fall-through mitigation, to stay behavior-preserving and focused. This
ticket is the orthogonal control-flow cleanup that WW-5540 deferred.
h2. Proposed change
Introduce an internal result type (e.g. {{sealed}}/enum-tagged: {{Found(String
value)}} vs {{ContinueSearch}}) used by the raw-resolution + formatting path,
unwrapped to {{String}}/{{null}} at the public {{findText}} boundary (the
public method signatures return {{String}} and must not change). Consider
whether {{formatWithNullDetection}}, {{getMessage}}, {{findDefaultText}}, and
{{getDefaultMessage}} should adopt the same type for consistency, or whether
the change should be scoped to the hierarchy path only.
h2. Scope / risk notes
* Behavior must remain identical (this is a refactor, not a behavior change).
* Touches long-standing framework internals many call sites branch on, plus the
{{@Deprecated}} {{getMessage}}/{{findMessage}} retained for descendant classes
— review the ripple carefully.
* Purely a readability/maintainability gain; the branch itself does not
disappear, it becomes explicit ({{result.isFound()}} instead of {{msg !=
null}}).
h2. Acceptance
* No public API signature changes.
* Existing {{StrutsLocalizedTextProviderTest}} (and related i18n tests) stay
green with no behavior change.
* {{null}} is no longer used to mean "found but continue searching" anywhere in
the resolution path.
was:
h2. Summary
Replace the {{null}}-overloaded control flow in
{{AbstractLocalizedTextProvider}} / {{StrutsLocalizedTextProvider}} message
resolution with an explicit result type, so that "not found" and "found but the
value degraded to the literal {{"null"}}" are represented distinctly instead of
both collapsing to a Java {{null}} that callers must branch on.
h2. Background
Message lookup uses {{null}} as an overloaded signal:
* {{formatWithNullDetection}} returns {{null}} when a formatted message equals
the literal string {{"null"}} (e.g. a {{ {0} }} pattern rendered with a
{{null}} argument).
* Callers ({{findText}}'s tiers, the deprecated {{findMessage}}) treat that
{{null}} identically to a genuine "key not found" and fall through to the next
source: {{if (msg != null) return msg;}}.
Both cases legitimately mean "keep searching", so the overloading is not a bug
— but it is a readability/robustness wart. The codebase already has a wrapper
for the analogous default-message path — {{GetDefaultMessageReturnArg}}
(holding {{String message}} and {{boolean foundInBundle}}). Extending a similar
explicit result to the hierarchy path would make the intent self-documenting
and consistent.
Context: this was surfaced while implementing WW-5540 (hierarchy-traversal
caching). WW-5540 deliberately kept the existing {{null}} convention and only
added a cache-boundary marker ({{NOT_FOUND}} sentinel + {{isNotFound(...)}})
plus a fall-through mitigation, to stay behavior-preserving and focused. This
ticket is the orthogonal control-flow cleanup that WW-5540 deferred.
h2. Proposed change
Introduce an internal result type (e.g. {{sealed}}/enum-tagged: {{Found(String
value)}} vs {{ContinueSearch}}) used by the raw-resolution + formatting path,
unwrapped to {{String}}/{{null}} at the public {{findText}} boundary (the
public method signatures return {{String}} and must not change). Consider
whether {{formatWithNullDetection}}, {{getMessage}}, {{findDefaultText}}, and
{{getDefaultMessage}} should adopt the same type for consistency, or whether
the change should be scoped to the hierarchy path only.
h2. Scope / risk notes
* Behavior must remain identical (this is a refactor, not a behavior change).
* Touches long-standing framework internals many call sites branch on, plus the
{{@Deprecated}} {{getMessage}}/{{findMessage}} retained for descendant classes
— review the ripple carefully.
* Purely a readability/maintainability gain; the branch itself does not
disappear, it becomes explicit ({{result.isFound()}} instead of {{msg !=
null}}).
h2. Acceptance
* No public API signature changes.
* Existing {{StrutsLocalizedTextProviderTest}} (and related i18n tests) stay
green with no behavior change.
* {{null}} is no longer used to mean "found but continue searching" anywhere in
the resolution path.
> Replace null-overloaded control flow in LocalizedTextProvider with an
> explicit result type
> ------------------------------------------------------------------------------------------
>
> Key: WW-5655
> URL: https://issues.apache.org/jira/browse/WW-5655
> Project: Struts 2
> Issue Type: Improvement
> Components: Core
> Reporter: Lukasz Lenart
> Priority: Major
> Fix For: 8.0.0
>
>
> h2. Summary
> Replace the {{null}}-overloaded control flow in
> {{AbstractLocalizedTextProvider}} / {{StrutsLocalizedTextProvider}} message
> resolution with an explicit result type, so that "not found" and "found but
> the value degraded to the literal {{"null"}}" are represented distinctly
> instead of both collapsing to a Java {{null}} that callers must branch on.
> h2. Background
> Message lookup uses {{null}} as an overloaded signal:
> * {{formatWithNullDetection}} returns {{null}} when a formatted message
> equals the literal string {{"null"}} (e.g. a {{ {0\} }} pattern rendered with
> a {{null}} argument).
> * Callers ({{findText}}'s tiers, the deprecated {{findMessage}}) treat that
> {{null}} identically to a genuine "key not found" and fall through to the
> next source: {{if (msg != null) return msg;}}.
> Both cases legitimately mean "keep searching", so the overloading is not a
> bug — but it is a readability/robustness wart. The codebase already has a
> wrapper for the analogous default-message path —
> {{GetDefaultMessageReturnArg}} (holding {{String message}} and {{boolean
> foundInBundle}}). Extending a similar explicit result to the hierarchy path
> would make the intent self-documenting and consistent.
> Context: this was surfaced while implementing WW-5540 (hierarchy-traversal
> caching). WW-5540 deliberately kept the existing {{null}} convention and only
> added a cache-boundary marker ({{NOT_FOUND}} sentinel + {{isNotFound(...)}})
> plus a fall-through mitigation, to stay behavior-preserving and focused. This
> ticket is the orthogonal control-flow cleanup that WW-5540 deferred.
> h2. Proposed change
> Introduce an internal result type (e.g. {{sealed}}/enum-tagged:
> {{Found(String value)}} vs {{ContinueSearch}}) used by the raw-resolution +
> formatting path, unwrapped to {{String}}/{{null}} at the public {{findText}}
> boundary (the public method signatures return {{String}} and must not
> change). Consider whether {{formatWithNullDetection}}, {{getMessage}},
> {{findDefaultText}}, and {{getDefaultMessage}} should adopt the same type for
> consistency, or whether the change should be scoped to the hierarchy path
> only.
> h2. Scope / risk notes
> * Behavior must remain identical (this is a refactor, not a behavior change).
> * Touches long-standing framework internals many call sites branch on, plus
> the {{@Deprecated}} {{getMessage}}/{{findMessage}} retained for descendant
> classes — review the ripple carefully.
> * Purely a readability/maintainability gain; the branch itself does not
> disappear, it becomes explicit ({{result.isFound()}} instead of {{msg !=
> null}}).
> h2. Acceptance
> * No public API signature changes.
> * Existing {{StrutsLocalizedTextProviderTest}} (and related i18n tests) stay
> green with no behavior change.
> * {{null}} is no longer used to mean "found but continue searching" anywhere
> in the resolution path.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
