[
https://issues.apache.org/jira/browse/WW-5678?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Lukasz Lenart updated WW-5678:
------------------------------
Description:
Follow-up to [WW-5674|https://issues.apache.org/jira/browse/WW-5674], deferred
to 8.0.0 because part of it is a source-breaking change.
{panel:title=Check WW-5675 before starting}
Item 1 below may already be gone. WW-5675 carries a design note proposing that
the two allowlist sets be replaced by a single precomputed union, which would
delete the three-argument overload outright — and with it the name collision.
Confirm the state of WW-5675 before planning any work here.
{panel}
h2. 1. The package-private overload shares a name with a public method
WW-5674 left {{SecurityMemberAccess}} with three related static methods:
{code:java}
public static boolean isClassBelongsToPackages(Class clazz, Set
matchingPackages) // public API
static boolean isClassBelongsToPackages(Class clazz, Set first, Set
second) // package-private
static boolean isPackageBelongsToPackages(String packageName, Set first,
Set second)
{code}
(Generics omitted above — the issue tracker mangles them.)
The two-argument form is public; the three-argument overload deliberately is
not, because it has a single caller ({{isClassAllowlisted}}) and its only test
lives in the same package. Publishing it would freeze it as {{struts2-core}}
API.
Sharing a name across the visibility boundary is a hazard: a contributor
tidying up "an inconsistent modifier" can widen the overload to {{public}} by
reflex and never realise they have added permanent API surface. Giving the
package-private overload a distinct name removes the trap. This part is not
source-breaking and could land earlier if convenient.
As noted above, WW-5675 may remove this method altogether. The two-set
parameter exists only because a precomputed union of
{{ALLOWLIST_REQUIRED_PACKAGES}} and {{allowlistPackageNames}} is not currently
worthwhile under the prototype bean scope; once WW-5675 makes the parsed
configuration shared, the union becomes the better shape and the overload
disappears.
h2. 2. The names are grammatically awkward
{{isClassBelongsToPackages}} and {{isPackageBelongsToPackages}} read as "is
class belongs to packages". Something like {{classBelongsToAnyPackage}} /
{{packageMatchesAny}} would be clearer. The public one cannot be renamed before
a major release.
This item stands regardless of what WW-5675 does.
h2. 3. Two public statics have no callers outside the class
{{isClassBelongsToPackages(Class, Set)}} and {{toPackageName(Class)}} are both
{{public static}} on a public class, but a repository-wide search finds no
caller outside {{SecurityMemberAccess}} itself — not in core, not in any
plugin, not in the tests. They appear to be public by accident rather than by
design.
Narrowing them to package-private would shrink the surface that has to be
preserved. This _is_ source-breaking for anyone who happens to call them, hence
8.0.0. This item also stands regardless of WW-5675.
h2. Why this matters now
WW-4759 is drawing the {{struts2-api}} boundary for 8.0.0. Deciding what in
{{SecurityMemberAccess}} is genuinely API is part of the same exercise, and it
is cheaper to settle it while that work is open than to inherit these three
methods into the new artefact unexamined.
h2. Constraints
{{SecurityMemberAccessPackageMatchingTest}} calls all three methods directly
and will need updating. Any rename must not disturb the frozen
{{legacyPrefixMatch}} and {{legacyToPackageName}} reference oracles in that
test — they are verbatim copies of the pre-WW-5674 implementation kept as
differential oracles, and they must continue to _not_ delegate to production
code.
Renaming or narrowing anything public needs a migration-guide entry.
was:
Follow-up to [WW-5674|https://issues.apache.org/jira/browse/WW-5674], deferred
to 8.0.0 because part of it is a source-breaking change.
h2. 1. The package-private overload shares a name with a public method
WW-5674 left {{SecurityMemberAccess}} with three related static methods:
{code:java}
public static boolean isClassBelongsToPackages(Class clazz, Set
matchingPackages) // public API
static boolean isClassBelongsToPackages(Class clazz, Set first, Set
second) // package-private
static boolean isPackageBelongsToPackages(String packageName, Set first,
Set second)
{code}
(Generics omitted above — the issue tracker mangles them.)
The two-argument form is public; the three-argument overload deliberately is
not, because it has a single caller ({{isClassAllowlisted}}) and its only test
lives in the same package. Publishing it would freeze it as {{struts2-core}}
API.
Sharing a name across the visibility boundary is a hazard: a contributor
tidying up "an inconsistent modifier" can widen the overload to {{public}} by
reflex and never realise they have added permanent API surface. Giving the
package-private overload a distinct name removes the trap. This part is not
source-breaking and could land earlier if convenient.
h2. 2. The names are grammatically awkward
{{isClassBelongsToPackages}} and {{isPackageBelongsToPackages}} read as "is
class belongs to packages". Something like {{classBelongsToAnyPackage}} /
{{packageMatchesAny}} would be clearer. The public one cannot be renamed before
a major release.
h2. 3. Two public statics have no callers outside the class
{{isClassBelongsToPackages(Class, Set)}} and {{toPackageName(Class)}} are both
{{public static}} on a public class, but a repository-wide search finds no
caller outside {{SecurityMemberAccess}} itself — not in core, not in any
plugin, not in the tests. They appear to be public by accident rather than by
design.
Narrowing them to package-private would shrink the surface that has to be
preserved. This _is_ source-breaking for anyone who happens to call them, hence
8.0.0.
h2. Why this matters now
WW-4759 is drawing the {{struts2-api}} boundary for 8.0.0. Deciding what in
{{SecurityMemberAccess}} is genuinely API is part of the same exercise, and it
is cheaper to settle it while that work is open than to inherit these three
methods into the new artefact unexamined.
h2. Constraints
{{SecurityMemberAccessPackageMatchingTest}} calls all three methods directly
and will need updating. Any rename must not disturb the frozen
{{legacyPrefixMatch}} and {{legacyToPackageName}} reference oracles in that
test — they are verbatim copies of the pre-WW-5674 implementation kept as
differential oracles, and they must continue to _not_ delegate to production
code.
Renaming or narrowing anything public needs a migration-guide entry.
> Tidy up the SecurityMemberAccess package-matching helper names and visibility
> -----------------------------------------------------------------------------
>
> Key: WW-5678
> URL: https://issues.apache.org/jira/browse/WW-5678
> Project: Struts 2
> Issue Type: Improvement
> Reporter: Lukasz Lenart
> Priority: Major
> Fix For: 8.0.0
>
>
> Follow-up to [WW-5674|https://issues.apache.org/jira/browse/WW-5674],
> deferred to 8.0.0 because part of it is a source-breaking change.
> {panel:title=Check WW-5675 before starting}
> Item 1 below may already be gone. WW-5675 carries a design note proposing
> that the two allowlist sets be replaced by a single precomputed union, which
> would delete the three-argument overload outright — and with it the name
> collision. Confirm the state of WW-5675 before planning any work here.
> {panel}
> h2. 1. The package-private overload shares a name with a public method
> WW-5674 left {{SecurityMemberAccess}} with three related static methods:
> {code:java}
> public static boolean isClassBelongsToPackages(Class clazz, Set
> matchingPackages) // public API
> static boolean isClassBelongsToPackages(Class clazz, Set first, Set
> second) // package-private
> static boolean isPackageBelongsToPackages(String packageName, Set
> first, Set second)
> {code}
> (Generics omitted above — the issue tracker mangles them.)
> The two-argument form is public; the three-argument overload deliberately is
> not, because it has a single caller ({{isClassAllowlisted}}) and its only
> test lives in the same package. Publishing it would freeze it as
> {{struts2-core}} API.
> Sharing a name across the visibility boundary is a hazard: a contributor
> tidying up "an inconsistent modifier" can widen the overload to {{public}} by
> reflex and never realise they have added permanent API surface. Giving the
> package-private overload a distinct name removes the trap. This part is not
> source-breaking and could land earlier if convenient.
> As noted above, WW-5675 may remove this method altogether. The two-set
> parameter exists only because a precomputed union of
> {{ALLOWLIST_REQUIRED_PACKAGES}} and {{allowlistPackageNames}} is not
> currently worthwhile under the prototype bean scope; once WW-5675 makes the
> parsed configuration shared, the union becomes the better shape and the
> overload disappears.
> h2. 2. The names are grammatically awkward
> {{isClassBelongsToPackages}} and {{isPackageBelongsToPackages}} read as "is
> class belongs to packages". Something like {{classBelongsToAnyPackage}} /
> {{packageMatchesAny}} would be clearer. The public one cannot be renamed
> before a major release.
> This item stands regardless of what WW-5675 does.
> h2. 3. Two public statics have no callers outside the class
> {{isClassBelongsToPackages(Class, Set)}} and {{toPackageName(Class)}} are
> both {{public static}} on a public class, but a repository-wide search finds
> no caller outside {{SecurityMemberAccess}} itself — not in core, not in any
> plugin, not in the tests. They appear to be public by accident rather than by
> design.
> Narrowing them to package-private would shrink the surface that has to be
> preserved. This _is_ source-breaking for anyone who happens to call them,
> hence 8.0.0. This item also stands regardless of WW-5675.
> h2. Why this matters now
> WW-4759 is drawing the {{struts2-api}} boundary for 8.0.0. Deciding what in
> {{SecurityMemberAccess}} is genuinely API is part of the same exercise, and
> it is cheaper to settle it while that work is open than to inherit these
> three methods into the new artefact unexamined.
> h2. Constraints
> {{SecurityMemberAccessPackageMatchingTest}} calls all three methods directly
> and will need updating. Any rename must not disturb the frozen
> {{legacyPrefixMatch}} and {{legacyToPackageName}} reference oracles in that
> test — they are verbatim copies of the pre-WW-5674 implementation kept as
> differential oracles, and they must continue to _not_ delegate to production
> code.
> Renaming or narrowing anything public needs a migration-guide entry.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
