[jira] [Comment Edited] (MRESOLVER-157) Get rid of ServiceLocator in Resolver

["Grzegorz Grzybek (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/MRESOLVER-157?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17612236#comment-17612236 ] 

Grzegorz Grzybek edited comment on MRESOLVER-157 at 10/3/22 10:02 AM:
----------------------------------------------------------------------

I'm checking CAMEL-18555 - to replace shrinkwrap-resolver (which uses {{org.jboss.shrinkwrap.resolver.impl.maven.bootstrap.ShrinkWrapResolverServiceLocator}} implementation of now deprecated {{org.eclipse.aether.spi.locator.ServiceLocator}}).

Checking [this comment|https://github.com/ops4j/org.ops4j.pax.url/issues/397#issuecomment-915116644] from [~cstamas] I feel there still should be a dumb, reflection/DI free way to configure a graph of objects. In theory it is possible, because I can:
# instantiate {{org.eclipse.aether.internal.impl.DefaultRepositorySystem}}
# instantiate {{org.apache.maven.repository.internal.DefaultVersionResolver}}
# call {{org.eclipse.aether.internal.impl.DefaultRepositorySystem#setVersionResolver()}} on the first passing the second

However such dumb (but google code free) solution requires public constructors and setters. While {{DefaultRepositorySystem}}'s setter is fine (maven-resolver-impl-1.8.2):
{code:java}
public DefaultRepositorySystem()
{
    // enables default constructor
}
{code}

It's more problematic in {{org.eclipse.aether.internal.impl.collect.df.DfDependencyCollector}}:
{code:java}
/**
 * Default ctor for SL.
 *
 * @deprecated Will be dropped once SL gone.
 */
@Deprecated
public DfDependencyCollector()
{
    // enables default constructor
}
{code}

If/when the no-arg constructor is dropped, I'll be left with package-private:
{code:java}
@Inject
DfDependencyCollector( RemoteRepositoryManager remoteRepositoryManager,
                       ArtifactDescriptorReader artifactDescriptorReader,
                       VersionRangeResolver versionRangeResolver )
{
    super( remoteRepositoryManager, artifactDescriptorReader, versionRangeResolver );
}
{code}

So back to sisu/guice or reflection ({{setAccessible()}} code (or shadowing the packages)).

I think it'd be good to keep the injection points (constructors, setters) public. WDYT?


was (Author: gzres):
I'm checking CAMEL-18555 - to replace shrinkwrap-resolver (which uses {{org.jboss.shrinkwrap.resolver.impl.maven.bootstrap.ShrinkWrapResolverServiceLocator}} implementation of now deprecated {{org.eclipse.aether.spi.locator.ServiceLocator}}).

Checking [this comment|https://github.com/ops4j/org.ops4j.pax.url/issues/397#issuecomment-915116644] from [~cstamas] I feel there still should be a dumb, reflection/DI free way to configure a graph of objects. In theory it is possible, because I can:
# instantiate {{org.eclipse.aether.internal.impl.DefaultRepositorySystem}}
# instantiate {{org.apache.maven.repository.internal.DefaultVersionResolver}}
# call {{org.eclipse.aether.internal.impl.DefaultRepositorySystem#setVersionResolver()}} on the first passing the second

However such dumb (but google code free) solution requires public constructors and setters. While {{DefaultRepositorySystem}}'s setter is fine (maven-resolver-impl-1.8.2):
{code:java}
public DefaultRepositorySystem()
{
    // enables default constructor
}
{code}

It's more problematic in {{org.eclipse.aether.internal.impl.collect.df.DfDependencyCollector}}:
{code:java}
/**
 * Default ctor for SL.
 *
 * @deprecated Will be dropped once SL gone.
 */
@Deprecated
public DfDependencyCollector()
{
    // enables default constructor
}
{code}

If/when the no-arg constructor is dropped, I'll be left with package-private:
{code:java}
@Inject
DfDependencyCollector( RemoteRepositoryManager remoteRepositoryManager,
                       ArtifactDescriptorReader artifactDescriptorReader,
                       VersionRangeResolver versionRangeResolver )
{
    super( remoteRepositoryManager, artifactDescriptorReader, versionRangeResolver );
}
{code}

So back to sisu/guice or reflection ({{setAccessible()}} code (or shadowing the packages).

I think it'd be good to keep the injection points (constructors, setters) public. WDYT?

> Get rid of ServiceLocator in Resolver
> -------------------------------------
>
>                 Key: MRESOLVER-157
>                 URL: https://issues.apache.org/jira/browse/MRESOLVER-157
>             Project: Maven Resolver
>          Issue Type: Task
>          Components: Resolver
>            Reporter: Tamás Cservenák
>            Priority: Major
>
> maven-resolver currently supports:
>  * ServiceLocator (deprecated as of 1.7.0 see MRESOLVER-160)
>  * "vanilla" Guice (provides a module)
>  * DI using Sisu, as used in Maven
> IMO, it makes not much sense to support 3 vastly different "DI"s (in quotes as ServiceLocator is really just a dumb factory pattern).
> Not only just complicates the code base, makes changes error prone at least, yields for "exceptions" (this or that will never work with it, as seen with SyncContext), and, for me most importantly, prevents proper constructor initialization and validation of components.



--
This message was sent by Atlassian Jira
(v8.20.10#820010)