[
https://issues.apache.org/jira/browse/WW-5790?focusedWorklogId=1049555&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1049555
]
ASF GitHub Bot logged work on WW-5790:
--------------------------------------
Author: ASF GitHub Bot
Created on: 23/Oct/26 06:26
Start Date: 23/Oct/26 06:26
Worklog Time Spent: 10m
Work Description: lukaszlenart commented on code in PR #2051:
URL: https://github.com/apache/struts/pull/2051#discussion_r4356758699
##########
plugins/rest/src/main/java/org/apache/struts2/rest/ContentTypeInterceptor.java:
##########
@@ -236,15 +241,21 @@ private void copyAuthorizedProperties(
}
Object sourceValue = readMethod.invoke(source);
+ Object baselineValue = baseline == null ? null :
readMethod.invoke(baseline);
+ // A null, or a leaf or collection equal to the baseline's
constructor default, reads as "not in
+ // the body": copying it would overwrite what params or prepare()
put on the target. The cost is
+ // that an explicit null, a primitive at its zero value, an empty
collection, or any body value
+ // equal to the constructor default is not applied either. A
nested bean is compared leaf by leaf
+ // below, never through its own equals.
Review Comment:
Reworded in 022f1373c to "non-nested value (leaf, collection, map, array)".
##########
plugins/rest/src/main/java/org/apache/struts2/rest/ContentTypeInterceptor.java:
##########
@@ -236,15 +241,21 @@ private void copyAuthorizedProperties(
}
Object sourceValue = readMethod.invoke(source);
+ Object baselineValue = baseline == null ? null :
readMethod.invoke(baseline);
+ // A null, or a leaf or collection equal to the baseline's
constructor default, reads as "not in
+ // the body": copying it would overwrite what params or prepare()
put on the target. The cost is
+ // that an explicit null, a primitive at its zero value, an empty
collection, or any body value
+ // equal to the constructor default is not applied either. A
nested bean is compared leaf by leaf
+ // below, never through its own equals.
if (sourceValue == null) {
- // Intentionally skip null values: in two-phase
deserialization, properties NOT present in the
- // request body will be null in the fresh instance. Copying
null would clear pre-initialized
- // fields on the target. This is the safer default — an
explicit JSON null and a missing field
- // are indistinguishable after deserialization into a fresh
POJO.
+ continue;
+ }
Review Comment:
Same as the thread above: fixed in 022f1373c.
Issue Time Tracking
-------------------
Worklog Id: (was: 1049555)
Time Spent: 1h 10m (was: 1h)
> REST two-phase body copy writes constructor defaults over values already set
> on the target
> ------------------------------------------------------------------------------------------
>
> Key: WW-5790
> URL: https://issues.apache.org/jira/browse/WW-5790
> Project: Struts 2
> Issue Type: Bug
> Components: Plugin - REST
> Reporter: Lukasz Lenart
> Assignee: Lukasz Lenart
> Priority: Major
> Fix For: 7.5.0
>
> Time Spent: 1h 10m
> Remaining Estimate: 0h
>
> Follow-up to [WW-5789|https://issues.apache.org/jira/browse/WW-5789], which
> covered the handlers that do not read the body at all.
> When {{struts.parameters.requireAnnotations=true}} and the selected
> content-type handler does not implement
> {{AuthorizationAwareContentTypeHandler}},
> {{ContentTypeInterceptor.applyTwoPhaseDeserialize}} creates a fresh instance
> of the target with its no-arg constructor, lets the handler fill it from the
> body, and then copies every non-null property from that instance onto the
> real target. The copy cannot tell a value the body supplied from a value the
> constructor assigned, so a property the body omits is still copied when it
> has a non-null constructor default. That overwrites whatever was on the
> target before the {{rest}} interceptor ran: a value bound by {{params}}, or
> one loaded in {{prepare()}}.
> The same happens in three shapes: a scalar with a constructor default, a
> collection initialised in the constructor (an empty list replaces a populated
> one on the target), and a nested bean initialised in the constructor (the
> copy recurses into it and resets the target's nested leaves).
> Affected handlers: {{XStreamHandler}} (shipped, registered by the
> application) and any application-provided handler that does not declare the
> marker. The handlers shipped as default ({{JacksonJsonHandler}},
> {{JacksonXmlHandler}}, {{JuneauXmlHandler}}) are not affected, they authorize
> during deserialization. The JSON plugin uses a different mechanism and is not
> affected.
> Example: a ModelDriven model declares {{private String status = "new";}}. The
> request carries {{status=approved}} as a query parameter and an XML body that
> only contains {{name}}. After the stack runs, {{status}} is {{new}}.
> h2. Design
> Fix the copy, not the handlers. The affected handlers are opt-in or external
> and cannot report which properties they set, so the interceptor has to work
> it out on its own.
> * Build a second, untouched fresh instance of the target as a baseline next
> to the one the handler fills.
> * Copy a property only when its filled value differs from the baseline value,
> compared with {{Objects.deepEquals}} so arrays compare by content. A nested
> bean the handler instantiated compares by identity and therefore counts as
> changed, which is what is wanted.
> * Thread the baseline through the nested-bean recursion. When the baseline's
> nested value is null but the filled one is not, use a fresh instance of the
> nested class as the baseline at that level. When that class has no no-arg
> constructor, fall back to today's behaviour for that subtree.
> * Collections, maps and arrays are compared against the baseline at the
> property level only. Their elements are already rebuilt as new objects, so
> skipping defaults inside them would change nothing.
> * No public API change: {{ContentTypeHandler}},
> {{AuthorizationAwareContentTypeHandler}} and the public surface of
> {{ContentTypeInterceptor}} stay as they are. No new configuration constant: a
> switch whose old setting is the defect has no sensible default.
> * The existing null-skip rule stays. Document the new default-skip rule next
> to it, in one place.
> Known limits, to be stated in the docs:
> * A body value equal to the constructor default, sent while the target holds
> a different value, is no longer applied. This mirrors the null ambiguity
> accepted in WW-5624.
> * Non-deterministic defaults such as {{UUID.randomUUID()}} or
> {{Instant.now()}} always differ from the baseline and keep being copied, as
> today.
> * The target's constructor runs twice per request on this path.
> Rejected: deserializing into a deep copy of the target. It would remove the
> ambiguity entirely, but needs a reflective deep copy of arbitrary object
> graphs, and a shallow copy would let the handler mutate the target's nested
> objects before authorization.
> Behaviour changes only when all three hold: {{requireAnnotations=true}}, a
> non-aware handler, and a property with a non-null constructor default.
> Applications on the default handlers see nothing.
> Tests to add in {{ContentTypeInterceptorIntegrationTest}}, with a non-aware
> handler that sets some properties and leaves others: omitted scalar default
> kept; set scalar applied; omitted collection default kept; omitted nested
> default kept; partially set nested bean applies only the set leaf; the
> documented limit pinned by name.
> Docs: a paragraph under the 7.2.0 enforcement note on the REST plugin page,
> and a line in the Version Notes.
> Out of scope, candidates for their own tickets: making {{XStreamHandler}}
> authorization-aware so no shipped handler is left on the two-phase path, and
> deprecating the two-phase path for removal at 8.0.0 once that is done.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
