Re: Please avoid non-backward compatible API changes - Was: Re: svn commit: r617944 - in /maven/artifact/trunk/src: main/java/org/apache/maven/artifact/ main/java/org/apache/maven/artifact/deployer/ main/java/org/apache/maven/artifact/factory/ main/java/

Sun, 03 Feb 2008 08:37:11 -0800 


On 3-Feb-08, at 3:45 AM, Milos Kleint wrote:


Hello Jason,

Please revert this change or make it backward comaptible. Removing
getArtifactHandler() from Artifact interface might be conceptually
right, but it breaks lots of thing.
Not intentional, I ran everything on this side and the ITs so I'll go track that down. But that change I consider the correction of a fundamental flaw. What I started last night is trying to put all the new artifact code in a separate package and I'm just going to work on it with Oleg. That we have no way to test changes against standard plugins is not good. There are probably more then what's on your list there. 


1. a few tests are broken in the maven components.
2. More important is that every plugin have ever made that call will
break with a nasty NoSuchMethodException when run in 2.1.
A quick search in mojo.codehaus.org reveals that
appassembler-maven-plugin,
aspectj-maven-plugin,
cobertura-maven-plugin,
commons-attributes-maven-plugin,
native-maven-plugin,
taglist-maven-plugin
and webstart-maven-plugin are all affected.
(Done by grepping the current trunk, not counting stuff from sandbox)
3.You've also broken my code in the netbeans integration that uses the
Artifact's getArtifactHandler method as well. I can rewrite my own
code against the latest embedder, however point 2. still applies.
People won't be able to run many projects with the embedded version of
maven and will have to use some 2.0.x command line version. And of
course everyone will blame me :)


A more general note.
The light-heartedness of making non-backward compatible changes in
maven components  and the core code in general gives me nightmares.
On a minor version change yes, but there are things that can never be corrected if what's currently there can never be fixed. I don't think this one will be that hard to bridge using the same method we have using aspects and I'll do that now. But I'm not willing to live with the current Artifact API being around forever. I don't think this is a case where it's irreconcilable, but if there are changes that are required to make it the Artifact resolution work properly then I won't agree. The presence of a component in a piece of data is just so wrong, but I will find a way to bridge it for past users. The factory is a severe pain in the ass, and then it doesn't do anything because the handler is exposed to everything. So they are annoying to construct and additionally we have a component stuck in a piece of data. I have no doubt that grabbing hold of the ArtifactHandlerManager is correct. I'll look at a couple of plugin on that list but why they are using artifact handler seems strange. In the tag-list plugin? 



Plugins can reach any component from the maven core via the
${components.*} property, the MavenProject instance via ${project.*},
the MavenSession and Settings also via the property evaluation.
Whoever added this, effectively commited to making all those
components backward compatible public API. You might get away with
adding methods to the interfaces (it's plugins responsibility to track
when the method was added and setup <mavenPrerequisite>
approproately).
But the removal of any method or changing the signature is unacceptable.
I agree that plugins have to be backward compatible. But I also feel the artifact classes, api, and process is so woefully broken that I am going to correct it. Fortunately I can use the tactic John has setup and put an aspect in place to catch people trying to grab the artifact handler directly and I'll deprecate that method. 


Thanks.

Milos

On Feb 3, 2008 5:19 AM,  <[EMAIL PROTECTED]> wrote:

Author: jvanzyl
Date: Sat Feb  2 20:19:44 2008
New Revision: 617944

URL: http://svn.apache.org/viewvc?rev=617944&view=rev
Log:
o The Artifact should serve as datum, or representation of resource in a Maven repository. It currently has way too much information, and also has references to components. In a first pass I have decoupled the ArtifactHandler from Artifact so that it is no longer required. This means that the primary requirement of the ArtifactFactory is gone and you can now use a simple constructor to create an 
 Artifact.

