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

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

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

Hervé BOUTEMY
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]

Reply | Threaded
Open this post in threaded view
|

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

hboutemy
MNG-6799 issue created [1]
proposed fix added to the reproducible branch [2] by simply removing the @Named annotation for the deprecated StringSearchModelInterpolator: CI seems ok
ok for merge?

Regards,

Hervé

[1] https://issues.apache.org/jira/browse/MNG-6799

[2] https://github.com/apache/maven/commit/16961b32b7dccbc9447ed4e18b4e0949d5fe3b56

On 2019/11/03 21:27:21, Stuart McCulloch <[hidden email]> wrote:

> I did a quick review of the various components in core
> and StringVisitorModelInterpolator / StringSearchModelInterpolator appears
> to be the only case where a role (ModelInterpolator) has two non-default
> implementations and another component only asks for the first
> implementation. There are other roles which have multiple implementations,
> but in those cases that's expected because each implementation has a
> specific name and handles a specific case (see the various ProfileActivator
> implementations which are all then injected into a list in
> DefaultProfileSelector.)
>
> It also appears that StringVisitorModelInterpolator was only added recently
> - if it's meant to be a replacement of StringSearchModelInterpolator then
> one option would be to simply remove StringSearchModelInterpolator.
> Alternatively if StringSearchModelInterpolator is needed to be kept around
> for compatibility reasons (such as someone directly extending it outside of
> Maven) then the Named annotation could be removed
> from StringSearchModelInterpolator so it isn't automatically picked up.
>
> Wrt. extra checks, checking for multiple implementations in the same
> realm/classloader that haven't been given specific names is a possibility -
> you can open feature requests at
> https://bugs.eclipse.org/bugs/describecomponents.cgi?product=Sisu
>
> On Sun, 3 Nov 2019 at 19:05, Gabriel Belingueres <[hidden email]>
> wrote:
>
> > 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]
> > >
> > >
> >
>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

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

Gabriel Belingueres-2
@Hervé: The patch worked fine. Thanks!

@Stuart: In the light that the model interpolator was the only case found,
that is, both equally qualifying classes were located inside the same
javax.injected.Named file, I wonder if it would be easier to check for
ambiguities at generation time, in the sisu-maven-plugin (adding an
optional parameter to fail the build if this kind of case is found). WDYT?

Regards,
Gabriel


El mié., 6 de nov. de 2019 a la(s) 13:39, Herve Boutemy ([hidden email])
escribió:

> MNG-6799 issue created [1]
> proposed fix added to the reproducible branch [2] by simply removing the
> @Named annotation for the deprecated StringSearchModelInterpolator: CI
> seems ok
> ok for merge?
>
> Regards,
>
> Hervé
>
> [1] https://issues.apache.org/jira/browse/MNG-6799
>
> [2]
> https://github.com/apache/maven/commit/16961b32b7dccbc9447ed4e18b4e0949d5fe3b56
>
> On 2019/11/03 21:27:21, Stuart McCulloch <[hidden email]> wrote:
> > I did a quick review of the various components in core
> > and StringVisitorModelInterpolator / StringSearchModelInterpolator
> appears
> > to be the only case where a role (ModelInterpolator) has two non-default
> > implementations and another component only asks for the first
> > implementation. There are other roles which have multiple
> implementations,
> > but in those cases that's expected because each implementation has a
> > specific name and handles a specific case (see the various
> ProfileActivator
> > implementations which are all then injected into a list in
> > DefaultProfileSelector.)
> >
> > It also appears that StringVisitorModelInterpolator was only added
> recently
> > - if it's meant to be a replacement of StringSearchModelInterpolator then
> > one option would be to simply remove StringSearchModelInterpolator.
> > Alternatively if StringSearchModelInterpolator is needed to be kept
> around
> > for compatibility reasons (such as someone directly extending it outside
> of
> > Maven) then the Named annotation could be removed
> > from StringSearchModelInterpolator so it isn't automatically picked up.
> >
> > Wrt. extra checks, checking for multiple implementations in the same
> > realm/classloader that haven't been given specific names is a
> possibility -
> > you can open feature requests at
> > https://bugs.eclipse.org/bugs/describecomponents.cgi?product=Sisu
> >
> > On Sun, 3 Nov 2019 at 19:05, Gabriel Belingueres <[hidden email]>
> > wrote:
> >
> > > 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]
> > > >
> > > >
> > >
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>
Reply | Threaded
Open this post in threaded view
|

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

