refactor maven-artifact: first pass, reduce the usage of setPath()

git-svn-id: https://svn.apache.org/repos/asf/maven/components/trunk@163681 13f79535-47bb-0310-9956-ffa450edef68

maven-artifact/src/main/java/org/apache/maven/artifact/Artifact.java

@@ -39,6 +39,13 @@ public interface Artifact
 
     String getVersion();
 
+    /**
+     * Get the scope of the artifact. If the artifact is a standalone rather than a dependency, it's scope will be
+     * <code>null</code>. The scope may not be the same as it was declared on the original dependency, as this is the
+     * result of combining it with the main project scope.
+     *
+     * @return the scope
+     */
     String getScope();
 
     String getType();
@@ -48,19 +55,9 @@ public interface Artifact
     // only providing this since classifier is *very* optional...
     boolean hasClassifier();
 
-    // ----------------------------------------------------------------------
-
-    void setPath( String path );
-
-    String getPath();
-
     File getFile();
 
-    boolean exists();
+    void setFile( File destination );
-
-    // ----------------------------------------------------------------------
-
-    File getChecksumFile();
 
     // ----------------------------------------------------------------------
 

maven-artifact/src/main/java/org/apache/maven/artifact/DefaultArtifact.java

@@ -27,15 +27,11 @@
 /**
  * @author <a href="mailto:jason@maven.org">Jason van Zyl </a>
  * @version $Id$
+ * @todo this should possibly be replaced by type handler
  */
 public class DefaultArtifact
     implements Artifact
 {
-
-    // ----------------------------------------------------------------------
-    // These are the only things i need to specify
-    // ----------------------------------------------------------------------
-
     private final String groupId;
 
     private final String artifactId;
@@ -48,12 +44,11 @@ public class DefaultArtifact
 
     private final String scope;
 
-    private String path;
-
     private List metadataList;
 
+    private File file;
+
     /**
-     * @todo this should be replaced by type handler
      * !!! WARNING !!! Never put <classifier/> in the POM. It is for mojo use
      * only. Classifier is for specifying derived artifacts, like ejb-client.
      */
@@ -124,33 +119,18 @@ public String getType()
         return type;
     }
 
-    // ----------------------------------------------------------------------
+    public void setFile( File file )
-    //
-    // ----------------------------------------------------------------------
-
-    public String getPath()
     {
-        return path;
+        this.file = file;
-    }
-
-    public void setPath( String path )
-    {
-        this.path = path;
-    }
-
-    public boolean exists()
-    {
-        return getFile().exists();
     }
 
     public File getFile()
     {
-        return new File( getPath() );
+        if ( file == null )
-    }
+        {
-
+            throw new IllegalStateException( "Artifact's local file has not yet been assigned - not resolved" );
-    public File getChecksumFile()
+        }
-    {
+        return file;
-        return new File( getFile().getAbsolutePath() + ".md5" );
     }
 
     // ----------------------------------------------------------------------

maven-artifact/src/main/java/org/apache/maven/artifact/deployer/DefaultArtifactDeployer.java

