From a00b1570d19d8dd2745cc45dbbd620c8be82cf41 Mon Sep 17 00:00:00 2001 From: Seetaramayya <3521036+Seetaramayya@users.noreply.github.com> Date: Mon, 20 Mar 2023 12:31:16 +0100 Subject: [PATCH 1/3] fixed retry and duplication replacements --- .../core/application/Context.scala | 2 +- .../core/client/ClientConfiguration.scala | 31 ++++++-- .../org/scalasteward/core/edit/EditAlg.scala | 2 +- .../core/edit/update/data/Substring.scala | 4 +- .../core/client/ClientConfigurationTest.scala | 74 +++++++++++-------- 5 files changed, 74 insertions(+), 39 deletions(-) diff --git a/modules/core/src/main/scala/org/scalasteward/core/application/Context.scala b/modules/core/src/main/scala/org/scalasteward/core/application/Context.scala index 53b654905b..19fc9a393f 100644 --- a/modules/core/src/main/scala/org/scalasteward/core/application/Context.scala +++ b/modules/core/src/main/scala/org/scalasteward/core/application/Context.scala @@ -115,7 +115,7 @@ object Context { userAgent <- Resource.eval(F.fromEither(`User-Agent`.parse(1)(userAgentString))) middleware = ClientConfiguration .setUserAgent[F](userAgent) - .andThen(ClientConfiguration.retryAfter[F](maxAttempts = 5)) + .andThen(ClientConfiguration.withRetry[F](maxAttempts = 5)) defaultClient <- ClientConfiguration.build( ClientConfiguration.BuilderMiddleware.default, middleware diff --git a/modules/core/src/main/scala/org/scalasteward/core/client/ClientConfiguration.scala b/modules/core/src/main/scala/org/scalasteward/core/client/ClientConfiguration.scala index 1e6f6ae0a7..b9a3ef8096 100644 --- a/modules/core/src/main/scala/org/scalasteward/core/client/ClientConfiguration.scala +++ b/modules/core/src/main/scala/org/scalasteward/core/client/ClientConfiguration.scala @@ -22,11 +22,12 @@ import eu.timepit.refined.auto._ import eu.timepit.refined.types.numeric.PosInt import java.net.http.HttpClient import java.net.http.HttpClient.Builder -import org.http4s.Response +import org.http4s.{Headers, Response} import org.http4s.client._ import org.http4s.headers.`User-Agent` import org.http4s.jdkhttpclient.JdkHttpClient import org.typelevel.ci._ +import java.time.Instant import scala.concurrent.duration._ object ClientConfiguration { @@ -65,20 +66,38 @@ object ClientConfiguration { private val RetryAfterStatuses = Set(403, 429, 503) + implicit class FunctionExtender[T, U](f: T => Option[U]) { + def orElse(g: T => Option[U])(t: T): Option[U] = f(t).orElse(g(t)) + } + + private val retryAfter: Headers => Option[Int] = + _.get(ci"Retry-After").flatMap(_.head.value.toIntOption) + + private val retryWaitMax60Seconds: Long => Int = { + case i if i > 0 && i < 60 => i.toInt + case i if i > 60 => 60 + case _ => 1 + } + + private val rateLimitReset: Headers => Option[Int] = _.get(ci"X-Ratelimit-Reset") + .flatMap(_.head.value.toLongOption) + .map(_ - Instant.now.getEpochSecond) + .map(retryWaitMax60Seconds) + + private val retryInterval: Headers => Option[Int] = retryAfter.orElse(rateLimitReset) + /** @param maxAttempts * max number times the HTTP request should be sent useful to avoid unexpected cloud provider * costs */ - def retryAfter[F[_]: Temporal](maxAttempts: PosInt = 5): Middleware[F] = { client => + def withRetry[F[_]: Temporal](maxAttempts: PosInt = 5): Middleware[F] = { client => Client[F] { req => def run(attempt: Int = 1): Resource[F, Response[F]] = client .run(req.putHeaders("X-Attempt" -> attempt.toString)) .flatMap { response => val maybeRetried = for { - header <- response.headers.get(ci"Retry-After") - seconds <- header.head.value.toIntOption - if seconds > 0 - duration = seconds.seconds + interval <- retryInterval(response.headers) + duration = interval.seconds if RetryAfterStatuses.contains(response.status.code) if attempt < maxAttempts.value } yield Resource diff --git a/modules/core/src/main/scala/org/scalasteward/core/edit/EditAlg.scala b/modules/core/src/main/scala/org/scalasteward/core/edit/EditAlg.scala index 01fd314686..bff7d86f9b 100644 --- a/modules/core/src/main/scala/org/scalasteward/core/edit/EditAlg.scala +++ b/modules/core/src/main/scala/org/scalasteward/core/edit/EditAlg.scala @@ -106,7 +106,7 @@ final class EditAlg[F[_]](implicit repoDir <- workspaceAlg.repoDir(data.repo) replacementsByPath = updateReplacements.groupBy(_.position.path).toList _ <- replacementsByPath.traverse { case (path, replacements) => - fileAlg.editFile(repoDir / path, Substring.Replacement.applyAll(replacements)) + fileAlg.editFile(repoDir / path, Substring.Replacement.applyAll(replacements.toSet)) } _ <- reformatChangedFiles(data) msgTemplate = data.config.commits.messageOrDefault diff --git a/modules/core/src/main/scala/org/scalasteward/core/edit/update/data/Substring.scala b/modules/core/src/main/scala/org/scalasteward/core/edit/update/data/Substring.scala index 526155b6be..50d181b8a5 100644 --- a/modules/core/src/main/scala/org/scalasteward/core/edit/update/data/Substring.scala +++ b/modules/core/src/main/scala/org/scalasteward/core/edit/update/data/Substring.scala @@ -41,10 +41,10 @@ object Substring { final case class Replacement(position: Position, replacement: String) object Replacement { - def applyAll(replacements: List[Replacement])(source: String): String = { + def applyAll(replacements: Set[Replacement])(source: String): String = { var start = 0 val sb = new java.lang.StringBuilder(source.length) - replacements.sortBy(_.position.start).foreach { r => + replacements.toSeq.sortBy(_.position.start).foreach { r => val before = source.substring(start, r.position.start) start = r.position.start + r.position.value.length sb.append(before).append(r.replacement) diff --git a/modules/core/src/test/scala/org/scalasteward/core/client/ClientConfigurationTest.scala b/modules/core/src/test/scala/org/scalasteward/core/client/ClientConfigurationTest.scala index 607f299699..cc9997f5e9 100644 --- a/modules/core/src/test/scala/org/scalasteward/core/client/ClientConfigurationTest.scala +++ b/modules/core/src/test/scala/org/scalasteward/core/client/ClientConfigurationTest.scala @@ -5,21 +5,28 @@ import cats.implicits._ import eu.timepit.refined.auto._ import eu.timepit.refined.types.numeric.PosInt import munit.CatsEffectSuite -import org.http4s.HttpRoutes +import org.http4s._ import org.http4s.client._ +import org.http4s.client.dsl.io._ +import org.http4s.dsl.io._ +import org.http4s.ember.server.EmberServerBuilder import org.http4s.headers.{`Retry-After`, `User-Agent`, Location} import org.http4s.implicits._ import org.typelevel.ci._ + +import java.time.Instant import scala.concurrent.duration._ +import scala.util.Try class ClientConfigurationTest extends CatsEffectSuite { + private val previousEpochSecond = Instant.now().minusSeconds(1).getEpochSecond + private val nextEpochSecond = Instant.now().plusSeconds(1).getEpochSecond private val userAgentValue = "my-user-agent" private val dummyUserAgent = `User-Agent`.parse(1)(userAgentValue).getOrElse(fail("unable to create user agent")) - private val routes: HttpRoutes[IO] = { - import org.http4s.dsl.io._ + private val routes: HttpRoutes[IO] = HttpRoutes.of[IO] { case req @ GET -> Root / "user-agent" => req.headers.get(ci"user-agent") match { @@ -45,15 +52,22 @@ class ClientConfigurationTest extends CatsEffectSuite { case Some(attempt) if attempt >= 2 => Ok() case _ => - Forbidden().map(_.putHeaders(`Retry-After`.fromLong(1))) + val resetHeader = + ParseResult.success(Header.Raw(ci"X-Ratelimit-Reset", s"$nextEpochSecond")) + Forbidden().map(_.putHeaders(`Retry-After`.fromLong(1), resetHeader)) + } + case req @ GET -> Root / "rate-limit-reset" / epochSecondsParam => + req.headers.get(ci"X-Attempt").flatMap(_.head.value.toIntOption) match { + case Some(attempt) if attempt >= 2 => Ok() + case _ => + val seconds = Try(epochSecondsParam.toLong).getOrElse(0L) + val resetHeader = + ParseResult.success(Header.Raw(ci"X-Ratelimit-Reset", seconds.toString)) + Forbidden().map(_.putHeaders(resetHeader)) } } - } test("setUserAgent add a specific user agent to requests") { - import org.http4s.Method._ - import org.http4s.client.dsl.io._ - val initialClient = Client.fromHttpApp[IO](routes.orNotFound) val setUserAgent = ClientConfiguration.setUserAgent[IO](dummyUserAgent) val newClient = setUserAgent(initialClient) @@ -71,10 +85,6 @@ class ClientConfigurationTest extends CatsEffectSuite { } test("disableFollowRedirect does not follow redirect") { - import org.http4s.Method._ - import org.http4s.ember.server._ - import org.http4s.client.dsl.io._ - val regularClient = ClientConfiguration.build[IO]( ClientConfiguration.BuilderMiddleware.default, ClientConfiguration.setUserAgent(dummyUserAgent) @@ -101,26 +111,32 @@ class ClientConfigurationTest extends CatsEffectSuite { test.assertEquals((400, 302)) } - test("retries on retry-after response header") { - import org.http4s.Method._ - import org.http4s.client.dsl.io._ + test("retries on retry-after response header even though 'X-Ratelimit-Reset' exists") { + val notEnoughRetries = request(uri"/retry-after", 1).assertEquals(403) + val exactlyEnoughRetries = request(uri"/retry-after", 2).assertEquals(200) + notEnoughRetries.flatMap(_ => exactlyEnoughRetries) + } - def clientWithMaxAttempts(maxAttempts: PosInt): Client[IO] = { - val initialClient = Client.fromHttpApp[IO](routes.orNotFound) - val retryAfter = ClientConfiguration.retryAfter[IO](maxAttempts) - retryAfter(initialClient) - } + test("retries with the value mentioned in 'X-Ratelimit-Reset' in the absense of 'Retry-After'") { + val uri = Uri.unsafeFromString(s"/rate-limit-reset/$nextEpochSecond") + val notEnoughRetries = request(uri, 1).assertEquals(403) + val exactlyEnoughRetries = request(uri, 2).assertEquals(200) + notEnoughRetries.flatMap(_ => exactlyEnoughRetries) + } - val notEnoughRetries = clientWithMaxAttempts(1) - .run(GET(uri"/retry-after")) - .use(r => r.status.code.pure[IO]) - .assertEquals(403) + test("retries after 1 second when the given value in 'X-Ratelimit-Reset' elapsed") { + val uri: Uri = Uri.unsafeFromString(s"/rate-limit-reset/$previousEpochSecond") + val notEnoughRetries = request(uri, 1).assertEquals(403) + val exactlyEnoughRetries = request(uri, 2).assertEquals(200) + notEnoughRetries.flatMap(_ => exactlyEnoughRetries) + } - val exactlyEnoughRetries = clientWithMaxAttempts(2) - .run(GET(uri"/retry-after")) - .use(r => r.status.code.pure[IO]) - .assertEquals(200) + private def request(uri: Uri, attempts: PosInt): IO[Int] = + clientWithMaxAttempts(attempts).run(GET(uri)).use(r => r.status.code.pure[IO]) - notEnoughRetries.flatMap(_ => exactlyEnoughRetries) + private def clientWithMaxAttempts(maxAttempts: PosInt): Client[IO] = { + val initialClient = Client.fromHttpApp[IO](routes.orNotFound) + val withRetry = ClientConfiguration.withRetry[IO](maxAttempts) + withRetry(initialClient) } } From 7277c4fac9f38ad820ed2580b7b1f58aa50970df Mon Sep 17 00:00:00 2001 From: Seetaramayya <3521036+Seetaramayya@users.noreply.github.com> Date: Mon, 20 Mar 2023 17:52:31 +0100 Subject: [PATCH 2/3] Added test case to prove duplicated updates are causing StringIndexOutOfBoundsException --- .../scalasteward/core/edit/EditAlgTest.scala | 71 +++++++++++++++++++ 1 file changed, 71 insertions(+) diff --git a/modules/core/src/test/scala/org/scalasteward/core/edit/EditAlgTest.scala b/modules/core/src/test/scala/org/scalasteward/core/edit/EditAlgTest.scala index 8e68975b46..677f8d3b37 100644 --- a/modules/core/src/test/scala/org/scalasteward/core/edit/EditAlgTest.scala +++ b/modules/core/src/test/scala/org/scalasteward/core/edit/EditAlgTest.scala @@ -1,6 +1,7 @@ package org.scalasteward.core.edit import better.files.File +import cats.data.NonEmptyList import cats.effect.unsafe.implicits.global import munit.FunSuite import org.scalasteward.core.TestInstances.dummyRepoCache @@ -160,4 +161,74 @@ class EditAlgTest extends FunSuite { case Log(_) => false }) } + + test("applyUpdate should be successful with duplicated updates as well") { + val repo = Repo("edit-alg", "test-1") + val data = RepoData(repo, dummyRepoCache, RepoConfig.empty) + val repoDir = workspaceAlg.repoDir(repo).unsafeRunSync() + val artifactId = Update.ForArtifactId( + CrossDependency("com.pauldijou".g % "jwt-play-json".a % "5.0.0"), + newerVersions = NonEmptyList.of(Version("9.2.0")), + newerGroupId = Some(GroupId("com.github.jwt-scala")), + newerArtifactId = Some("jwt-play-json") + ) + val duplicatedUpdates = Update.ForGroupId(NonEmptyList.of(artifactId, artifactId)) + + val buildSbtFile = repoDir / "build.sbt" + val dependenciesScalaFile = repoDir / "project/Dependencies.scala" + val gitignore = repoDir / ".gitignore" + val buildSbtContent = + """ + | lazy val root = (project in file(".")) + | .settings( + | scalafmtOnCompile := true, + | scalaVersion := scala213, + | libraryDependencies ++= Seq( + | "com.pauldijou" %% "jwt-play-json" % "5.0.0", // JWT parsing + | "org.scalatestplus" %% "mockito-3-4" % "3.2.10.0" % Test + | ), + | crossScalaVersions := supportedScalaVersions + | ) + |""".stripMargin + + val state = MockState.empty + .addFiles(buildSbtFile -> buildSbtContent, gitignore -> "", dependenciesScalaFile -> "") + .flatMap(editAlg.applyUpdate(data, duplicatedUpdates).runS) + .unsafeRunSync() + + val expectedSbtContent = buildSbtContent + .replaceAll("com.pauldijou", "com.github.jwt-scala") + .replaceAll("5.0.0", "9.2.0") + + val expected = MockState.empty.copy( + trace = Vector( + Cmd("read", gitignore.pathAsString), + Cmd("test", "-f", repoDir.pathAsString), + Cmd("test", "-f", gitignore.pathAsString), + Cmd("test", "-f", buildSbtFile.pathAsString), + Cmd("read", buildSbtFile.pathAsString), + Cmd("test", "-f", (repoDir / "project").pathAsString), + Cmd("test", "-f", dependenciesScalaFile.pathAsString), + Cmd("read", dependenciesScalaFile.pathAsString), + Cmd("read", gitignore.pathAsString), + Cmd("test", "-f", repoDir.pathAsString), + Cmd("test", "-f", gitignore.pathAsString), + Cmd("test", "-f", buildSbtFile.pathAsString), + Cmd("read", buildSbtFile.pathAsString), + Cmd("test", "-f", (repoDir / "project").pathAsString), + Cmd("test", "-f", dependenciesScalaFile.pathAsString), + Cmd("read", dependenciesScalaFile.pathAsString), + Cmd("read", buildSbtFile.pathAsString), + Cmd("write", buildSbtFile.pathAsString), + Cmd(gitStatus(repoDir)) + ), + files = Map( + buildSbtFile -> expectedSbtContent, + dependenciesScalaFile -> "", + gitignore -> "" + ) + ) + + assertEquals(state, expected) + } } From 9ce0db9f3bb86b41deda7f8006203306d21a99ca Mon Sep 17 00:00:00 2001 From: Seetaramayya <3521036+Seetaramayya@users.noreply.github.com> Date: Mon, 20 Mar 2023 20:03:39 +0100 Subject: [PATCH 3/3] Removed fix for StringIndexOutOfBoundsException (it will be created in another PR) --- .../org/scalasteward/core/edit/EditAlg.scala | 2 +- .../core/edit/update/data/Substring.scala | 4 +- .../scalasteward/core/edit/EditAlgTest.scala | 71 ------------------- 3 files changed, 3 insertions(+), 74 deletions(-) diff --git a/modules/core/src/main/scala/org/scalasteward/core/edit/EditAlg.scala b/modules/core/src/main/scala/org/scalasteward/core/edit/EditAlg.scala index bff7d86f9b..01fd314686 100644 --- a/modules/core/src/main/scala/org/scalasteward/core/edit/EditAlg.scala +++ b/modules/core/src/main/scala/org/scalasteward/core/edit/EditAlg.scala @@ -106,7 +106,7 @@ final class EditAlg[F[_]](implicit repoDir <- workspaceAlg.repoDir(data.repo) replacementsByPath = updateReplacements.groupBy(_.position.path).toList _ <- replacementsByPath.traverse { case (path, replacements) => - fileAlg.editFile(repoDir / path, Substring.Replacement.applyAll(replacements.toSet)) + fileAlg.editFile(repoDir / path, Substring.Replacement.applyAll(replacements)) } _ <- reformatChangedFiles(data) msgTemplate = data.config.commits.messageOrDefault diff --git a/modules/core/src/main/scala/org/scalasteward/core/edit/update/data/Substring.scala b/modules/core/src/main/scala/org/scalasteward/core/edit/update/data/Substring.scala index 50d181b8a5..526155b6be 100644 --- a/modules/core/src/main/scala/org/scalasteward/core/edit/update/data/Substring.scala +++ b/modules/core/src/main/scala/org/scalasteward/core/edit/update/data/Substring.scala @@ -41,10 +41,10 @@ object Substring { final case class Replacement(position: Position, replacement: String) object Replacement { - def applyAll(replacements: Set[Replacement])(source: String): String = { + def applyAll(replacements: List[Replacement])(source: String): String = { var start = 0 val sb = new java.lang.StringBuilder(source.length) - replacements.toSeq.sortBy(_.position.start).foreach { r => + replacements.sortBy(_.position.start).foreach { r => val before = source.substring(start, r.position.start) start = r.position.start + r.position.value.length sb.append(before).append(r.replacement) diff --git a/modules/core/src/test/scala/org/scalasteward/core/edit/EditAlgTest.scala b/modules/core/src/test/scala/org/scalasteward/core/edit/EditAlgTest.scala index 677f8d3b37..8e68975b46 100644 --- a/modules/core/src/test/scala/org/scalasteward/core/edit/EditAlgTest.scala +++ b/modules/core/src/test/scala/org/scalasteward/core/edit/EditAlgTest.scala @@ -1,7 +1,6 @@ package org.scalasteward.core.edit import better.files.File -import cats.data.NonEmptyList import cats.effect.unsafe.implicits.global import munit.FunSuite import org.scalasteward.core.TestInstances.dummyRepoCache @@ -161,74 +160,4 @@ class EditAlgTest extends FunSuite { case Log(_) => false }) } - - test("applyUpdate should be successful with duplicated updates as well") { - val repo = Repo("edit-alg", "test-1") - val data = RepoData(repo, dummyRepoCache, RepoConfig.empty) - val repoDir = workspaceAlg.repoDir(repo).unsafeRunSync() - val artifactId = Update.ForArtifactId( - CrossDependency("com.pauldijou".g % "jwt-play-json".a % "5.0.0"), - newerVersions = NonEmptyList.of(Version("9.2.0")), - newerGroupId = Some(GroupId("com.github.jwt-scala")), - newerArtifactId = Some("jwt-play-json") - ) - val duplicatedUpdates = Update.ForGroupId(NonEmptyList.of(artifactId, artifactId)) - - val buildSbtFile = repoDir / "build.sbt" - val dependenciesScalaFile = repoDir / "project/Dependencies.scala" - val gitignore = repoDir / ".gitignore" - val buildSbtContent = - """ - | lazy val root = (project in file(".")) - | .settings( - | scalafmtOnCompile := true, - | scalaVersion := scala213, - | libraryDependencies ++= Seq( - | "com.pauldijou" %% "jwt-play-json" % "5.0.0", // JWT parsing - | "org.scalatestplus" %% "mockito-3-4" % "3.2.10.0" % Test - | ), - | crossScalaVersions := supportedScalaVersions - | ) - |""".stripMargin - - val state = MockState.empty - .addFiles(buildSbtFile -> buildSbtContent, gitignore -> "", dependenciesScalaFile -> "") - .flatMap(editAlg.applyUpdate(data, duplicatedUpdates).runS) - .unsafeRunSync() - - val expectedSbtContent = buildSbtContent - .replaceAll("com.pauldijou", "com.github.jwt-scala") - .replaceAll("5.0.0", "9.2.0") - - val expected = MockState.empty.copy( - trace = Vector( - Cmd("read", gitignore.pathAsString), - Cmd("test", "-f", repoDir.pathAsString), - Cmd("test", "-f", gitignore.pathAsString), - Cmd("test", "-f", buildSbtFile.pathAsString), - Cmd("read", buildSbtFile.pathAsString), - Cmd("test", "-f", (repoDir / "project").pathAsString), - Cmd("test", "-f", dependenciesScalaFile.pathAsString), - Cmd("read", dependenciesScalaFile.pathAsString), - Cmd("read", gitignore.pathAsString), - Cmd("test", "-f", repoDir.pathAsString), - Cmd("test", "-f", gitignore.pathAsString), - Cmd("test", "-f", buildSbtFile.pathAsString), - Cmd("read", buildSbtFile.pathAsString), - Cmd("test", "-f", (repoDir / "project").pathAsString), - Cmd("test", "-f", dependenciesScalaFile.pathAsString), - Cmd("read", dependenciesScalaFile.pathAsString), - Cmd("read", buildSbtFile.pathAsString), - Cmd("write", buildSbtFile.pathAsString), - Cmd(gitStatus(repoDir)) - ), - files = Map( - buildSbtFile -> expectedSbtContent, - dependenciesScalaFile -> "", - gitignore -> "" - ) - ) - - assertEquals(state, expected) - } }