Modified:
maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ Artifact.java maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ ArtifactUtils.java maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ DefaultArtifact.java maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ deployer/DefaultArtifactDeployer.java maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ factory/DefaultArtifactFactory.java maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ installer/DefaultArtifactInstaller.java maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ repository/layout/DefaultRepositoryLayout.java maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ repository/layout/FlatRepositoryLayout.java maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ repository/layout/LegacyRepositoryLayout.java maven/artifact/trunk/src/test/java/org/apache/maven/artifact/ DefaultArtifactTest.java maven/artifact/trunk/src/test/java/org/apache/maven/artifact/ manager/DefaultWagonManagerTest.java
Modified: maven/artifact/trunk/src/main/java/org/apache/maven/ artifact/Artifact.java 
URL: 
http://svn.apache.org/viewvc/maven/artifact/trunk/src/main/java/org/apache/maven/artifact/Artifact.java?rev=617944&r1=617943&r2=617944&view=diff
= = = = = = = = = ===================================================================== --- maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ Artifact.java (original) +++ maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ Artifact.java Sat Feb 2 20:19:44 2008 
@@ -119,8 +119,6 @@

    void setDependencyFilter( ArtifactFilter artifactFilter );

-    ArtifactHandler getArtifactHandler();
-
    List getDependencyTrail();

    void setDependencyTrail( List dependencyTrail );
@@ -144,9 +142,6 @@
    boolean isResolved();

    void setResolvedVersion( String version );
-
-    /** @todo remove, a quick hack for the lifecycle executor */
-    void setArtifactHandler( ArtifactHandler handler );

    boolean isRelease();
Modified: maven/artifact/trunk/src/main/java/org/apache/maven/ artifact/ArtifactUtils.java 
URL: 
http://svn.apache.org/viewvc/maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ArtifactUtils.java?rev=617944&r1=617943&r2=617944&view=diff
= = = = = = = = = ===================================================================== --- maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ ArtifactUtils.java (original) +++ maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ ArtifactUtils.java Sat Feb 2 20:19:44 2008 
@@ -150,10 +150,10 @@
        {
range = VersionRange.createFromVersion( artifact.getVersion() ); 
        }
-
+
DefaultArtifact clone = new DefaultArtifact( artifact.getGroupId(), artifact.getArtifactId(), range.cloneOf(), - artifact.getScope(), artifact.getType(), artifact.getClassifier(), 
-            artifact.getArtifactHandler(), artifact.isOptional() );
+ artifact.getScope(), artifact.getType(), artifact.getClassifier(), artifact.isOptional() ); 
+
        clone.setRelease( artifact.isRelease() );
        clone.setResolvedVersion( artifact.getVersion() );
        clone.setResolved( artifact.isResolved() );
Modified: maven/artifact/trunk/src/main/java/org/apache/maven/ artifact/DefaultArtifact.java 
URL: 
http://svn.apache.org/viewvc/maven/artifact/trunk/src/main/java/org/apache/maven/artifact/DefaultArtifact.java?rev=617944&r1=617943&r2=617944&view=diff
= = = = = = = = = ===================================================================== --- maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ DefaultArtifact.java (original) +++ maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ DefaultArtifact.java Sat Feb 2 20:19:44 2008 
@@ -37,7 +37,7 @@
import java.util.regex.Matcher;

/**
- * @author <a href="mailto:[EMAIL PROTECTED]">Jason van Zyl </a>
+ * @author Jason van Zyl
 * @version $Id$
 * @todo this should possibly be replaced by type handler
 */
@@ -48,13 +48,6 @@

    private String artifactId;

-    /**
- * The resolved version for the artifact after conflict resolution, that has not been transformed. 
-     *
-     * @todo should be final
-     */
-    private String baseVersion;
-
    private final String type;

    private final String classifier;
@@ -63,28 +56,40 @@

    private File file;
