Repository navigation
Feat: Ability to filter JSR330 beans based on some predicate - #266
Conversation
Sisu Inject is extended with a generic ability to filter by predicate. Plexus Shim makes use of this, and applies filtering, making both, JSR330 and Plexus beans behave the same. This is very maven specific, so we might want to add a config in container configuration, but the current code state is what Maven needs.
|
Maven 3.10.x ITs apache/maven#12539 |
|
gnodet
left a comment
There was a problem hiding this comment.
PR #266 Review: Feat — Ability to filter JSR330 beans based on some predicate
What works well
- Clean generalization of
FilteredBeans. Extracting realm-specific filtering from the Plexus module into a genericSupplier<Predicate>-basedFilteredBeansinsisu.injectis the right abstraction. It keeps the inject module free of ClassRealm dependencies while enabling the same visibility rules to apply to pure JSR330 beans. visibilityPredicate()onRealmManagereliminates logic duplication — theString.valueOf(source).startsWith("ClassRealm")check is now in exactly one place.- The
Supplier<Predicate>design is correct for this use case — the predicate must capture the context realm at iteration time (via TCCL), so a supplier that produces a fresh predicate periterator()call is necessary. - Thorough test coverage on
FilteredBeans— null supplier, supplier-returns-null, always-true, always-false, and selective filtering.
Suggestion: Integration test for DefaultBeanLocator filtering path
The tests cover FilteredBeans in isolation, but there's no test that exercises the DefaultBeanLocator.locate() → FilteredBeans wiring. A test calling setBeanEntryPredicateSupplier() and then locate() would guard against regressions if the wiring in DefaultBeanLocator.locate() were accidentally removed.
Confirmed non-issues
(DefaultBeanLocator) qualifiedBeanLocatordowncast is safe — field isfinal MutableBeanLocator qualifiedBeanLocator = new DefaultBeanLocator()(line 119), initialized inline.AtomicReferencethread safety is correct — set once during construction;get()inlocate()provides happens-before.ContainerConfigurationinterface methods withoutdefault— consistent with the established pattern (setStrictClassPathScanningadded identically inedfceea6).- Raw types in
DefaultBeanLocator— consistent with the class-level@SuppressWarnings({"rawtypes", "unchecked"}).
Note: This review does not replace specialized AI review tools (CodeRabbit, Sourcery) or static analyzers (SonarCloud).
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
| Iterable<BeanEntry<Named, T>> beans = (Iterable<BeanEntry<Named, T>>) beanLocator.<Named, T>locate(key); | ||
| if (PlexusConstants.REALM_VISIBILITY.equalsIgnoreCase(visibility)) { | ||
| beans = new RealmFilteredBeans<>(realmManager, beans); | ||
| beans = new FilteredBeans<>(realmManager::visibilityPredicate, beans); |
There was a problem hiding this comment.
Double filtering for Plexus beans in the default configuration
Under the default configuration (jsr330ComponentVisibilityFollowsPlexusVisibility = true + componentVisibility = "realm"), the beans iterable arriving here is already a FilteredBeans — because DefaultBeanLocator.locate() wraps in FilteredBeans(realmManager::visibilityPredicate, result) when the predicate supplier is set.
This line then wraps it in a second FilteredBeans with the same predicate. Every Plexus bean entry gets evaluated twice by the same realm-visibility logic — not a correctness bug (the filter is idempotent), but redundant work on every Plexus component lookup.
Two possible approaches:
(a) Skip wrapping when the underlying locator already filters:
| beans = new FilteredBeans<>(realmManager::visibilityPredicate, beans); | |
| beans = beans instanceof FilteredBeans ? beans : new FilteredBeans<>(realmManager::visibilityPredicate, beans); |
(b) Accept as a known tradeoff and add a comment documenting that double filtering is harmless. The per-entry cost is small (two string comparisons), so this may be acceptable depending on the project's preference.
There was a problem hiding this comment.
I would go with b), as this is true ONLY if using PlexusDefaultContainer in Maven-like scenario.
OTOH, if Sisu Inject is used with some custom filtering, combined with something else, this does not stand (the whole premise of "double filtering" is Maven specific and true there only).
@mcculls ?
There was a problem hiding this comment.
Oh, and also, this is true for plexus beans only. The goal is to move off from plexus beans (Maven core already did), so this double-filtering in Maven case stands only for legacy stuff... as Maven core in 3.10+ is not Plexus anymore, is migrated to JSR330.



Add feature that allows JSR330 filtering.
Inject:
DefaultBeanLocatorhas new field, that may hold (initially isnull) a bean entry predicate supplier.FilteredBeans(based on RealmFilteredBeans in Plexus).Plexus:
RealmManagergot new method, that may create predicate for filtering (or just returnsnullwhen no filtering needed).RealmFilteredBeansmodified to reuse this predicate (prevent logic duplication).DefaultPlexusContainer(and config) modified, and by default, if "realm" visibility (which is default), the JSR330 components will have same filtering applied (by default). Use config new method to control this.This is very maven specific, hence the config. Also, filtering in Inject module is ONLY applied by default if it is created as part of
DefaultPlexusContainer, as it happens in Maven.Fixes #107