This is an automated email from the ASF dual-hosted git repository. lukaszlenart pushed a commit to branch WW-5659-lazy-params-request-scoping in repository https://gitbox.apache.org/repos/asf/struts.git
commit af6e5fb81809dedacf29f7f05c9a8690c779627c Author: Lukasz Lenart <[email protected]> AuthorDate: Mon Jul 27 10:42:20 2026 +0200 WW-5659 refactor(core): hold upload policy in one value object Introduce UploadPolicy (extends DisableParams) to consolidate the three loose maximumSize/allowedTypes/allowedExtensions fields on AbstractFileUploadInterceptor into a single config-time value object. acceptFile now takes the effective policy as an explicit parameter instead of reading interceptor-level state directly. Pure refactor, no behaviour change: the existing setters still mutate the shared singleton via configuredPolicy, and ActionFileUploadInterceptor copies it once per invocation via copyConfiguredPolicy() before calling acceptFile. This groundwork lets a later change route lazily-resolved per-request params into the copy instead of the singleton. --- .../interceptor/AbstractFileUploadInterceptor.java | 43 ++++++---- .../interceptor/ActionFileUploadInterceptor.java | 4 +- .../apache/struts2/interceptor/UploadPolicy.java | 95 ++++++++++++++++++++++ .../ActionFileUploadInterceptorTest.java | 61 +++++++++++--- 4 files changed, 172 insertions(+), 31 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/interceptor/AbstractFileUploadInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/AbstractFileUploadInterceptor.java index c7cbb65c8..f8529732b 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/AbstractFileUploadInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/AbstractFileUploadInterceptor.java @@ -24,7 +24,6 @@ import org.apache.struts2.text.TextProvider; import org.apache.struts2.text.TextProviderFactory; import org.apache.struts2.inject.Container; import org.apache.struts2.inject.Inject; -import org.apache.struts2.util.TextParseUtil; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.apache.struts2.dispatcher.LocalizedMessage; @@ -35,7 +34,6 @@ import org.apache.struts2.util.ContentTypeMatcher; import java.text.NumberFormat; import java.util.Arrays; import java.util.Collection; -import java.util.Collections; import java.util.HashMap; import java.util.HashSet; import java.util.Set; @@ -59,9 +57,7 @@ public abstract class AbstractFileUploadInterceptor extends AbstractInterceptor public static final String STRUTS_MESSAGES_ERROR_CONTENT_TYPE_NOT_ALLOWED_KEY = "struts.messages.error.content.type.not.allowed"; public static final String STRUTS_MESSAGES_ERROR_FILE_EXTENSION_NOT_ALLOWED_KEY = "struts.messages.error.file.extension.not.allowed"; - private Long maximumSize; - private Set<String> allowedTypesSet = Collections.emptySet(); - private Set<String> allowedExtensionsSet = Collections.emptySet(); + private final UploadPolicy configuredPolicy = new UploadPolicy(); private ContentTypeMatcher<Object> matcher; private Container container; @@ -77,35 +73,48 @@ public abstract class AbstractFileUploadInterceptor extends AbstractInterceptor } /** - * Sets the allowed extensions + * Sets the allowed extensions. Applied at configuration time only; the effective policy for + * an invocation is a copy, see {@link ActionFileUploadInterceptor#newLazyParams()}. * * @param allowedExtensions A comma-delimited list of extensions */ public void setAllowedExtensions(String allowedExtensions) { - allowedExtensionsSet = TextParseUtil.commaDelimitedStringToSet(allowedExtensions); + configuredPolicy.setAllowedExtensions(allowedExtensions); } /** - * Sets the allowed mimetypes + * Sets the allowed mimetypes. Applied at configuration time only; the effective policy for + * an invocation is a copy, see {@link ActionFileUploadInterceptor#newLazyParams()}. * * @param allowedTypes A comma-delimited list of types */ public void setAllowedTypes(String allowedTypes) { - allowedTypesSet = TextParseUtil.commaDelimitedStringToSet(allowedTypes); + configuredPolicy.setAllowedTypes(allowedTypes); } /** - * Sets the maximum size of an uploaded file + * Sets the maximum size of an uploaded file. Applied at configuration time only; the + * effective policy for an invocation is a copy, see + * {@link ActionFileUploadInterceptor#newLazyParams()}. * * @param maximumSize The maximum size in bytes */ public void setMaximumSize(Long maximumSize) { - this.maximumSize = maximumSize; + configuredPolicy.setMaximumSize(maximumSize); + } + + /** + * @return an independent copy of the configured policy, to be resolved for one invocation + * @since 7.3.0 + */ + protected UploadPolicy copyConfiguredPolicy() { + return configuredPolicy.copy(); } /** * Override for added functionality. Checks if the proposed file is acceptable based on contentType and size. * + * @param policy - the effective upload policy for this invocation. * @param action - uploading action for message retrieval. * @param file - proposed upload file. * @param originalFilename - name of the file. @@ -113,7 +122,7 @@ public abstract class AbstractFileUploadInterceptor extends AbstractInterceptor * @param inputName - inputName of the file. * @return true if the proposed file is acceptable by contentType and size. */ - protected boolean acceptFile(Object action, UploadedFile file, String originalFilename, String contentType, String inputName) { + protected boolean acceptFile(UploadPolicy policy, Object action, UploadedFile file, String originalFilename, String contentType, String inputName) { Set<String> errorMessages = new HashSet<>(); ValidationAware validation = null; @@ -131,21 +140,21 @@ public abstract class AbstractFileUploadInterceptor extends AbstractInterceptor return false; } - if (maximumSize != null && maximumSize < file.length()) { + if (policy.getMaximumSize() != null && policy.getMaximumSize() < file.length()) { String errMsg = getTextMessage(action, STRUTS_MESSAGES_ERROR_FILE_TOO_LARGE_KEY, new String[]{ - inputName, originalFilename, file.getName(), "" + file.length(), getMaximumSizeStr(action) + inputName, originalFilename, file.getName(), "" + file.length(), getMaximumSizeStr(action, policy.getMaximumSize()) }); errorMessages.add(errMsg); LOG.warn(errMsg); } - if ((!allowedTypesSet.isEmpty()) && (!containsItem(allowedTypesSet, contentType))) { + if ((!policy.getAllowedTypes().isEmpty()) && (!containsItem(policy.getAllowedTypes(), contentType))) { String errMsg = getTextMessage(action, STRUTS_MESSAGES_ERROR_CONTENT_TYPE_NOT_ALLOWED_KEY, new String[]{ inputName, originalFilename, file.getName(), contentType }); errorMessages.add(errMsg); LOG.warn(errMsg); } - if ((!allowedExtensionsSet.isEmpty()) && (!hasAllowedExtension(allowedExtensionsSet, originalFilename))) { + if ((!policy.getAllowedExtensions().isEmpty()) && (!hasAllowedExtension(policy.getAllowedExtensions(), originalFilename))) { String errMsg = getTextMessage(action, STRUTS_MESSAGES_ERROR_FILE_EXTENSION_NOT_ALLOWED_KEY, new String[]{ inputName, originalFilename, file.getName(), contentType }); @@ -161,7 +170,7 @@ public abstract class AbstractFileUploadInterceptor extends AbstractInterceptor return errorMessages.isEmpty(); } - private String getMaximumSizeStr(Object action) { + private String getMaximumSizeStr(Object action, Long maximumSize) { return NumberFormat.getNumberInstance(getLocaleProvider(action).getLocale()).format(maximumSize); } diff --git a/core/src/main/java/org/apache/struts2/interceptor/ActionFileUploadInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/ActionFileUploadInterceptor.java index 79d020a34..4aea0f782 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/ActionFileUploadInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/ActionFileUploadInterceptor.java @@ -228,6 +228,8 @@ public class ActionFileUploadInterceptor extends AbstractFileUploadInterceptor i return invocation.invoke(); } + UploadPolicy policy = copyConfiguredPolicy(); + applyValidation(action, multiWrapper); // bind allowed Files @@ -245,7 +247,7 @@ public class ActionFileUploadInterceptor extends AbstractFileUploadInterceptor i } } else { for (UploadedFile uploadedFile : uploadedFiles) { - if (acceptFile(action, uploadedFile, uploadedFile.getOriginalName(), uploadedFile.getContentType(), inputName)) { + if (acceptFile(policy, action, uploadedFile, uploadedFile.getOriginalName(), uploadedFile.getContentType(), inputName)) { acceptedFiles.add(uploadedFile); } } diff --git a/core/src/main/java/org/apache/struts2/interceptor/UploadPolicy.java b/core/src/main/java/org/apache/struts2/interceptor/UploadPolicy.java new file mode 100644 index 000000000..a9e444157 --- /dev/null +++ b/core/src/main/java/org/apache/struts2/interceptor/UploadPolicy.java @@ -0,0 +1,95 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.struts2.interceptor; + +import org.apache.struts2.util.TextParseUtil; + +import java.util.Collections; +import java.util.Set; + +/** + * Per-invocation upload validation policy for {@link ActionFileUploadInterceptor}. + * <p> + * A configured instance is held by the interceptor and copied for each invocation, so lazily + * resolved values never reach the shared interceptor. + * + * @since 7.3.0 + */ +public class UploadPolicy extends DisableParams { + + private Long maximumSize; + private Set<String> allowedTypes = Collections.emptySet(); + private Set<String> allowedExtensions = Collections.emptySet(); + + public UploadPolicy() { + } + + private UploadPolicy(UploadPolicy other) { + super(other); + this.maximumSize = other.maximumSize; + this.allowedTypes = other.allowedTypes; + this.allowedExtensions = other.allowedExtensions; + } + + /** + * @param allowedTypes a comma-delimited list of content types, or null for no restriction + */ + public void setAllowedTypes(String allowedTypes) { + this.allowedTypes = toSet(allowedTypes); + } + + /** + * @param allowedExtensions a comma-delimited list of extensions, or null for no restriction + */ + public void setAllowedExtensions(String allowedExtensions) { + this.allowedExtensions = toSet(allowedExtensions); + } + + /** + * @param maximumSize the maximum size in bytes, or null for no limit + */ + public void setMaximumSize(Long maximumSize) { + this.maximumSize = maximumSize; + } + + public Long getMaximumSize() { + return maximumSize; + } + + public Set<String> getAllowedTypes() { + return allowedTypes; + } + + public Set<String> getAllowedExtensions() { + return allowedExtensions; + } + + /** + * @return an independent copy, used to seed a per-invocation policy from the configured one + */ + public UploadPolicy copy() { + return new UploadPolicy(this); + } + + private static Set<String> toSet(String commaDelimited) { + return commaDelimited == null + ? Collections.emptySet() + : TextParseUtil.commaDelimitedStringToSet(commaDelimited); + } +} diff --git a/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java b/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java index 058dabe11..c45091d77 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/ActionFileUploadInterceptorTest.java @@ -63,7 +63,7 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { public void testAcceptFileWithEmptyAllowedTypesAndExtensions() { // when allowed type is empty ValidationAwareSupport validation = new ValidationAwareSupport(); - boolean ok = interceptor.acceptFile(validation, createTestFile(Files.newTemporaryFile()), "filename", "text/plain", "inputName"); + boolean ok = interceptor.acceptFile(interceptor.copyConfiguredPolicy(), validation, createTestFile(Files.newTemporaryFile()), "filename", "text/plain", "inputName"); assertThat(ok).isTrue(); assertThat(validation.getFieldErrors()).isEmpty(); @@ -75,7 +75,7 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { // when file is of allowed types ValidationAwareSupport validation = new ValidationAwareSupport(); - boolean ok = interceptor.acceptFile(validation, createTestFile(Files.newTemporaryFile()), "filename.txt", "text/plain", "inputName"); + boolean ok = interceptor.acceptFile(interceptor.copyConfiguredPolicy(), validation, createTestFile(Files.newTemporaryFile()), "filename.txt", "text/plain", "inputName"); assertThat(ok).isTrue(); assertThat(validation.getFieldErrors()).isEmpty(); @@ -83,7 +83,7 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { // when file is not of allowed types validation = new ValidationAwareSupport(); - boolean notOk = interceptor.acceptFile(validation, createTestFile(Files.newTemporaryFile()), "filename.html", "text/html", "inputName"); + boolean notOk = interceptor.acceptFile(interceptor.copyConfiguredPolicy(), validation, createTestFile(Files.newTemporaryFile()), "filename.html", "text/html", "inputName"); assertThat(notOk).isFalse(); assertThat(validation.getFieldErrors()).isNotEmpty(); @@ -94,7 +94,7 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { interceptor.setAllowedTypes("text/*"); ValidationAwareSupport validation = new ValidationAwareSupport(); - boolean ok = interceptor.acceptFile(validation, createTestFile(Files.newTemporaryFile()), "filename.txt", "text/plain", "inputName"); + boolean ok = interceptor.acceptFile(interceptor.copyConfiguredPolicy(), validation, createTestFile(Files.newTemporaryFile()), "filename.txt", "text/plain", "inputName"); assertThat(ok).isTrue(); assertThat(validation.getFieldErrors()).isEmpty(); @@ -102,7 +102,7 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { interceptor.setAllowedTypes("text/h*"); validation = new ValidationAwareSupport(); - boolean notOk = interceptor.acceptFile(validation, createTestFile(Files.newTemporaryFile()), "filename.html", "text/plain", "inputName"); + boolean notOk = interceptor.acceptFile(interceptor.copyConfiguredPolicy(), validation, createTestFile(Files.newTemporaryFile()), "filename.html", "text/plain", "inputName"); assertThat(notOk).isFalse(); assertThat(validation.getFieldErrors()).isNotEmpty(); @@ -114,7 +114,7 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { // when file is of allowed extensions ValidationAwareSupport validation = new ValidationAwareSupport(); - boolean ok = interceptor.acceptFile(validation, createTestFile(Files.newTemporaryFile()), "filename.txt", "text/plain", "inputName"); + boolean ok = interceptor.acceptFile(interceptor.copyConfiguredPolicy(), validation, createTestFile(Files.newTemporaryFile()), "filename.txt", "text/plain", "inputName"); assertThat(ok).isTrue(); assertThat(validation.getFieldErrors()).isEmpty(); @@ -122,7 +122,7 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { // when file is not of allowed extensions validation = new ValidationAwareSupport(); - boolean notOk = interceptor.acceptFile(validation, createTestFile(Files.newTemporaryFile()), "filename.html", "text/html", "inputName"); + boolean notOk = interceptor.acceptFile(interceptor.copyConfiguredPolicy(), validation, createTestFile(Files.newTemporaryFile()), "filename.html", "text/html", "inputName"); assertThat(notOk).isFalse(); assertThat(validation.getFieldErrors()).isNotEmpty(); @@ -130,7 +130,7 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { interceptor.setAllowedExtensions(".txt,.lol"); validation = new ValidationAwareSupport(); - ok = interceptor.acceptFile(validation, createTestFile(Files.newTemporaryFile()), "filename.lol", "text/plain", "inputName"); + ok = interceptor.acceptFile(interceptor.copyConfiguredPolicy(), validation, createTestFile(Files.newTemporaryFile()), "filename.lol", "text/plain", "inputName"); assertThat(ok).isTrue(); assertThat(validation.getFieldErrors()).isEmpty(); @@ -142,7 +142,7 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { // when file is not of allowed types ValidationAwareSupport validation = new ValidationAwareSupport(); - boolean notOk = interceptor.acceptFile(validation, null, "filename.html", "text/html", "inputName"); + boolean notOk = interceptor.acceptFile(interceptor.copyConfiguredPolicy(), validation, null, "filename.html", "text/html", "inputName"); assertThat(notOk).isFalse(); assertThat(validation.getFieldErrors()).isNotEmpty(); @@ -159,7 +159,7 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { interceptor.setAllowedTypes("text/plain"); ValidationAwareSupport validation = new ValidationAwareSupport(); - boolean notOk = interceptor.acceptFile(validation, createTestFile(null), "filename.html", "text/plain", "inputName"); + boolean notOk = interceptor.acceptFile(interceptor.copyConfiguredPolicy(), validation, createTestFile(null), "filename.html", "text/plain", "inputName"); assertThat(notOk).isFalse(); assertThat(validation.getFieldErrors()).isNotEmpty(); @@ -183,7 +183,7 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { .withInputName("inputName") .build(); - boolean ok = interceptor.acceptFile(validation, file, "f.txt", "text/plain", "inputName"); + boolean ok = interceptor.acceptFile(interceptor.copyConfiguredPolicy(), validation, file, "f.txt", "text/plain", "inputName"); assertThat(ok).isTrue(); assertThat(validation.hasErrors()).isFalse(); @@ -203,7 +203,7 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { .build(); // wrong content type -> rejected - boolean ok = interceptor.acceptFile(validation, file, "f.html", "text/html", "inputName"); + boolean ok = interceptor.acceptFile(interceptor.copyConfiguredPolicy(), validation, file, "f.html", "text/html", "inputName"); assertThat(ok).isFalse(); assertThat(validation.hasErrors()).isTrue(); @@ -221,7 +221,7 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { File file = new File(new URI(url.toString())); assertThat(file).exists(); UploadedFile uploadedFile = StrutsUploadedFile.Builder.create(file).withContentType("text/html").withOriginalName("filename").build(); - boolean notOk = interceptor.acceptFile(validation, uploadedFile, "filename", "text/html", "inputName"); + boolean notOk = interceptor.acceptFile(interceptor.copyConfiguredPolicy(), validation, uploadedFile, "filename", "text/html", "inputName"); assertThat(notOk).isFalse(); assertThat(validation.getFieldErrors()).isNotEmpty(); @@ -920,4 +920,39 @@ public class ActionFileUploadInterceptorTest extends StrutsInternalTestCase { } } + public void testUploadPolicyParsesAndCopies() { + UploadPolicy policy = new UploadPolicy(); + policy.setAllowedTypes("text/plain, text/html"); + policy.setAllowedExtensions(".txt,.html"); + policy.setMaximumSize(1024L); + policy.setDisabled("true"); + + UploadPolicy copy = policy.copy(); + + assertThat(copy.getAllowedTypes()).containsExactlyInAnyOrder("text/plain", "text/html"); + assertThat(copy.getAllowedExtensions()).containsExactlyInAnyOrder(".txt", ".html"); + assertThat(copy.getMaximumSize()).isEqualTo(1024L); + assertThat(copy.isDisabled()).isTrue(); + } + + public void testUploadPolicyCopyIsIndependentOfTheOriginal() { + UploadPolicy policy = new UploadPolicy(); + policy.setAllowedTypes("text/plain"); + + UploadPolicy copy = policy.copy(); + copy.setAllowedTypes("text/html"); + + assertThat(policy.getAllowedTypes()).containsExactly("text/plain"); + assertThat(copy.getAllowedTypes()).containsExactly("text/html"); + } + + public void testUploadPolicyTreatsNullAsNoRestriction() { + UploadPolicy policy = new UploadPolicy(); + policy.setAllowedTypes(null); + policy.setAllowedExtensions(null); + + assertThat(policy.getAllowedTypes()).isEmpty(); + assertThat(policy.getAllowedExtensions()).isEmpty(); + } + }