+ // Why is this here? What repository is determined at runtime and is therefore a 
+    // runtime charactistic. This needs to go. jvz.
    private ArtifactRepository repository;

    private String downloadUrl;

+    // Why is this here? jvz.
    private ArtifactFilter dependencyFilter;

-    private ArtifactHandler artifactHandler;
-
+    // Why is this here? jvz?
    private List dependencyTrail;

+    /**
+ * The resolved version for the artifact after conflict resolution, that has not been transformed. 
+     *
+     * @todo should be final
+     */
+    private String baseVersion;
+
    private String version;

    private VersionRange versionRange;

    private boolean resolved;

+    // This is specific to maven. jvz.
    private boolean release;
+ // If the version is stored here (above), why on earth do we store the available versions here? jvz. 
    private List availableVersions;

    private Map metadataMap;

+    // This is Maven specific. jvz/
    private boolean optional;

    public DefaultArtifact( String groupId,
@@ -92,10 +97,9 @@
                            VersionRange versionRange,
                            String scope,
                            String type,
-                            String classifier,
-                            ArtifactHandler artifactHandler )
+                            String classifier )
    {
- this( groupId, artifactId, versionRange, scope, type, classifier, artifactHandler, false ); + this( groupId, artifactId, versionRange, scope, type, classifier, false ); 
    }

    public DefaultArtifact( String groupId,
@@ -104,7 +108,6 @@
                            String scope,
                            String type,
                            String classifier,
-                            ArtifactHandler artifactHandler,
                            boolean optional )
    {
        this.groupId = groupId;
@@ -115,17 +118,10 @@

        selectVersionFromNewRangeIfAvailable();

-        this.artifactHandler = artifactHandler;
-
        this.scope = scope;

        this.type = type;

-        if ( classifier == null )
-        {
-            classifier = artifactHandler.getClassifier();
-        }
-
        this.classifier = classifier;

        this.optional = optional;
@@ -474,11 +470,6 @@
        dependencyFilter = artifactFilter;
    }

-    public ArtifactHandler getArtifactHandler()
-    {
-        return artifactHandler;
-    }
-
    public List getDependencyTrail()
    {
        return dependencyTrail;
@@ -561,11 +552,6 @@
    {
        this.version = version;
        // retain baseVersion
-    }
-
- public void setArtifactHandler( ArtifactHandler artifactHandler ) 
-    {
-        this.artifactHandler = artifactHandler;
    }

    public void setRelease( boolean release )
Modified: maven/artifact/trunk/src/main/java/org/apache/maven/ artifact/deployer/DefaultArtifactDeployer.java 
URL: 
http://svn.apache.org/viewvc/maven/artifact/trunk/src/main/java/org/apache/maven/artifact/deployer/DefaultArtifactDeployer.java?rev=617944&r1=617943&r2=617944&view=diff
= = = = = = = = = ===================================================================== --- maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ deployer/DefaultArtifactDeployer.java (original) +++ maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ deployer/DefaultArtifactDeployer.java Sat Feb 2 20:19:44 2008 
@@ -20,6 +20,7 @@
 */

import org.apache.maven.artifact.Artifact;
+import org.apache.maven.artifact.handler.manager.ArtifactHandlerManager; 
import org.apache.maven.artifact.manager.WagonManager;
import org.apache.maven.artifact.metadata.ArtifactMetadata;
import org .apache.maven.artifact.metadata.ArtifactMetadataRetrievalException; 
@@ -63,6 +64,9 @@
    /** @plexus.requirement role-hint="default" */
    private ArtifactRepositoryLayout defaultLayout;

+    /** @plexus.requirement */
+    private ArtifactHandlerManager artifactHandlerManager;
+
/** @deprecated we want to use the artifact method only, and ensure artifact.file is set correctly. */ 
    public void deploy( String basedir,
                        String finalName,
@@ -71,7 +75,8 @@
                        ArtifactRepository localRepository )
        throws ArtifactDeploymentException
    {
- String extension = artifact.getArtifactHandler().getExtension(); + String extension = artifactHandlerManager .getArtifactHandler( artifact.getType() ).getExtension(); 
+
File source = new File( basedir, finalName + "." + extension ); deploy( source, artifact, deploymentRepository, localRepository ); 
    }
Modified: maven/artifact/trunk/src/main/java/org/apache/maven/ artifact/factory/DefaultArtifactFactory.java 
URL: 
http://svn.apache.org/viewvc/maven/artifact/trunk/src/main/java/org/apache/maven/artifact/factory/DefaultArtifactFactory.java?rev=617944&r1=617943&r2=617944&view=diff
= = = = = = = = = ===================================================================== --- maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ factory/DefaultArtifactFactory.java (original) +++ maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ factory/DefaultArtifactFactory.java Sat Feb 2 20:19:44 2008 
@@ -211,9 +211,6 @@
            desiredScope = Artifact.SCOPE_SYSTEM;
        }