Stuart McCulloch
The index generator is very simple and just indexes named classes, which
makes it very fast with negligible impact on the build.

To spot this kind of ambiguity at generation time would require a deep
analysis of the type hierarchy and caching of those hierarchies to check
for overlaps.

This would quickly become messy (especially for the case where we're
indexing via an annotation processor) compared to checking at runtime.

Also in some ways this is better handled by application ITs. The container
doesn't really have enough context to definitively say "this is wrong"
because it could be that the developer intended to have multiple
implementations and plans to have them all injected into a list. It could
also be that two implementations happen to share the same superclass or
interface which would make them ambiguous if someone asked for an
implementation at that particular type level, but in reality they are only
ever injected as different sub-types where there is no ambiguity.

Compare this to checking at the application level: when adding a
replacement component you'd just need an IT confirming that new component
is being used. Most of the time the replacement is there to fix a bug, so
you'd expect regression tests to catch if it wasn't used. Other reasons to
rewrite or replace a component are performance related, in which case you
might also expect a performance related IT to verify the new implementation
is having an effect.

On Thu, 7 Nov 2019 at 03:08, Gabriel Belingueres <[hidden email]>
wrote:

> @Hervé: The patch worked fine. Thanks!
>
> @Stuart: In the light that the model interpolator was the only case found,
> that is, both equally qualifying classes were located inside the same
> javax.injected.Named file, I wonder if it would be easier to check for
> ambiguities at generation time, in the sisu-maven-plugin (adding an
> optional parameter to fail the build if this kind of case is found). WDYT?
>
> Regards,
> Gabriel
>
>
> El mié., 6 de nov. de 2019 a la(s) 13:39, Herve Boutemy (
> [hidden email])
> escribió:
>
> > MNG-6799 issue created [1]
> > proposed fix added to the reproducible branch [2] by simply removing the
> > @Named annotation for the deprecated StringSearchModelInterpolator: CI
> > seems ok
> > ok for merge?
> >
> > Regards,
> >
> > Hervé
> >
> > [1] https://issues.apache.org/jira/browse/MNG-6799
> >
> > [2]
> >
> https://github.com/apache/maven/commit/16961b32b7dccbc9447ed4e18b4e0949d5fe3b56
> >
> > On 2019/11/03 21:27:21, Stuart McCulloch <[hidden email]> wrote:
> > > I did a quick review of the various components in core
> > > and StringVisitorModelInterpolator / StringSearchModelInterpolator
> > appears
> > > to be the only case where a role (ModelInterpolator) has two
> non-default
> > > implementations and another component only asks for the first
> > > implementation. There are other roles which have multiple
> > implementations,
> > > but in those cases that's expected because each implementation has a
> > > specific name and handles a specific case (see the various
> > ProfileActivator
> > > implementations which are all then injected into a list in
> > > DefaultProfileSelector.)
> > >
> > > It also appears that StringVisitorModelInterpolator was only added
> > recently
> > > - if it's meant to be a replacement of StringSearchModelInterpolator
> then
> > > one option would be to simply remove StringSearchModelInterpolator.
> > > Alternatively if StringSearchModelInterpolator is needed to be kept
> > around
> > > for compatibility reasons (such as someone directly extending it
> outside
> > of
> > > Maven) then the Named annotation could be removed
> > > from StringSearchModelInterpolator so it isn't automatically picked up.
> > >
> > > Wrt. extra checks, checking for multiple implementations in the same
> > > realm/classloader that haven't been given specific names is a
> > possibility -
> > > you can open feature requests at
> > > https://bugs.eclipse.org/bugs/describecomponents.cgi?product=Sisu
> > >
> > > On Sun, 3 Nov 2019 at 19:05, Gabriel Belingueres <
> [hidden email]>
> > > wrote:
> > >
> > > > 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]
> > > > >
> > > > >
> > > >
> > >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: [hidden email]
> > For additional commands, e-mail: [hidden email]
> >
> >
>
Reply | Threaded
Open this post in threaded view
|

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

Tibor Digana
Question.
Why we used @Named? I was talking about it in before that the names must
not be the same but i do not understand why we use @Named since there is no
benefit from expression language in Maven where the name of bean is only
useful as a reference to the bean.
Additionally, two beans having @Named @Singleton implementing the same
interface is a strange combination. Even if somebody wants to inject the
interface it will be the same problem. And the same with names.

On Thu, Nov 7, 2019 at 4:48 AM Stuart McCulloch <[hidden email]> wrote:

