[
https://issues.apache.org/jira/browse/WW-5770?focusedWorklogId=1047369&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1047369
]
ASF GitHub Bot logged work on WW-5770:
--------------------------------------
Author: ASF GitHub Bot
Created on: 14/Oct/26 06:12
Start Date: 14/Oct/26 06:12
Worklog Time Spent: 10m
Work Description: Copilot commented on code in PR #2009:
URL: https://github.com/apache/struts/pull/2009#discussion_r4267377733
##########
core/src/main/java/org/apache/struts2/config/ScopedConstantProvider.java:
##########
@@ -32,4 +34,17 @@ public interface ScopedConstantProvider {
* @throws IllegalArgumentException when {@code name} is not scopable
*/
String getValue(String name);
+
+ /**
+ * @param name a constant registered through a {@link
ScopableConstants} bean
+ * @param packageConfig the package whose scoped constants, inherited ones
included, take precedence; may be
+ * {@code null}
+ * @return the package's scoped value, or else {@link #getValue(String)}
+ * @throws IllegalArgumentException when {@code name} is not scopable
+ * @since 7.5.0
+ */
+ default String getValue(String name, PackageConfig packageConfig) {
+ String value = packageConfig != null ?
packageConfig.getAllScopedConstants().get(name) : null;
+ return value != null ? value : getValue(name);
Review Comment:
The Javadoc contract says this method throws `IllegalArgumentException` when
`name` is not scopable, but the default implementation can return a value from
`packageConfig.getAllScopedConstants()` without ever validating that `name` is
scopable (i.e., it won’t throw if a non-scopable name is present
programmatically). Consider enforcing the same validation behavior in the
default method (e.g., validate via `getValue(name)` before returning a non-null
package value) or relax the `@throws` statement to match the actual behavior.
##########
plugins/convention/src/test/resources/struts-convention-scoped-result-path.xml:
##########
@@ -0,0 +1,36 @@
+<?xml version="1.0" encoding="UTF-8" ?>
+<!--
+/*
+ * 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.
+ */
+-->
+
+<!DOCTYPE struts PUBLIC
+ "-//Apache Software Foundation//DTD Struts Configuration 7.5//EN"
+ "https://struts.apache.org/dtds/struts-7.5.dtd";>
+
+<struts>
+ <constant name="struts.convention.result.path" value="/"/>
+ <constant name="struts.convention.default.parent.package"
value="scoped-default"/>
+
+ <package name="convention-default" extends="struts-default"/>
+
Review Comment:
`ConventionScopedResultPathTest` loads `struts-plugin.xml` plus this XML.
The Convention plugin’s `struts-plugin.xml` typically already defines the
`convention-default` package, so redefining `<package name="convention-default"
.../>` here can trigger a duplicate-package configuration error (or override
unintended package settings). Prefer removing the `convention-default`
declaration from this test XML and have `scoped-default` extend the
plugin-provided `convention-default` directly.
##########
plugins/convention/src/main/java/org/apache/struts2/convention/ConventionsServiceImpl.java:
##########
@@ -51,54 +53,84 @@ public
ConventionsServiceImpl(@Inject(ConventionConstants.CONVENTION_RESULT_PATH
this.resultPath = resultPath;
}
+ @Inject
+ public void setScopedConstantProvider(ScopedConstantProvider
scopedConstantProvider) {
+ this.scopedConstantProvider = scopedConstantProvider;
+ }
+
/**
* {@inheritDoc}
*/
public String determineResultPath(Class<?> actionClass) {
- String localResultPath = resultPath;
- ResultPath resultPathAnnotation =
AnnotationUtils.findAnnotation(actionClass, ResultPath.class);
- if (resultPathAnnotation != null) {
- if (resultPathAnnotation.value().equals("") &&
resultPathAnnotation.property().equals("")) {
- throw new ConfigurationException("The ResultPath annotation
must have either" +
- " a value or property specified.");
- }
-
- String property = resultPathAnnotation.property();
- if (property.equals("")) {
- localResultPath = resultPathAnnotation.value();
- } else {
- try {
- ResourceBundle strutsBundle =
ResourceBundle.getBundle("struts");
- localResultPath = strutsBundle.getString(property);
- } catch (Exception e) {
- throw new ConfigurationException("The action class [" +
actionClass + "] defines" +
- " a @ResultPath annotation and a property definition
however the" +
- " struts.properties could not be found in the
classpath using ResourceBundle" +
- " OR the bundle exists but the property [" + property
+ "] is not defined" +
- " in the file.", e);
- }
- }
- }
+ return determineResultPath(actionClass, null);
+ }
- return localResultPath;
+ /**
+ * {@inheritDoc}
+ */
+ @Override
+ public String determineResultPath(Class<?> actionClass, PackageConfig
packageConfig) {
+ String annotatedResultPath = findAnnotatedResultPath(actionClass);
+ return annotatedResultPath != null ? annotatedResultPath :
scopedResultPath(packageConfig);
}
/**
* {@inheritDoc}
*/
public String determineResultPath(ActionConfig actionConfig) {
+ return determineResultPath(actionConfig, null);
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ @Override
+ public String determineResultPath(ActionConfig actionConfig, PackageConfig
packageConfig) {
if (actionConfig == null) {
- return resultPath;
+ return scopedResultPath(packageConfig);
}
try {
- return
determineResultPath(ClassLoaderUtil.loadClass(actionConfig.getClassName(),
this.getClass()));
+ return
determineResultPath(ClassLoaderUtil.loadClass(actionConfig.getClassName(),
this.getClass()), packageConfig);
} catch (ClassNotFoundException e) {
throw new RuntimeException("Invalid action class configuration
that references an unknown " +
"class named [" + actionConfig.getClassName() + "]", e);
}
}
+ private String scopedResultPath(PackageConfig packageConfig) {
+ if (packageConfig == null) {
+ return resultPath;
+ }
+ return
scopedConstantProvider.getValue(ConventionConstants.CONVENTION_RESULT_PATH,
packageConfig);
+ }
Review Comment:
`scopedConstantProvider` is injected via setter, but `scopedResultPath` will
throw a `NullPointerException` if `ConventionsServiceImpl` is instantiated
manually (outside the DI container) and a non-null `packageConfig` is passed
(which now happens from framework call sites like `DefaultResultMapBuilder`).
To make failure mode predictable, consider either (a) falling back to
`resultPath` when `scopedConstantProvider` is unset, or (b) throwing a
`ConfigurationException` with a clear message indicating the provider must be
injected before using the package-aware methods.
##########
docs/superpowers/specs/2026-10-14-WW-5770-scoped-convention-result-path-design.md:
##########
@@ -0,0 +1,121 @@
+# WW-5770 — Scopable `struts.convention.result.path`
+
+**Ticket:** [WW-5770](https://issues.apache.org/jira/browse/WW-5770) — *Make
struts.convention.result.path scopable per package*
+**Target:** 7.5.0 (ticket fix version)
+**Date:** 2026-10-14
+**Status:** Design approved, pending implementation plan
Review Comment:
The spec status says “pending implementation plan”, but this PR includes the
implementation plan document (and implements the design). Updating the status
line to reflect the current state would prevent the spec from becoming
misleading.
Issue Time Tracking
-------------------
Worklog Id: (was: 1047369)
Time Spent: 40m (was: 0.5h)
> Make struts.convention.result.path scopable per package
> -------------------------------------------------------
>
> Key: WW-5770
> URL: https://issues.apache.org/jira/browse/WW-5770
> Project: Struts 2
> Issue Type: Improvement
> Components: Plugin - Convention
> Reporter: Lukasz Lenart
> Assignee: Lukasz Lenart
> Priority: Major
> Fix For: 7.5.0
>
> Time Spent: 40m
> Remaining Estimate: 0h
>
> A follow-up to the package-scoped constants from
> [WW-3476|https://issues.apache.org/jira/browse/WW-3476], whose first adopter
> was [WW-5768|https://issues.apache.org/jira/browse/WW-5768]
> ({{struts.ui.theme}}, {{struts.ui.templateDir}}).
> h2. Goal
> Allow a parent package to set the Convention result path for the actions
> placed under it:
> {code:xml}<package name="admin-base" extends="convention-default">
> <scoped-constant name="struts.convention.result.path"
> value="/WEB-INF/admin/"/>
> </package>
> {code}
> Today the same effect needs {{@ResultPath}} on every action class or package.
> h2. Why it needs more than WW-5768
> {{ScopedConstantProvider.getValue(name)}} resolves the package from the
> current {{ActionInvocation}}. The Convention plugin reads the result path in
> two places:
> # *At startup*, in {{DefaultResultMapBuilder.build(...)}} via
> {{ConventionsService.determineResultPath(Class)}}. There is no invocation, so
> {{getValue(name)}} always returns the global value. However, {{build()}}
> receives the action's {{PackageConfig}}. {{PackageBasedActionConfigBuilder}}
> passes {{pkgCfg.build()}}, which was created with {{addParent(parentPkg)}},
> so {{getAllScopedConstants()}} already includes the parent's overrides.
> # *At runtime*, in {{ConventionUnknownHandler.determinePath(...)}} via
> {{determineResultPath(ActionConfig)}}, for action-less results.
> {{actionConfig}} may be {{null}} there.
> Both need a lookup by package rather than by invocation. One option is a
> {{getValue(String name, PackageConfig packageConfig)}} on
> {{ScopedConstantProvider}}, with {{getValue(name)}} delegating to it for the
> current invocation's package.
> h2. Proposed change
> * Register a {{ScopableConstants}} bean in the Convention plugin's
> {{struts-plugin.xml}} returning {{struts.convention.result.path}}.
> * Resolve the result path from the action's package in both places, keeping
> this precedence:
> ## {{@ResultPath}} on the action class or package
> ## the package's scoped constant (including inherited ones)
> ## the global {{struts.convention.result.path}}
> h2. Tests
> * A scoped override on the parent package changes the location of results
> built at startup.
> * {{@ResultPath}} still wins over the override.
> * An action-less result resolved by {{ConventionUnknownHandler}} uses the
> override.
> * With no override, the global path is used.
> h2. Docs
> On the struts-site, list the constant on the scoped-constants section of
> {{core-developers/package-configuration.md}}, and mention it on the
> Convention plugin page.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
