From ae2fddebf2ff79126b192a532d4ff74155584db8 Mon Sep 17 00:00:00 2001 From: danicheg Date: Sun, 16 Feb 2025 23:07:48 +0300 Subject: [PATCH 1/7] Take redirects into account in UrlChecker --- .../scalasteward/core/util/UrlChecker.scala | 62 ++++++++++++++++--- 1 file changed, 53 insertions(+), 9 deletions(-) diff --git a/modules/core/src/main/scala/org/scalasteward/core/util/UrlChecker.scala b/modules/core/src/main/scala/org/scalasteward/core/util/UrlChecker.scala index 92f2a7d536..8a4ebb8286 100644 --- a/modules/core/src/main/scala/org/scalasteward/core/util/UrlChecker.scala +++ b/modules/core/src/main/scala/org/scalasteward/core/util/UrlChecker.scala @@ -20,6 +20,7 @@ import cats.effect.Sync import cats.syntax.all.* import com.github.benmanes.caffeine.cache.Caffeine import org.http4s.client.Client +import org.http4s.headers.Location import org.http4s.{Method, Request, Status, Uri} import org.scalasteward.core.application.Config import org.typelevel.log4cats.Logger @@ -27,21 +28,43 @@ import scalacache.Entry import scalacache.caffeine.CaffeineCache trait UrlChecker[F[_]] { - def exists(url: Uri): F[Boolean] + def validate(url: Uri): F[UrlChecker.UrlValidationResult] } final case class UrlCheckerClient[F[_]](client: Client[F]) extends AnyVal object UrlChecker { + + sealed abstract class UrlValidationResult extends Product with Serializable { + def exists: Boolean = + fold(exists = true, notExists = false, redirectTo = _ => false) + + def notExists: Boolean = + fold(exists = false, notExists = true, redirectTo = _ => false) + + def fold[A](exists: => A, notExists: => A, redirectTo: Uri => A): A = + this match { + case UrlValidationResult.Exists => exists + case UrlValidationResult.NotExists => notExists + case UrlValidationResult.RedirectTo(url) => redirectTo(url) + } + } + + object UrlValidationResult { + case object Exists extends UrlValidationResult + case object NotExists extends UrlValidationResult + final case class RedirectTo(url: Uri) extends UrlValidationResult + } + private def buildCache[F[_]](config: Config)(implicit F: Sync[F] - ): F[CaffeineCache[F, String, Status]] = + ): F[CaffeineCache[F, String, UrlValidationResult]] = F.delay { val cache = Caffeine .newBuilder() .maximumSize(16384L) .expireAfterWrite(config.cacheTtl.length, config.cacheTtl.unit) - .build[String, Entry[Status]]() + .build[String, Entry[UrlValidationResult]]() CaffeineCache(cache) } @@ -52,15 +75,36 @@ object UrlChecker { ): F[UrlChecker[F]] = buildCache(config).map { statusCache => new UrlChecker[F] { - override def exists(url: Uri): F[Boolean] = - status(url).map(_ === Status.Ok).handleErrorWith { throwable => - logger.debug(throwable)(s"Failed to check if $url exists").as(false) - } + override def validate(url: Uri): F[UrlValidationResult] = + check(url).handleErrorWith(th => + logger + .debug(th)(s"Failed to check if $url exists") + .as(UrlValidationResult.NotExists) + ) - private def status(url: Uri): F[Status] = + private def check(url: Uri): F[UrlValidationResult] = statusCache.cachingF(url.renderString)(None) { val req = Request[F](method = Method.HEAD, uri = url) - modify(req).flatMap(urlCheckerClient.client.status) + + modify(req).flatMap(r => + urlCheckerClient.client.run(r).use { + case resp if resp.status === Status.Ok => + F.pure(UrlValidationResult.Exists) + + case resp if resp.status === Status.MovedPermanently => + val uriMaybe = + resp.headers + .get[Location] + .fold[UrlValidationResult](UrlValidationResult.NotExists)(h => + UrlValidationResult.RedirectTo(h.uri) + ) + + F.pure(uriMaybe) + + case _ => + F.pure(UrlValidationResult.NotExists) + } + ) } } } From 67e38f2700e2ff71772c6bfb107fb7bb644f8afb Mon Sep 17 00:00:00 2001 From: danicheg Date: Sun, 16 Feb 2025 23:08:23 +0300 Subject: [PATCH 2/7] Use new API from UrlChecker --- .../org/scalasteward/core/application/SelfCheckAlg.scala | 4 ++-- .../main/scala/org/scalasteward/core/nurture/NurtureAlg.scala | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/modules/core/src/main/scala/org/scalasteward/core/application/SelfCheckAlg.scala b/modules/core/src/main/scala/org/scalasteward/core/application/SelfCheckAlg.scala index 826d6f5a10..462cdad32e 100644 --- a/modules/core/src/main/scala/org/scalasteward/core/application/SelfCheckAlg.scala +++ b/modules/core/src/main/scala/org/scalasteward/core/application/SelfCheckAlg.scala @@ -81,9 +81,9 @@ final class SelfCheckAlg[F[_]](config: Config)(implicit private def checkUrlChecker: F[Unit] = config.urlCheckerTestUrls.traverse_ { url => - urlChecker.exists(url).flatMap { urlExists => + urlChecker.validate(url).flatMap { res => val msg = s"Self check of UrlChecker failed: checking that $url exists failed" - F.whenA(!urlExists)(logger.warn(msg)) + F.whenA(res.notExists)(logger.warn(msg)) } } } diff --git a/modules/core/src/main/scala/org/scalasteward/core/nurture/NurtureAlg.scala b/modules/core/src/main/scala/org/scalasteward/core/nurture/NurtureAlg.scala index 9ab3c02d08..dc58b5d4f1 100644 --- a/modules/core/src/main/scala/org/scalasteward/core/nurture/NurtureAlg.scala +++ b/modules/core/src/main/scala/org/scalasteward/core/nurture/NurtureAlg.scala @@ -206,7 +206,7 @@ final class NurtureAlg[F[_]](config: ForgeCfg)(implicit .traverse { case (_, dependency) => coursierAlg .getMetadata(dependency, resolvers) - .flatMap(_.filterUrls(urlChecker.exists)) + .flatMap(_.filterUrls(uri => urlChecker.validate(uri).map(_.exists))) .tupleLeft(dependency) } .map(_.toMap) From fa62dd767cd645cb3f79f20ea69b62a1e0bbd9ea Mon Sep 17 00:00:00 2001 From: danicheg Date: Sun, 16 Feb 2025 23:09:14 +0300 Subject: [PATCH 3/7] Update URLs in UpdateInfoUrlFinder on redirects --- .../core/nurture/UpdateInfoUrl.scala | 18 ++++++++++++++---- .../core/nurture/UpdateInfoUrlFinder.scala | 15 +++++++++++++-- 2 files changed, 27 insertions(+), 6 deletions(-) diff --git a/modules/core/src/main/scala/org/scalasteward/core/nurture/UpdateInfoUrl.scala b/modules/core/src/main/scala/org/scalasteward/core/nurture/UpdateInfoUrl.scala index fae0fedbef..b7c9aa38f2 100644 --- a/modules/core/src/main/scala/org/scalasteward/core/nurture/UpdateInfoUrl.scala +++ b/modules/core/src/main/scala/org/scalasteward/core/nurture/UpdateInfoUrl.scala @@ -22,13 +22,23 @@ import org.http4s.Uri /** A URL of a resource that provides additional information for an update. */ sealed trait UpdateInfoUrl { def url: Uri + + def withUrl(url: Uri): UpdateInfoUrl } object UpdateInfoUrl { - final case class CustomChangelog(url: Uri) extends UpdateInfoUrl - final case class CustomReleaseNotes(url: Uri) extends UpdateInfoUrl - final case class GitHubReleaseNotes(url: Uri) extends UpdateInfoUrl - final case class VersionDiff(url: Uri) extends UpdateInfoUrl + final case class CustomChangelog(url: Uri) extends UpdateInfoUrl { + override def withUrl(url: Uri): UpdateInfoUrl = CustomChangelog(url) + } + final case class CustomReleaseNotes(url: Uri) extends UpdateInfoUrl { + override def withUrl(url: Uri): UpdateInfoUrl = CustomReleaseNotes(url) + } + final case class GitHubReleaseNotes(url: Uri) extends UpdateInfoUrl { + override def withUrl(url: Uri): UpdateInfoUrl = GitHubReleaseNotes(url) + } + final case class VersionDiff(url: Uri) extends UpdateInfoUrl { + override def withUrl(url: Uri): UpdateInfoUrl = VersionDiff(url) + } implicit val updateInfoUrlOrder: Order[UpdateInfoUrl] = Order.by { diff --git a/modules/core/src/main/scala/org/scalasteward/core/nurture/UpdateInfoUrlFinder.scala b/modules/core/src/main/scala/org/scalasteward/core/nurture/UpdateInfoUrlFinder.scala index 5341a6aeec..91542ceef5 100644 --- a/modules/core/src/main/scala/org/scalasteward/core/nurture/UpdateInfoUrlFinder.scala +++ b/modules/core/src/main/scala/org/scalasteward/core/nurture/UpdateInfoUrlFinder.scala @@ -16,7 +16,7 @@ package org.scalasteward.core.nurture -import cats.Monad +import cats.{Monad, Parallel} import cats.syntax.all.* import org.http4s.Uri import org.scalasteward.core.application.Config.ForgeCfg @@ -31,6 +31,7 @@ import org.scalasteward.core.util.UrlChecker final class UpdateInfoUrlFinder[F[_]](implicit config: ForgeCfg, urlChecker: UrlChecker[F], + parallel: Parallel[F], F: Monad[F] ) { def findUpdateInfoUrls( @@ -46,7 +47,17 @@ final class UpdateInfoUrlFinder[F[_]](implicit updateInfoUrls .sorted(UpdateInfoUrl.updateInfoUrlOrder.toOrdering) .distinctBy(_.url) - .filterA(updateInfoUrl => urlChecker.exists(updateInfoUrl.url)) + .parFlatTraverse(updateInfoUrl => + urlChecker + .validate(updateInfoUrl.url) + .map( + _.fold( + exists = List(updateInfoUrl), + notExists = List.empty, + redirectTo = u => List(updateInfoUrl.withUrl(u)) + ) + ) + ) } } From 8c93ac6b43df8d3fcce16c044cd0e488d3827b0f Mon Sep 17 00:00:00 2001 From: danicheg Date: Sun, 16 Feb 2025 23:10:07 +0300 Subject: [PATCH 4/7] Add UrlCheckerTest --- .../core/util/UrlCheckerTest.scala | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) create mode 100644 modules/core/src/test/scala/org/scalasteward/core/util/UrlCheckerTest.scala diff --git a/modules/core/src/test/scala/org/scalasteward/core/util/UrlCheckerTest.scala b/modules/core/src/test/scala/org/scalasteward/core/util/UrlCheckerTest.scala new file mode 100644 index 0000000000..1a02ac6826 --- /dev/null +++ b/modules/core/src/test/scala/org/scalasteward/core/util/UrlCheckerTest.scala @@ -0,0 +1,52 @@ +package org.scalasteward.core.util + +import cats.syntax.all.* +import munit.CatsEffectSuite +import org.http4s.{Headers, HttpApp, Response, Status} +import org.http4s.dsl.Http4sDsl +import org.http4s.headers.Location +import org.http4s.syntax.all.* +import org.scalasteward.core.mock.* +import org.scalasteward.core.mock.MockContext.context.* +import org.scalasteward.core.mock.{MockEff, MockState} +import org.scalasteward.core.util.UrlChecker.UrlValidationResult + +class UrlCheckerTest extends CatsEffectSuite with Http4sDsl[MockEff] { + private val baseUrl = uri"https://github.com/scala-steward-org" + private val redirectUrl = baseUrl / "scala-steward" + + private val state = + MockState.empty.copy(clientResponses = GitHubAuth.api(List.empty) <+> HttpApp { + case HEAD -> Root / "scala-steward-org" => Ok() + case HEAD -> Root / "scala-steward-org" / "wrong-uri" => NotFound() + case HEAD -> Root / "scala-steward-org" / "redirect-uri" => + Response[MockEff]( + status = Status.MovedPermanently, + headers = Headers(Location(redirectUrl)) + ).pure[MockEff] + case _ => + ServiceUnavailable() + }) + + test("An URL exists") { + val url = baseUrl + val check = urlChecker.validate(url).runA(state) + + assertIOBoolean(check.map(_.exists)) + } + + test("An URL does not exist") { + val url = baseUrl / "wrong-uri" + val check = urlChecker.validate(url).runA(state) + + assertIOBoolean(check.map(_.notExists)) + } + + test("An URL redirects to another URL") { + val httpUrl = uri"https://github.com/scala-steward-org/redirect-uri" + val check = urlChecker.validate(httpUrl).runA(state) + val anticipatedResult = UrlValidationResult.RedirectTo(redirectUrl) + + assertIO(check, anticipatedResult) + } +} From 6ee6639e5399c0dbcb3a718f310a1c945c11b7d4 Mon Sep 17 00:00:00 2001 From: danicheg Date: Sun, 16 Feb 2025 23:46:26 +0300 Subject: [PATCH 5/7] Address an edge case in UrlChecker --- .../scalasteward/core/util/UrlChecker.scala | 24 +++++++++++-------- .../core/util/UrlCheckerTest.scala | 24 +++++++++++++++---- 2 files changed, 34 insertions(+), 14 deletions(-) diff --git a/modules/core/src/main/scala/org/scalasteward/core/util/UrlChecker.scala b/modules/core/src/main/scala/org/scalasteward/core/util/UrlChecker.scala index 8a4ebb8286..96b1ec35ca 100644 --- a/modules/core/src/main/scala/org/scalasteward/core/util/UrlChecker.scala +++ b/modules/core/src/main/scala/org/scalasteward/core/util/UrlChecker.scala @@ -76,13 +76,13 @@ object UrlChecker { buildCache(config).map { statusCache => new UrlChecker[F] { override def validate(url: Uri): F[UrlValidationResult] = - check(url).handleErrorWith(th => + check(url, recursion = true).handleErrorWith(th => logger .debug(th)(s"Failed to check if $url exists") .as(UrlValidationResult.NotExists) ) - private def check(url: Uri): F[UrlValidationResult] = + private def check(url: Uri, recursion: Boolean): F[UrlValidationResult] = statusCache.cachingF(url.renderString)(None) { val req = Request[F](method = Method.HEAD, uri = url) @@ -91,15 +91,19 @@ object UrlChecker { case resp if resp.status === Status.Ok => F.pure(UrlValidationResult.Exists) - case resp if resp.status === Status.MovedPermanently => - val uriMaybe = - resp.headers - .get[Location] - .fold[UrlValidationResult](UrlValidationResult.NotExists)(h => - UrlValidationResult.RedirectTo(h.uri) + case resp if recursion && resp.status === Status.MovedPermanently => + resp.headers + .get[Location] + .traverse(h => + check(h.uri, recursion = false).map( + _.fold( + exists = UrlValidationResult.RedirectTo(h.uri), + notExists = UrlValidationResult.NotExists, + redirectTo = _ => UrlValidationResult.NotExists + ) ) - - F.pure(uriMaybe) + ) + .map(_.getOrElse(UrlValidationResult.NotExists)) case _ => F.pure(UrlValidationResult.NotExists) diff --git a/modules/core/src/test/scala/org/scalasteward/core/util/UrlCheckerTest.scala b/modules/core/src/test/scala/org/scalasteward/core/util/UrlCheckerTest.scala index 1a02ac6826..4ea6b194b7 100644 --- a/modules/core/src/test/scala/org/scalasteward/core/util/UrlCheckerTest.scala +++ b/modules/core/src/test/scala/org/scalasteward/core/util/UrlCheckerTest.scala @@ -13,17 +13,24 @@ import org.scalasteward.core.util.UrlChecker.UrlValidationResult class UrlCheckerTest extends CatsEffectSuite with Http4sDsl[MockEff] { private val baseUrl = uri"https://github.com/scala-steward-org" - private val redirectUrl = baseUrl / "scala-steward" + private val redirectUrl = baseUrl / "finished-redirect" + private val anotherRedirectUrl = baseUrl / "yet-another-redirect" private val state = MockState.empty.copy(clientResponses = GitHubAuth.api(List.empty) <+> HttpApp { case HEAD -> Root / "scala-steward-org" => Ok() case HEAD -> Root / "scala-steward-org" / "wrong-uri" => NotFound() - case HEAD -> Root / "scala-steward-org" / "redirect-uri" => + case HEAD -> Root / "scala-steward-org" / "single-redirect" => Response[MockEff]( status = Status.MovedPermanently, headers = Headers(Location(redirectUrl)) ).pure[MockEff] + case HEAD -> Root / "scala-steward-org" / "finished-redirect" => Ok() + case HEAD -> Root / "scala-steward-org" / "yet-another-redirect" => + Response[MockEff]( + status = Status.MovedPermanently, + headers = Headers(Location(anotherRedirectUrl)) + ).pure[MockEff] case _ => ServiceUnavailable() }) @@ -42,11 +49,20 @@ class UrlCheckerTest extends CatsEffectSuite with Http4sDsl[MockEff] { assertIOBoolean(check.map(_.notExists)) } - test("An URL redirects to another URL") { - val httpUrl = uri"https://github.com/scala-steward-org/redirect-uri" + test("An URL redirects to another existing URL") { + val httpUrl = uri"https://github.com/scala-steward-org/single-redirect" val check = urlChecker.validate(httpUrl).runA(state) val anticipatedResult = UrlValidationResult.RedirectTo(redirectUrl) assertIO(check, anticipatedResult) } + + // basically, we prohibit the infinite loop of traversing redirect URLs + test("An URL redirects to another redirecting URL") { + val httpUrl = uri"https://github.com/scala-steward-org/yet-another-redirect" + val check = urlChecker.validate(httpUrl).runA(state) + val anticipatedResult = UrlValidationResult.NotExists + + assertIO(check, anticipatedResult) + } } From 14d6d6a24469b69145588adc1fa958a02895051f Mon Sep 17 00:00:00 2001 From: danicheg Date: Sat, 1 Mar 2025 15:02:51 +0300 Subject: [PATCH 6/7] Check repoUrl in UpdateInfoUrlFinder before preparing URLs --- .../core/nurture/UpdateInfoUrlFinder.scala | 68 ++++++++++++------- .../nurture/UpdateInfoUrlFinderTest.scala | 63 ++++++++++++----- 2 files changed, 88 insertions(+), 43 deletions(-) diff --git a/modules/core/src/main/scala/org/scalasteward/core/nurture/UpdateInfoUrlFinder.scala b/modules/core/src/main/scala/org/scalasteward/core/nurture/UpdateInfoUrlFinder.scala index 91542ceef5..8077051dd1 100644 --- a/modules/core/src/main/scala/org/scalasteward/core/nurture/UpdateInfoUrlFinder.scala +++ b/modules/core/src/main/scala/org/scalasteward/core/nurture/UpdateInfoUrlFinder.scala @@ -16,7 +16,7 @@ package org.scalasteward.core.nurture -import cats.{Monad, Parallel} +import cats.{Functor, Monad, Parallel} import cats.syntax.all.* import org.http4s.Uri import org.scalasteward.core.application.Config.ForgeCfg @@ -38,26 +38,30 @@ final class UpdateInfoUrlFinder[F[_]](implicit dependency: DependencyMetadata, versionUpdate: Version.Update ): F[List[UpdateInfoUrl]] = { - val updateInfoUrls: List[UpdateInfoUrl] = - dependency.releaseNotesUrl.toList.map(CustomReleaseNotes.apply) ++ - dependency.forgeRepo.toSeq.flatMap(forgeRepo => - possibleUpdateInfoUrls(forgeRepo, versionUpdate) - ) + val updateInfoUrls: F[List[UpdateInfoUrl]] = + dependency.forgeRepo.toList + .flatTraverse(forgeRepo => possibleUpdateInfoUrls(urlChecker)(forgeRepo, versionUpdate)) + .map(dependency.releaseNotesUrl.toList.map(CustomReleaseNotes.apply) ++ _) - updateInfoUrls - .sorted(UpdateInfoUrl.updateInfoUrlOrder.toOrdering) - .distinctBy(_.url) - .parFlatTraverse(updateInfoUrl => - urlChecker - .validate(updateInfoUrl.url) - .map( - _.fold( - exists = List(updateInfoUrl), - notExists = List.empty, - redirectTo = u => List(updateInfoUrl.withUrl(u)) + for { + urls <- updateInfoUrls + .map( + _.sorted(UpdateInfoUrl.updateInfoUrlOrder.toOrdering) + .distinctBy(_.url) + ) + checkedUrls <- + urls.parFlatTraverse(updateInfoUrl => + urlChecker + .validate(updateInfoUrl.url) + .map( + _.fold( + exists = List(updateInfoUrl), + notExists = List.empty, + redirectTo = u => List(updateInfoUrl.withUrl(u)) + ) ) - ) - ) + ) + } yield checkedUrls } } @@ -92,17 +96,31 @@ object UpdateInfoUrlFinder { repoForge.diffUrlFor(tagName(update.currentVersion), tagName(update.nextVersion)) ) - private[nurture] def possibleUpdateInfoUrls( + private[nurture] def possibleUpdateInfoUrls[F[_]: Functor](urlChecker: UrlChecker[F])( forgeRepo: ForgeRepo, update: Version.Update - ): List[UpdateInfoUrl] = { + ): F[List[UpdateInfoUrl]] = { def customUrls(wrap: Uri => UpdateInfoUrl, fileNames: List[String]): List[UpdateInfoUrl] = fileNames.map(f => wrap(forgeRepo.fileUrlFor(f))) - gitHubReleaseNotesFor(forgeRepo, update.nextVersion) ++ - customUrls(CustomReleaseNotes.apply, possibleReleaseNotesFilenames) ++ - customUrls(CustomChangelog.apply, possibleChangelogFilenames) ++ - possibleVersionDiffs(forgeRepo, update) + for { + forgeRepoMaybe <- urlChecker + .validate(forgeRepo.repoUrl) + .map( + _.fold( + exists = forgeRepo.some, + notExists = none, + redirectTo = u => forgeRepo.copy(repoUrl = u).some + ) + ) + urls = + forgeRepoMaybe.toList.flatMap(forgeRepoChecked => + gitHubReleaseNotesFor(forgeRepoChecked, update.nextVersion) ++ + customUrls(CustomReleaseNotes.apply, possibleReleaseNotesFilenames) ++ + customUrls(CustomChangelog.apply, possibleChangelogFilenames) ++ + possibleVersionDiffs(forgeRepoChecked, update) + ) + } yield urls } private def gitHubReleaseNotesFor( diff --git a/modules/core/src/test/scala/org/scalasteward/core/nurture/UpdateInfoUrlFinderTest.scala b/modules/core/src/test/scala/org/scalasteward/core/nurture/UpdateInfoUrlFinderTest.scala index 8765032eb3..ea88479160 100644 --- a/modules/core/src/test/scala/org/scalasteward/core/nurture/UpdateInfoUrlFinderTest.scala +++ b/modules/core/src/test/scala/org/scalasteward/core/nurture/UpdateInfoUrlFinderTest.scala @@ -2,8 +2,9 @@ package org.scalasteward.core.nurture import cats.syntax.all.* import munit.CatsEffectSuite -import org.http4s.HttpApp +import org.http4s.{Headers, HttpApp, Response, Status} import org.http4s.dsl.Http4sDsl +import org.http4s.headers.Location import org.http4s.syntax.literals.* import org.scalasteward.core.application.Config.ForgeCfg import org.scalasteward.core.coursier.DependencyMetadata @@ -18,12 +19,24 @@ import org.scalasteward.core.nurture.UpdateInfoUrlFinder.* class UpdateInfoUrlFinderTest extends CatsEffectSuite with Http4sDsl[MockEff] { private val httpApp = HttpApp[MockEff] { + case HEAD -> Root / "foo" / "foo" => Ok() + case HEAD -> Root / "foo" / "bar" => Ok() + case HEAD -> Root / "foo" / "bar2" => Ok() + case req @ HEAD -> Root / "foo" / "bar3" => + val redirectUrl = req.uri.withPath(Path.empty / "foo" / "bar3-redirected") + + Response[MockEff]( + status = Status.MovedPermanently, + headers = Headers(Location(redirectUrl)) + ).pure[MockEff] + case HEAD -> Root / "foo" / "bar3-redirected" => Ok() case HEAD -> Root / "foo" / "bar" / "README.md" => Ok() case HEAD -> Root / "foo" / "bar" / "compare" / "v0.1.0...v0.2.0" => Ok() case HEAD -> Root / "foo" / "bar1" / "blob" / "master" / "RELEASES.md" => Ok() case HEAD -> Root / "foo" / "buz" / "compare" / "v0.1.0...v0.2.0" => PermanentRedirect() case HEAD -> Root / "foo" / "bar2" / "releases" / "tag" / "v0.2.0" => Ok() - case _ => NotFound() + case HEAD -> Root / "foo" / "bar3-redirected" / "releases" / "tag" / "v0.2.0" => Ok() + case _ => NotFound() } private val authApp = GitHubAuth.api(List(Repository("foo/bar"))) private val state = MockState.empty.copy(clientResponses = authApp <+> httpApp) @@ -68,6 +81,18 @@ class UpdateInfoUrlFinderTest extends CatsEffectSuite with Http4sDsl[MockEff] { assertIO(obtained, expected) } + test( + "findUpdateInfoUrls: GitHubReleaseNotes and CustomReleaseNotes with the same URL after redirection" + ) { + val metadata = DependencyMetadata.empty.copy( + scmUrl = uri"https://github.com/foo/bar3".some, + releaseNotesUrl = uri"https://github.com/foo/bar3-redirected/releases/tag/v0.2.0".some + ) + val obtained = updateInfoUrlFinder.findUpdateInfoUrls(metadata, versionUpdate).runA(state) + val expected = List(GitHubReleaseNotes(metadata.releaseNotesUrl.get)) + assertIO(obtained, expected) + } + test("findUpdateInfoUrls: releaseNotesUrl is in possibleReleaseRelatedUrls") { val metadata = DependencyMetadata.empty.copy( scmUrl = uri"https://github.com/foo/bar1".some, @@ -92,8 +117,8 @@ class UpdateInfoUrlFinderTest extends CatsEffectSuite with Http4sDsl[MockEff] { addLabels = false ) private val onPremUpdateUrlFinder = new UpdateInfoUrlFinder[MockEff] - private val gitHubFooBarRepo = ForgeRepo(GitHub, uri"https://github.com/foo/bar/") - private val bitbucketFooBarRepo = ForgeRepo(Bitbucket, uri"https://bitbucket.org/foo/bar/") + private val gitHubFooBarRepo = ForgeRepo(GitHub, uri"https://github.com/foo/bar") + private val bitbucketFooBarRepo = ForgeRepo(Bitbucket, uri"https://bitbucket.org/foo/bar") private val gitLabFooBarRepo = ForgeRepo(GitLab, uri"https://gitlab.com/foo/bar") test("findUpdateInfoUrls: on-prem, repoUrl not found") { @@ -185,10 +210,10 @@ class UpdateInfoUrlFinderTest extends CatsEffectSuite with Http4sDsl[MockEff] { } test("possibleUpdateInfoUrls: github.com") { - val obtained = possibleUpdateInfoUrls( + val obtained = possibleUpdateInfoUrls(urlChecker)( gitHubFooBarRepo, versionUpdate - ).map(_.url.renderString) + ).map(_.map(_.url.renderString)).runA(state) val expected = List( s"https://github.com/foo/bar/releases/tag/v$v2", s"https://github.com/foo/bar/releases/tag/$v2", @@ -221,14 +246,14 @@ class UpdateInfoUrlFinderTest extends CatsEffectSuite with Http4sDsl[MockEff] { s"https://github.com/foo/bar/compare/$v1...$v2", s"https://github.com/foo/bar/compare/release-$v1...release-$v2" ) - assertEquals(obtained, expected) + assertIO(obtained, expected) } test("possibleUpdateInfoUrls: gitlab.com") { - val obtained = possibleUpdateInfoUrls( + val obtained = possibleUpdateInfoUrls(urlChecker)( gitLabFooBarRepo, versionUpdate - ).map(_.url.renderString) + ).map(_.map(_.url.renderString)).runA(state) val expected = possibleReleaseNotesFilenames.map(name => s"https://gitlab.com/foo/bar/blob/master/$name") ++ possibleChangelogFilenames.map(name => s"https://gitlab.com/foo/bar/blob/master/$name") ++ @@ -237,14 +262,14 @@ class UpdateInfoUrlFinderTest extends CatsEffectSuite with Http4sDsl[MockEff] { s"https://gitlab.com/foo/bar/compare/$v1...$v2", s"https://gitlab.com/foo/bar/compare/release-$v1...release-$v2" ) - assertEquals(obtained, expected) + assertIO(obtained, expected) } test("possibleUpdateInfoUrls: on-prem gitlab") { - val obtained = possibleUpdateInfoUrls( + val obtained = possibleUpdateInfoUrls(urlChecker)( ForgeRepo(GitLab, uri"https://gitlab.on-prem.net/foo/bar"), versionUpdate - ).map(_.url.renderString) + ).map(_.map(_.url.renderString)).runA(state) val expected = possibleReleaseNotesFilenames.map(name => s"https://gitlab.on-prem.net/foo/bar/blob/master/$name" ) ++ possibleChangelogFilenames.map(name => @@ -254,14 +279,14 @@ class UpdateInfoUrlFinderTest extends CatsEffectSuite with Http4sDsl[MockEff] { s"https://gitlab.on-prem.net/foo/bar/compare/$v1...$v2", s"https://gitlab.on-prem.net/foo/bar/compare/release-$v1...release-$v2" ) - assertEquals(obtained, expected) + assertIO(obtained, expected) } test("possibleUpdateInfoUrls: bitbucket.org") { - val obtained = possibleUpdateInfoUrls( + val obtained = possibleUpdateInfoUrls(urlChecker)( bitbucketFooBarRepo, versionUpdate - ).map(_.url.renderString) + ).map(_.map(_.url.renderString)).runA(state) val expected = possibleReleaseNotesFilenames.map(name => s"https://bitbucket.org/foo/bar/src/master/$name" @@ -272,14 +297,16 @@ class UpdateInfoUrlFinderTest extends CatsEffectSuite with Http4sDsl[MockEff] { s"https://bitbucket.org/foo/bar/compare/$v2..$v1#diff", s"https://bitbucket.org/foo/bar/compare/release-$v2..release-$v1#diff" ) - assertEquals(obtained, expected) + assertIO(obtained, expected) } test("possibleUpdateInfoUrls: on-prem Bitbucket Server") { val repoUrl = uri"https://bitbucket-server.on-prem.com" / "foo" / "bar" val obtained = - possibleUpdateInfoUrls(ForgeRepo(BitbucketServer, repoUrl), versionUpdate).map(_.url) + possibleUpdateInfoUrls(urlChecker)(ForgeRepo(BitbucketServer, repoUrl), versionUpdate) + .map(_.map(_.url)) + .runA(state) val expected = repoUrl / "browse" / "ReleaseNotes.md" - assert(clue(obtained).contains(expected)) + assertIOBoolean(obtained.map(_.contains(expected))) } } From 56b8ad9405fa3ca596213bc0ebd46ecbc2a4de0a Mon Sep 17 00:00:00 2001 From: danicheg Date: Sun, 2 Mar 2025 12:26:17 +0300 Subject: [PATCH 7/7] Perform several recursive lookups in UrlChecker --- .../scalasteward/core/util/UrlChecker.scala | 58 ++++++++++--------- .../nurture/UpdateInfoUrlFinderTest.scala | 4 +- .../core/util/UrlCheckerTest.scala | 43 +++++++++----- 3 files changed, 61 insertions(+), 44 deletions(-) diff --git a/modules/core/src/main/scala/org/scalasteward/core/util/UrlChecker.scala b/modules/core/src/main/scala/org/scalasteward/core/util/UrlChecker.scala index 96b1ec35ca..06edd1a713 100644 --- a/modules/core/src/main/scala/org/scalasteward/core/util/UrlChecker.scala +++ b/modules/core/src/main/scala/org/scalasteward/core/util/UrlChecker.scala @@ -76,40 +76,44 @@ object UrlChecker { buildCache(config).map { statusCache => new UrlChecker[F] { override def validate(url: Uri): F[UrlValidationResult] = - check(url, recursion = true).handleErrorWith(th => + check(url).handleErrorWith(th => logger .debug(th)(s"Failed to check if $url exists") .as(UrlValidationResult.NotExists) ) - private def check(url: Uri, recursion: Boolean): F[UrlValidationResult] = + /** While checking for the [[Uri]]s presence, we perform up to 3 recursive lookups when + * receiving a `MovedPermanently` response. + */ + private def check(url: Uri): F[UrlValidationResult] = { + def lookup(url: Uri, maxDepth: Int): F[Option[Uri]] = + Option(maxDepth).filter(_ > 0).flatTraverse { depth => + val req = Request[F](method = Method.HEAD, uri = url) + + modify(req).flatMap(r => + urlCheckerClient.client.run(r).use { + case resp if resp.status === Status.Ok => + F.pure(url.some) + + case resp if resp.status === Status.MovedPermanently => + resp.headers + .get[Location] + .flatTraverse(location => lookup(location.uri, depth - 1)) + + case _ => + F.pure(none) + } + ) + } + statusCache.cachingF(url.renderString)(None) { - val req = Request[F](method = Method.HEAD, uri = url) - - modify(req).flatMap(r => - urlCheckerClient.client.run(r).use { - case resp if resp.status === Status.Ok => - F.pure(UrlValidationResult.Exists) - - case resp if recursion && resp.status === Status.MovedPermanently => - resp.headers - .get[Location] - .traverse(h => - check(h.uri, recursion = false).map( - _.fold( - exists = UrlValidationResult.RedirectTo(h.uri), - notExists = UrlValidationResult.NotExists, - redirectTo = _ => UrlValidationResult.NotExists - ) - ) - ) - .map(_.getOrElse(UrlValidationResult.NotExists)) - - case _ => - F.pure(UrlValidationResult.NotExists) - } - ) + lookup(url, maxDepth = 3).map { + case Some(u) if u === url => UrlValidationResult.Exists + case Some(u) => UrlValidationResult.RedirectTo(u) + case None => UrlValidationResult.NotExists + } } + } } } } diff --git a/modules/core/src/test/scala/org/scalasteward/core/nurture/UpdateInfoUrlFinderTest.scala b/modules/core/src/test/scala/org/scalasteward/core/nurture/UpdateInfoUrlFinderTest.scala index ea88479160..e78217ed4a 100644 --- a/modules/core/src/test/scala/org/scalasteward/core/nurture/UpdateInfoUrlFinderTest.scala +++ b/modules/core/src/test/scala/org/scalasteward/core/nurture/UpdateInfoUrlFinderTest.scala @@ -29,14 +29,14 @@ class UpdateInfoUrlFinderTest extends CatsEffectSuite with Http4sDsl[MockEff] { status = Status.MovedPermanently, headers = Headers(Location(redirectUrl)) ).pure[MockEff] - case HEAD -> Root / "foo" / "bar3-redirected" => Ok() + case HEAD -> Root / "foo" / "bar3-redirected" => Ok() case HEAD -> Root / "foo" / "bar" / "README.md" => Ok() case HEAD -> Root / "foo" / "bar" / "compare" / "v0.1.0...v0.2.0" => Ok() case HEAD -> Root / "foo" / "bar1" / "blob" / "master" / "RELEASES.md" => Ok() case HEAD -> Root / "foo" / "buz" / "compare" / "v0.1.0...v0.2.0" => PermanentRedirect() case HEAD -> Root / "foo" / "bar2" / "releases" / "tag" / "v0.2.0" => Ok() case HEAD -> Root / "foo" / "bar3-redirected" / "releases" / "tag" / "v0.2.0" => Ok() - case _ => NotFound() + case _ => NotFound() } private val authApp = GitHubAuth.api(List(Repository("foo/bar"))) private val state = MockState.empty.copy(clientResponses = authApp <+> httpApp) diff --git a/modules/core/src/test/scala/org/scalasteward/core/util/UrlCheckerTest.scala b/modules/core/src/test/scala/org/scalasteward/core/util/UrlCheckerTest.scala index 4ea6b194b7..3d0d673adb 100644 --- a/modules/core/src/test/scala/org/scalasteward/core/util/UrlCheckerTest.scala +++ b/modules/core/src/test/scala/org/scalasteward/core/util/UrlCheckerTest.scala @@ -2,7 +2,7 @@ package org.scalasteward.core.util import cats.syntax.all.* import munit.CatsEffectSuite -import org.http4s.{Headers, HttpApp, Response, Status} +import org.http4s.{Headers, HttpApp, Response, Status, Uri} import org.http4s.dsl.Http4sDsl import org.http4s.headers.Location import org.http4s.syntax.all.* @@ -13,24 +13,29 @@ import org.scalasteward.core.util.UrlChecker.UrlValidationResult class UrlCheckerTest extends CatsEffectSuite with Http4sDsl[MockEff] { private val baseUrl = uri"https://github.com/scala-steward-org" - private val redirectUrl = baseUrl / "finished-redirect" - private val anotherRedirectUrl = baseUrl / "yet-another-redirect" + private val finishedRedirectUrl = baseUrl / "finished-redirect" + private val infiniteRedirectUrl = baseUrl / "infinite-redirect" + private val secondRedirectUrl = baseUrl / "second-redirect" + + private def movedPermanentlyResponse(uri: Uri): MockEff[Response[MockEff]] = + Response[MockEff]( + status = Status.MovedPermanently, + headers = Headers(Location(uri)) + ).pure[MockEff] private val state = MockState.empty.copy(clientResponses = GitHubAuth.api(List.empty) <+> HttpApp { case HEAD -> Root / "scala-steward-org" => Ok() case HEAD -> Root / "scala-steward-org" / "wrong-uri" => NotFound() case HEAD -> Root / "scala-steward-org" / "single-redirect" => - Response[MockEff]( - status = Status.MovedPermanently, - headers = Headers(Location(redirectUrl)) - ).pure[MockEff] + movedPermanentlyResponse(finishedRedirectUrl) + case HEAD -> Root / "scala-steward-org" / "first-redirect" => + movedPermanentlyResponse(secondRedirectUrl) + case HEAD -> Root / "scala-steward-org" / "second-redirect" => + movedPermanentlyResponse(finishedRedirectUrl) case HEAD -> Root / "scala-steward-org" / "finished-redirect" => Ok() - case HEAD -> Root / "scala-steward-org" / "yet-another-redirect" => - Response[MockEff]( - status = Status.MovedPermanently, - headers = Headers(Location(anotherRedirectUrl)) - ).pure[MockEff] + case HEAD -> Root / "scala-steward-org" / "infinite-redirect" => + movedPermanentlyResponse(infiniteRedirectUrl) case _ => ServiceUnavailable() }) @@ -52,14 +57,22 @@ class UrlCheckerTest extends CatsEffectSuite with Http4sDsl[MockEff] { test("An URL redirects to another existing URL") { val httpUrl = uri"https://github.com/scala-steward-org/single-redirect" val check = urlChecker.validate(httpUrl).runA(state) - val anticipatedResult = UrlValidationResult.RedirectTo(redirectUrl) + val anticipatedResult = UrlValidationResult.RedirectTo(finishedRedirectUrl) + + assertIO(check, anticipatedResult) + } + + test("An URL redirects to another redirecting URL (two redirects)") { + val httpUrl = uri"https://github.com/scala-steward-org/first-redirect" + val check = urlChecker.validate(httpUrl).runA(state) + val anticipatedResult = UrlValidationResult.RedirectTo(finishedRedirectUrl) assertIO(check, anticipatedResult) } // basically, we prohibit the infinite loop of traversing redirect URLs - test("An URL redirects to another redirecting URL") { - val httpUrl = uri"https://github.com/scala-steward-org/yet-another-redirect" + test("An URL redirects to another redirecting URL (more than two redirects)") { + val httpUrl = uri"https://github.com/scala-steward-org/infinite-redirect" val check = urlChecker.validate(httpUrl).runA(state) val anticipatedResult = UrlValidationResult.NotExists