> The index generator is very simple and just indexes named classes, which
> makes it very fast with negligible impact on the build.
>
> To spot this kind of ambiguity at generation time would require a deep
> analysis of the type hierarchy and caching of those hierarchies to check
> for overlaps.
>
> This would quickly become messy (especially for the case where we're
> indexing via an annotation processor) compared to checking at runtime.
>
> Also in some ways this is better handled by application ITs. The container
> doesn't really have enough context to definitively say "this is wrong"
> because it could be that the developer intended to have multiple
> implementations and plans to have them all injected into a list. It could
> also be that two implementations happen to share the same superclass or
> interface which would make them ambiguous if someone asked for an
> implementation at that particular type level, but in reality they are only
> ever injected as different sub-types where there is no ambiguity.
>
> Compare this to checking at the application level: when adding a
> replacement component you'd just need an IT confirming that new component
> is being used. Most of the time the replacement is there to fix a bug, so
> you'd expect regression tests to catch if it wasn't used. Other reasons to
> rewrite or replace a component are performance related, in which case you
> might also expect a performance related IT to verify the new implementation
> is having an effect.
>
> On Thu, 7 Nov 2019 at 03:08, Gabriel Belingueres <[hidden email]>
> wrote:
>
> > @Hervé: The patch worked fine. Thanks!
> >
> > @Stuart: In the light that the model interpolator was the only case
> found,
> > that is, both equally qualifying classes were located inside the same
> > javax.injected.Named file, I wonder if it would be easier to check for
> > ambiguities at generation time, in the sisu-maven-plugin (adding an
> > optional parameter to fail the build if this kind of case is found).
> WDYT?
> >
> > Regards,
> > Gabriel
> >
> >
> > El mié., 6 de nov. de 2019 a la(s) 13:39, Herve Boutemy (
> > [hidden email])
> > escribió:
> >
> > > MNG-6799 issue created [1]
> > > proposed fix added to the reproducible branch [2] by simply removing
> the
> > > @Named annotation for the deprecated StringSearchModelInterpolator: CI
> > > seems ok
> > > ok for merge?
> > >
> > > Regards,
> > >
> > > Hervé
> > >
> > > [1] https://issues.apache.org/jira/browse/MNG-6799
> > >
> > > [2]
> > >
> >
> https://github.com/apache/maven/commit/16961b32b7dccbc9447ed4e18b4e0949d5fe3b56
> > >
> > > On 2019/11/03 21:27:21, Stuart McCulloch <[hidden email]> wrote:
> > > > I did a quick review of the various components in core
> > > > and StringVisitorModelInterpolator / StringSearchModelInterpolator
> > > appears
> > > > to be the only case where a role (ModelInterpolator) has two
> > non-default
> > > > implementations and another component only asks for the first
> > > > implementation. There are other roles which have multiple
> > > implementations,
> > > > but in those cases that's expected because each implementation has a
> > > > specific name and handles a specific case (see the various
> > > ProfileActivator
> > > > implementations which are all then injected into a list in
> > > > DefaultProfileSelector.)
> > > >
> > > > It also appears that StringVisitorModelInterpolator was only added
> > > recently
> > > > - if it's meant to be a replacement of StringSearchModelInterpolator
> > then
> > > > one option would be to simply remove StringSearchModelInterpolator.
> > > > Alternatively if StringSearchModelInterpolator is needed to be kept
> > > around
> > > > for compatibility reasons (such as someone directly extending it
> > outside
> > > of
> > > > Maven) then the Named annotation could be removed
> > > > from StringSearchModelInterpolator so it isn't automatically picked
> up.
> > > >
> > > > Wrt. extra checks, checking for multiple implementations in the same
> > > > realm/classloader that haven't been given specific names is a
> > > possibility -
> > > > you can open feature requests at
> > > > https://bugs.eclipse.org/bugs/describecomponents.cgi?product=Sisu
> > > >
> > > > On Sun, 3 Nov 2019 at 19:05, Gabriel Belingueres <
> > [hidden email]>
> > > > wrote:
> > > >
> > > > > 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]
> > > > > >
> > > > > >
> > > > >
> > > >
> > >
> > > ---------------------------------------------------------------------
> > > To unsubscribe, e-mail: [hidden email]
> > > For additional commands, e-mail: [hidden email]
> > >
> > >
> >
>
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 Stuart McCulloch
Thank you Stuart for your in deep explanations.
I agree runtime checking is the way to go. I was looking for a shorter path
but I see it would become problematic.

