[
https://issues.apache.org/jira/browse/WW-5790?focusedWorklogId=1049553&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1049553
]
ASF GitHub Bot logged work on WW-5790:
--------------------------------------
Author: ASF GitHub Bot
Created on: 23/Oct/26 06:22
Start Date: 23/Oct/26 06:22
Worklog Time Spent: 10m
Work Description: Copilot commented on code in PR #2051:
URL: https://github.com/apache/struts/pull/2051#discussion_r4356737137
##########
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);
Review Comment:
`baselineValue` is computed (via reflective getter invocation) even when
`sourceValue` is null and the method immediately `continue`s. You can avoid
that extra reflective call by moving the `baselineValue` computation to after
the `sourceValue == null` check (and only computing it when it’s actually
needed for comparisons / nested-baseline derivation).
##########
plugins/rest/src/test/java/org/apache/struts2/rest/ContentTypeInterceptorIntegrationTest.java:
##########
@@ -421,10 +512,124 @@ public static class MergedPropertyAction extends
ActionSupport {
*/
public static class DefaultedModel {
private String status = "new";
+ private String name;
+ private List<String> tags = new ArrayList<>();
+ private DefaultedAddress address = new DefaultedAddress();
+ private PinnedAddress pinned;
+ private IdEqualAddress billing = new IdEqualAddress();
+ private boolean enabled;
public String getStatus() { return status; }
+ public boolean isEnabled() { return enabled; }
+
+ public void setEnabled(boolean enabled) { this.enabled = enabled; }
+
+ public IdEqualAddress getBilling() { return billing; }
+
+ public void setBilling(IdEqualAddress billing) { this.billing =
billing; }
+
public void setStatus(String status) { this.status = status; }
+
+ public String getName() { return name; }
+
+ public void setName(String name) { this.name = name; }
+
+ public List<String> getTags() { return tags; }
+
+ public void setTags(List<String> tags) { this.tags = tags; }
+
+ public DefaultedAddress getAddress() { return address; }
+
+ public void setAddress(DefaultedAddress address) { this.address =
address; }
+
+ public PinnedAddress getPinned() { return pinned; }
+
+ public void setPinned(PinnedAddress pinned) { this.pinned = pinned; }
+ }
+
+ public static class DefaultedAddress {
+ private String country = "PL";
+ private String city;
+
+ public String getCountry() { return country; }
+
+ public void setCountry(String country) { this.country = country; }
+
+ public String getCity() { return city; }
+
+ public void setCity(String city) { this.city = city; }
+ }
+
+ public static class HomeAddress extends DefaultedAddress {
+ private String floor;
+
+ public String getFloor() { return floor; }
+
+ public void setFloor(String floor) { this.floor = floor; }
+ }
+
+ /**
+ * WW-5790: equals by id only, the shape of a JPA entity; a leaf-blind
equals must not hide a changed leaf.
+ */
+ public static class IdEqualAddress {
+ private Long id;
+ private String city;
+
+ public Long getId() { return id; }
+
+ public void setId(Long id) { this.id = id; }
+
+ public String getCity() { return city; }
+
+ public void setCity(String city) { this.city = city; }
+
+ @Override
+ public boolean equals(Object other) {
+ return other instanceof IdEqualAddress that &&
java.util.Objects.equals(id, that.id);
+ }
+
+ @Override
+ public int hashCode() { return java.util.Objects.hashCode(id); }
+ }
+
+ /**
+ * WW-5790: no no-arg constructor, so the copy cannot build a baseline for
this subtree.
+ */
+ public static class PinnedAddress {
+ private String country = "PL";
+ private String city;
+
+ public PinnedAddress(String city) { this.city = city; }
+
+ public String getCountry() { return country; }
+
+ public void setCountry(String country) { this.country = country; }
+
+ public String getCity() { return city; }
+
+ public void setCity(String city) { this.city = city; }
+ }
+
+ /**
+ * WW-5790: a handler without the authorization-aware marker; what it sets
stands in for the body.
+ */
+ public static class ScriptedHandler implements ContentTypeHandler {
+ private final Consumer<DefaultedModel> body;
+
+ ScriptedHandler(Consumer<DefaultedModel> body) { this.body = body; }
+
+ @Override
+ public void toObject(ActionInvocation invocation, Reader in, Object
target) { body.accept((DefaultedModel) target); }
+
+ @Override
+ public String fromObject(ActionInvocation invocation, Object obj,
String resultCode, Writer stream) { return null; }
Review Comment:
Returning null from `ContentTypeHandler#fromObject` can cause fragile tests
(and potential NPEs) if the execution path ever calls serialization/result-code
logic. Prefer returning `resultCode` (pass-through) or a concrete result code
(e.g., `Action.SUCCESS`) to keep the handler behavior well-defined even when
response generation is exercised.
##########
docs/superpowers/plans/2026-10-23-WW-5790-two-phase-copy-baseline.md:
##########
@@ -0,0 +1,235 @@
+# WW-5790 Two-Phase Copy Baseline Implementation Plan
Review Comment:
This PR introduces a large internal implementation plan document under
`docs/superpowers/plans/`. The PR description focuses on the functional fix +
tests + an external struts-site docs PR, but doesn’t mention adding this
planning artifact. If this repository doesn’t intend to version internal agent
execution plans, consider dropping this file from the PR (or relocating it to
the appropriate contributor/process location if such docs are expected here).
##########
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:
`baselineValue` is computed (via reflective getter invocation) even when
`sourceValue` is null and the method immediately `continue`s. You can avoid
that extra reflective call by moving the `baselineValue` computation to after
the `sourceValue == null` check (and only computing it when it’s actually
needed for comparisons / nested-baseline derivation).
##########
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:
The skip-rule comment says \"leaf or collection\" but the
`Objects.deepEquals` check is also applied to maps and arrays (both are
non-nested types and will flow through this branch). To keep the comment
accurate, please update it to explicitly include maps/arrays (or refer more
generally to \"non-nested values\", including collections/maps/arrays).
Issue Time Tracking
-------------------
Worklog Id: (was: 1049553)
Time Spent: 50m (was: 40m)
> 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: 50m
> 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)
