Skip to content

Commit

Permalink
Address review feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
@benjben
benjben committed Nov 19, 2024
1 parent 2401af9 commit 2b530b6
Show file tree
Hide file tree
Showing 4 changed files with 97 additions and 56 deletions.
9 changes: 7 additions & 2 deletions modules/core/src/main/scala/com.snowplowanalytics.iglu/client/resolver/Resolver.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -190,14 +190,19 @@ final case class Resolver[F[_]](repos: List[Registry], cache: Option[ResolverCac
/**
* If Iglu Central or any of its mirrors doesn't have a schema,
* it should be considered NotFound, even if one of them returned an error.
* All 4xx (`ClientFailure`) are also considered NotFound.
*/
private[resolver] def isNotFound(error: ResolutionError): Boolean = {
val (igluCentral, custom) = error.value.partition { case (repo, _) =>
allIgluCentral.contains(repo)
}
(igluCentral.isEmpty || igluCentral.values.exists(
_.errors.exists(_ == RegistryError.NotFound)
)) && custom.values.flatMap(_.errors).forall(_ == RegistryError.NotFound)
_.errors.exists(e =>
e == RegistryError.NotFound || e.isInstanceOf[RegistryError.ClientFailure]
)
)) && custom.values
.flatMap(_.errors)
.forall(e => e == RegistryError.NotFound || e.isInstanceOf[RegistryError.ClientFailure])
}

