Re: [08/17] maven git commit: [MNG-6182] ModelResolver interface enhancements.

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: [08/17] maven git commit: [MNG-6182] ModelResolver interface enhancements.

stephenconnolly
Given that these new methods actually have implementations, could we see
about having at least a unit test of the new code - since it will not be
covered by any test.

If we have no test and no usage, then we could realistically replace the
implementations with `throw new UnsupportedOperationsException("Not
implemented yet");` which would defeat the claimed purpose on the JIRA of
making this code available to IDE integrators.

So I think to make "live" code available, we need at least a sanity check
of the new code with a unit test or two... nothing fancy.

On 12 March 2017 at 16:32, <[hidden email]> wrote:

> [MNG-6182] ModelResolver interface enhancements.
>
>
> Project: http://git-wip-us.apache.org/repos/asf/maven/repo
> Commit: http://git-wip-us.apache.org/repos/asf/maven/commit/ab800b0c
> Tree: http://git-wip-us.apache.org/repos/asf/maven/tree/ab800b0c
> Diff: http://git-wip-us.apache.org/repos/asf/maven/diff/ab800b0c
>
> Branch: refs/heads/MNG-6169
> Commit: ab800b0cfae4e3ca9453304e3b9727ba4a4b712b
> Parents: 114ef6c
> Author: Christian Schulte <[hidden email]>
> Authored: Sat Jan 30 19:17:34 2016 +0100
> Committer: Christian Schulte <[hidden email]>
> Committed: Wed Mar 8 18:24:18 2017 +0100
>
> ----------------------------------------------------------------------
>  .../maven/project/ProjectModelResolver.java     | 84 +++++++++++++++----
>  .../maven/model/resolution/ModelResolver.java   | 32 ++++++++
>  .../internal/DefaultModelResolver.java          | 85 ++++++++++++++++----
>  3 files changed, 167 insertions(+), 34 deletions(-)
> ----------------------------------------------------------------------
>
>
> http://git-wip-us.apache.org/repos/asf/maven/blob/ab800b0c/
> maven-core/src/main/java/org/apache/maven/project/
> ProjectModelResolver.java
> ----------------------------------------------------------------------
> diff --git a/maven-core/src/main/java/org/apache/maven/project/ProjectModelResolver.java
> b/maven-core/src/main/java/org/apache/maven/project/
> ProjectModelResolver.java
> index 7b93217..3a31d33 100644
> --- a/maven-core/src/main/java/org/apache/maven/project/
> ProjectModelResolver.java
> +++ b/maven-core/src/main/java/org/apache/maven/project/
> ProjectModelResolver.java
> @@ -28,7 +28,7 @@ import java.util.Set;
>
>  import com.google.common.base.Predicate;
>  import com.google.common.collect.Iterables;
> -
> +import org.apache.maven.model.Dependency;
>  import org.apache.maven.model.Parent;
>  import org.apache.maven.model.Repository;
>  import org.apache.maven.model.building.FileModelSource;
> @@ -203,24 +203,26 @@ public class ProjectModelResolver
>          return new FileModelSource( pomFile );
>      }
>
> -    public ModelSource resolveModel( Parent parent )
> +    @Override
> +    public ModelSource resolveModel( final Parent parent )
>          throws UnresolvableModelException
>      {
> -        Artifact artifact = new DefaultArtifact( parent.getGroupId(),
> parent.getArtifactId(), "", "pom",
> -                                                 parent.getVersion() );
> -
> -        VersionRangeRequest versionRangeRequest = new
> VersionRangeRequest( artifact, repositories, context );
> -        versionRangeRequest.setTrace( trace );
> -
>          try
>          {
> -            VersionRangeResult versionRangeResult =
> resolver.resolveVersionRange( session, versionRangeRequest );
> +            final Artifact artifact = new DefaultArtifact(
> parent.getGroupId(), parent.getArtifactId(), "", "pom",
> +
>  parent.getVersion() );
> +
> +            final VersionRangeRequest versionRangeRequest = new
> VersionRangeRequest( artifact, repositories, context );
> +            versionRangeRequest.setTrace( trace );
> +
> +            final VersionRangeResult versionRangeResult =
> resolver.resolveVersionRange( session, versionRangeRequest );
>
>              if ( versionRangeResult.getHighestVersion() == null )
>              {
> -                throw new UnresolvableModelException( "No versions
> matched the requested range '" + parent.getVersion()
> -                                                          + "'",
> parent.getGroupId(), parent.getArtifactId(),
> -                                                      parent.getVersion()
> );
> +                throw new UnresolvableModelException(
> +                    String.format( "No versions matched the requested
> parent version range '%s'",
> +                                   parent.getVersion() ),
> +                    parent.getGroupId(), parent.getArtifactId(),
> parent.getVersion() );
>
>              }
>
> @@ -229,21 +231,69 @@ public class ProjectModelResolver
>                       && versionRangeResult.getVersionConstraint().getRange().getUpperBound()
> == null )
>              {
>                  // Message below is checked for in the MNG-2199 core IT.
> -                throw new UnresolvableModelException( "The requested
> version range '" + parent.getVersion()
> -                                                          + "' does not
> specify an upper bound", parent.getGroupId(),
> -
> parent.getArtifactId(), parent.getVersion() );
> +                throw new UnresolvableModelException(
> +                    String.format( "The requested parent version range
> '%s' does not specify an upper bound",
> +                                   parent.getVersion() ),
> +                    parent.getGroupId(), parent.getArtifactId(),
> parent.getVersion() );
>
>              }
>
>              parent.setVersion( versionRangeResult.getHighestVersion().toString()
> );
> +
> +            return resolveModel( parent.getGroupId(),
> parent.getArtifactId(), parent.getVersion() );
>          }
> -        catch ( VersionRangeResolutionException e )
> +        catch ( final VersionRangeResolutionException e )
>          {
>              throw new UnresolvableModelException( e.getMessage(),
> parent.getGroupId(), parent.getArtifactId(),
>                                                    parent.getVersion(), e
> );
>
>          }
> +    }
>
> -        return resolveModel( parent.getGroupId(), parent.getArtifactId(),
> parent.getVersion() );
> +    @Override
> +    public ModelSource resolveModel( final Dependency dependency )
> +        throws UnresolvableModelException
> +    {
> +        try
> +        {
> +            final Artifact artifact = new DefaultArtifact(
> dependency.getGroupId(), dependency.getArtifactId(), "",
> +                                                           "pom",
> dependency.getVersion() );
> +
> +            final VersionRangeRequest versionRangeRequest = new
> VersionRangeRequest( artifact, repositories, context );
> +            versionRangeRequest.setTrace( trace );
> +
> +            final VersionRangeResult versionRangeResult =
> resolver.resolveVersionRange( session, versionRangeRequest );
> +
> +            if ( versionRangeResult.getHighestVersion() == null )
> +            {
> +                throw new UnresolvableModelException(
> +                    String.format( "No versions matched the requested
> dependency version range '%s'",
> +                                   dependency.getVersion() ),
> +                    dependency.getGroupId(), dependency.getArtifactId(),
> dependency.getVersion() );
> +
> +            }
> +
> +            if ( versionRangeResult.getVersionConstraint() != null
> +                     && versionRangeResult.getVersionConstraint().getRange()
> != null
> +                     && versionRangeResult.getVersionConstraint().getRange().getUpperBound()
> == null )
> +            {
> +                // Message below is checked for in the MNG-4463 core IT.
> +                throw new UnresolvableModelException(
> +                    String.format( "The requested dependency version
> range '%s' does not specify an upper bound",
> +                                   dependency.getVersion() ),
> +                    dependency.getGroupId(), dependency.getArtifactId(),
> dependency.getVersion() );
> +
> +            }
> +
> +            dependency.setVersion( versionRangeResult.getHighestVersion().toString()
> );
> +
> +            return resolveModel( dependency.getGroupId(),
> dependency.getArtifactId(), dependency.getVersion() );
> +        }
> +        catch ( VersionRangeResolutionException e )
> +        {
> +            throw new UnresolvableModelException( e.getMessage(),
> dependency.getGroupId(), dependency.getArtifactId(),
> +
> dependency.getVersion(), e );
> +
> +        }
>      }
>  }
>
> http://git-wip-us.apache.org/repos/asf/maven/blob/ab800b0c/
> maven-model-builder/src/main/java/org/apache/maven/model/
> resolution/ModelResolver.java
> ----------------------------------------------------------------------
> diff --git a/maven-model-builder/src/main/java/org/apache/maven/
> model/resolution/ModelResolver.java b/maven-model-builder/src/
> main/java/org/apache/maven/model/resolution/ModelResolver.java
> index c81a536..39695f7 100644
> --- a/maven-model-builder/src/main/java/org/apache/maven/model/resolution/
> ModelResolver.java
> +++ b/maven-model-builder/src/main/java/org/apache/maven/model/resolution/
> ModelResolver.java
> @@ -19,6 +19,7 @@ package org.apache.maven.model.resolution;
>   * under the License.
>   */
>
> +import org.apache.maven.model.Dependency;
>  import org.apache.maven.model.Parent;
>  import org.apache.maven.model.Repository;
>  import org.apache.maven.model.building.ModelSource;
> @@ -47,16 +48,47 @@ public interface ModelResolver
>
>      /**
>       * Tries to resolve the POM for the specified parent coordinates
> possibly updating {@code parent}.
> +     * <p>
> +     * Unlike the {@link #resolveModel(java.lang.String,
> java.lang.String, java.lang.String)} method, this method
> +     * supports version ranges and updates the given {@code parent}
> instance to match the returned {@code ModelSource}.
> +     * If {@code parent} declares a version range, the version
> corresponding to the returned {@code ModelSource} will
> +     * be set on the given {@code parent}.
> +     * </p>
>       *
>       * @param parent The parent coordinates to resolve, must not be
> {@code null}.
> +     *
>       * @return The source of the requested POM, never {@code null}.
> +     *
>       * @throws UnresolvableModelException If the POM could not be
> resolved from any configured repository.
>       * @since 3.2.2
> +     *
> +     * @see Parent#clone()
>       */
>      ModelSource resolveModel( Parent parent )
>          throws UnresolvableModelException;
>
>      /**
> +     * Tries to resolve the POM for the specified dependency coordinates
> possibly updating {@code dependency}.
> +     * <p>
> +     * Unlike the {@link #resolveModel(java.lang.String,
> java.lang.String, java.lang.String)} method, this method
> +     * supports version ranges and updates the given {@code dependency}
> instance to match the returned
> +     * {@code ModelSource}. If {@code dependency} declares a version
> range, the version corresponding to the returned
> +     * {@code ModelSource} will be set on the given {@code dependency}.
> +     * </p>
> +     *
> +     * @param dependency The dependency coordinates to resolve, must not
> be {@code null}.
> +     *
> +     * @return The source of the requested POM, never {@code null}.
> +     *
> +     * @throws UnresolvableModelException If the POM could not be
> resolved from any configured repository.
> +     * @since 3.5.0-alpha-2
> +     *
> +     * @see Dependency#clone()
> +     */
> +    ModelSource resolveModel( Dependency dependency )
> +        throws UnresolvableModelException;
> +
> +    /**
>       * Adds a repository to use for subsequent resolution requests. The
> order in which repositories are added matters,
>       * repositories that were added first should also be searched first.
> When multiple repositories with the same
>       * identifier are added, only the first repository being added will
> be used.
>
> http://git-wip-us.apache.org/repos/asf/maven/blob/ab800b0c/
> maven-resolver-provider/src/main/java/org/apache/maven/
> repository/internal/DefaultModelResolver.java
> ----------------------------------------------------------------------
> diff --git a/maven-resolver-provider/src/main/java/org/apache/maven/
> repository/internal/DefaultModelResolver.java
> b/maven-resolver-provider/src/main/java/org/apache/maven/
> repository/internal/DefaultModelResolver.java
> index 9ea91ff..3e82eb9 100644
> --- a/maven-resolver-provider/src/main/java/org/apache/maven/
> repository/internal/DefaultModelResolver.java
> +++ b/maven-resolver-provider/src/main/java/org/apache/maven/
> repository/internal/DefaultModelResolver.java
> @@ -28,6 +28,7 @@ import java.util.Set;
>
>  import com.google.common.base.Predicate;
>  import com.google.common.collect.Iterables;
> +import org.apache.maven.model.Dependency;
>  import org.apache.maven.model.Parent;
>  import org.apache.maven.model.Repository;
>  import org.apache.maven.model.building.FileModelSource;
> @@ -182,25 +183,27 @@ class DefaultModelResolver
>          return new FileModelSource( pomFile );
>      }
>
> -    public ModelSource resolveModel( Parent parent )
> +    @Override
> +    public ModelSource resolveModel( final Parent parent )
>          throws UnresolvableModelException
>      {
> -        Artifact artifact = new DefaultArtifact( parent.getGroupId(),
> parent.getArtifactId(), "", "pom",
> -                                                 parent.getVersion() );
> -
> -        VersionRangeRequest versionRangeRequest = new
> VersionRangeRequest( artifact, repositories, context );
> -        versionRangeRequest.setTrace( trace );
> -
>          try
>          {
> -            VersionRangeResult versionRangeResult =
> +            final Artifact artifact = new DefaultArtifact(
> parent.getGroupId(), parent.getArtifactId(), "", "pom",
> +
>  parent.getVersion() );
> +
> +            final VersionRangeRequest versionRangeRequest = new
> VersionRangeRequest( artifact, repositories, context );
> +            versionRangeRequest.setTrace( trace );
> +
> +            final VersionRangeResult versionRangeResult =
>                  versionRangeResolver.resolveVersionRange( session,
> versionRangeRequest );
>
>              if ( versionRangeResult.getHighestVersion() == null )
>              {
> -                throw new UnresolvableModelException( "No versions
> matched the requested range '" + parent.getVersion()
> -                                                          + "'",
> parent.getGroupId(), parent.getArtifactId(),
> -                                                      parent.getVersion()
> );
> +                throw new UnresolvableModelException(
> +                    String.format( "No versions matched the requested
> parent version range '%s'",
> +                                   parent.getVersion() ),
> +                    parent.getGroupId(), parent.getArtifactId(),
> parent.getVersion() );
>
>              }
>
> @@ -209,22 +212,70 @@ class DefaultModelResolver
>                       && versionRangeResult.getVersionConstraint().getRange().getUpperBound()
> == null )
>              {
>                  // Message below is checked for in the MNG-2199 core IT.
> -                throw new UnresolvableModelException( "The requested
> version range '" + parent.getVersion()
> -                                                          + "' does not
> specify an upper bound", parent.getGroupId(),
> -
> parent.getArtifactId(), parent.getVersion() );
> +                throw new UnresolvableModelException(
> +                    String.format( "The requested parent version range
> '%s' does not specify an upper bound",
> +                                   parent.getVersion() ),
> +                    parent.getGroupId(), parent.getArtifactId(),
> parent.getVersion() );
>
>              }
>
>              parent.setVersion( versionRangeResult.getHighestVersion().toString()
> );
> +
> +            return resolveModel( parent.getGroupId(),
> parent.getArtifactId(), parent.getVersion() );
>          }
> -        catch ( VersionRangeResolutionException e )
> +        catch ( final VersionRangeResolutionException e )
>          {
>              throw new UnresolvableModelException( e.getMessage(),
> parent.getGroupId(), parent.getArtifactId(),
>                                                    parent.getVersion(), e
> );
>
>          }
> -
> -        return resolveModel( parent.getGroupId(), parent.getArtifactId(),
> parent.getVersion() );
>      }
>
> +    @Override
> +    public ModelSource resolveModel( final Dependency dependency )
> +        throws UnresolvableModelException
> +    {
> +        try
> +        {
> +            final Artifact artifact = new DefaultArtifact(
> dependency.getGroupId(), dependency.getArtifactId(), "",
> +                                                           "pom",
> dependency.getVersion() );
> +
> +            final VersionRangeRequest versionRangeRequest = new
> VersionRangeRequest( artifact, repositories, context );
> +            versionRangeRequest.setTrace( trace );
> +
> +            final VersionRangeResult versionRangeResult =
> +                versionRangeResolver.resolveVersionRange( session,
> versionRangeRequest );
> +
> +            if ( versionRangeResult.getHighestVersion() == null )
> +            {
> +                throw new UnresolvableModelException(
> +                    String.format( "No versions matched the requested
> dependency version range '%s'",
> +                                   dependency.getVersion() ),
> +                    dependency.getGroupId(), dependency.getArtifactId(),
> dependency.getVersion() );
> +
> +            }
> +
> +            if ( versionRangeResult.getVersionConstraint() != null
> +                     && versionRangeResult.getVersionConstraint().getRange()
> != null
> +                     && versionRangeResult.getVersionConstraint().getRange().getUpperBound()
> == null )
> +            {
> +                // Message below is checked for in the MNG-4463 core IT.
> +                throw new UnresolvableModelException(
> +                    String.format( "The requested dependency version
> range '%s' does not specify an upper bound",
> +                                   dependency.getVersion() ),
> +                    dependency.getGroupId(), dependency.getArtifactId(),
> dependency.getVersion() );
> +
> +            }
> +
> +            dependency.setVersion( versionRangeResult.getHighestVersion().toString()
> );
> +
> +            return resolveModel( dependency.getGroupId(),
> dependency.getArtifactId(), dependency.getVersion() );
> +        }
> +        catch ( VersionRangeResolutionException e )
> +        {
> +            throw new UnresolvableModelException( e.getMessage(),
> dependency.getGroupId(), dependency.getArtifactId(),
> +
> dependency.getVersion(), e );
> +
> +        }
> +    }
>  }
>
>
Reply | Threaded
Open this post in threaded view
|

Re: [08/17] maven git commit: [MNG-6182] ModelResolver interface enhancements.

Christian Schulte
Am 03/18/17 um 14:04 schrieb Stephen Connolly:
> Given that these new methods actually have implementations, could we see
> about having at least a unit test of the new code - since it will not be
> covered by any test.

There would be an integration test for the code. Since nothing uses it,
no way to add anything to the ITs.

>
> If we have no test and no usage, then we could realistically replace the
> implementations with `throw new UnsupportedOperationsException("Not
> implemented yet");` which would defeat the claimed purpose on the JIRA of
> making this code available to IDE integrators.

Throwing an UnsupportedOperationException would be an option. Reverting
the commit also would be an option.

> So I think to make "live" code available, we need at least a sanity check
> of the new code with a unit test or two... nothing fancy.

Just tell me what I should do.

a) revert the commit and postpone to 3.6.0.
b) remove the implementation and throw an UnsupportedOperationException

I do not know how the public Maven API has been versioned in the past.
The method will be needed to add support for dependency management
*import* version ranges to the model builder. If we would add this in
3.5.1, we would add methods to an interface of Maven's public API in a
patch release. No one would expect this to happen. We have done this in
the past. Just take a look at the history of the 'ModelResolver'
interface. This is something we should try to avoid to happen as much as
possible.

Regards,
--
Christian


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

Reply | Threaded
Open this post in threaded view
|

Re: [08/17] maven git commit: [MNG-6182] ModelResolver interface enhancements.

stephenconnolly
On Sat 18 Mar 2017 at 21:58, Christian Schulte <[hidden email]> wrote:

> Am 03/18/17 um 14:04 schrieb Stephen Connolly:
> > Given that these new methods actually have implementations, could we see
> > about having at least a unit test of the new code - since it will not be
> > covered by any test.
>
> There would be an integration test for the code. Since nothing uses it,
> no way to add anything to the ITs.
>
> >
> > If we have no test and no usage, then we could realistically replace the
> > implementations with `throw new UnsupportedOperationsException("Not
> > implemented yet");` which would defeat the claimed purpose on the JIRA of
> > making this code available to IDE integrators.
>
> Throwing an UnsupportedOperationException would be an option. Reverting
> the commit also would be an option.
>
> > So I think to make "live" code available, we need at least a sanity check
> > of the new code with a unit test or two... nothing fancy.
>
> Just tell me what I should do.
>
> a) revert the commit and postpone to 3.6.0.
> b) remove the implementation and throw an UnsupportedOperationExceptio


Can you write a unit test (perhaps with mocks) showing that it does
effectively the same as the variant taking Project)?

That'd be good enough for 3.5.0


>
> I do not know how the public Maven API has been versioned in the past.
> The method will be needed to add support for dependency management
> *import* version ranges to the model builder. If we would add this in
> 3.5.1, we would add methods to an interface of Maven's public API in a
> patch release. No one would expect this to happen. We have done this in
> the past. Just take a look at the history of the 'ModelResolver'
> interface. This is something we should try to avoid to happen as much as
> possible.


We do not claim to be following semver.

Semver is not great for a large multi-component system.

If you have a small library, semver makes sense... there is a single
component and people are just consumers of the API so breaking changes are
important.

Now Maven is a build tool chain... we have different classes of consumer:

* users who interact via the pom
* plugin authors who interact via the plugin api
* ide integrators who interact via a different api
* 3rd parties consuming some of core as libraries
* etc

Changes will be "breaking" for some consumers and not for others.

We cannot be "semver" for all those consumers otherwise we would be on
Maven 375.0.0

If we are even close to semver it will. E for the biggest group of
consumers: i.e. Our users.

If we change the dependency resolution in a backwards incompatible way such
that their existing pom will not build... they want to know about it.

So where we are semver-like is the version number we give *to the users*

Everyone else are considered to be a smaller set of people that have to
just suck it up and read the release notes.

So if we are adding a new method to help ide integrators in 3.5.1... whoop
de doo. The user's don't care about that enough to bump it to 4.0.0 (yep
adding a method to an interface pre Java 8 is a *breaking change* if strict
semver we'd have to go to 4.0.0... I told you we weren't strict semver)


>
> Regards,
> --
> Christian
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
> --
Sent from my phone
Reply | Threaded
Open this post in threaded view
|

Re: [08/17] maven git commit: [MNG-6182] ModelResolver interface enhancements.

Christian Schulte
Branch name is MNG-6182

<https://git-wip-us.apache.org/repos/asf?p=maven.git;a=shortlog;h=refs/heads/MNG-6182>

Commit is

<https://git-wip-us.apache.org/repos/asf?p=maven.git;a=commit;h=4fc5e2180b3f230740778b7106b79c5a9cf8619f>

Should I create a separate JIRA issue for this?

Regards,
--
Christian


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

Reply | Threaded
Open this post in threaded view
|

Re: [08/17] maven git commit: [MNG-6182] ModelResolver interface enhancements.

stephenconnolly
On Sun 19 Mar 2017 at 02:58, Christian Schulte <[hidden email]> wrote:


Brief look on my phone says that's ok

Will review more closely later


>
> Should I create a separate JIRA issue for this?
>

No need, tests are for the feature and we should not be adding features
without testing


> Regards,
> --
> Christian
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
> --
Sent from my phone