- ArtifactHandler handler = artifactHandlerManager.getArtifactHandler( type ); 
-
- return new DefaultArtifact( groupId, artifactId, versionRange, desiredScope, type, classifier, handler, 
-            optional );
+ return new DefaultArtifact( groupId, artifactId, versionRange, desiredScope, type, classifier, optional ); 
    }
}
Modified: maven/artifact/trunk/src/main/java/org/apache/maven/ artifact/installer/DefaultArtifactInstaller.java 
URL: 
http://svn.apache.org/viewvc/maven/artifact/trunk/src/main/java/org/apache/maven/artifact/installer/DefaultArtifactInstaller.java?rev=617944&r1=617943&r2=617944&view=diff
= = = = = = = = = ===================================================================== --- maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ installer/DefaultArtifactInstaller.java (original) +++ maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ installer/DefaultArtifactInstaller.java Sat Feb 2 20:19:44 2008 
@@ -20,6 +20,7 @@
 */

import org.apache.maven.artifact.Artifact;
+import org.apache.maven.artifact.handler.manager.ArtifactHandlerManager; 
import org.apache.maven.artifact.metadata.ArtifactMetadata;
import org.apache.maven.artifact.repository.ArtifactRepository;
import org .apache .maven .artifact .repository.metadata.RepositoryMetadataInstallationException; 
@@ -46,6 +47,9 @@
    /** @plexus.requirement */
    private RepositoryMetadataManager repositoryMetadataManager;

