This is an automated email from the ASF dual-hosted git repository. lukaszlenart pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/struts.git
commit 462670a13824cf91be5e075f91d961aca4e9bdd1 Author: Lukasz Lenart <[email protected]> AuthorDate: Fri Oct 23 07:36:11 2026 +0200 WW-5790 fix(rest): copy only what the body handler changed in the two-phase copy Co-Authored-By: Claude Mythos 5.1 <[email protected]> --- .../struts2/rest/ContentTypeInterceptor.java | 28 +++-- .../ContentTypeInterceptorIntegrationTest.java | 135 +++++++++++++++++++++ 2 files changed, 152 insertions(+), 11 deletions(-) diff --git a/plugins/rest/src/main/java/org/apache/struts2/rest/ContentTypeInterceptor.java b/plugins/rest/src/main/java/org/apache/struts2/rest/ContentTypeInterceptor.java index 8096152f2..4e51a6e2a 100644 --- a/plugins/rest/src/main/java/org/apache/struts2/rest/ContentTypeInterceptor.java +++ b/plugins/rest/src/main/java/org/apache/struts2/rest/ContentTypeInterceptor.java @@ -49,6 +49,7 @@ import java.util.LinkedHashMap; import java.util.LinkedHashSet; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Set; import java.util.SortedMap; import java.util.SortedSet; @@ -196,7 +197,8 @@ public class ContentTypeInterceptor extends AbstractInterceptor { return; } handler.toObject(invocation, reader, freshInstance); - copyAuthorizedProperties(freshInstance, target, invocation.getAction(), target, ""); + Object baseline = createFreshInstance(target.getClass()); + copyAuthorizedProperties(freshInstance, baseline, target, invocation.getAction(), target, ""); } private Object createFreshInstance(Class<?> clazz) { @@ -215,9 +217,12 @@ public class ContentTypeInterceptor extends AbstractInterceptor { * <p>{@code authTarget} is always the root action/model passed unchanged through all levels. * {@code isAuthorized} uses the full dot/bracket path against the root class, so the root * target must be used — not the nested object being visited at the current recursion depth. + * + * <p>{@code baseline} is an untouched fresh instance of the same class as {@code source}, or null when + * none could be built; with a baseline, a property the handler left at its constructor value is not copied. */ private void copyAuthorizedProperties( - Object source, Object target, Object action, Object authTarget, String prefix) throws Exception { + Object source, Object baseline, Object target, Object action, Object authTarget, String prefix) throws Exception { BeanInfo beanInfo = Introspector.getBeanInfo(source.getClass(), Object.class); for (PropertyDescriptor pd : beanInfo.getPropertyDescriptors()) { Method readMethod = pd.getReadMethod(); @@ -236,16 +241,17 @@ public class ContentTypeInterceptor extends AbstractInterceptor { } Object sourceValue = readMethod.invoke(source); - 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. + Object baselineValue = baseline == null ? null : readMethod.invoke(baseline); + // A null, or a value 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, or a body value equal to the constructor default, is not applied either. + if (sourceValue == null || (baseline != null && Objects.deepEquals(sourceValue, baselineValue))) { continue; } if (isNestedBeanType(sourceValue.getClass())) { // Complex bean: recurse to authorize nested fields, passing authTarget unchanged + Object nestedBaseline = baselineValue != null ? baselineValue : createFreshInstance(sourceValue.getClass()); Object targetValue = readMethod.invoke(target); if (targetValue == null) { Object newTarget = createFreshInstance(sourceValue.getClass()); @@ -262,7 +268,7 @@ public class ContentTypeInterceptor extends AbstractInterceptor { continue; } } - copyAuthorizedProperties(sourceValue, targetValue, action, authTarget, fullPath); + copyAuthorizedProperties(sourceValue, nestedBaseline, targetValue, action, authTarget, fullPath); } else if (sourceValue instanceof Collection) { writeMethod.invoke(target, deepCopyAuthorizedCollection((Collection<?>) sourceValue, fullPath, authTarget, action)); @@ -305,7 +311,7 @@ public class ContentTypeInterceptor extends AbstractInterceptor { } Object newElement = createFreshInstance(element.getClass()); if (newElement != null) { - copyAuthorizedProperties(element, newElement, action, authTarget, elementPath); + copyAuthorizedProperties(element, null, newElement, action, authTarget, elementPath); result.add(newElement); } else { // No no-arg constructor: skip element rather than copy an unfiltered object graph @@ -344,7 +350,7 @@ public class ContentTypeInterceptor extends AbstractInterceptor { } Object newValue = createFreshInstance(value.getClass()); if (newValue != null) { - copyAuthorizedProperties(value, newValue, action, authTarget, valuePath); + copyAuthorizedProperties(value, null, newValue, action, authTarget, valuePath); result.put(entry.getKey(), newValue); } else { LOG.warn("REST map value [{}] skipped — no no-arg constructor for [{}]", @@ -376,7 +382,7 @@ public class ContentTypeInterceptor extends AbstractInterceptor { } Object newElement = createFreshInstance(element.getClass()); if (newElement != null) { - copyAuthorizedProperties(element, newElement, action, authTarget, elementPath); + copyAuthorizedProperties(element, null, newElement, action, authTarget, elementPath); Array.set(result, i, newElement); } else { LOG.warn("REST array element [{}] skipped — no no-arg constructor for [{}]", diff --git a/plugins/rest/src/test/java/org/apache/struts2/rest/ContentTypeInterceptorIntegrationTest.java b/plugins/rest/src/test/java/org/apache/struts2/rest/ContentTypeInterceptorIntegrationTest.java index 68a875e61..67d381283 100644 --- a/plugins/rest/src/test/java/org/apache/struts2/rest/ContentTypeInterceptorIntegrationTest.java +++ b/plugins/rest/src/test/java/org/apache/struts2/rest/ContentTypeInterceptorIntegrationTest.java @@ -49,8 +49,11 @@ import org.springframework.mock.web.MockHttpServletRequest; import java.io.Reader; import java.io.Writer; +import java.util.ArrayList; import java.util.LinkedHashMap; +import java.util.List; import java.util.Map; +import java.util.function.Consumer; import static org.apache.struts2.ognl.OgnlCacheFactory.CacheType.LRU; @@ -201,6 +204,66 @@ public class ContentTypeInterceptorIntegrationTest extends TestCase { "bound-by-params", model.getStatus()); } + // --- WW-5790: the two-phase copy copies only what the handler changed --- + + public void testTwoPhaseCopyKeepsAnOmittedScalarDefault() throws Exception { + DefaultedModel model = runScriptedBody(m -> m.setStatus("approved"), m -> m.setName("alice")); + + assertEquals("approved", model.getStatus()); + assertEquals("alice", model.getName()); + } + + public void testTwoPhaseCopyAppliesAScalarTheBodySets() throws Exception { + DefaultedModel model = runScriptedBody(m -> m.setStatus("approved"), m -> m.setStatus("rejected")); + + assertEquals("rejected", model.getStatus()); + } + + public void testTwoPhaseCopyKeepsAnOmittedCollectionDefault() throws Exception { + DefaultedModel model = runScriptedBody(m -> m.setTags(new ArrayList<>(List.of("kept"))), m -> m.setName("alice")); + + assertEquals(List.of("kept"), model.getTags()); + } + + public void testTwoPhaseCopyKeepsAnOmittedNestedDefault() throws Exception { + DefaultedModel model = runScriptedBody(m -> m.getAddress().setCountry("DE"), m -> m.setName("alice")); + + assertEquals("DE", model.getAddress().getCountry()); + } + + public void testTwoPhaseCopyAppliesOnlyTheNestedLeafTheBodySets() throws Exception { + DefaultedModel model = runScriptedBody(m -> m.getAddress().setCountry("DE"), m -> m.getAddress().setCity("Warsaw")); + + assertEquals("Warsaw", model.getAddress().getCity()); + assertEquals("DE", model.getAddress().getCountry()); + } + + public void testTwoPhaseCopyWithoutNestedNoArgConstructorStillAppliesTheSetLeaf() throws Exception { + DefaultedModel model = runScriptedBody(m -> { + m.setPinned(new PinnedAddress("Gdansk")); + m.getPinned().setCountry("DE"); + }, m -> m.setPinned(new PinnedAddress("Warsaw"))); + + assertEquals("Warsaw", model.getPinned().getCity()); + assertEquals("fallback keeps today's copy-non-null rule for this subtree", "PL", model.getPinned().getCountry()); + } + + public void testTwoPhaseCopyDoesNotApplyABodyValueEqualToTheConstructorDefault() throws Exception { + DefaultedModel model = runScriptedBody(m -> m.setStatus("approved"), m -> m.setStatus("new")); + + assertEquals("documented limit of the baseline comparison", "approved", model.getStatus()); + } + + private DefaultedModel runScriptedBody(Consumer<DefaultedModel> alreadyOnTheModel, Consumer<DefaultedModel> body) throws Exception { + ModelDrivenRestAction modelDriven = new ModelDrivenRestAction(); + DefaultedModel model = modelDriven.getModel(); + alreadyOnTheModel.accept(model); + setupInterceptorWithHandler(modelDriven, new ScriptedHandler(body)); + + runWithBody("ignored", "application/x-scripted"); + return model; + } + // --- Tests proving the new Jackson authorization path is in use --- public void testJacksonHandlerDoesNotRequireNoArgConstructor() throws Exception { @@ -421,10 +484,82 @@ public class ContentTypeInterceptorIntegrationTest extends TestCase { */ 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; public String getStatus() { return status; } 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; } + } + + /** + * 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; } + + @Override + public String getContentType() { return "application/x-scripted"; } + + @Override + public String getExtension() { return null; } } public static class ModelDrivenRestAction extends ActionSupport implements ModelDriven<DefaultedModel> {