El jue., 7 de nov. de 2019 a la(s) 00:48, Stuart McCulloch (
[hidden email]) escribió:

> The index generator is very simple and just indexes named classes, which
> makes it very fast with negligible impact on the build.
>
> To spot this kind of ambiguity at generation time would require a deep
> analysis of the type hierarchy and caching of those hierarchies to check
> for overlaps.
>
> This would quickly become messy (especially for the case where we're
> indexing via an annotation processor) compared to checking at runtime.
>
> Also in some ways this is better handled by application ITs. The container
> doesn't really have enough context to definitively say "this is wrong"
> because it could be that the developer intended to have multiple
> implementations and plans to have them all injected into a list. It could
> also be that two implementations happen to share the same superclass or
> interface which would make them ambiguous if someone asked for an
> implementation at that particular type level, but in reality they are only
> ever injected as different sub-types where there is no ambiguity.
>
> Compare this to checking at the application level: when adding a
> replacement component you'd just need an IT confirming that new component
> is being used. Most of the time the replacement is there to fix a bug, so
> you'd expect regression tests to catch if it wasn't used. Other reasons to
> rewrite or replace a component are performance related, in which case you
> might also expect a performance related IT to verify the new implementation
> is having an effect.
>
> On Thu, 7 Nov 2019 at 03:08, Gabriel Belingueres <[hidden email]>
> wrote:
>
> > @Hervé: The patch worked fine. Thanks!
> >
> > @Stuart: In the light that the model interpolator was the only case
> found,
> > that is, both equally qualifying classes were located inside the same
> > javax.injected.Named file, I wonder if it would be easier to check for
> > ambiguities at generation time, in the sisu-maven-plugin (adding an
> > optional parameter to fail the build if this kind of case is found).
> WDYT?
> >
> > Regards,
> > Gabriel
> >
> >
> > El mié., 6 de nov. de 2019 a la(s) 13:39, Herve Boutemy (
> > [hidden email])
> > escribió:
> >
> > > MNG-6799 issue created [1]
> > > proposed fix added to the reproducible branch [2] by simply removing
> the
> > > @Named annotation for the deprecated StringSearchModelInterpolator: CI
> > > seems ok
> > > ok for merge?
> > >
> > > Regards,
> > >
> > > Hervé
> > >
> > > [1] https://issues.apache.org/jira/browse/MNG-6799
> > >
> > > [2]
> > >
> >
> https://github.com/apache/maven/commit/16961b32b7dccbc9447ed4e18b4e0949d5fe3b56
> > >
> > > On 2019/11/03 21:27:21, Stuart McCulloch <[hidden email]> wrote:
> > > > I did a quick review of the various components in core
> > > > and StringVisitorModelInterpolator / StringSearchModelInterpolator
> > > appears
> > > > to be the only case where a role (ModelInterpolator) has two
> > non-default
> > > > implementations and another component only asks for the first
> > > > implementation. There are other roles which have multiple
> > > implementations,
> > > > but in those cases that's expected because each implementation has a
> > > > specific name and handles a specific case (see the various
> > > ProfileActivator
> > > > implementations which are all then injected into a list in
> > > > DefaultProfileSelector.)
> > > >
> > > > It also appears that StringVisitorModelInterpolator was only added
> > > recently
> > > > - if it's meant to be a replacement of StringSearchModelInterpolator
> > then
> > > > one option would be to simply remove StringSearchModelInterpolator.
> > > > Alternatively if StringSearchModelInterpolator is needed to be kept
> > > around
> > > > for compatibility reasons (such as someone directly extending it
> > outside
> > > of
> > > > Maven) then the Named annotation could be removed
> > > > from StringSearchModelInterpolator so it isn't automatically picked
> up.
> > > >
> > > > Wrt. extra checks, checking for multiple implementations in the same
> > > > realm/classloader that haven't been given specific names is a
> > > possibility -
> > > > you can open feature requests at
> > > > https://bugs.eclipse.org/bugs/describecomponents.cgi?product=Sisu
> > > >
> > > > On Sun, 3 Nov 2019 at 19:05, Gabriel Belingueres <
> > [hidden email]>
> > > > wrote:
> > > >
> > > > > 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]
> > > > > >
> > > > > >
> > > > >
> > > >
> > >
> > > ---------------------------------------------------------------------
> > > To unsubscribe, e-mail: [hidden email]
> > > For additional commands, e-mail: [hidden email]
> > >
> > >
> >
>