[
https://issues.apache.org/jira/browse/WW-4164?focusedWorklogId=1051822&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1051822
]
ASF GitHub Bot logged work on WW-4164:
--------------------------------------
Author: ASF GitHub Bot
Created on: 02/Nov/26 08:58
Start Date: 02/Nov/26 08:58
Worklog Time Spent: 10m
Work Description: lukaszlenart commented on code in PR #2093:
URL: https://github.com/apache/struts/pull/2093#discussion_r4440474088
##########
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:
Fixed in fdc8bc093: after dropping the query string the resolver cuts at `!`
only when dynamic method invocation is enabled, and at the last `!`, as
`DefaultActionMapper.parseActionName` does. Covered by
`testLiteralBangNameWithoutDmi` (`report!daily` → `html`) and a DMI-on case.
##########
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:
Fixed in fdc8bc093: for a target that declares extensions, a current request
mapping without an extension now counts as `""` and is kept when allowed. With
no current mapping, or for an undeclared target, the first entry is still used,
which keeps today's output for undeclared targets. The design doc states the
rule (9012a4c34).
##########
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:
Fixed in fdc8bc093 and 5ae23ce15: the probe refuses to claim when
`cleanupActionName` substituted the requested name, so `/manual(1).html` falls
through to the container again. The check compares the full requested name, so
it also holds with `struts.enable.SlashesInActionNames=true`
(`/x(1)/index.html` → no mapping).
##########
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:
Fixed in fdc8bc093: the probe now goes through the same `parseActionName` as
the final mapping, so literal bangs (DMI off) and the last-bang split (DMI on)
agree. Covered by `testDeclaredExtensionProbeSplitsDmiNameAtLastBang` and a
literal `report!daily` case.
Issue Time Tracking
-------------------
Worklog Id: (was: 1051822)
Time Spent: 1h 10m (was: 1h)
> 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: 1h 10m
> 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)
