OK, so this rabbit-hole is exactly what I wanted to avoid and which is why
I added a detailed description in the original patch (and not just the
annotation change)
To clarify: plugins and extensions can already override any component,
because their baseline priority is always higher than maven core (this
mimics the same behaviour as Plexus as configured by Maven, but the
underlying approach can support different models and also allows individual
priorities per-component.)
So the patch was _not_ about allowing overriding in the general case, but
instead to fix a small corner-case where a default component uses @Typed
(which requests that it is directly bound to the given type) and another
component just @Injects that type without any qualification. I've pasted
the full explanation at the end of this email just to increase its
visibility.
In other words... if you don't use @Typed (which is almost every case) then
you can continue to use "default" or a blank name for default components
and any other name for non-default components.
The _only_ time you need to be careful is when you:
1) start off with a default component which either uses @Named("default")
or uses @Named and the class is DefaultXYZ
and... 2) decide to add @Typed to restrict what types a component is
visible as (ie. what it can be injected as)
and... 3) want to allow overriding of that component
when all 3 of those are true then you need to use another name such
as @Named("typed-default") to stop the container from taking the
short-circuit.
If anyone needs further details I'm happy to supply them, I just want to
make clear that this is a corner-case which shouldn't affect the current
recommendations around @Named when moving from Plexus (which is to just use
the hint) because the advice here really is only necessary when the above 3
conditions apply which is very rare.
--------8<-------
* Note: uses @Typed to limit the types it is available for injection to
just ModelProcessor.
*
* This is because the ModelProcessor interface extends ModelLocator and
ModelReader. If we
* made this component available under all its interfaces then it could end
up being injected
* into itself leading to a stack overflow.
*
* A side-effect of using @Typed is that it translates to explicit bindings
in the container.
* So instead of binding the component under a 'wildcard' key it is now
bound with an explicit
* key. Since this is a default component this will be a plain binding of
ModelProcessor to
* this implementation type, ie. no hint/name.
*
* This leads to a second side-effect in that any @Inject request for just
ModelProcessor in
* the same injector is immediately matched to this explicit binding, which
means extensions
* cannot override this binding. This is because the lookup is always
short-circuited in this
* specific situation (plain @Inject request, and plain explicit binding
for the same type.)
*
* The simplest solution is to use a custom @Named here so it isn't bound
under the plain key.
* This is only necessary for default components using @Typed that want to
support overriding.
*
* As a non-default component this now gets a -ve priority relative to
other implementations
* of the same interface. Since we want to allow overriding this doesn't
matter in this case.
* (if it did we could add @Priority of 0 to match the priority given to
default components.)
On Sat, 19 Oct 2019 at 10:18, Robert Scholte <rfscho...@apache.org> wrote:
> On Sat, 19 Oct 2019 02:25:47 +0200, Jason van Zyl <ja...@vanzyl.ca> wrote:
>
> > Inline:
> >
> > On Oct 18, 2019, at 5:15 PM, Stuart McCulloch <mccu...@gmail.com>
> wrote:
> >>
> >> Any string apart from "default" or the empty string will work here - I
> >> happened to chose "core" because I viewed it as a core implementation.
> >>
> >> Also a quick reminder that this is only needed when a component uses
> >> @Typed
> >> and wants to allow overriding, it's not necessary in any other
> >> situation -
> >> so in that sense "allowDefaultOverride" wouldn't be quite accurate. (See
> >> the javadoc in the patch for more detail.)
> >>
> >
> > Stuart,
> >
> > There is an idiom like this used in the ReactorReader and the
> > GraphBuilder where there are implementations of them in extensions out
> > in the wild, and while those are not @Typed they use a @Named(
> > “not-default” ) key pattern. If these components are intended to allow
> > for custom implementations then a common way of doing this would be
> > easier to understand. Something like @Named( “coreAllowingOverride” )
> or
> > @Named( “coreExtensionPoint” ) or whatever most appropriate. Right now
> > there’s a bit of fiddly magic that works in Plexus with a lookup and
> > Plexus with its annotations, and slightly different way with Sisu with
> > @Inject annotations. So, sure, this particular case of requiring
> @Named(
> > “core” ) to fix the case where you want to override a component with
> the
> > implicit “default” key is required, but maybe something we should avoid
> > and if it’s meant to be extended just have an explicit common pattern.
> I
> > realize with DI you can override anything, but I don’t think being a
> > little more explicit would hurt. The nuance of how the bindings work
> > maybe you and Igor know/remember how it works, I had to use a debugger
> > this afternoon :-)
>
> I guess the best value would have been "default", but that won't work.
> Since all components can be overridden, I suggest to change it to simply
> "DefaultModelProcessor".
>
> Some people already want to add module descriptors to their
> plugins/extensions. In order to support that we already need to
> restructure interfaces and make a clear separatation of SPIs and APIs.
> That's likely better than trying to do more magic with the @Named
> annotation that's already around for quite some time.
>
> >
> > Robert,
> >
> > What’s overridden in polyglot is to be injected into Maven’s core is
> the
> > ModelProcessor, not the ModelBuilder:
> >
> >
> https://github.com/takari/polyglot-maven/blob/master/polyglot-common/src/main/java/org/sonatype/maven/polyglot/TeslaModelProcessor.java
> >
> > The ModelReaders are injected into a manager within the
> > TeslaModelProcessor, sure, but it’s the ModelProcessor being overridden
> > which makes the magic happen in polyglot. When the ModelProcessor
> > polyglot requires doesn't get injected it’s trying to use the XML-based
> > model reader to read Kotlin which doesn’t work out so well. Trying to
> > build a polyglot project without the extensions loaded in 3.6.1 yields
> > the same result as trying to use 3.6.2 with a polyglot project: the
> same
> > error which is a result of the right ModelProcessor not being found.
> > You’ll see the the DefaultModelProcessor being used instead of the
> > TeslaModelProcessor in the stack trace.
> >
>
> Right, so the stacktrace fooled me. ModelProcessor it is.
>
> >> PS. the reason DefaultModelProcessor needs to use @Typed is because it
> >> has
> >> an "interesting" interface hierarchy where ModelProcessor extends
> >> both ModelLocator and ModelReader, so it can act as both and delegate
> >> accordingly - but then it also asks to have a ModelLocator and
> >> ModelReader
> >> injected, which is where things get messy. If it had a cleaner hierarchy
> >> (ie. it wasn't asking to inject something that it also implements)
> then
> >> it
> >> wouldn't need @Typed and wouldn't then need the custom name.
> >>
> >
> > Agreed. Looks fixable and there are probably only two consumers in the
> > world. Polyglot and
> > https://github.com/qoomon/maven-git-versioning-extension where it was
> > attempt to fix the problem in 3.6.2.
> >
> > jvz
> >
> >>
> >> On Fri, 18 Oct 2019 at 20:35, Robert Scholte <rfscho...@apache.org>
> >> wrote:
> >>
> >>> Hi,
> >>>
> >>> the adjusted @Named annotation is on DefaultModelProcessor, not
> >>> DefaultModelBuilder.
> >>> They both implement the ModelBuilder interface, but the one that
> >>> extensions like to overwrite is the implementation of
> >>> DefaultModelBuilder.
> >>> So I'd prefer to stick to "core" as proposed my Stuart.
> >>>
> >>> thanks for the confirmation that this works,
> >>> Robert
> >>>
> >>> On Fri, 18 Oct 2019 21:10:18 +0200, Jason van Zyl <jvan...@apache.org>
> >>> wrote:
> >>>
> >>>> Hi,
> >>>>
> >>>> As noted in Slack I think it would be more clear if we used something
> >>>> like
> >>>>
> >>>> @Named( “allowDefaultOverride” )
> >>>>
> >>>> vs
> >>>>
> >>>> @Named( “core” )
> >>>>
> >>>> As that expresses the intent and can be used anywhere it's allowed for
> >>> a
> >>>> client to override the default component.
> >>>>
> >>>> The tests in polyglot all pass with this change, and I’m able to run
> >>>> polyglot example projects again, so I assume the Tycho POM-less are
> >>>> happy again as well but hopefully someone can verify.
> >>>>
> >>>> Jason
> >>>>
> >>>>> On Oct 18, 2019, at 2:04 PM, Robert Scholte <rfscho...@apache.org>
> >>>>> wrote:
> >>>>>
> >>>>> Hi,
> >>>>>
> >>>>> with the help from Stuart McCulloch we've been able to provide a
> >>>>> patch
> >>>>> for MNG-6765[1]
> >>>>> Please review and test.
> >>>>>
> >>>>> thanks,
> >>>>> Robert
> >>>>>
> >>>>> [1] https://issues.apache.org/jira/browse/MNG-67
> >>>>> [2]
> >>>>>
> >>>
> https://github.com/apache/maven/commit/24e6c0ec0a87b6682513287a23c36db6996b874c
> >>>>> [3]
> >>>>>
> >>>
> https://github.com/apache/maven/commit/53a70bc8543124569ee787725b2004bc92a681b6
> >>>>>
> >>>>> ---------------------------------------------------------------------
> >>>>> To unsubscribe, e-mail: dev-unsubscr...@maven.apache.org
> >>>>> For additional commands, e-mail: dev-h...@maven.apache.org
> >>>>>
> >>>>
> >>>>
> >>>> ---------------------------------------------------------------------
> >>>> To unsubscribe, e-mail: dev-unsubscr...@maven.apache.org
> >>>> For additional commands, e-mail: dev-h...@maven.apache.org
> >>>
> >>> ---------------------------------------------------------------------
> >>> To unsubscribe, e-mail: dev-unsubscr...@maven.apache.org
> >>> For additional commands, e-mail: dev-h...@maven.apache.org
> >>>
> >>>
> >
> >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscr...@maven.apache.org
> > For additional commands, e-mail: dev-h...@maven.apache.org
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscr...@maven.apache.org
> For additional commands, e-mail: dev-h...@maven.apache.org
>
>
