Re: Regression in model interpolation coming from using JSR-330?

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: Regression in model interpolation coming from using JSR-330?

Stuart McCulloch
Yes, when there are multiple non-default implementations with the same
priority then it will pick the first in the list (where the "list" in
question is actually a combination of plugins + extensions + core
bindings for the given type)

If a particular implementation should be treated as the default then it
should either start with "Default" or be annotated with @Named("default") -
the benefit of this approach is that it documents that this is the default
implementation, to be favoured over the others. Alternatively if you want
to have an ordering between implementations then you can use @Priority to
assign specific priorities and favour one over the other.

If you don't mark an implementation as default and don't define any
priorities then the only current guarantees are that implementations in
plugins and extensions will be favoured over implementations in core (to
allow overriding). But there is an upcoming improvement to sort the list
inside each module that would make this more deterministic from build to
build, at least when ranking implementations inside a particular module:
https://github.com/eclipse/sisu.inject/pull/5 - with that change then
you'll get an extra guarantee that inside a module it will be ordered by
package+name.

PS. even with the old Plexus runtime annotations you could be at the whim
of classpath ordering, similarly Plexus XML descriptors are influenced by
classpath resource ordering - so ideally it's better to be explicit about
ordering

On Sat, 2 Nov 2019 at 23:07, Gabriel Belingueres <[hidden email]>
wrote:

> Hi:
>
> I just built maven core from source and found that it was using
> StringSearchModelInterpolator instead of StringVisitorModelInterpolator, a
> regression from the last 3.6.2 release.
>
> I found that in maven-model-builder/META-INF/sisu/javax.injected.Named
> file, both interpolators are listed but in reverse order (comparing it with
> the same file in 3.6.2), that is StringSearchModelInterpolator appear first
> in the file and StringVisitorModelInterpolator second.
>
> (I believe the dependency injection picks up the first one it finds with
> the right type.)
>
> Deleting the @Named annotation from StringSearchModelInterpolator solved
> the issue, as this make the entry disappear from the javax.injected.Named
> file. Then the dependency injection uses the
> StringVisitorModelInterpolator.
>
> Can anyone confirm this issue?
> And if so, how to best prevent in the future that this type of dependency
> injection regressions from happening?
>
> Kind regards,
> Gabriel
>
Reply | Threaded
Open this post in threaded view
|

Re: Regression in model interpolation coming from using JSR-330?

Gabriel Belingueres-2
Thanks Stuart.

The reproducibility PR you mention helps in having a deterministic ordering
inside the Named components file to buld exactly the same executable, but
it does not mean it is the right priority for component injection.

