You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "ASF GitHub Bot (JIRA)" <ji...@apache.org> on 2018/06/04 11:03:00 UTC
[jira] [Commented] (WAGON-495) Fix checkoutDirectory leak
[ https://issues.apache.org/jira/browse/WAGON-495?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16500067#comment-16500067 ]
ASF GitHub Bot commented on WAGON-495:
--------------------------------------
asfgit closed pull request #46: [WAGON-495] fix testWagonPutDirectoryWhenDirectoryAlreadyExists false positive
URL: https://github.com/apache/maven-wagon/pull/46
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
diff --git a/wagon-provider-test/src/main/java/org/apache/maven/wagon/WagonTestCase.java b/wagon-provider-test/src/main/java/org/apache/maven/wagon/WagonTestCase.java
index fb277c7f..e89f1e29 100644
--- a/wagon-provider-test/src/main/java/org/apache/maven/wagon/WagonTestCase.java
+++ b/wagon-provider-test/src/main/java/org/apache/maven/wagon/WagonTestCase.java
@@ -42,6 +42,7 @@
import java.io.File;
import java.io.IOException;
+import java.nio.charset.StandardCharsets;
import java.security.NoSuchAlgorithmException;
import java.text.SimpleDateFormat;
import java.util.ArrayList;
@@ -74,6 +75,10 @@ public int getSize()
}
}
+ protected static final String TEST_CONTENT = "test-resource.txt\n";
+
+ protected static final String TEST_CKSUM = cksum( TEST_CONTENT );
+
protected static final String POM = "pom.xml";
protected Repository localRepository;
@@ -233,16 +238,13 @@ protected void message( String message )
public void testWagon()
throws Exception
{
- if ( supportsGetIfNewer() )
- {
- setupRepositories();
+ setupRepositories();
- setupWagonTestingFixtures();
+ setupWagonTestingFixtures();
- fileRoundTripTesting();
+ fileRoundTripTesting();
- tearDownWagonTestingFixtures();
- }
+ tearDownWagonTestingFixtures();
}
public void testWagonGetIfNewerIsNewer()
@@ -341,7 +343,7 @@ protected void assertGetIfNewerTest( ProgressAnswer progressAnswer, boolean expe
assertNotNull( "check checksum is not null", checksumObserver.getActualChecksum() );
- assertEquals( "compare checksums", "6b144b7285ffd6b0bc8300da162120b9",
+ assertEquals( "compare checksums", TEST_CKSUM,
checksumObserver.getActualChecksum() );
// Now compare the contents of the artifact that was placed in
@@ -923,7 +925,7 @@ protected TransferEvent createTransferEvent( Wagon wagon, Resource resource, int
protected int putFile()
throws Exception
{
- String content = "test-resource.txt\n";
+ String content = TEST_CONTENT;
putFile( resource, "test-resource", content );
return content.length();
}
@@ -1049,7 +1051,7 @@ protected void fileRoundTripTesting()
assertNotNull( "check checksum is not null", checksumObserver.getActualChecksum() );
- assertEquals( "compare checksums", "6b144b7285ffd6b0bc8300da162120b9", checksumObserver.getActualChecksum() );
+ assertEquals( "compare checksums", TEST_CKSUM, checksumObserver.getActualChecksum() );
checksumObserver = new ChecksumObserver();
@@ -1057,7 +1059,7 @@ protected void fileRoundTripTesting()
assertNotNull( "check checksum is not null", checksumObserver.getActualChecksum() );
- assertEquals( "compare checksums", "6b144b7285ffd6b0bc8300da162120b9", checksumObserver.getActualChecksum() );
+ assertEquals( "compare checksums", TEST_CKSUM, checksumObserver.getActualChecksum() );
// Now compare the conents of the artifact that was placed in
// the repository with the contents of the artifact that was
@@ -1087,4 +1089,22 @@ protected Repository createFileRepository( String url )
return repository;
}
+ protected static String cksum( String content )
+ {
+ String checkSum;
+ try
+ {
+ ChecksumObserver obs = new ChecksumObserver();
+ byte[] buf = content.getBytes( StandardCharsets.ISO_8859_1 );
+ obs.transferProgress( null, buf, buf.length );
+ obs.transferCompleted( null );
+ checkSum = obs.getActualChecksum();
+ }
+ catch ( Exception e )
+ {
+ checkSum = null;
+ }
+ return checkSum;
+ }
+
}
diff --git a/wagon-providers/wagon-scm/src/main/java/org/apache/maven/wagon/providers/scm/ScmWagon.java b/wagon-providers/wagon-scm/src/main/java/org/apache/maven/wagon/providers/scm/ScmWagon.java
index 8fbbdeff..d6531eef 100644
--- a/wagon-providers/wagon-scm/src/main/java/org/apache/maven/wagon/providers/scm/ScmWagon.java
+++ b/wagon-providers/wagon-scm/src/main/java/org/apache/maven/wagon/providers/scm/ScmWagon.java
@@ -19,6 +19,15 @@
* under the License.
*/
+import java.io.File;
+import java.io.IOException;
+import java.text.DecimalFormat;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Random;
+
+import org.apache.maven.scm.CommandParameter;
+import org.apache.maven.scm.CommandParameters;
import org.apache.maven.scm.ScmBranch;
import org.apache.maven.scm.ScmException;
import org.apache.maven.scm.ScmFile;
@@ -30,6 +39,7 @@
import org.apache.maven.scm.command.add.AddScmResult;
import org.apache.maven.scm.command.checkout.CheckOutScmResult;
import org.apache.maven.scm.command.list.ListScmResult;
+import org.apache.maven.scm.command.update.UpdateScmResult;
import org.apache.maven.scm.manager.NoSuchScmProviderException;
import org.apache.maven.scm.manager.ScmManager;
import org.apache.maven.scm.provider.ScmProvider;
@@ -47,14 +57,6 @@
import org.codehaus.plexus.util.FileUtils;
import org.codehaus.plexus.util.StringUtils;
-import java.io.File;
-import java.io.IOException;
-import java.text.DecimalFormat;
-import java.util.ArrayList;
-import java.util.List;
-import java.util.Random;
-import java.util.Stack;
-
/**
* Wagon provider to get and put files from and to SCM systems, using Maven-SCM as underlying transport.
* <p/>
@@ -93,6 +95,11 @@
*/
private String scmVersionType;
+ /**
+ * Empty string or subdir ending with slash.
+ */
+ private String partCOSubdir = "";
+
private File checkoutDirectory;
/**
@@ -369,18 +376,19 @@ private void putInternal( File source, String targetName )
ScmProvider scmProvider = getScmProvider( scmRepository.getProvider() );
- String checkoutTargetName = source.isDirectory() ? targetName : getDirname( targetName );
- String relPath = checkOut( scmProvider, scmRepository, checkoutTargetName, target );
+ boolean isDirectory = source.isDirectory();
+ String checkoutTargetName = isDirectory ? targetName : getDirname( targetName );
+ String relPath = ensureDirs( scmProvider, scmRepository, checkoutTargetName, target );
File newCheckoutDirectory = new File( checkoutDirectory, relPath );
- File scmFile = new File( newCheckoutDirectory, source.isDirectory() ? "" : getFilename( targetName ) );
+ File scmFile = new File( newCheckoutDirectory, isDirectory ? "" : FileUtils.removePath( targetName, '/' ) );
boolean fileAlreadyInScm = scmFile.exists();
if ( !scmFile.equals( source ) )
{
- if ( source.isDirectory() )
+ if ( isDirectory )
{
FileUtils.copyDirectoryStructure( source, scmFile );
}
@@ -393,7 +401,7 @@ private void putInternal( File source, String targetName )
if ( !fileAlreadyInScm || scmFile.isDirectory() )
{
int addedFiles = addFiles( scmProvider, scmRepository, newCheckoutDirectory,
- source.isDirectory() ? "" : scmFile.getName() );
+ isDirectory ? "" : scmFile.getName() );
if ( !fileAlreadyInScm && addedFiles == 0 )
{
@@ -437,14 +445,16 @@ private void putInternal( File source, String targetName )
* @param targetName
* @return
* @throws TransferFailedException
+ * @throws IOException
*/
- private String checkOut( ScmProvider scmProvider, ScmRepository scmRepository, String targetName,
+ private String ensureDirs( ScmProvider scmProvider, ScmRepository scmRepository, String targetName,
Resource resource )
- throws TransferFailedException
+ throws TransferFailedException, IOException
{
- checkoutDirectory = createCheckoutDirectory();
-
- Stack<String> stack = new Stack<String>();
+ if ( checkoutDirectory == null )
+ {
+ checkoutDirectory = createCheckoutDirectory();
+ }
String target = targetName;
@@ -453,81 +463,82 @@ private String checkOut( ScmProvider scmProvider, ScmRepository scmRepository, S
// Check whether targetName, which is a relative path into the scm, exists.
// If it doesn't, check the parent, etc.
- try
+ for ( ;; )
{
- while ( target.length() > 0 && !scmProvider.list( scmRepository,
- new ScmFileSet( new File( "." ), new File( target ) ),
- false, makeScmVersion() ).isSuccess() )
+ try
+ {
+ ScmResult res = tryPartialCheckout( target );
+ if ( !res.isSuccess() )
+ {
+ throw new ScmException( "command failed: " + res.getCommandOutput().trim() );
+ }
+ break;
+ }
+ catch ( ScmException e )
{
- stack.push( getFilename( target ) );
+ if ( partCOSubdir.length() == 0 )
+ {
+ fireTransferError( resource, e, TransferEvent.REQUEST_GET );
+
+ throw new TransferFailedException( "Error checking out: " + e.getMessage(), e );
+ }
target = getDirname( target );
}
}
- catch ( ScmException e )
- {
- fireTransferError( resource, e, TransferEvent.REQUEST_GET );
- throw new TransferFailedException( "Error listing repository: " + e.getMessage(), e );
- }
+ // now create the subdirs in target, if it's a parent of targetName
- // ok, we've established that target exists, or is empty.
- // Check the resource out; if it doesn't exist, that means we're in the svn repo url root,
- // and the configuration is incorrect. We will not try repo.getParent since most scm's don't
- // implement that.
+ String res =
+ partCOSubdir.length() >= targetName.length() ? "" : targetName.substring( partCOSubdir.length() ) + '/';
+ ArrayList<File> createdDirs = new ArrayList<File>();
+ File deepDir = new File( checkoutDirectory, res );
+
+ boolean added = false;
try
{
- String repoUrl = getRepository().getUrl();
- if ( "svn".equals( scmProvider.getScmType() ) )
+ mkdirsThrow( deepDir, createdDirs );
+ if ( createdDirs.size() != 0 )
{
- // Subversion is the only SCM that adds path structure to represent tags and branches.
- // The rest use scmVersion and scmVersionType.
- repoUrl += "/" + target.replace( '\\', '/' );
- }
- scmRepository = getScmRepository( repoUrl );
- CheckOutScmResult ret =
- scmProvider.checkOut( scmRepository, new ScmFileSet( new File( checkoutDirectory, "" ) ),
- makeScmVersion(), false );
+ File topNewDir = createdDirs.get( 0 );
+ String relTopNewDir =
+ topNewDir.getPath().substring( checkoutDirectory.getPath().length() + 1 ).replace( '\\', '/' );
- checkScmResult( ret );
+ addFiles( scmProvider, scmRepository, checkoutDirectory, relTopNewDir );
+ added = true;
+ }
}
catch ( ScmException e )
{
- fireTransferError( resource, e, TransferEvent.REQUEST_GET );
+ fireTransferError( resource, e, TransferEvent.REQUEST_PUT );
- throw new TransferFailedException( "Error checking out: " + e.getMessage(), e );
+ throw new TransferFailedException( "Failed to add directory " + createdDirs.get( 0 ) + " to working copy",
+ e );
}
-
- // now create the subdirs in target, if it's a parent of targetName
-
- String relPath = "";
-
- while ( !stack.isEmpty() )
+ finally
{
- String p = stack.pop();
- relPath += p + "/";
-
- File newDir = new File( checkoutDirectory, relPath );
- if ( !newDir.mkdirs() )
+ if ( !added && createdDirs.size() != 0 )
{
- throw new TransferFailedException(
- "Failed to create directory " + newDir.getAbsolutePath() + "; parent should exist: "
- + checkoutDirectory );
+ FileUtils.deleteDirectory( createdDirs.get( 0 ) );
}
+ }
- try
- {
- addFiles( scmProvider, scmRepository, checkoutDirectory, relPath );
- }
- catch ( ScmException e )
- {
- fireTransferError( resource, e, TransferEvent.REQUEST_PUT );
+ return res;
+ }
- throw new TransferFailedException( "Failed to add directory " + newDir + " to working copy", e );
+ private static void mkdirsThrow( File f, List<File> createdDirs )
+ throws IOException
+ {
+ if ( !f.isDirectory() )
+ {
+ File parent = f.getParentFile();
+ mkdirsThrow( parent, createdDirs );
+ if ( !f.mkdir() )
+ {
+ throw new IOException( "Failed to create directory " + f.getAbsolutePath() );
}
+ createdDirs.add( f );
}
-
- return relPath;
}
/**
@@ -551,7 +562,8 @@ private int addFiles( ScmProvider scmProvider, ScmRepository scmRepository, File
if ( scmFilePath.length() != 0 )
{
- AddScmResult result = scmProvider.add( scmRepository, new ScmFileSet( basedir, new File( scmFilePath ) ) );
+ AddScmResult result =
+ scmProvider.add( scmRepository, new ScmFileSet( basedir, new File( scmFilePath ) ), mkBinaryFlag() );
/*
* TODO dirty fix to work around files with property svn:eol-style=native if a file has that property, first
@@ -560,7 +572,9 @@ private int addFiles( ScmProvider scmProvider, ScmRepository scmRepository, File
*/
if ( !result.isSuccess() )
{
- result = scmProvider.add( scmRepository, new ScmFileSet( basedir, new File( scmFilePath ) ) );
+ result =
+ scmProvider.add( scmRepository, new ScmFileSet( basedir, new File( scmFilePath ) ),
+ mkBinaryFlag() );
}
addedFiles = result.getAddedFiles().size();
@@ -584,6 +598,26 @@ private int addFiles( ScmProvider scmProvider, ScmRepository scmRepository, File
return addedFiles;
}
+ private CheckOutScmResult checkOut( ScmProvider scmProvider, ScmRepository scmRepository, ScmFileSet fileSet )
+ throws ScmException
+ {
+ ScmVersion ver = makeScmVersion();
+ CommandParameters parameters = mkBinaryFlag();
+ // TODO: AbstractScmProvider 6f7dd0c ignores checkOut() parameter "version"
+ parameters.setScmVersion( CommandParameter.SCM_VERSION, ver );
+ parameters.setString( CommandParameter.RECURSIVE, Boolean.toString( false ) );
+ parameters.setString( CommandParameter.SHALLOW, Boolean.toString( true ) );
+
+ return scmProvider.checkOut( scmRepository, fileSet, ver, parameters );
+ }
+
+ private CommandParameters mkBinaryFlag() throws ScmException
+ {
+ CommandParameters parameters = new CommandParameters();
+ parameters.setString( CommandParameter.BINARY, Boolean.toString( true ) );
+ return parameters;
+ }
+
/**
* @return true
*/
@@ -592,6 +626,12 @@ public boolean supportsDirectoryCopy()
return true;
}
+ private boolean supportsPartialCheckout( ScmProvider scmProvider )
+ {
+ String scmType = scmProvider.getScmType();
+ return ( "svn".equals( scmType ) || "cvs".equals( scmType ) );
+ }
+
public void putDirectory( File sourceDirectory, String destinationDirectory )
throws TransferFailedException, ResourceDoesNotExistException, AuthorizationException
{
@@ -646,16 +686,19 @@ public void get( String resourceName, File destination )
fireGetInitiated( resource, destination );
- String url = getRepository().getUrl() + "/" + resourceName;
-
- // remove the file
- url = url.substring( 0, url.lastIndexOf( '/' ) );
+ fireGetStarted( resource, destination );
try
{
- ScmRepository scmRepository = getScmRepository( url );
-
- fireGetStarted( resource, destination );
+ String subdir = getDirname( resourceName );
+ ScmResult res = tryPartialCheckout( subdir );
+ if ( !res.isSuccess() && ( partCOSubdir.length() == 0 || res instanceof UpdateScmResult ) )
+ {
+ // inability to checkout SVN or CVS subdir is not fatal. We just assume it doesn't exist
+ // inability to update existing subdir or checkout root is fatal
+ throw new ScmException( "command failed: " + res.getCommandOutput().trim() );
+ }
+ resourceName = resourceName.substring( partCOSubdir.length() );
// TODO: limitations:
// - destination filename must match that in the repository - should allow the "-d" CVS equiv to be passed
@@ -666,24 +709,6 @@ public void get( String resourceName, File destination )
File scmFile = new File( checkoutDirectory, resourceName );
- File basedir = scmFile.getParentFile();
-
- ScmProvider scmProvider = getScmProvider( scmRepository.getProvider() );
-
- String reservedScmFile = scmProvider.getScmSpecificFilename();
-
- if ( reservedScmFile != null && new File( basedir, reservedScmFile ).exists() )
- {
- scmProvider.update( scmRepository, new ScmFileSet( basedir ), makeScmVersion() );
- }
- else
- {
- // TODO: this should be checking out a full hierarchy (requires the -d equiv)
- basedir.mkdirs();
-
- scmProvider.checkOut( scmRepository, new ScmFileSet( basedir ), makeScmVersion() );
- }
-
if ( !scmFile.exists() )
{
throw new ResourceDoesNotExistException( "Unable to find resource " + destination + " after checkout" );
@@ -712,6 +737,49 @@ public void get( String resourceName, File destination )
fireGetCompleted( resource, destination );
}
+ private ScmResult tryPartialCheckout( String subdir )
+ throws ScmException, IOException
+ {
+ String url = getRepository().getUrl();
+
+ String desiredPartCOSubdir = "";
+
+ ScmRepository scmRepository = getScmRepository( url );
+ ScmProvider scmProvider = getScmProvider( scmRepository.getProvider() );
+ if ( subdir.length() != 0 && supportsPartialCheckout( scmProvider ) )
+ {
+ url += ( url.endsWith( "/" ) ? "" : "/" ) + subdir;
+
+ desiredPartCOSubdir = subdir + "/";
+
+ scmRepository = getScmRepository( url );
+ }
+
+ if ( !desiredPartCOSubdir.equals( partCOSubdir ) )
+ {
+ FileUtils.deleteDirectory( checkoutDirectory );
+ partCOSubdir = desiredPartCOSubdir;
+ }
+
+ ScmResult res;
+ if ( checkoutDirExists( scmProvider ) )
+ {
+ res = scmProvider.update( scmRepository, new ScmFileSet( checkoutDirectory ), makeScmVersion() );
+ }
+ else
+ {
+ res = checkOut( scmProvider, scmRepository, new ScmFileSet( checkoutDirectory ) );
+ }
+ return res;
+ }
+
+ private boolean checkoutDirExists( ScmProvider scmProvider )
+ {
+ String reservedScmFile = scmProvider.getScmSpecificFilename();
+ File pathToCheck = reservedScmFile == null ? checkoutDirectory : new File( checkoutDirectory, reservedScmFile );
+ return pathToCheck.exists();
+ }
+
/**
* @return a List<String> with filenames/directories at the resourcepath.
* @see org.apache.maven.wagon.AbstractWagon#getFileList(java.lang.String)
@@ -764,15 +832,8 @@ public boolean resourceExists( String resourceName )
}
}
- private String getFilename( String filename )
- {
- String fname = StringUtils.replace( filename, "/", File.separator );
- return FileUtils.filename( fname );
- }
-
- private String getDirname( String filename )
+ private String getDirname( String resourceName )
{
- String fname = StringUtils.replace( filename, "/", File.separator );
- return FileUtils.dirname( fname );
+ return FileUtils.getPath( resourceName, '/' );
}
}
diff --git a/wagon-providers/wagon-scm/src/test/java/org/apache/maven/wagon/providers/scm/AbstractScmCvsWagonTest.java b/wagon-providers/wagon-scm/src/test/java/org/apache/maven/wagon/providers/scm/AbstractScmCvsWagonTest.java
index fc7b7f82..eac45626 100644
--- a/wagon-providers/wagon-scm/src/test/java/org/apache/maven/wagon/providers/scm/AbstractScmCvsWagonTest.java
+++ b/wagon-providers/wagon-scm/src/test/java/org/apache/maven/wagon/providers/scm/AbstractScmCvsWagonTest.java
@@ -41,6 +41,6 @@ protected String getTestRepositoryUrl()
{
String repository = getTestFile( "target/test-classes/test-repo-cvs" ).getAbsolutePath();
- return "scm:cvs|local|" + repository + "|repository/newfolder";
+ return "scm:cvs|local|" + repository + "|repository";
}
}
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
> Fix checkoutDirectory leak
> --------------------------
>
> Key: WAGON-495
> URL: https://issues.apache.org/jira/browse/WAGON-495
> Project: Maven Wagon
> Issue Type: Bug
> Components: wagon-scm
> Affects Versions: 3.0.0, 3.1.0
> Reporter: Ilya Basin
> Assignee: Michael Osipov
> Priority: Major
> Fix For: 3.1.0
>
>
> During deploy artifacts to SVN an instance of ScmWagon is initialized and artifact metadata is downloaded to a local folder. After that maven tries to upload the jar file. ScmWagon.put(File,String) is called which internally overwrites the checkoutDirectory field and checks out the repo again to another temporary folder. The original folder is forgotten.
> Maven uploads jars, poms, checksums and for each file ScmWagon checks out a new directory.
> In the end the closeConnection() method is called which removes the last used folder.
> UPD: this causes false positive in testWagonPutDirectoryWhenDirectoryAlreadyExists(): the test deletes the test checkout dir before calling get(), but the wagon checkout dir remains and the file is retrieved from there.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)