+    /** @plexus.requirement */
+    private ArtifactHandlerManager artifactHandlerManager;
+
/** @deprecated we want to use the artifact method only, and ensure artifact.file is set correctly. */ 
    public void install( String basedir,
                         String finalName,
@@ -53,7 +57,8 @@
                         ArtifactRepository localRepository )
        throws ArtifactInstallationException
    {
- String extension = artifact.getArtifactHandler().getExtension(); + String extension = artifactHandlerManager .getArtifactHandler( artifact.getType() ).getExtension(); 
+
File source = new File( basedir, finalName + "." + extension ); 

        install( source, artifact, localRepository );
Modified: maven/artifact/trunk/src/main/java/org/apache/maven/ artifact/repository/layout/DefaultRepositoryLayout.java 
URL: 
http://svn.apache.org/viewvc/maven/artifact/trunk/src/main/java/org/apache/maven/artifact/repository/layout/DefaultRepositoryLayout.java?rev=617944&r1=617943&r2=617944&view=diff
= = = = = = = = = ===================================================================== --- maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ repository/layout/DefaultRepositoryLayout.java (original) +++ maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ repository/layout/DefaultRepositoryLayout.java Sat Feb 2 20:19:44 2008 
@@ -21,6 +21,7 @@

import org.apache.maven.artifact.Artifact;
import org.apache.maven.artifact.handler.ArtifactHandler;
+import org.apache.maven.artifact.handler.manager.ArtifactHandlerManager; 
import org.apache.maven.artifact.metadata.ArtifactMetadata;
import org.apache.maven.artifact.repository.ArtifactRepository;

@@ -37,9 +38,12 @@

    private static final char ARTIFACT_SEPARATOR = '-';

+    /** @plexus.requirement */
+    private ArtifactHandlerManager artifactHandlerManager;
+
    public String pathOf( Artifact artifact )
    {
- ArtifactHandler artifactHandler = artifact.getArtifactHandler(); + ArtifactHandler artifactHandler = artifactHandlerManager.getArtifactHandler( artifact.getType() ); 

        StringBuffer path = new StringBuffer();
Modified: maven/artifact/trunk/src/main/java/org/apache/maven/ artifact/repository/layout/FlatRepositoryLayout.java 
URL: 
http://svn.apache.org/viewvc/maven/artifact/trunk/src/main/java/org/apache/maven/artifact/repository/layout/FlatRepositoryLayout.java?rev=617944&r1=617943&r2=617944&view=diff
= = = = = = = = = ===================================================================== --- maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ repository/layout/FlatRepositoryLayout.java (original) +++ maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ repository/layout/FlatRepositoryLayout.java Sat Feb 2 20:19:44 2008 
@@ -2,9 +2,9 @@

import org.apache.maven.artifact.Artifact;
import org.apache.maven.artifact.handler.ArtifactHandler;
+import org.apache.maven.artifact.handler.manager.ArtifactHandlerManager; 
import org.apache.maven.artifact.metadata.ArtifactMetadata;
import org.apache.maven.artifact.repository.ArtifactRepository;
-import org.apache.maven.artifact.repository.layout.ArtifactRepositoryLayout; 

/**
* The code in this class is taken from DefaultRepositorylayout, located at: 
@@ -20,9 +20,12 @@

    private static final char GROUP_SEPARATOR = '.';

+    /** @plexus.requirement */
+    private ArtifactHandlerManager artifactHandlerManager;
+
    public String pathOf( Artifact artifact )
    {
- ArtifactHandler artifactHandler = artifact.getArtifactHandler(); + ArtifactHandler artifactHandler = artifactHandlerManager.getArtifactHandler( artifact.getType() ); 

        StringBuffer path = new StringBuffer();
Modified: maven/artifact/trunk/src/main/java/org/apache/maven/ artifact/repository/layout/LegacyRepositoryLayout.java 
URL: 
http://svn.apache.org/viewvc/maven/artifact/trunk/src/main/java/org/apache/maven/artifact/repository/layout/LegacyRepositoryLayout.java?rev=617944&r1=617943&r2=617944&view=diff
= = = = = = = = = ===================================================================== --- maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ repository/layout/LegacyRepositoryLayout.java (original) +++ maven/artifact/trunk/src/main/java/org/apache/maven/artifact/ repository/layout/LegacyRepositoryLayout.java Sat Feb 2 20:19:44 2008 
@@ -21,6 +21,7 @@

import org.apache.maven.artifact.Artifact;
import org.apache.maven.artifact.handler.ArtifactHandler;
+import org.apache.maven.artifact.handler.manager.ArtifactHandlerManager; 
import org.apache.maven.artifact.metadata.ArtifactMetadata;
import org.apache.maven.artifact.repository.ArtifactRepository;

@@ -33,9 +34,12 @@
{
    private static final String PATH_SEPARATOR = "/";

+    /** @plexus.requirement */
+    private ArtifactHandlerManager artifactHandlerManager;
+
    public String pathOf( Artifact artifact )
    {
- ArtifactHandler artifactHandler = artifact.getArtifactHandler(); + ArtifactHandler artifactHandler = artifactHandlerManager.getArtifactHandler( artifact.getType() ); 

        StringBuffer path = new StringBuffer();
Modified: maven/artifact/trunk/src/test/java/org/apache/maven/ artifact/DefaultArtifactTest.java 
URL: 
http://svn.apache.org/viewvc/maven/artifact/trunk/src/test/java/org/apache/maven/artifact/DefaultArtifactTest.java?rev=617944&r1=617943&r2=617944&view=diff
= = = = = = = = = ===================================================================== --- maven/artifact/trunk/src/test/java/org/apache/maven/artifact/ DefaultArtifactTest.java (original) +++ maven/artifact/trunk/src/test/java/org/apache/maven/artifact/ DefaultArtifactTest.java Sat Feb 2 20:19:44 2008 
@@ -47,12 +47,11 @@
        throws Exception
    {
        super.setUp();
-        artifactHandler = new ArtifactHandlerMock();
        versionRange = VersionRange.createFromVersion( version );
- artifact = new DefaultArtifact( groupId, artifactId, versionRange, scope, type, classifier, artifactHandler ); + artifact = new DefaultArtifact( groupId, artifactId, versionRange, scope, type, classifier );
snapshotVersionRange = VersionRange.createFromVersion( snapshotResolvedVersion ); - snapshotArtifact = new DefaultArtifact( groupId, artifactId, snapshotVersionRange, scope, type, classifier, artifactHandler ); + snapshotArtifact = new DefaultArtifact( groupId, artifactId, snapshotVersionRange, scope, type, classifier ); 
    }

    public void testGetVersionReturnsResolvedVersionOnSnapshot()
@@ -78,7 +77,7 @@

    public void testGetDependencyConflictIdNullClassifier()
    {
- artifact = new DefaultArtifact( groupId, artifactId, versionRange, scope, type, null, artifactHandler ); + artifact = new DefaultArtifact( groupId, artifactId, versionRange, scope, type, null ); assertEquals( groupId + ":" + artifactId + ":" + type, artifact.getDependencyConflictId() ); 
    }

@@ -102,7 +101,7 @@

    public void testToStringNullClassifier()
    {
- artifact = new DefaultArtifact( groupId, artifactId, versionRange, scope, type, null, artifactHandler ); + artifact = new DefaultArtifact( groupId, artifactId, versionRange, scope, type, null ); assertEquals( groupId + ":" + artifactId + ":" + type + ":" + version + ":" + scope, artifact.toString() ); 
    }
Modified: maven/artifact/trunk/src/test/java/org/apache/maven/ artifact/manager/DefaultWagonManagerTest.java 
URL: 
http://svn.apache.org/viewvc/maven/artifact/trunk/src/test/java/org/apache/maven/artifact/manager/DefaultWagonManagerTest.java?rev=617944&r1=617943&r2=617944&view=diff
= = = = = = = = = ===================================================================== --- maven/artifact/trunk/src/test/java/org/apache/maven/artifact/ manager/DefaultWagonManagerTest.java (original) +++ maven/artifact/trunk/src/test/java/org/apache/maven/artifact/ manager/DefaultWagonManagerTest.java Sat Feb 2 20:19:44 2008 
@@ -138,7 +138,7 @@
        {
            tmpFile.deleteOnExit();
Artifact artifact = new DefaultArtifact( "sample.group", "sample-art", VersionRange - .createFromVersion( "1.0" ), "artifactScope", "type", "classifier", null ); + .createFromVersion( "1.0" ), "artifactScope", "type", "classifier" ); 
            artifact.setFile( tmpFile );
ArtifactRepository repo = new DefaultArtifactRepository( "id", "noop://url", new ArtifactRepositoryLayoutStub() ); 





---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]



Thanks,

Jason

----------------------------------------------------------
Jason van Zyl
Founder,  Apache Maven
jason at sonatype dot com
----------------------------------------------------------

A party which is not afraid of letting culture,
business, and welfare go to ruin completely can
be omnipotent for a while.
-- Jakob Burckhardt 



---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]

Reply via email to