gnodet commented on code in PR #1923:
URL: https://github.com/apache/maven-resolver/pull/1923#discussion_r3410083467
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/filter/DefaultRemoteRepositoryFilterManager.java:
##########
@@ -60,21 +59,22 @@ public DefaultRemoteRepositoryFilterManager(Map<String,
RemoteRepositoryFilterSo
@Override
public RemoteRepositoryFilter
getRemoteRepositoryFilter(RepositorySystemSession session) {
// use session specific key to distinguish between "derived" sessions
- String instanceSpecificKey = INSTANCE_KEY + "." + session.hashCode();
- return (RemoteRepositoryFilter)
session.getData().computeIfAbsent(instanceSpecificKey, () -> {
- HashMap<String, RemoteRepositoryFilter> filters = new HashMap<>();
- for (Map.Entry<String, RemoteRepositoryFilterSource> entry :
sources.entrySet()) {
- RemoteRepositoryFilter filter =
entry.getValue().getRemoteRepositoryFilter(session);
- if (filter != null) {
- filters.put(entry.getKey(), filter);
- }
- }
- if (!filters.isEmpty()) {
- return new Participants(filters);
- } else {
- return null;
- }
- });
+ return (RemoteRepositoryFilter) session.getData()
+ .computeIfAbsent(
+ Keys.of(DefaultRemoteRepositoryFilterManager.class,
"instance", session.hashCode()), () -> {
Review Comment:
Pre-existing issue (not introduced by this PR), but worth noting: using
\`session.hashCode()\` as a key element means that two different derived
sessions with a hashCode collision would share the same filter instance.
\`Integer.hashCode()\` is just the int value itself, so this relies on
\`RepositorySystemSession.hashCode()\` being sufficiently unique. This was the
same behavior before the PR (\`INSTANCE_KEY + \".\" + session.hashCode()\`), so
no regression here.
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/scope/OptionalDependencySelector.java:
##########
@@ -37,8 +38,8 @@
* @see Dependency#isOptional()
*/
public final class OptionalDependencySelector implements DependencySelector {
- public static final String IGNORED_KEYS =
OptionalDependencySelector.class.getName() + ".ignored";
- public static final String UNSELECTED_KEYS =
OptionalDependencySelector.class.getName() + ".unselected";
+ public static final Object IGNORED_KEYS =
Keys.of(OptionalDependencySelector.class, "ignored");
Review Comment:
Note: this changes two \`public static final\` fields from type \`String\`
to type \`Object\`. While \`OptionalDependencySelector\` is in the
\`internal.impl\` package (so the PR description's claim of "no API breakage in
api, spi, util" is accurate), these fields are \`public\` and the class is
\`public final\`.
If any external code (e.g., a custom \`DependencySelector\` implementation
or a Maven plugin) references \`OptionalDependencySelector.IGNORED_KEYS\` and
assigns it to a \`String\` variable, that code will fail to compile. Worth
considering whether to keep the \`String\` type here (since these are only used
as \`SessionData\` keys, and \`SessionData.get()\` accepts \`Object\`).
That said, the fields were only added in commit \`0447a67c\` (very
recently), so the blast radius is likely near zero.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
