[
https://issues.apache.org/jira/browse/WW-5790?focusedWorklogId=1049554&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1049554
]
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_r4356758061
##########
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:
Plan files are versioned here on purpose: docs/superpowers/plans/ holds the
plan of every ticket that went through this workflow, committed in the same PR
(for example #2042 for WW-3576 and #2009 for WW-5770). Keeping it.
##########
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:
The interceptor under test never calls fromObject; that is the result side,
driven by RestActionInvocation. The existing StatePlantingHandler fixture in
this class returns null for the same reason, so this one matches it.
##########
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:
Moved below the null check in 022f1373c; the baseline getter now runs only
when a comparison or a nested baseline needs it.
Issue Time Tracking
-------------------
Worklog Id: (was: 1049554)
Time Spent: 1h (was: 50m)
> 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
> 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)
