Repository navigation
WW-5770 Make struts.convention.result.path scopable per package - #2009
Merged
Merged
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…lt in the design Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…kage Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…s package Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… results Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ctions and results Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…o the current invocation Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
4 open findings
ConventionScopedResultPathTestloadsstruts-plugin.xmlplus this XML. The Convention plugin’s… · NewIllegalArgumentExceptionwhennameis not… · NewscopedConstantProvideris injected via setter, butscopedResultPathwill throw a… · New
What changed in this PR
This PR makes struts.convention.result.path package-scopable for the Convention plugin, enabling parent packages to override result template locations without requiring @ResultPath everywhere.
Changes:
- Adds package-aware lookup to
ScopedConstantProviderand implements the “explicit package wins” behavior inStrutsScopedConstantProvider. - Updates Convention’s
ConventionsService/ConventionsServiceImpl,DefaultResultMapBuilder, andConventionUnknownHandlerto pass/consumePackageConfigwhen determining result paths. - Adds/updates Convention and core tests plus an integration-style Convention test and supporting resources; registers Convention’s scopable constant in
struts-plugin.xml.
| File | Description |
|---|---|
| plugins/convention/src/test/resources/struts-convention-scoped-result-path.xml | Adds test Struts config to validate scoped result-path behavior via real plugin wiring |
| plugins/convention/src/test/resources/scoped/message-success.ftl | Adds scoped FreeMarker template used by the integration test |
| plugins/convention/src/test/java/org/apache/struts2/convention/PackageBasedActionConfigBuilderTest.java | Updates tests to construct ConventionsServiceImpl with a scoped-constant provider |
| plugins/convention/src/test/java/org/apache/struts2/convention/DefaultResultMapBuilderTest.java | Updates test wiring and adds coverage for parent-package scoped result paths |
| plugins/convention/src/test/java/org/apache/struts2/convention/ConventionsServiceImplTest.java | Adds unit tests for precedence rules and package-aware overloads |
| plugins/convention/src/test/java/org/apache/struts2/convention/ConventionUnknownHandlerTest.java | Adds runtime tests ensuring unknown action/result resolution uses package-scoped paths |
| plugins/convention/src/test/java/org/apache/struts2/convention/ConventionScopedResultPathTest.java | Adds integration-style test to verify scoped path works when booting plugin XML |
| plugins/convention/src/main/resources/struts-plugin.xml | Registers Convention’s scopable constants bean |
| plugins/convention/src/main/java/org/apache/struts2/convention/DefaultResultMapBuilder.java | Uses package-aware determineResultPath during startup result scanning |
| plugins/convention/src/main/java/org/apache/struts2/convention/ConventionsServiceImpl.java | Implements package-aware result-path resolution and injects ScopedConstantProvider |
| plugins/convention/src/main/java/org/apache/struts2/convention/ConventionsService.java | Adds default overloads to accept a PackageConfig |
| plugins/convention/src/main/java/org/apache/struts2/convention/ConventionUnknownHandler.java | Passes package into result-path determination for unknown actions/results |
| plugins/convention/src/main/java/org/apache/struts2/convention/ConventionScopableConstants.java | Declares struts.convention.result.path as a scopable constant for Convention |
| docs/superpowers/specs/2026-10-14-WW-5770-scoped-convention-result-path-design.md | Adds design spec documenting decisions and API changes |
| docs/superpowers/plans/2026-10-14-WW-5770-scoped-convention-result-path.md | Adds implementation plan and verification steps |
| core/src/test/java/org/apache/struts2/config/providers/StrutsScopedConstantProviderTest.java | Adds tests for explicit-package scoped-constant lookup behavior |
| core/src/main/java/org/apache/struts2/config/providers/StrutsScopedConstantProvider.java | Implements getValue(name, PackageConfig) and adjusts resolution logic |
| core/src/main/java/org/apache/struts2/config/ScopedConstantProvider.java | Adds default method for package-aware lookup |
🧠 Review effort: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Drop the inert convention-default redeclaration from the test fixture, narrow the @throws of the default getValue(String, PackageConfig), and mark the design as implemented. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Copilot stopped reviewing on behalf of
lukaszlenart due to an error
October 14, 2026 06:40
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes WW-5770
Summary
A follow-up to package-scoped constants (WW-3476). A parent package can now set the Convention result path for every action placed under it:
Until now, the same effect needed
@ResultPathon every action class or Java package.The result path of an action is taken from, in order:
@ResultPathon the action class or its Java packagestruts.convention.result.path, including inherited onesstruts.convention.result.pathWhy this needs a lookup by package
ScopedConstantProvider.getValue(name)resolves the package from the currentActionInvocation, but Convention reads the result path in two places that have none:DefaultResultMapBuilder.build(...)runs without an invocation. It does receive the action'sPackageConfig, and that config already includes the parent's scoped constants.ConventionUnknownHandlerresolves results for template-only requests, before any invocation of the requested action exists.Changes
ScopedConstantProvidergetsdefault String getValue(String name, PackageConfig packageConfig). It is adefaultmethod so the interface stays functional; tests and custom providers keep using lambdas.StrutsScopedConstantProvideroverrides it. A package that does not declare the constant gives the global value, never the current invocation's package. A nested request (<s:action>, a chain result) reacheshandleUnknownActionwhile the calling action's invocation is still bound, so falling back to the current invocation would apply the caller's override there.ConventionScopableConstantsregistersstruts.convention.result.pathas scopable instruts-plugin.xml.ConventionsServicegets two package-awaredefaultoverloads ofdetermineResultPath. Custom implementations that only implement the one-argument methods keep their behaviour.ConventionsServiceImplimplements them. It keeps its constructor and uses the constructor's global path when there is no package. TheScopedConstantProviderarrives through an@Injectsetter.DefaultResultMapBuilderpasses the action's package.ConventionUnknownHandler.determinePath(ActionConfig, PackageConfig, String)takes the package.handleUnknownResultpasses the action's package.handleUnknownActionhas no action, so it passes the default parent package (struts.convention.default.parent.package), the package it already uses for result types.Compatibility
These need lines in the 7.5.0 migration notes:
ConventionUnknownHandler.determinePathchanged signature (it isprotected). A subclass that overrode the old two-argument form without@Overridekeeps compiling, but its method is no longer called.ConventionsServiceImplthat override the one-argumentdetermineResultPathmethods must override the two-argument forms, which the framework now calls.ScopedConstantProvider.getValue(String, PackageConfig)is new; custom providers may override it.Tests
StrutsScopedConstantProviderTest: an explicit package resolves inherited values, wins over the invocation's package, and ignores the invocation's package when it has no override. An empty value is used as-is, a non-scopable name throws, and a lambda provider gets the package lookup.ConventionsServiceImplTest: class-level and package-level@ResultPathwin over the scoped value; the scoped value wins over the global one; an empty value is used as-is; a null package gives the global path without consulting the provider; a custom one-argument-only service keeps its behaviour.DefaultResultMapBuilderTest: a parent-package override moves results found at startup, with and without a trailing slash, and@ResultPathstill wins.ConventionUnknownHandlerTestrecords the paths asked of theServletContext. An unknown action uses the default parent package's override, or the global path when it has none; an unknown result uses the action package's override.ConventionScopedResultPathTestboots the real Conventionstruts-plugin.xmlwith an XML parent package declaring the constant, and renders the template from the scoped path.Docs: apache/struts-site#354 (package-configuration scoped-constants list and the Convention plugin page).
🤖 Generated with Claude Code