[
https://issues.apache.org/jira/browse/WW-4164?focusedWorklogId=1051817&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1051817
]
ASF GitHub Bot logged work on WW-4164:
--------------------------------------
Author: ASF GitHub Bot
Created on: 02/Nov/26 08:17
Start Date: 02/Nov/26 08:17
Worklog Time Spent: 10m
Work Description: Copilot commented on code in PR #2093:
URL: https://github.com/apache/struts/pull/2093#discussion_r4440147062
##########
core/src/main/java/org/apache/struts2/dispatcher/mapper/ActionExtensionResolver.java:
##########
@@ -0,0 +1,122 @@
+/*
+ * 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.dispatcher.mapper;
+
+import org.apache.commons.lang3.StringUtils;
+import org.apache.struts2.ActionContext;
+import org.apache.struts2.StrutsConstants;
+import org.apache.struts2.config.Configuration;
+import org.apache.struts2.config.RuntimeConfiguration;
+import org.apache.struts2.config.entities.ActionConfig;
+import org.apache.struts2.inject.Inject;
+
+import java.util.List;
+
+/**
+ * Chooses the extension of a generated action URL from the extensions the
target action accepts, so links,
+ * forms and redirects point at an extension the target can be reached by.
+ *
+ * @since 7.5.0
+ */
+public class ActionExtensionResolver {
+
+ private Configuration configuration;
+ private List<String> extensions;
+ private volatile DeclarationState declarationState;
+
+ /**
+ * @param configuration the configuration used to look up the target action
+ * @since 7.5.0
+ */
+ @Inject
+ public void setConfiguration(Configuration configuration) {
+ this.configuration = configuration;
+ }
+
+ /**
+ * @param extensions the global {@code struts.action.extension} list
+ * @since 7.5.0
+ */
+ @Inject(StrutsConstants.STRUTS_ACTION_EXTENSION)
+ public void setExtensions(String extensions) {
+ this.extensions = StringUtils.isNotEmpty(extensions) ?
ActionExtensions.parse(extensions) : null;
+ }
+
+ /**
+ * Sets the extension of {@code target} to the current request's extension
when the target accepts it, otherwise
+ * to the target's first accepted extension, which is also used when the
current request has no extension, as in
+ * {@link DefaultActionMapper#lookupExtension(String)}. Leaves {@code
target} unchanged when it already has an extension,
+ * when no action in the configuration declares extensions, when the
target action is unknown (a name reached only
+ * through the namespace's {@code default-action-ref} counts as unknown),
or when neither the
+ * target nor the global constant declares extensions.
+ *
+ * @param target the mapping of the action the URL points at
+ * @since 7.5.0
+ */
+ public void applyTo(ActionMapping target) {
+ if (target.getExtension() != null) {
+ return;
+ }
+ RuntimeConfiguration runtimeConfiguration =
configuration.getRuntimeConfiguration();
+ if (!anyActionDeclaresExtensions(runtimeConfiguration)) {
+ return;
+ }
+ String actionName =
StringUtils.substringBefore(StringUtils.substringBefore(target.getName(), "?"),
"!");
Review Comment:
With dynamic method invocation disabled, this lookup truncates the literal
action name `report!daily` to `report`, selecting the wrong extension list or
leaving the extension unset. For example, if `report!daily` allows only `html`
and `report` allows only `json`, a link generated from an `.action` request
becomes `/report!daily.json` and is rejected by the target. After removing the
query string, use the configured mapper's action-name parsing rules and add a
literal-bang regression test.
##########
core/src/main/java/org/apache/struts2/dispatcher/mapper/DefaultActionMapper.java:
##########
@@ -476,6 +474,92 @@ protected void extractMethodName(ActionMapping mapping,
ConfigurationManager con
mapping.setMethod(methodName);
}
+ /**
+ * Strips an extension that only package or action {@code extension}
attributes declare, but only when the
+ * remaining URI matches an action (exactly or by wildcard, never through
a {@code default-action-ref}) that itself
+ * declares extensions. When only those attributes declare the empty
extension, an extensionless URI matching such
+ * an action is returned as is.
+ *
+ * @param uri the request URI
+ * @param mapping the action mapping to store the extension in
+ * @param configManager configuration manager
+ * @return the URI without the extension, or {@code null} when no
declared-only extension claims it
+ * @since 7.5.0
+ */
+ protected String stripDeclaredExtension(String uri, ActionMapping mapping,
ConfigurationManager configManager) {
+ RuntimeConfiguration runtimeConfiguration =
configManager.getConfiguration().getRuntimeConfiguration();
+ if (runtimeConfiguration == null) {
+ return null;
+ }
+ DeclaredExtensions declared = declaredExtensions(runtimeConfiguration);
+ for (String ext : declared.declaredOnly) {
+ String suffix = "." + ext;
+ if (uri.endsWith(suffix)) {
+ String candidate = uri.substring(0, uri.length() -
suffix.length());
+ if (declaresExtensions(candidate, runtimeConfiguration,
configManager)) {
+ mapping.setExtension(ext);
+ return candidate;
+ }
+ }
+ }
+ if (declared.emptyDeclaredOnly && hasNoExtension(uri) &&
declaresExtensions(uri, runtimeConfiguration, configManager)) {
+ return uri;
+ }
+ return null;
+ }
+
+ private boolean declaresExtensions(String candidate, RuntimeConfiguration
runtimeConfiguration, ConfigurationManager configManager) {
+ ActionMapping probe = new ActionMapping();
+ parseNameAndNamespace(candidate, probe, configManager);
Review Comment:
With `index` declaring `html` and global extensions `action,,`,
`/manual(1).html` is claimed and executes `index` instead of falling through to
the container as before. `parseNameAndNamespace` replaces the unmatched name
with `index` through `cleanupActionName`, so the probe mistakes a cleanup
fallback for a matching action. Parse the declaration probe without
substituting fallback names, while retaining cleanup behavior for normal
mapping.
##########
core/src/main/java/org/apache/struts2/dispatcher/mapper/ActionExtensionResolver.java:
##########
@@ -0,0 +1,122 @@
+/*
+ * 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.dispatcher.mapper;
+
+import org.apache.commons.lang3.StringUtils;
+import org.apache.struts2.ActionContext;
+import org.apache.struts2.StrutsConstants;
+import org.apache.struts2.config.Configuration;
+import org.apache.struts2.config.RuntimeConfiguration;
+import org.apache.struts2.config.entities.ActionConfig;
+import org.apache.struts2.inject.Inject;
+
+import java.util.List;
+
+/**
+ * Chooses the extension of a generated action URL from the extensions the
target action accepts, so links,
+ * forms and redirects point at an extension the target can be reached by.
+ *
+ * @since 7.5.0
+ */
+public class ActionExtensionResolver {
+
+ private Configuration configuration;
+ private List<String> extensions;
+ private volatile DeclarationState declarationState;
+
+ /**
+ * @param configuration the configuration used to look up the target action
+ * @since 7.5.0
+ */
+ @Inject
+ public void setConfiguration(Configuration configuration) {
+ this.configuration = configuration;
+ }
+
+ /**
+ * @param extensions the global {@code struts.action.extension} list
+ * @since 7.5.0
+ */
+ @Inject(StrutsConstants.STRUTS_ACTION_EXTENSION)
+ public void setExtensions(String extensions) {
+ this.extensions = StringUtils.isNotEmpty(extensions) ?
ActionExtensions.parse(extensions) : null;
+ }
+
+ /**
+ * Sets the extension of {@code target} to the current request's extension
when the target accepts it, otherwise
+ * to the target's first accepted extension, which is also used when the
current request has no extension, as in
+ * {@link DefaultActionMapper#lookupExtension(String)}. Leaves {@code
target} unchanged when it already has an extension,
+ * when no action in the configuration declares extensions, when the
target action is unknown (a name reached only
+ * through the namespace's {@code default-action-ref} counts as unknown),
or when neither the
+ * target nor the global constant declares extensions.
+ *
+ * @param target the mapping of the action the URL points at
+ * @since 7.5.0
+ */
+ public void applyTo(ActionMapping target) {
+ if (target.getExtension() != null) {
+ return;
+ }
+ RuntimeConfiguration runtimeConfiguration =
configuration.getRuntimeConfiguration();
+ if (!anyActionDeclaresExtensions(runtimeConfiguration)) {
+ return;
+ }
+ String actionName =
StringUtils.substringBefore(StringUtils.substringBefore(target.getName(), "?"),
"!");
+ ActionConfig actionConfig =
runtimeConfiguration.getMatchedActionConfig(target.getNamespace(), actionName);
+ if (actionConfig == null) {
+ return;
+ }
+ List<String> effective = actionConfig.getExtensions() != null ?
actionConfig.getExtensions() : extensions;
+ if (effective == null) {
+ return;
+ }
+ String current = currentExtension();
+ target.setExtension(current == null ? effective.get(0) :
ActionExtensions.select(effective, current));
Review Comment:
From an extensionless request such as `/page`, a target declaring
`extension="html,"` receives `.html` even though it allows the current empty
extension. A null extension on an existing mapping means an extensionless URL,
so this violates the promised rule of keeping an allowed current extension.
Distinguish an absent mapping from an extensionless request for declared
targets, while preserving legacy defaults for undeclared targets.
##########
core/src/main/java/org/apache/struts2/dispatcher/mapper/DefaultActionMapper.java:
##########
@@ -476,6 +474,92 @@ protected void extractMethodName(ActionMapping mapping,
ConfigurationManager con
mapping.setMethod(methodName);
}
+ /**
+ * Strips an extension that only package or action {@code extension}
attributes declare, but only when the
+ * remaining URI matches an action (exactly or by wildcard, never through
a {@code default-action-ref}) that itself
+ * declares extensions. When only those attributes declare the empty
extension, an extensionless URI matching such
+ * an action is returned as is.
+ *
+ * @param uri the request URI
+ * @param mapping the action mapping to store the extension in
+ * @param configManager configuration manager
+ * @return the URI without the extension, or {@code null} when no
declared-only extension claims it
+ * @since 7.5.0
+ */
+ protected String stripDeclaredExtension(String uri, ActionMapping mapping,
ConfigurationManager configManager) {
+ RuntimeConfiguration runtimeConfiguration =
configManager.getConfiguration().getRuntimeConfiguration();
+ if (runtimeConfiguration == null) {
+ return null;
+ }
+ DeclaredExtensions declared = declaredExtensions(runtimeConfiguration);
+ for (String ext : declared.declaredOnly) {
+ String suffix = "." + ext;
+ if (uri.endsWith(suffix)) {
+ String candidate = uri.substring(0, uri.length() -
suffix.length());
+ if (declaresExtensions(candidate, runtimeConfiguration,
configManager)) {
+ mapping.setExtension(ext);
+ return candidate;
+ }
+ }
+ }
+ if (declared.emptyDeclaredOnly && hasNoExtension(uri) &&
declaresExtensions(uri, runtimeConfiguration, configManager)) {
+ return uri;
+ }
+ return null;
+ }
+
+ private boolean declaresExtensions(String candidate, RuntimeConfiguration
runtimeConfiguration, ConfigurationManager configManager) {
+ ActionMapping probe = new ActionMapping();
+ parseNameAndNamespace(candidate, probe, configManager);
+ String name = probe.getName();
+ int bang = name.indexOf('!');
+ String actionName = bang == -1 ? name : name.substring(0, bang);
+ ActionConfig config =
runtimeConfiguration.getMatchedActionConfig(probe.getNamespace(), actionName);
Review Comment:
With dynamic method invocation disabled (the default), `/report!daily.html`
fails to reach an action named `report!daily` declaring `html` when that
extension is absent from the global list, because this probe looks up `report`
instead. When dynamic method invocation is enabled, the probe also splits at
the first bang while the final mapping splits at the last. Reuse
`parseActionName` here so both lookups agree, and cover literal bangs in a
regression test.
Issue Time Tracking
-------------------
Worklog Id: (was: 1051817)
Time Spent: 40m (was: 0.5h)
> Improve support for multiple extensions
> ---------------------------------------
>
> Key: WW-4164
> URL: https://issues.apache.org/jira/browse/WW-4164
> Project: Struts 2
> Issue Type: Improvement
> Components: XML Configuration
> Affects Versions: 2.3.15.1
> Reporter: Jasper Rosenberg
> Assignee: Lukasz Lenart
> Priority: Major
> Fix For: 7.5.0
>
> Time Spent: 40m
> Remaining Estimate: 0h
>
> Currently if you support multiple extensions (eg.
> struts.action.extension=action,html,,) there is no way to tell struts for a
> particular action which extension you always want to use. This has two
> downsides:
> 1. When struts has to generate an url, for example in the form tag, or the
> "redirectAction" result type, there is no way to specify the extension to
> use, so it ends up using the current invocation's extension if possible, or
> otherwise the default, neither of which might be correct.
> 2. You can invoke the action with any of the supported extensions, creating
> duplicate pages with no clear canonical version. (Bad for SEO as well)
> What I propose is allowing the user to specify an optional specific extension
> at the package and action level in the struts xml configuration like so:
> {code:xml}
> <package name="test" namespace="/" extends="default" extension="action">
> <action name="a1" extension="">
> <result>a1.ftl</result>
> </action>
> <action name="b1" extension="html">
> <result>b1.ftl</result>
> </action>
> <action name="c1">
> <result>b1.ftl</result>
> </action>
> </package>
> {code}
> 1. When selecting an extension when building an URL for an action, it would
> first see if there was an extension to use at the action level. If not it
> would check the package level (including package inheritance). If still no
> specific extension, it would behave as it currently does.
> 2. When mapping an incoming URL to an action, the reverse will also hold. If
> the action or its package specify a specific extension, then the action can
> only be matched if the URL has that extension.
> So in the above example, the following urls would be expected to work:
> 1. /a1 (action level no extension)
> 2. /b1.html (action level "html" extension)
> 3. /c1.action (no action level extension, using package level "action"
> extension)
> However, an url like "/b1.action" will not match any actions because "action"
> extension doesn't match the action's "html" extension.
> This change would be 100% backwards because if you don't specify any
> extension attributes on the package or action, it falls back to the current
> behavior.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