/**
Expand Down
98 changes: 84 additions & 14 deletions modules/core/src/test/scala/com.snowplowanalytics.iglu.client/resolver/ResolverSpec.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,13 +90,16 @@ class ResolverSpec extends Specification with CatsEffect {
result from SchemaListLike should contain the exact schemaKey provided $e13
isNotFound should
return true if custom repo and Iglu Central repos don't have a schema $e14
return true if one Iglu Central repo returns an error and the other one NotFound $e15
return false if custom repo returns an error $e16
return true if there is no custom repo, one Iglu Central repo returns an error and the other one NotFound $e17
return false if there is no custom repo and Iglu Central ones return an error $e18
return true if there is just one custom repo that returns NotFound $e19
return false if there is just one custom repo that returns an error $e20
return true if one Iglu Central repo returns 2 errors and the other one returns one error and one NotFound $e21
return true if one Iglu Central repo returns a RepoFailure and the other one NotFound $e15
return false if custom repo returns a RepoFailure $e16
return true if custom repo returns a ClientFailure $e17
return true if there is no custom repo, one Iglu Central repo returns an error and the other one NotFound $e18
return false if there is no custom repo and Iglu Central ones return a RepoFailure $e19
return true if there is no custom repo and Iglu Central ones return a ClientFailure $e20
return true if there is just one custom repo that returns NotFound $e21
return false if there is just one custom repo that returns a RepoFailure $e22
return true if there is just one custom repo that returns a ClientFailure $e23
return true if one Iglu Central repo returns 2 errors and the other one returns one error and one NotFound $e24
"""

import ResolverSpec._
Expand Down Expand Up @@ -556,7 +559,7 @@ class ResolverSpec extends Specification with CatsEffect {
Instant.now()
),
Repos.custom.config.name -> LookupHistory(
Set(RegistryError.ClientFailure("Something went wrong")),
Set(RegistryError.RepoFailure("Something went wrong")),
1,
Instant.now()
)
Expand All @@ -567,6 +570,34 @@ class ResolverSpec extends Specification with CatsEffect {
}

def e17 = {
val resolver: Resolver[Id] =
Resolver
.init[Id](0, None, SpecHelpers.IgluCentral, SpecHelpers.IgluCentralMirror, Repos.custom)

val resolutionError = ResolutionError(
SortedMap(
SpecHelpers.IgluCentral.config.name -> LookupHistory(
Set(RegistryError.NotFound),
1,
Instant.now()
),
SpecHelpers.IgluCentralMirror.config.name -> LookupHistory(
Set(RegistryError.NotFound),
1,
Instant.now()
),
Repos.custom.config.name -> LookupHistory(
Set(RegistryError.ClientFailure("402")),
1,
Instant.now()
)
)
)

resolver.isNotFound(resolutionError) should beTrue
}

def e18 = {
val resolver: Resolver[Id] =
Resolver.init[Id](0, None, SpecHelpers.IgluCentral, SpecHelpers.IgluCentralMirror)

Expand All @@ -588,7 +619,7 @@ class ResolverSpec extends Specification with CatsEffect {
resolver.isNotFound(resolutionError) should beTrue
}

def e18 = {
def e19 = {
val resolver: Resolver[Id] =
Resolver.init[Id](0, None, SpecHelpers.IgluCentral, SpecHelpers.IgluCentralMirror)

Expand All @@ -600,7 +631,7 @@ class ResolverSpec extends Specification with CatsEffect {
Instant.now()
),
SpecHelpers.IgluCentralMirror.config.name -> LookupHistory(
Set(RegistryError.ClientFailure("Network issue")),
Set(RegistryError.RepoFailure("Network issue")),
1,
Instant.now()
)
Expand All @@ -610,7 +641,29 @@ class ResolverSpec extends Specification with CatsEffect {
resolver.isNotFound(resolutionError) should beFalse
}

def e19 = {
def e20 = {
val resolver: Resolver[Id] =
Resolver.init[Id](0, None, SpecHelpers.IgluCentral, SpecHelpers.IgluCentralMirror)

val resolutionError = ResolutionError(
SortedMap(
SpecHelpers.IgluCentral.config.name -> LookupHistory(
Set(RegistryError.RepoFailure("Problem")),
1,
Instant.now()
),
SpecHelpers.IgluCentralMirror.config.name -> LookupHistory(
Set(RegistryError.ClientFailure("403 Forbidden")),
1,
Instant.now()
)
)
)

resolver.isNotFound(resolutionError) should beTrue
}

def e21 = {
val resolver: Resolver[Id] =
Resolver.init[Id](0, None, Repos.custom)

Expand All @@ -623,14 +676,14 @@ class ResolverSpec extends Specification with CatsEffect {
resolver.isNotFound(resolutionError) should beTrue
}

def e20 = {
def e22 = {
val resolver: Resolver[Id] =
Resolver.init[Id](0, None, Repos.custom)

val resolutionError = ResolutionError(
SortedMap(
Repos.custom.config.name -> LookupHistory(
Set(RegistryError.ClientFailure("Boom")),
Set(RegistryError.RepoFailure("Boom")),
1,
Instant.now()
)
Expand All @@ -640,7 +693,24 @@ class ResolverSpec extends Specification with CatsEffect {
resolver.isNotFound(resolutionError) should beFalse
}

def e21 = {
def e23 = {
val resolver: Resolver[Id] =
Resolver.init[Id](0, None, Repos.custom)

val resolutionError = ResolutionError(
SortedMap(
Repos.custom.config.name -> LookupHistory(
Set(RegistryError.ClientFailure("401")),
1,
Instant.now()
)
)
)

resolver.isNotFound(resolutionError) should beTrue
}

def e24 = {
val resolver: Resolver[Id] =
Resolver
.init[Id](0, None, SpecHelpers.IgluCentral, SpecHelpers.IgluCentralMirror, Repos.custom)
Expand Down
7 changes: 6 additions & 1 deletion ...in/scala/com.snowplowanalytics.iglu/client/resolver/registries/Http4sRegistryLookup.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -126,13 +126,18 @@ object Http4sRegistryLookup {
.handleError { e =>
RegistryError.ClientFailure(s"Could not decode server response. $e").asLeft[A]
}
case Status.ClientError(_) =>
case Status.ClientError(response) if response.status.code == 404 =>
(RegistryError.NotFound: RegistryError).asLeft[A].pure[F]
case Status.ServerError(response) =>
response.bodyText.compile.string.map { body =>
val error = s"Unexpected server response: $body"
RegistryError.RepoFailure(error).asLeft
}
case Status.ClientError(response) =>
response.bodyText.compile.string.map { body =>
val error = s"Unexpected server response: $body"
RegistryError.ClientFailure(error).asLeft
}
case response =>
response.bodyText.compile.string.map { body =>
val error = s"Unexpected response: $body"
Expand Down
39 changes: 0 additions & 39 deletions ...cala/com.snowplowanalytics.iglu/client/resolver/registries/Http4sRegistryLookupSpec.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import io.circe.Json
import org.http4s.client.{Client => HttpClient}
import org.http4s.dsl.Http4sDsl
import org.http4s.implicits._
import org.http4s.Response
import org.specs2.mutable.Specification

import java.net.URI
Expand Down Expand Up @@ -85,44 +84,6 @@ class Http4sRegistryLookupSpec extends Specification with CatsEffect {
result should beLeft
}
}

"return NotFound in case of 403 Forbidden" in {

val repositoryRef =
Registry.Http(
Registry.Config("name", 1, Nil),
Registry.HttpConnection(URI.create("http://custom-iglu.com"), None)
)
val schemaKey = SchemaKey("com.myvendor", "myname", "jsonschema", SchemaVer.Full(42, 42, 42))

val client = HttpClient[IO] { _ =>
val dsl = new Http4sDsl[IO] {}; import dsl._
Resource.pure(Response[IO](Forbidden))
}

Http4sRegistryLookup(client).lookup(repositoryRef, schemaKey).map { result =>
result should beEqualTo(Left(RegistryError.NotFound))
}
}

"not return NotFound in case of 500 Internal Server Error" in {

val repositoryRef =
Registry.Http(
Registry.Config("name", 1, Nil),
Registry.HttpConnection(URI.create("http://custom-iglu.com"), None)
)
val schemaKey = SchemaKey("com.myvendor", "myname", "jsonschema", SchemaVer.Full(42, 42, 42))

val client = HttpClient[IO] { _ =>
val dsl = new Http4sDsl[IO] {}; import dsl._
Resource.pure(Response[IO](InternalServerError))
}

Http4sRegistryLookup(client).lookup(repositoryRef, schemaKey).map { result =>
result shouldNotEqual (Left(RegistryError.NotFound))
}
}
}

"The Http4sRegistryLookup list" should {
Expand Down

0 comments on commit 2b530b6

Please sign in to comment.