Don't use missing directory listing fallback if it can be avoided

If the POM is in cache, but not the directory listing (nor a .error file for it), the LocalUpdate and LocalUpdateChanging policies make MavenRepository do as if the directory listing is missing - it is not even checked with the subsequent cache policies if any (because the POM was found with LocalUpdate / LocalUpdateChanging in the first place).

This PR fixes that - getting the POM will fail if the directory listing or an error file for it is missing from cache. That way, subsequent cache policies can fetch the directory listing.

As this only happens depending on what's in cache, it's a bit cumbersome to test as is. Relying fully on NIO2 in subsequent coursier versions should make it easier to add test cases for that (by using a virtual fs like jimfs to test what happens depending on what's in cache).

The test should basically do:

    $ coursier fetch org.apache.maven:apache-maven:3.3.9 # fill cache
    $ rm -f $CACHE_PATH/https/repo1.maven.org/maven2/org/apache/maven/apache-maven/3.3.9/.directory
    $ rm -f $CACHE_PATH/https/repo1.maven.org/maven2/org/apache/maven/apache-maven/3.3.9/..directory.checked
    $ coursier fetch org.apache.maven:apache-maven:3.3.9

 The second fetch should work fine, fetching the directory listing, seeing that it lists no JAR, and then not attempting to download one. Before this commit, the second attempt would assume that the directory listing is not available, do without it, so assume that a JAR exists, and fail to download it.
This commit is contained in:
Alexandre Archambault 2017-04-18 18:31:37 +02:00
parent f9de965638
commit 68b9eeeda8
2 changed files with 82 additions and 19 deletions

View File

@ -624,14 +624,16 @@ object Cache {
}
}
def errFile(file: File) = new File(file.getParentFile, "." + file.getName + ".error")
def remoteKeepErrors(file: File, url: String): EitherT[Task, FileError, Unit] = {
val errFile = new File(file.getParentFile, "." + file.getName + ".error")
val errFile0 = errFile(file)
def validErrFileExists =
EitherT {
Task {
(referenceFileExists && errFile.exists()).right[FileError]
(referenceFileExists && errFile0.exists()).right[FileError]
}
}
@ -639,8 +641,8 @@ object Cache {
EitherT {
Task {
if (referenceFileExists) {
if (!errFile.exists())
FileUtil.write(errFile, "".getBytes("UTF-8"))
if (!errFile0.exists())
FileUtil.write(errFile0, "".getBytes("UTF-8"))
}
().right[FileError]
@ -650,8 +652,8 @@ object Cache {
def deleteErrFile =
EitherT {
Task {
if (errFile.exists())
errFile.delete()
if (errFile0.exists())
errFile0.delete()
().right[FileError]
}
@ -681,6 +683,23 @@ object Cache {
}
}
def localInfo(file: File, url: String): EitherT[Task, FileError, Boolean] = {
val errFile0 = errFile(file)
// memo-ized
lazy val res =
if (file.exists())
true.right[FileError]
else if (referenceFileExists && errFile0.exists())
FileError.NotFound(url, Some(true)).left[Boolean]
else
false.right[FileError]
EitherT(Task(res))
}
def checkFileExists(file: File, url: String, log: Boolean = true): EitherT[Task, FileError, Unit] =
EitherT {
Task {
@ -699,11 +718,38 @@ object Cache {
.flatMap(artifact.checksumUrls.get)
}
val cachePolicy0 = cachePolicy match {
case CachePolicy.UpdateChanging if !artifact.changing =>
CachePolicy.FetchMissing
case CachePolicy.LocalUpdateChanging if !artifact.changing =>
CachePolicy.LocalOnly
case other =>
other
}
val requiredArtifactCheck = artifact.extra.get("required") match {
case None =>
EitherT(Task.now(().right[FileError]))
case Some(required) =>
cachePolicy0 match {
case CachePolicy.LocalOnly | CachePolicy.LocalUpdateChanging | CachePolicy.LocalUpdate =>
val file = localFile(required.url, cache, artifact.authentication.map(_.user))
localInfo(file, required.url).flatMap {
case true =>
EitherT(Task.now(().right[FileError]))
case false =>
EitherT(Task.now(FileError.NotFound(file.toString).left[Unit]))
}
case _ =>
EitherT(Task.now(().right[FileError]))
}
}
val tasks =
for (url <- urls) yield {
val file = localFile(url, cache, artifact.authentication.map(_.user))
val res =
def res =
if (url.startsWith("file:/")) {
// for debug purposes, flaky with URL-encoded chars anyway
// def filtered(s: String) =
@ -721,15 +767,6 @@ object Cache {
EitherT(Task.now[FileError \/ Unit](().right))
}
val cachePolicy0 = cachePolicy match {
case CachePolicy.UpdateChanging if !artifact.changing =>
CachePolicy.FetchMissing
case CachePolicy.LocalUpdateChanging if !artifact.changing =>
CachePolicy.LocalOnly
case other =>
other
}
cachePolicy0 match {
case CachePolicy.LocalOnly =>
checkFileExists(file, url)
@ -746,8 +783,10 @@ object Cache {
}
}
res.run.map((file, url) -> _)
requiredArtifactCheck
.flatMap(_ => res)
.run
.map((file, url) -> _)
}
Nondeterminism[Task].gather(tasks)

View File

@ -282,10 +282,34 @@ final case class MavenRepository(
None
val projectArtifact0 = projectArtifact(module, version, versioningValue)
val listFilesUrl = urlFor(modulePath(module, version)) + "/"
val listFilesArtifact =
Artifact(
listFilesUrl,
Map.empty,
Map(
"metadata" -> projectArtifact0
),
Attributes("", ""),
changing = changing.getOrElse(version.contains("-SNAPSHOT")),
authentication
)
val requiringDirListingProjectArtifact = projectArtifact0
.copy(
extra = projectArtifact0.extra + (
// In LocalUpdate and LocalUpdateChanging mode, makes getting the POM fail if the POM
// is in cache, but no info about the directory listing is (directory listing not in cache and no kept error
// for it).
"required" -> listFilesArtifact
)
)
for {
str <- fetch(projectArtifact(module, version, versioningValue))
str <- fetch(requiringDirListingProjectArtifact)
rawListFilesPageOpt <- EitherT(F.map(fetch(artifactFor(listFilesUrl)).run) {
e => \/-(e.toOption): String \/ Option[String]
})