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 ( mccu...@gmail.com) 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 <belingue...@gmail.com> > 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 ( > > hbout...@apache.org) > > 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 <mccu...@gmail.com> 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 < > > belingue...@gmail.com> > > > > 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 ( > > > > > herve.bout...@free.fr) 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 ( > > > > > > > > > > > > > > mccu...@gmail.com) 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 < > > > > > > belingue...@gmail.com> > > > > > > > > > > > > > > > > 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: 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 > > > > > > > > >