Do you know if it is possible to configure sisu to throw an exception if
trying to inject an ambiguous component? Otherwise, I guess we must
implement some sort of "ambiguity checker", either as an integration test
(don't know if it is possible) or either built-it into maven executable.




El sáb., 2 de nov. de 2019 a la(s) 20:54, Stuart McCulloch (
[hidden email]) escribió:

> Yes, when there are multiple non-default implementations with the same
> priority then it will pick the first in the list (where the "list" in
> question is actually a combination of plugins + extensions + core
> bindings for the given type)
>
> If a particular implementation should be treated as the default then it
> should either start with "Default" or be annotated with @Named("default") -
> the benefit of this approach is that it documents that this is the default
> implementation, to be favoured over the others. Alternatively if you want
> to have an ordering between implementations then you can use @Priority to
> assign specific priorities and favour one over the other.
>
> If you don't mark an implementation as default and don't define any
> priorities then the only current guarantees are that implementations in
> plugins and extensions will be favoured over implementations in core (to
> allow overriding). But there is an upcoming improvement to sort the list
> inside each module that would make this more deterministic from build to
> build, at least when ranking implementations inside a particular module:
> https://github.com/eclipse/sisu.inject/pull/5 - with that change then
> you'll get an extra guarantee that inside a module it will be ordered by
> package+name.
>
> PS. even with the old Plexus runtime annotations you could be at the whim
> of classpath ordering, similarly Plexus XML descriptors are influenced by
> classpath resource ordering - so ideally it's better to be explicit about
> ordering
>
> On Sat, 2 Nov 2019 at 23:07, Gabriel Belingueres <[hidden email]>
> wrote:
>
> > Hi:
> >
> > I just built maven core from source and found that it was using
> > StringSearchModelInterpolator instead of StringVisitorModelInterpolator,
> a
> > regression from the last 3.6.2 release.
> >
> > I found that in maven-model-builder/META-INF/sisu/javax.injected.Named
> > file, both interpolators are listed but in reverse order (comparing it
> with
> > the same file in 3.6.2), that is StringSearchModelInterpolator appear
> first
> > in the file and StringVisitorModelInterpolator second.
> >
> > (I believe the dependency injection picks up the first one it finds with
> > the right type.)
> >
> > Deleting the @Named annotation from StringSearchModelInterpolator solved
> > the issue, as this make the entry disappear from the javax.injected.Named
> > file. Then the dependency injection uses the
> > StringVisitorModelInterpolator.
> >
> > Can anyone confirm this issue?
> > And if so, how to best prevent in the future that this type of dependency
> > injection regressions from happening?
> >
> > Kind regards,
> > Gabriel
> >
>
Reply | Threaded
Open this post in threaded view
|

Re: Regression in model interpolation coming from using JSR-330?

Stuart McCulloch
As Hervé noted, much of this is the way Maven (and Plexus) has always
behaved - the core defines component roles that plugins/extensions can
implement. There will typically be a core implementation (sometimes named
as the default, but not always) but that implementation can be
overridden by implementations in plugins/extensions so people can extend
and augment Maven. Sometimes the consumer of a component role wants to see
all implementations and will inject a list or a map (keyed by name) - for
example handlers of different archive types where you might ship a stock
set of handlers, but want to allow plugins to add handlers for other
archive types.

So there has always been multiple implementations of a given component
role, and an ordering to decide which one to return when someone asks for
just one implementation of that role (compared to asking for a list or a
map). Historically Plexus always gave implementations in plugins/extensions
a slightly higher priority than core - and "newer" plugins in terms of when
they were added to the runtime build a slightly higher priority than
"older" plugins. The ordering of implementations inside a given realm or
classloader followed the "classpath" ordering of the jars, while the
ordering of implementations inside a jar followed their order in the Plexus
XML descriptor.

Almost 10 years ago Maven switched from the old Plexus container to a new
shim built on top of Guice that supports the same Plexus API and behaviour.
In other words, the same ranking between core and plugin/extension
implementations and the same classpath/descriptor ordering for
implementations in the same classloader/jar. When moving from Plexus
annotations to JSR330 annotations the same behaviour was applied to the
order of implementations in the javax.injected.Named descriptor (which is
in some ways a replacement for Plexus' components.xml.) The code and
behaviour to manage all this has been there for a while since the shim
manages both sets of annotations and maps them to Guice bindings, it's only
recently that selected parts of Maven have been switching from Plexus
annotations to JSR330 annotations.

This is why the ordering in the javax.injected.Named descriptor makes a
difference in terms of multiple implementations in the same jar - it's
following the same historical ordering that Plexus would apply to
implementations in components.xml. The difference is that
the javax.injected.Named descriptor is typically built as the code is
compiled by using an annotation processor, rather than as a separate
packaging step (although that can be configured when it makes sense such as
generating an uber-index for a collection of archives.) The use of
an annotation processor means that differences in the order that javac
compiles the classes can currently affect the order of them in the
descriptor. A lot of the time that ordering doesn't change, especially when
you're using the same OS and version of java, but to make sure we get
completely reproducible builds Hervé has proposed a stricter ordering on
the javax.injected.Named descriptor.

On Sun, 3 Nov 2019 at 04:31, Gabriel Belingueres <[hidden email]>
wrote:

> Thanks Stuart.
>
> The reproducibility PR you mention helps in having a deterministic ordering
> inside the Named components file to buld exactly the same executable, but
> it does not mean it is the right priority for component injection.
>
> Do you know if it is possible to configure sisu to throw an exception if
> trying to inject an ambiguous component? Otherwise, I guess we must
> implement some sort of "ambiguity checker", either as an integration test
> (don't know if it is possible) or either built-it into maven executable.
>
>
>
>
> El sáb., 2 de nov. de 2019 a la(s) 20:54, Stuart McCulloch (
> [hidden email]) escribió:
>
> > Yes, when there are multiple non-default implementations with the same
> > priority then it will pick the first in the list (where the "list" in
> > question is actually a combination of plugins + extensions + core
> > bindings for the given type)
> >
> > If a particular implementation should be treated as the default then it
> > should either start with "Default" or be annotated with
> @Named("default") -
> > the benefit of this approach is that it documents that this is the
> default
> > implementation, to be favoured over the others. Alternatively if you want
> > to have an ordering between implementations then you can use @Priority to
> > assign specific priorities and favour one over the other.
> >
> > If you don't mark an implementation as default and don't define any
> > priorities then the only current guarantees are that implementations in
> > plugins and extensions will be favoured over implementations in core (to
> > allow overriding). But there is an upcoming improvement to sort the list
> > inside each module that would make this more deterministic from build to
> > build, at least when ranking implementations inside a particular module:
> > https://github.com/eclipse/sisu.inject/pull/5 - with that change then
> > you'll get an extra guarantee that inside a module it will be ordered by
> > package+name.
> >
> > PS. even with the old Plexus runtime annotations you could be at the whim
> > of classpath ordering, similarly Plexus XML descriptors are influenced by
> > classpath resource ordering - so ideally it's better to be explicit about
> > ordering
> >
> > On Sat, 2 Nov 2019 at 23:07, Gabriel Belingueres <[hidden email]>
> > wrote:
> >
> > > Hi:
> > >
> > > I just built maven core from source and found that it was using
> > > StringSearchModelInterpolator instead of
> StringVisitorModelInterpolator,
> > a
> > > regression from the last 3.6.2 release.
> > >
> > > I found that in maven-model-builder/META-INF/sisu/javax.injected.Named
> > > file, both interpolators are listed but in reverse order (comparing it
> > with
> > > the same file in 3.6.2), that is StringSearchModelInterpolator appear
> > first
> > > in the file and StringVisitorModelInterpolator second.
> > >
> > > (I believe the dependency injection picks up the first one it finds
> with
> > > the right type.)
> > >
> > > Deleting the @Named annotation from StringSearchModelInterpolator
> solved
> > > the issue, as this make the entry disappear from the
> javax.injected.Named
> > > file. Then the dependency injection uses the
> > > StringVisitorModelInterpolator.
> > >
> > > Can anyone confirm this issue?
> > > And if so, how to best prevent in the future that this type of
> dependency
> > > injection regressions from happening?
> > >
> > > Kind regards,
> > > Gabriel
> > >
> >
>
Reply | Threaded
Open this post in threaded view
|

Re: Regression in model interpolation coming from using JSR-330?

Gabriel Belingueres-2
In reply to this post by Gabriel Belingueres-2
Hi Hervé:

I was thinking in the lines of some checking at the maven core level only,
enough to detect problems and ensure there are no regressions.

At the plugin level, I guess the assurance that the right component is
injected can be checked with an integration test.

The way I realized the problem was that I was adding some logging traces
into the StringVisitorModelInterpolator class but *nothing* showed (even
the build was successful and the interpolation indeed was happening
somewhere). Then added the same logging lines to
StringSearchModelInterpolator, and the logging appeared in the build, which
confirmed the issue. (I didn't test if there are any other component with
the same problem.)

El dom., 3 de nov. de 2019 a la(s) 06:27, Hervé BOUTEMY (
[hidden email]) escribió:

> nice idea, but overriding by adding a new implementation in classpath is
> the
> normal overriding way: not sure how "ambiguity checker" could make a
> difference
> between such wanted override vs unwanted ambiguity
>
> BTW, how did you see that StringVisitorModelInterpolator was not used but
> StringSearchModelInterpolator instead, please?
>
> Regards,
>
> Hervé
>
> Le dimanche 3 novembre 2019, 05:30:50 CET Gabriel Belingueres a écrit :
> > Thanks Stuart.
> >
> > The reproducibility PR you mention helps in having a deterministic
> ordering
> > inside the Named components file to buld exactly the same executable, but
> > it does not mean it is the right priority for component injection.
> >
> > Do you know if it is possible to configure sisu to throw an exception if
> > trying to inject an ambiguous component? Otherwise, I guess we must
> > implement some sort of "ambiguity checker", either as an integration test
> > (don't know if it is possible) or either built-it into maven executable.
> >
> >
> >
> >
> > El sáb., 2 de nov. de 2019 a la(s) 20:54, Stuart McCulloch (
> >
> > [hidden email]) escribió:
> > > Yes, when there are multiple non-default implementations with the same
> > > priority then it will pick the first in the list (where the "list" in
> > > question is actually a combination of plugins + extensions + core
> > > bindings for the given type)
> > >
> > > If a particular implementation should be treated as the default then it
> > > should either start with "Default" or be annotated with
> @Named("default")
> > > -
> > > the benefit of this approach is that it documents that this is the
> default
> > > implementation, to be favoured over the others. Alternatively if you
> want
> > > to have an ordering between implementations then you can use @Priority
> to
> > > assign specific priorities and favour one over the other.
> > >
> > > If you don't mark an implementation as default and don't define any
> > > priorities then the only current guarantees are that implementations in
> > > plugins and extensions will be favoured over implementations in core
> (to
> > > allow overriding). But there is an upcoming improvement to sort the
> list
> > > inside each module that would make this more deterministic from build
> to
> > > build, at least when ranking implementations inside a particular
> module:
> > > https://github.com/eclipse/sisu.inject/pull/5 - with that change then
> > > you'll get an extra guarantee that inside a module it will be ordered
> by
> > > package+name.
> > >
> > > PS. even with the old Plexus runtime annotations you could be at the
> whim
> > > of classpath ordering, similarly Plexus XML descriptors are influenced
> by
> > > classpath resource ordering - so ideally it's better to be explicit
> about
> > > ordering
> > >
> > > On Sat, 2 Nov 2019 at 23:07, Gabriel Belingueres <
> [hidden email]>
> > >
> > > wrote:
> > > > Hi:
> > > >
> > > > I just built maven core from source and found that it was using
> > > > StringSearchModelInterpolator instead of
> StringVisitorModelInterpolator,
> > >
> > > a
> > >
> > > > regression from the last 3.6.2 release.
> > > >
> > > > I found that in
> maven-model-builder/META-INF/sisu/javax.injected.Named
> > > > file, both interpolators are listed but in reverse order (comparing
> it
> > >
> > > with
> > >
> > > > the same file in 3.6.2), that is StringSearchModelInterpolator appear
> > >
> > > first
> > >
> > > > in the file and StringVisitorModelInterpolator second.
> > > >
> > > > (I believe the dependency injection picks up the first one it finds
> with
> > > > the right type.)
> > > >
> > > > Deleting the @Named annotation from StringSearchModelInterpolator
> solved
> > > > the issue, as this make the entry disappear from the
> > > > javax.injected.Named
> > > > file. Then the dependency injection uses the
> > > > StringVisitorModelInterpolator.
> > > >
> > > > Can anyone confirm this issue?
> > > > And if so, how to best prevent in the future that this type of
> > > > dependency
> > > > injection regressions from happening?
> > > >
> > > > Kind regards,
> > > > Gabriel
>
>
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>