@@ -56,7 +56,7 @@ public void deploy( File source, Artifact artifact, ArtifactRepository deploymen
     {
         try
         {
-            wagonManager.put( source, artifact, deploymentRepository );
+            wagonManager.putArtifact( source, artifact, deploymentRepository );
         }
         catch ( Exception e )
         {

maven-artifact/src/main/java/org/apache/maven/artifact/installer/DefaultArtifactInstaller.java

@@ -57,16 +57,18 @@ public void install( File source, Artifact artifact, ArtifactRepository localRep
     {
         try
         {
-            artifact.setPath( artifactHandlerManager.getLocalRepositoryArtifactPath( artifact, localRepository ) );
+            String localPath = artifactHandlerManager.getLocalRepositoryArtifactPath( artifact, localRepository );
 
-            if ( !artifact.getFile().getParentFile().exists() )
+            getLogger().info( "Installing " + source.getPath() + " to " + localPath );
+
+            // TODO: use a file: wagon and the wagon manager?
+            File destination = new File( localPath );
+            if ( !destination.getParentFile().exists() )
             {
-                artifact.getFile().getParentFile().mkdirs();
+                destination.getParentFile().mkdirs();
             }
 
-            getLogger().info( "Installing " + source.getPath() + " to " + artifact.getPath() );
+            FileUtils.copyFile( source, destination );
-
-            FileUtils.copyFile( source, artifact.getFile() );
 
             // must be after the artifact is installed
             for ( Iterator i = artifact.getMetadataList().iterator(); i.hasNext(); )

maven-artifact/src/main/java/org/apache/maven/artifact/manager/DefaultWagonManager.java

@@ -18,7 +18,9 @@
 
 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.layout.ArtifactPathFormatException;
 import org.apache.maven.wagon.ConnectionException;
 import org.apache.maven.wagon.ResourceDoesNotExistException;
 import org.apache.maven.wagon.TransferFailedException;
@@ -82,7 +84,7 @@ public void releaseWagon( Wagon wagon )
     }
 
     // TODO: don't throw exception
-    public void put( File source, Artifact artifact, ArtifactRepository repository )
+    public void putArtifact( File source, Artifact artifact, ArtifactRepository repository )
         throws Exception
     {
         Wagon wagon = getWagon( repository.getProtocol() );
@@ -96,148 +98,163 @@ public void put( File source, Artifact artifact, ArtifactRepository repository )
         releaseWagon( wagon );
     }
 
-    public void get( Artifact artifact, List remoteRepositories, ArtifactRepository localRepository )
+    public void getArtifact( Artifact artifact, List remoteRepositories, File destination )
         throws TransferFailedException
     {
-        get( artifact, artifact.getFile(), remoteRepositories );
-    }
-
-    /**
-     * Look in a set of repositories and return when the first valid artifact is
-     * found.
-     */
-
-    /**
-     * @param artifact
-     * @param destination
-     * @throws TransferFailedException
-     * @todo I want to somehow plug artifact validators at such low level.
-     * Simply if artifact was downloaded but it was rejected by
-     * validator(s) the loop should continue. Some of the validators can
-     * be feeded directly using events so number of i/o operation could be
-     * limited. <p/>If we won't plug validation process here the question
-     * is what we can do afterwards? We don't know from which
-     * ArtifactRepository artifact was fetched and where we should
-     * restart. We should be also fetching md5 sums and such from the same
-     * exact directory then artifacts <p/>
-     * @todo probably all exceptions should just be logged and continue
-     * @todo is the exception for warnings logged at debug level correct?
-     */
-    public void get( Artifact artifact, File destination, List repositories )
-        throws TransferFailedException
-    {
-        File temp = null;
-
-        // TODO [BP]: do this handling in Wagon itself
-        temp = new File( destination + ".tmp" );
-        temp.deleteOnExit();
-
         // TODO [BP]: The exception handling here needs some work
-        for ( Iterator iter = repositories.iterator(); iter.hasNext(); )
+        boolean successful = false;
+        for ( Iterator iter = remoteRepositories.iterator(); iter.hasNext() && !successful; )
         {
             ArtifactRepository repository = (ArtifactRepository) iter.next();
 
+            // TODO: should we avoid doing the transforms on this every time, and instead transform outside the loop?
+            String remotePath = null;
             try
             {
-                Wagon wagon = getWagon( repository.getProtocol() );
+                remotePath = artifactHandlerManager.getRemoteRepositoryArtifactPath( artifact, repository );
+            }
+            catch ( ArtifactPathFormatException e )
+            {
+                // TODO may be more appropriate to propogate the APFE
+                throw new TransferFailedException( "Failed to determine path for artifact", e );
+            }
 
-                // ----------------------------------------------------------------------
+            try
-                // These can certainly be configurable ... registering listeners
+            {
-                // ...
+                getRemoteFile( repository, destination, remotePath );
-
-                //ChecksumObserver md5SumObserver = new ChecksumObserver();
-
-                // ----------------------------------------------------------------------
-
-                //wagon.addTransferListener( md5SumObserver );
-
-                if ( downloadMonitor != null )
-                {
-                    wagon.addTransferListener( downloadMonitor );
-                }
-
-                wagon.connect( repository, getProxy( repository.getProtocol() ) );
-
-                // TODO: should we avoid doing the transforms on this every time, and instead transform outside the loop?
-                String remotePath = artifactHandlerManager.getRemoteRepositoryArtifactPath( artifact, repository );
-
-                wagon.get( remotePath, temp );
-
-                // TODO [BP]: put all disconnects in finally
-                wagon.disconnect();
-
-                releaseWagon( wagon );
 
+                successful = true;
             }
             catch ( ResourceDoesNotExistException e )
             {
                 // This one we will eat when looking through remote repositories
                 // because we want to cycle through them all before squawking.
 
-                continue;
+                getLogger().warn( "Unable to get resource from repository " + repository.getUrl() );
             }
-            catch ( UnsupportedProtocolException e )
+        }
-            {
-                throw new TransferFailedException( "Unsupported Protocol: ", e );
-            }
-            catch ( ConnectionException e )
-            {
-                throw new TransferFailedException( "Connection failed: ", e );
-            }
-            catch ( AuthenticationException e )
-            {
-                throw new TransferFailedException( "Authentication failed: ", e );
-            }
-            catch ( AuthorizationException e )
-            {
-                throw new TransferFailedException( "Authorization failed: ", e );
-            }
-            catch ( TransferFailedException e )
-            {
-                getLogger().warn( "Failure getting artifact from repository '" + repository + "': " + e );
 
-                getLogger().debug( "Stack trace", e );
+        if ( !successful )
+        {
+            throw new TransferFailedException( "Unable to download the artifact from any repository" );
+        }
+    }
 
-                continue;
+    public void getMetadata( ArtifactMetadata metadata, ArtifactRepository remoteRepository,
+                             ArtifactRepository localRepository )
+        throws TransferFailedException, ResourceDoesNotExistException
+    {
+        String remotePath;
+        String localPath;
+        try
+        {
+            remotePath = remoteRepository.pathOfMetadata( metadata );
+            localPath = localRepository.pathOfMetadata( metadata );
+        }
+        catch ( ArtifactPathFormatException e )
+        {
+            // TODO may be more appropriate to propogate APFE
+            throw new TransferFailedException( "Failed to determine path for artifact", e );
+        }
+
+        File metadataFile = new File( localRepository.getBasedir(), localPath );
+        getRemoteFile( remoteRepository, metadataFile, remotePath );
+    }
+
+    private void getRemoteFile( ArtifactRepository repository, File destination, String remotePath )
+        throws TransferFailedException, ResourceDoesNotExistException
+    {
+        Wagon wagon;
+
+        try
+        {
+            wagon = getWagon( repository.getProtocol() );
+        }
+        catch ( UnsupportedProtocolException e )
+        {
+            throw new TransferFailedException( "Unsupported Protocol: ", e );
+        }
+
+        // ----------------------------------------------------------------------
+        // These can certainly be configurable ... registering listeners
+        // ...
+
+        //ChecksumObserver md5SumObserver = new ChecksumObserver();
+
+        // ----------------------------------------------------------------------
+
+        //wagon.addTransferListener( md5SumObserver );
+
+        if ( downloadMonitor != null )
+        {
+            wagon.addTransferListener( downloadMonitor );
+        }
+
+        // TODO [BP]: do this handling in Wagon itself
+        if ( !destination.getParentFile().exists() )
+        {
+            destination.getParentFile().mkdirs();
+        }
+
+        File temp = new File( destination + ".tmp" );
+        temp.deleteOnExit();
+
+        try
+        {
+            wagon.connect( repository, getProxy( repository.getProtocol() ) );
+
+            wagon.get( remotePath, temp );
+
+            // TODO [BP]: put all disconnects in finally
+            wagon.disconnect();
+        }
+        catch ( ConnectionException e )
+        {
+            throw new TransferFailedException( "Connection failed: ", e );
+        }
+        catch ( AuthenticationException e )
+        {
+            throw new TransferFailedException( "Authentication failed: ", e );
+        }
+        catch ( AuthorizationException e )
+        {
+            throw new TransferFailedException( "Authorization failed: ", e );
+        }
+        finally
+        {
+            try
+            {
+                releaseWagon( wagon );
             }
             catch ( Exception e )
             {
                 throw new TransferFailedException( "Release of wagon failed: ", e );
             }
-
-            if ( !destination.getParentFile().exists() )
-            {
-                destination.getParentFile().mkdirs();
-            }
-
-            // The temporary file is named destination + ".tmp" and is done this
-            // way to ensure
-            // that the temporary file is in the same file system as the
-            // destination because the
-            // File.renameTo operation doesn't really work across file systems.
-            // So we will attempt
-            // to do a File.renameTo for efficiency and atomicity, if this fails
-            // then we will use
-            // a brute force copy and delete the temporary file.
-
-            if ( !temp.renameTo( destination ) )
-            {
-                try
-                {
-                    FileUtils.copyFile( temp, destination );
-
-                    temp.delete();
-                }
-                catch ( IOException e )
-                {
-                    throw new TransferFailedException( "Error copying temporary file to the final destination: ", e );
-                }
-            }
-
-            return;
         }
 
-        throw new TransferFailedException( "Unable to download the artifact from any repository" );
+        // The temporary file is named destination + ".tmp" and is done this
+        // way to ensure
+        // that the temporary file is in the same file system as the
+        // destination because the
+        // File.renameTo operation doesn't really work across file systems.
+        // So we will attempt
+        // to do a File.renameTo for efficiency and atomicity, if this fails
+        // then we will use
+        // a brute force copy and delete the temporary file.
+
+        if ( !temp.renameTo( destination ) )
+        {
+            try
+            {
+                FileUtils.copyFile( temp, destination );
+
+                temp.delete();
+            }
+            catch ( IOException e )
+            {
+                throw new TransferFailedException( "Error copying temporary file to the final destination: ", e );
+            }
+        }
     }
 
     private ProxyInfo getProxy( String protocol )

maven-artifact/src/main/java/org/apache/maven/artifact/manager/WagonManager.java

@@ -17,7 +17,9 @@
  */
 
 import org.apache.maven.artifact.Artifact;
+import org.apache.maven.artifact.metadata.ArtifactMetadata;
 import org.apache.maven.artifact.repository.ArtifactRepository;
+import org.apache.maven.wagon.ResourceDoesNotExistException;
 import org.apache.maven.wagon.TransferFailedException;
 import org.apache.maven.wagon.UnsupportedProtocolException;
 import org.apache.maven.wagon.Wagon;
@@ -41,13 +43,17 @@ Wagon getWagon( String protocol )
     void releaseWagon( Wagon wagon )
         throws Exception;
 
-    void get( Artifact artifact, List remoteRepositories, ArtifactRepository localRepository )
+    void getArtifact( Artifact artifact, List remoteRepositories, File destination )
         throws TransferFailedException;
 
     // TODO: don't throw exception
-    void put( File source, Artifact artifact, ArtifactRepository deploymentRepository )
+    void putArtifact( File source, Artifact artifact, ArtifactRepository deploymentRepository )
         throws Exception;
 
+    void getMetadata( ArtifactMetadata metadata, ArtifactRepository remoteRepository,
+                      ArtifactRepository localRepository )
+        throws TransferFailedException, ResourceDoesNotExistException;
+
     void setProxy( String protocol, String host, int port, String username, String password, String nonProxyHosts );
 
     void setDownloadMonitor( TransferListener downloadMonitor );

maven-artifact/src/main/java/org/apache/maven/artifact/metadata/ArtifactMetadata.java

@@ -19,6 +19,7 @@
 import org.apache.maven.artifact.Artifact;
 import org.apache.maven.artifact.repository.ArtifactRepository;
 import org.apache.maven.artifact.repository.layout.ArtifactPathFormatException;
+import org.apache.maven.artifact.resolver.ArtifactResolutionException;
 
 import java.io.IOException;
 
@@ -40,6 +41,15 @@ public interface ArtifactMetadata
     void storeInLocalRepository( ArtifactRepository localRepository )
         throws IOException, ArtifactPathFormatException;
 
+    /**
+     * Retrieve the metadata from the remote repository into the local repository.
+     *
+     * @param remoteRepository the remote repository
+     * @param localRepository  the local repository
+     */
+    void retrieveFromRemoteRepository( ArtifactRepository remoteRepository, ArtifactRepository localRepository )
+        throws IOException, ArtifactResolutionException;
+
     /**
      * Get the associated artifact.
      *

maven-artifact/src/main/java/org/apache/maven/artifact/metadata/SnapshotArtifactMetadata.java

@@ -19,6 +19,7 @@
 import org.apache.maven.artifact.Artifact;
 import org.apache.maven.artifact.repository.ArtifactRepository;
 import org.apache.maven.artifact.repository.layout.ArtifactPathFormatException;
+import org.apache.maven.artifact.resolver.ArtifactResolutionException;
 import org.codehaus.plexus.util.FileUtils;
 
 import java.io.IOException;
@@ -46,6 +47,8 @@ public class SnapshotArtifactMetadata
 
     private static final TimeZone UTC_TIME_ZONE = TimeZone.getTimeZone( "UTC" );
 
+    private static final String UTC_TIMESTAMP_PATTERN = "yyyyMMdd.HHmmss";
+
     private SnapshotArtifactMetadata( Artifact artifact, String filename )
     {
         super( artifact, filename );
@@ -56,6 +59,11 @@ public static SnapshotArtifactMetadata createLocalSnapshotMetadata( Artifact art
         return new SnapshotArtifactMetadata( artifact, SNAPSHOT_VERSION_LOCAL_FILE );
     }
 
+    public static ArtifactMetadata createRemoteSnapshotMetadata( Artifact artifact )
+    {
+        return new SnapshotArtifactMetadata( artifact, SNAPSHOT_VERSION_FILE );
+    }
+
     public void storeInLocalRepository( ArtifactRepository localRepository )
         throws IOException, ArtifactPathFormatException
     {
@@ -63,6 +71,22 @@ public void storeInLocalRepository( ArtifactRepository localRepository )
                              getTimestamp() + "-" + buildNumber );
     }
 
+    public void retrieveFromRemoteRepository( ArtifactRepository remoteRepository, ArtifactRepository localRepository )
+        throws IOException, ArtifactResolutionException
+    {
+/*
+// TODO: this is getting the artifact - needs to get the version.txt
+        resolver.resolve( artifact, Collections.singletonList( remoteRepository ), localRepository );
+
+        String version = FileUtils.fileRead( artifact.getPath() );
+
+        int index = UTC_TIMESTAMP_PATTERN.length();
+        timestamp = version.substring( 0, index );
+
+        buildNumber = Integer.valueOf( version.substring( index + 1 ) ).intValue();
+*/
+    }
+
     public String getTimestamp()
     {
         if ( timestamp == null )
@@ -72,9 +96,9 @@ public String getTimestamp()
         return timestamp;
     }
 
-    public DateFormat getUtcDateFormatter()
+    public static DateFormat getUtcDateFormatter()
     {
-        DateFormat utcDateFormatter = new SimpleDateFormat( "yyyyMMdd.HHmmss" );
+        DateFormat utcDateFormatter = new SimpleDateFormat( UTC_TIMESTAMP_PATTERN );
         utcDateFormatter.setTimeZone( UTC_TIME_ZONE );
         return utcDateFormatter;
     }

maven-artifact/src/main/java/org/apache/maven/artifact/resolver/DefaultArtifactResolver.java

@@ -29,6 +29,7 @@
 import org.codehaus.plexus.logging.AbstractLogEnabled;
 import org.codehaus.plexus.logging.Logger;
 
+import java.io.File;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
@@ -45,6 +46,8 @@ public class DefaultArtifactResolver
     extends AbstractLogEnabled
     implements ArtifactResolver
 {
+    private final ArtifactConstructionSupport artifactConstructionSupport = new ArtifactConstructionSupport();
+
     // ----------------------------------------------------------------------
     // Components
     // ----------------------------------------------------------------------
@@ -57,15 +60,10 @@ public class DefaultArtifactResolver
     // Implementation
     // ----------------------------------------------------------------------
 
-    private ArtifactConstructionSupport artifactConstructionSupport = new ArtifactConstructionSupport();
+    // TODO: would like to avoid the returning of a new artifact - is it ok to modify the original though?
-
     public Artifact resolve( Artifact artifact, List remoteRepositories, ArtifactRepository localRepository )
         throws ArtifactResolutionException
     {
-        // ----------------------------------------------------------------------
-        // Perform any transformation on the artifacts
-        // ----------------------------------------------------------------------
-
         // ----------------------------------------------------------------------
         // Check for the existence of the artifact in the specified local
         // ArtifactRepository. If it is present then simply return as the
@@ -73,41 +71,57 @@ public Artifact resolve( Artifact artifact, List remoteRepositories, ArtifactRep
         // for resolution has been satisfied.
         // ----------------------------------------------------------------------
 
+        Logger logger = getLogger();
+        logger.debug( "Resolving: " + artifact.getId() + " from:\n" + "{localRepository: " + localRepository + "}\n" +
+                      "{remoteRepositories: " + remoteRepositories + "}" );
+
+        String localPath;
+
         try
         {
-            Logger logger = getLogger();
+            localPath = artifactHandlerManager.getLocalRepositoryArtifactPath( artifact, localRepository );
-            logger.debug( "Resolving: " + artifact.getId() + " from:\n" + "{localRepository: " + localRepository +
-                          "}\n" + "{remoteRepositories: " + remoteRepositories + "}" );
-
-            artifact.setPath( artifactHandlerManager.getLocalRepositoryArtifactPath( artifact, localRepository ) );
-
-            if ( artifact.exists() )
-            {
-                return artifact;
-            }
-
-            wagonManager.get( artifact, remoteRepositories, localRepository );
-        }
-        catch ( TransferFailedException e )
-        {
-            throw new ArtifactResolutionException( artifactNotFound( artifact, remoteRepositories ), e );
         }
         catch ( ArtifactPathFormatException e )
         {
             throw new ArtifactResolutionException( "Error resolving artifact: ", e );
         }
 
+        // TODO: what if it were a snapshot that was transformed?
+        File destination = new File( localPath );
+        artifact.setFile( destination );
+
+        if ( destination.exists() )
+        {
+            return artifact;
+        }
+
+        try
+        {
+            wagonManager.getArtifact( artifact, remoteRepositories, destination );
+        }
+        catch ( TransferFailedException e )
+        {
+            throw new ArtifactResolutionException( artifactNotFound( localPath, remoteRepositories ), e );
+        }
+
         return artifact;
     }
 
     private static final String LS = System.getProperty( "line.separator" );
 
-    private String artifactNotFound( Artifact artifact, List remoteRepositories )
+    private String artifactNotFound( String path, List remoteRepositories )
     {
         StringBuffer sb = new StringBuffer();
 
-        sb.append( "The artifact is not present locally as:" ).append( LS ).append( LS ).append( artifact.getPath() ).append(
+        sb.append( "The artifact is not present locally as:" );
-            LS ).append( LS ).append( "or in any of the specified remote repositories:" ).append( LS ).append( LS );
+        sb.append( LS );
+        sb.append( LS );
+        sb.append( path );
+        sb.append( LS );
+        sb.append( LS );
+        sb.append( "or in any of the specified remote repositories:" );
+        sb.append( LS );
+        sb.append( LS );
 
         for ( Iterator i = remoteRepositories.iterator(); i.hasNext(); )
         {
@@ -160,11 +174,19 @@ public ArtifactResolutionResult resolveTransitively( Set artifacts, List remoteR
             throw new ArtifactResolutionException( "Error transitively resolving artifacts: ", e );
         }
 
-        for ( Iterator i = artifactResolutionResult.getArtifacts().values().iterator(); i.hasNext(); )
+        // TODO: this is unclean, but necessary as long as resolve may return a different artifact
+        Map collectedArtifacts = artifactResolutionResult.getArtifacts();
+        Map resolvedArtifacts = new HashMap( collectedArtifacts.size() );
+        for ( Iterator i = collectedArtifacts.keySet().iterator(); i.hasNext(); )
         {
-            resolve( (Artifact) i.next(), remoteRepositories, localRepository );
+            Object key = i.next();
+            resolvedArtifacts.put( key, resolve( (Artifact) collectedArtifacts.get( key ), remoteRepositories,
+                                                 localRepository ) );
         }
 
+        collectedArtifacts.clear();
+        collectedArtifacts.putAll( resolvedArtifacts );
+
         return artifactResolutionResult;
     }
 
@@ -242,6 +264,7 @@ private ArtifactResolutionResult collect( Set artifacts, ArtifactRepository loca
                     {
                         // TODO: Artifact factory?
                         // TODO: [jc] Is this a better way to centralize artifact construction here?
+
                         Artifact artifact = artifactConstructionSupport.createArtifact( knownArtifact.getGroupId(),
                                                                                         knownArtifact.getArtifactId(),
                                                                                         knownVersion,
@@ -294,15 +317,6 @@ private ArtifactResolutionResult collect( Set artifacts, ArtifactRepository loca
         {
             Artifact artifact = (Artifact) it.next();
 
-            try
-            {
-                artifact.setPath( artifactHandlerManager.getLocalRepositoryArtifactPath( artifact, localRepository ) );
-            }
-            catch ( ArtifactPathFormatException e )
-            {
-                throw new TransitiveArtifactResolutionException( "Error collecting artifact: ", e );
-            }
-
             artifactResult.put( artifact.getId(), artifact );
         }
 

maven-artifact/src/main/java/org/apache/maven/artifact/transform/ArtifactTransformation.java

@@ -32,7 +32,8 @@ public interface ArtifactTransformation
      * Take in a artifact and return the transformed artifact for locating in the local repository. If no
      * transformation has occured the original artifact is returned.
      *
-     * @param artifact Artifact to be transformed.
+     * @param artifact        Artifact to be transformed.
+     * @param localRepository the local repository it will be stored in
      * @return The transformed Artifact
      */
     Artifact transformLocalArtifact( Artifact artifact, ArtifactRepository localRepository );
@@ -43,6 +44,7 @@ public interface ArtifactTransformation
      *
      * @param artifact Artifact to be transformed.
      * @return The transformed Artifact
+     * @todo finish doco
      */
     Artifact transformRemoteArtifact( Artifact artifact, ArtifactRepository remoteRepository );
 }

maven-artifact/src/main/java/org/apache/maven/artifact/transform/SnapshotTransformation.java

@@ -189,7 +189,7 @@ public Artifact transformLocalArtifact( Artifact artifact, ArtifactRepository lo
     {
         if ( shouldProcessArtifact( artifact ) )
         {
-            // only store the snapshot-version-local.txt file for POMs as every file has an associated POM
+            // only store the version-local.txt file for POMs as every file has an associated POM
             ArtifactMetadata metadata = SnapshotArtifactMetadata.createLocalSnapshotMetadata( artifact );
             artifact.addMetadata( metadata );
         }
@@ -200,6 +200,9 @@ public Artifact transformRemoteArtifact( Artifact artifact, ArtifactRepository r
     {
         if ( shouldProcessArtifact( artifact ) )
         {
+            ArtifactMetadata metadata = SnapshotArtifactMetadata.createRemoteSnapshotMetadata( artifact );
+//            wagonManager.getMetadata( metadata, remoteRepository, localRepository );
+
             // TODO: implement
         }
         return artifact;

maven-artifact/src/test/java/org/apache/maven/artifact/ArtifactComponentTestCase.java

@@ -70,6 +70,11 @@ protected ArtifactRepository badLocalRepository()
         return localRepository;
     }
 
+    protected String getRepositoryLayout()
+    {
+        return "legacy";
+    }
+
     protected ArtifactRepository localRepository()
         throws Exception
     {

maven-artifact/src/test/java/org/apache/maven/artifact/NewLayoutArtifactComponentTestCase.java

@@ -1,5 +1,7 @@
+package org.apache.maven.artifact;
+
 /*
- * Copyright 2001-2004 The Apache Software Foundation.
+ * Copyright 2001-2005 The Apache Software Foundation.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -14,19 +16,6 @@
  * limitations under the License.
  */
 
-package org.apache.maven.artifact;
-
-import org.apache.maven.artifact.handler.manager.ArtifactHandlerManager;
-import org.apache.maven.artifact.repository.ArtifactRepository;
-import org.apache.maven.artifact.repository.layout.ArtifactRepositoryLayout;
-import org.codehaus.plexus.PlexusTestCase;
-
-import java.io.File;
-import java.io.FileWriter;
-import java.io.IOException;
-import java.io.Writer;
-import java.util.ArrayList;
-import java.util.List;
 
 /**
  * @author <a href="mailto:jason@maven.org">Jason van Zyl </a>
@@ -34,227 +23,11 @@
  *          jvanzyl Exp $
  */
 public abstract class NewLayoutArtifactComponentTestCase
-    extends PlexusTestCase
+    extends ArtifactComponentTestCase
 {
-    protected ArtifactHandlerManager artifactHandlerManager;
+    protected String getRepositoryLayout()
-
-    protected void setUp() throws Exception
     {
-        super.setUp();
+        return "default";
-
-        artifactHandlerManager = (ArtifactHandlerManager) lookup( ArtifactHandlerManager.ROLE );
-    }
-
-    protected abstract String component();
-
-    /** Return an existing file, not a directory - causes creation to fail. 
-     * @throws Exception*/
-    protected ArtifactRepository badLocalRepository() throws Exception
-    {
-        String path = "target/test-classes/repositories/" + component() + "/bad-local-repository";
-
-        File f = new File( getBasedir(), path );
-
-        f.createNewFile();
-
-        ArtifactRepositoryLayout repoLayout = (ArtifactRepositoryLayout) lookup( ArtifactRepositoryLayout.ROLE,
-                                                                                 "default" );
-
-        ArtifactRepository localRepository = new ArtifactRepository( "test", "file://" + f.getPath(), repoLayout );
-
-        return localRepository;
-    }
-
-    protected ArtifactRepository localRepository() throws Exception
-    {
-        String path = "target/test-classes/repositories/" + component() + "/local-repository";
-
-        File f = new File( getBasedir(), path );
-
-        ArtifactRepositoryLayout repoLayout = (ArtifactRepositoryLayout) lookup( ArtifactRepositoryLayout.ROLE,
-                                                                                 "default" );
-
-        ArtifactRepository localRepository = new ArtifactRepository( "local", "file://" + f.getPath(), repoLayout );
-
-        return localRepository;
-    }
-
-    protected ArtifactRepository remoteRepository() throws Exception
-    {
-        String path = "target/test-classes/repositories/" + component() + "/remote-repository";
-
-        File f = new File( getBasedir(), path );
-
-        ArtifactRepositoryLayout repoLayout = (ArtifactRepositoryLayout) lookup( ArtifactRepositoryLayout.ROLE,
-                                                                                 "default" );
-
-        ArtifactRepository repository = new ArtifactRepository( "test", "file://" + f.getPath(), repoLayout );
-
-        return repository;
-    }
-
-    protected ArtifactRepository badRemoteRepository() throws Exception
-    {
-        ArtifactRepositoryLayout repoLayout = (ArtifactRepositoryLayout) lookup( ArtifactRepositoryLayout.ROLE,
-                                                                                 "default" );
-
-        ArtifactRepository repository = new ArtifactRepository( "test", "http://foo.bar/repository", repoLayout );
-
-        return repository;
-    }
-
-    protected void assertRemoteArtifactPresent( Artifact artifact ) throws Exception
-    {
-        ArtifactRepository remoteRepo = remoteRepository();
-
-        String path = remoteRepo.pathOf( artifact );
-
-        File file = new File( remoteRepo.getBasedir(), path );
-
-        if ( !file.exists() )
-        {
-            fail( "Remote artifact " + file + " should be present." );
-        }
-    }
-
-    protected void assertLocalArtifactPresent( Artifact artifact ) throws Exception
-    {
-        ArtifactRepository localRepo = localRepository();
-
-        String path = localRepo.pathOf( artifact );
-
-        File file = new File( localRepo.getBasedir(), path );
-
-        if ( !file.exists() )
-        {
-            fail( "Local artifact " + file + " should be present." );
-        }
-    }
-
-    protected void assertRemoteArtifactNotPresent( Artifact artifact ) throws Exception
-    {
-        ArtifactRepository remoteRepo = remoteRepository();
-
-        String path = remoteRepo.pathOf( artifact );
-
-        File file = new File( remoteRepo.getBasedir(), path );
-
-        if ( file.exists() )
-        {
-            fail( "Remote artifact " + file + " should not be present." );
-        }
-    }
-
-    protected void assertLocalArtifactNotPresent( Artifact artifact ) throws Exception
-    {
-        ArtifactRepository localRepo = localRepository();
-
-        String path = localRepo.pathOf( artifact );
-
-        File file = new File( localRepo.getBasedir(), path );
-
-        if ( file.exists() )
-        {
-            fail( "Local artifact " + file + " should not be present." );
-        }
-    }
-
-    // ----------------------------------------------------------------------
-    //
-    // ----------------------------------------------------------------------
-
-    protected List remoteRepositories() throws Exception
-    {
-        List remoteRepositories = new ArrayList();
-
-        remoteRepositories.add( remoteRepository() );
-
-        return remoteRepositories;
-    }
-
-    // ----------------------------------------------------------------------
-    // Test artifact generation for unit tests
-    // ----------------------------------------------------------------------
-
-    protected Artifact createLocalArtifact( String artifactId, String version ) throws Exception
-    {
-        Artifact artifact = createArtifact( artifactId, version );
-
-        createArtifact( artifact, localRepository() );
-
-        return artifact;
-    }
-
-    protected Artifact createRemoteArtifact( String artifactId, String version ) throws Exception
-    {
-        Artifact artifact = createArtifact( artifactId, version );
-
-        createArtifact( artifact, remoteRepository() );
-
-        return artifact;
-    }
-
-    protected void createLocalArtifact( Artifact artifact ) throws Exception
-    {
-        createArtifact( artifact, localRepository() );
-    }
-
-    protected void createRemoteArtifact( Artifact artifact ) throws Exception
-    {
-        createArtifact( artifact, remoteRepository() );
-    }
-
-    protected void createArtifact( Artifact artifact, ArtifactRepository repository ) throws Exception
-    {
-        String path = repository.pathOf( artifact );
-
-        File artifactFile = new File( repository.getBasedir(), path );
-
-        if ( !artifactFile.getParentFile().exists() )
-        {
-            artifactFile.getParentFile().mkdirs();
-        }
-
-        Writer writer = new FileWriter( artifactFile );
-
-        writer.write( artifact.getId() );
-
-        writer.close();
-    }
-
-    protected Artifact createArtifact( String artifactId, String version )
-    {
-        return createArtifact( artifactId, version, "jar" );
-    }
-
-    protected Artifact createArtifact( String artifactId, String version, String type )
-    {
-        return new DefaultArtifact( "org.apache.maven", artifactId, version, type );
-    }
-
-    protected Artifact createArtifact( String groupId, String artifactId, String version, String type )
-    {
-        return new DefaultArtifact( groupId, artifactId, version, Artifact.SCOPE_COMPILE, type, type );
-    }
-
-    protected void deleteLocalArtifact( Artifact artifact ) throws Exception
-    {
-        deleteArtifact( artifact, localRepository() );
-    }
-
-    protected void deleteArtifact( Artifact artifact, ArtifactRepository repository ) throws Exception
-    {
-        String path = repository.pathOf( artifact );
-
-        File artifactFile = new File( repository.getBasedir(), path );
-
-        if ( artifactFile.exists() )
-        {
-            if ( !artifactFile.delete() )
-            {
-                throw new IOException( "Failure while attempting to delete artifact " + artifactFile );
-            }
-        }
     }
 }
 

maven-core/src/main/java/org/apache/maven/artifact/MavenMetadataSource.java

@@ -27,6 +27,8 @@
 import org.apache.maven.model.io.xpp3.MavenXpp3Reader;
 import org.apache.maven.project.MavenProject;
 import org.apache.maven.project.MavenProjectBuilder;
+import org.apache.maven.project.ProjectBuildingException;
+import org.apache.maven.wagon.util.IoUtils;
 
 import java.io.FileReader;
 import java.util.List;
@@ -69,56 +71,68 @@ public MavenMetadataSource( ArtifactResolver artifactResolver, MavenProjectBuild
     public Set retrieve( Artifact artifact, ArtifactRepository localRepository, List remoteRepositories )
         throws ArtifactMetadataRetrievalException
     {
-        try
+        List dependencies = null;
-        {
-            List dependencies = null;
 
-            if ( mavenProjectBuilder != null )
+        if ( mavenProjectBuilder != null )
+        {
+            Model model = mavenProjectBuilder.getCachedModel( artifact.getGroupId(), artifact.getArtifactId(),
+                                                              artifact.getVersion() );
+            if ( model != null )
             {
-                Model model = mavenProjectBuilder.getCachedModel( artifact.getGroupId(), artifact.getArtifactId(),
+                dependencies = model.getDependencies();
-                                                                  artifact.getVersion() );
+            }
-                if ( model != null )
+        }
-                {
+
-                    dependencies = model.getDependencies();
+        if ( dependencies == null )
-                }
+        {
+            Artifact metadataArtifact = artifactFactory.createArtifact( artifact.getGroupId(),
+                                                                        artifact.getArtifactId(),
+                                                                        artifact.getVersion(), artifact.getScope(),
+                                                                        "pom", null );
+
+            try
+            {
+                metadataArtifact = artifactResolver.resolve( metadataArtifact, remoteRepositories, localRepository );
+            }
+            catch ( ArtifactResolutionException e )
+            {
+                throw new ArtifactMetadataRetrievalException( "Error while resolving metadata artifact", e );
             }
 
-            if ( dependencies == null )
+            // [jdcasey/03-Feb-2005]: Replacing with ProjectBuilder, to enable
+            // post-processing and inheritance calculation before retrieving the
+            // associated artifacts. This should improve consistency.
+            if ( mavenProjectBuilder != null )
             {
-                Artifact metadataArtifact = artifactFactory.createArtifact( artifact.getGroupId(),
-                                                                            artifact.getArtifactId(),
-                                                                            artifact.getVersion(), artifact.getScope(),
-                                                                            "pom", null );
-
-                artifactResolver.resolve( metadataArtifact, remoteRepositories, localRepository );
-
-                // [jdcasey/03-Feb-2005]: Replacing with ProjectBuilder, to enable
-                // post-processing and inheritance calculation before retrieving the
-                // associated artifacts. This should improve consistency.
                 try
                 {
-                    if ( mavenProjectBuilder != null )
+                    MavenProject p = mavenProjectBuilder.buildFromRepository( metadataArtifact, localRepository );
-                    {
+                    dependencies = p.getDependencies();
-                        MavenProject p = mavenProjectBuilder.buildFromRepository( metadataArtifact, localRepository );
+                }
-                        dependencies = p.getDependencies();
+                catch ( ProjectBuildingException e )
-                    }
+                {
-                    else
+                    throw new ArtifactMetadataRetrievalException( "Unable to read the metadata file", e );
-                    {
+                }
-                        Model model = reader.read( new FileReader( metadataArtifact.getFile() ) );
+            }
-                        dependencies = model.getDependencies();
+            else
-                    }
+            {
+                FileReader reader = null;
+                try
+                {
+                    reader = new FileReader( metadataArtifact.getFile() );
+                    Model model = this.reader.read( reader );
+                    dependencies = model.getDependencies();
                 }
                 catch ( Exception e )
                 {
-                    throw new ArtifactMetadataRetrievalException(
+                    throw new ArtifactMetadataRetrievalException( "Unable to read the metadata file", e );
-                        "Cannot read artifact source: " + metadataArtifact.getPath(), e );
+                }
+                finally
+                {
+                    IoUtils.close( reader );
                 }
             }
-            return artifactFactory.createArtifacts( dependencies, localRepository, artifact.getScope() );
-        }
-        catch ( ArtifactResolutionException e )
-        {
-            throw new ArtifactMetadataRetrievalException( "Error while resolving metadata artifact", e );
         }
+        return artifactFactory.createArtifacts( dependencies, localRepository, artifact.getScope() );
     }
 }

maven-core/src/main/java/org/apache/maven/plugin/DefaultPluginManager.java

@@ -267,8 +267,9 @@ public void verifyPlugin( String groupId, String artifactId, MavenSession sessio
 
                 artifactFactory = (ArtifactFactory) container.lookup( ArtifactFactory.ROLE );
 
-                Artifact pluginArtifact = artifactFactory.createArtifact( AbstractPlugin.getDefaultPluginGroupId(), artifactId,
+                Artifact pluginArtifact = artifactFactory.createArtifact( AbstractPlugin.getDefaultPluginGroupId(),
-                                                                          version, null, MAVEN_PLUGIN, null );
+                                                                          artifactId, version, null, MAVEN_PLUGIN,
+                                                                          null );
 
                 addPlugin( pluginArtifact, session );
             }
@@ -774,6 +775,7 @@ private void downloadDependencies( MavenSession context, ArtifactResolver artifa
         {
             Artifact artifact = (Artifact) it.next();
 
+            // TODO: should I get the modified artifacts back into the project?
             artifactResolver.resolve( artifact, context.getRemoteRepositories(), context.getLocalRepository() );
         }
     }

maven-core/src/main/java/org/apache/maven/project/DefaultMavenProjectBuilder.java

@@ -459,7 +459,7 @@ private File findParentModel( Parent parent, List remoteArtifactRepositories, Ar
 
         try
         {
-            artifactResolver.resolve( artifact, remoteArtifactRepositories, localRepository );
+            artifact = artifactResolver.resolve( artifact, remoteArtifactRepositories, localRepository );
         }
         catch ( ArtifactResolutionException e )
         {

maven-core/src/main/java/org/apache/maven/project/MavenProject.java

@@ -235,7 +235,8 @@ public List getCompileClasspathElements()
             // TODO: let the scope handler deal with this
             if ( Artifact.SCOPE_COMPILE.equals( a.getScope() ) )
             {
-                list.add( a.getPath() );
+                // TODO: this assumes resolution, which may not have been the case - improve error reporting in that instance
+                list.add( a.getFile().getPath() );
             }
         }
         return list;
@@ -257,7 +258,8 @@ public List getTestClasspathElements()
                 if ( Artifact.SCOPE_TEST.equals( a.getScope() ) || Artifact.SCOPE_COMPILE.equals( a.getScope() ) ||
                     Artifact.SCOPE_RUNTIME.equals( a.getScope() ) )
                 {
-                    list.add( a.getPath() );
+                    // TODO: this assumes resolution, which may not have been the case - improve error reporting in that instance
+                    list.add( a.getFile().getPath() );
                 }
             }
         }
@@ -279,7 +281,8 @@ public List getRuntimeClasspathElements()
                 // TODO: let the scope handler deal with this
                 if ( Artifact.SCOPE_COMPILE.equals( a.getScope() ) || Artifact.SCOPE_RUNTIME.equals( a.getScope() ) )
                 {
-                    list.add( a.getPath() );
+                    // TODO: this assumes resolution, which may not have been the case - improve error reporting in that instance
+                    list.add( a.getFile().getPath() );
                 }
             }
         }