From c25539b628d477f340f34502b1389c1011bd80ee Mon Sep 17 00:00:00 2001 From: Zach Kirlew Date: Tue, 21 Jul 2020 16:56:57 +0200 Subject: [PATCH 01/12] Add check if a user is a repository collaborator --- .../github4s/algebras/Repositories.scala | 20 +++++++++++ .../main/scala/github4s/http/HttpClient.scala | 15 ++++++-- .../RepositoriesInterpreter.scala | 12 +++++++ .../github4s/integration/ReposSpec.scala | 34 +++++++++++++++++++ .../test/scala/github4s/unit/ReposSpec.scala | 19 +++++++++++ .../test/scala/github4s/utils/BaseSpec.scala | 15 ++++++-- microsite/docs/repository.md | 21 ++++++++++++ 7 files changed, 131 insertions(+), 5 deletions(-) diff --git a/github4s/src/main/scala/github4s/algebras/Repositories.scala b/github4s/src/main/scala/github4s/algebras/Repositories.scala index a8b6998f1..b5dde9f1f 100644 --- a/github4s/src/main/scala/github4s/algebras/Repositories.scala +++ b/github4s/src/main/scala/github4s/algebras/Repositories.scala @@ -271,6 +271,26 @@ trait Repositories[F[_]] { headers: Map[String, String] = Map() ): F[GHResponse[List[User]]] + /** + * Get whether a user is a repository collaborator + * + * For organization-owned repositories, the list of collaborators includes outside collaborators, + * organization members that are direct collaborators, organization members with access through team memberships, + * organization members with access through default organization permissions, and organization owners. + * + * @param owner of the repo + * @param repo name of the repo + * @param username Github username + * @param headers optional user headers to include in the request + * @return a unit GHResponse + */ + def userIsCollaborator( + owner: String, + repo: String, + username: String, + headers: Map[String, String] = Map() + ): F[GHResponse[Unit]] + /** * Get the repository permission of a collaborator * diff --git a/github4s/src/main/scala/github4s/http/HttpClient.scala b/github4s/src/main/scala/github4s/http/HttpClient.scala index 8a88928d3..c96a64d47 100644 --- a/github4s/src/main/scala/github4s/http/HttpClient.scala +++ b/github4s/src/main/scala/github4s/http/HttpClient.scala @@ -21,15 +21,15 @@ import cats.effect.Sync import cats.instances.string._ import cats.syntax.either._ import cats.syntax.functor._ -import github4s._ import github4s.GHError._ +import github4s._ import github4s.domain.Pagination import github4s.http.Http4sSyntax._ import io.circe.{Decoder, Encoder} -import org.http4s.{EntityDecoder, Request, Response, Status} -import org.http4s.client.Client import org.http4s.circe.CirceEntityDecoder._ import org.http4s.circe.jsonOf +import org.http4s.client.Client +import org.http4s.{EntityDecoder, Request, Response, Status} class HttpClient[F[_]: Sync](client: Client[F], val config: GithubConfig) { import HttpClient._ @@ -52,6 +52,15 @@ class HttpClient[F[_]: Sync](client: Client[F], val config: GithubConfig) { ) ) + def getWithoutResponse( + accessToken: Option[String] = None, + url: String, + headers: Map[String, String] = Map.empty + ): F[GHResponse[Unit]] = + run[Unit, Unit]( + RequestBuilder(buildURL(url)).deleteMethod.withHeaders(headers).withAuth(accessToken) + ) + def patch[Req: Encoder, Res: Decoder]( accessToken: Option[String] = None, method: String, diff --git a/github4s/src/main/scala/github4s/interpreters/RepositoriesInterpreter.scala b/github4s/src/main/scala/github4s/interpreters/RepositoriesInterpreter.scala index 563fbd033..cd069a5d7 100644 --- a/github4s/src/main/scala/github4s/interpreters/RepositoriesInterpreter.scala +++ b/github4s/src/main/scala/github4s/interpreters/RepositoriesInterpreter.scala @@ -217,6 +217,18 @@ class RepositoriesInterpreter[F[_]](implicit client: HttpClient[F], accessToken: pagination ) + override def userIsCollaborator( + owner: String, + repo: String, + username: String, + headers: Map[String, String] + ): F[GHResponse[Unit]] = + client.getWithoutResponse( + accessToken, + s"repos/$owner/$repo/collaborators/$username", + headers + ) + override def getRepoPermissionForUser( owner: String, repo: String, diff --git a/github4s/src/test/scala/github4s/integration/ReposSpec.scala b/github4s/src/test/scala/github4s/integration/ReposSpec.scala index b935af9f3..22cff6ba0 100644 --- a/github4s/src/test/scala/github4s/integration/ReposSpec.scala +++ b/github4s/src/test/scala/github4s/integration/ReposSpec.scala @@ -313,6 +313,40 @@ trait ReposSpec extends BaseIntegrationSpec { response.statusCode shouldBe notFoundStatusCode } + "Repos >> UserIsCollaborator" should "return an empty body with no content status code" taggedAs Integration in { + val response = clientResource + .use { client => + Github[IO](client, accessToken).repos + .userIsCollaborator( + validRepoOwner, + validRepoName, + validUsername, + headers = headerUserAgent + ) + } + .unsafeRunSync() + + testIsRight[Unit](response, r => r should be(())) + response.statusCode shouldBe noContentStatusCode + } + + it should "return a not found error when username is not a repo collaborator" taggedAs Integration in { + val response = clientResource + .use { client => + Github[IO](client, accessToken).repos + .userIsCollaborator( + validRepoName, + validRepoName, + invalidUsername, + headers = headerUserAgent + ) + } + .unsafeRunSync() + + testIsLeft[NotFoundError, Unit](response) + response.statusCode shouldBe notFoundStatusCode + } + "Repos >> GetRepoPermissionForUser" should "return user repo permission" taggedAs Integration in { val response = clientResource .use { client => diff --git a/github4s/src/test/scala/github4s/unit/ReposSpec.scala b/github4s/src/test/scala/github4s/unit/ReposSpec.scala index 20ef19266..dc5a35b85 100644 --- a/github4s/src/test/scala/github4s/unit/ReposSpec.scala +++ b/github4s/src/test/scala/github4s/unit/ReposSpec.scala @@ -342,6 +342,25 @@ class ReposSpec extends BaseSpec { repos.listCollaborators(validRepoOwner, validRepoName, headers = headerUserAgent) } + "Repos.userIsCollaborator" should "call to httpClient.getWithoutResponse with the right parameters" in { + val response: IO[GHResponse[Unit]] = + IO(GHResponse(().asRight, okStatusCode, Map.empty)) + + implicit val httpClientMock = httpClientMockGetWithoutResponse( + url = s"repos/$validRepoOwner/$validRepoName/collaborators/$validUsername", + response = response + ) + + val repos = new RepositoriesInterpreter[IO] + + repos.userIsCollaborator( + validRepoOwner, + validRepoName, + validUsername, + headers = headerUserAgent + ) + } + "Repos.getRepoPermissionForUser" should "call to httpClient.get with the right parameters" in { val response: IO[GHResponse[UserRepoPermission]] = IO(GHResponse(userRepoPermission.asRight, okStatusCode, Map.empty)) diff --git a/github4s/src/test/scala/github4s/utils/BaseSpec.scala b/github4s/src/test/scala/github4s/utils/BaseSpec.scala index 457ac4528..3d2515b62 100644 --- a/github4s/src/test/scala/github4s/utils/BaseSpec.scala +++ b/github4s/src/test/scala/github4s/utils/BaseSpec.scala @@ -17,10 +17,9 @@ package github4s.utils import cats.effect.IO -import github4s.GithubConfig -import github4s.GHResponse import github4s.domain.Pagination import github4s.http.HttpClient +import github4s.{GHResponse, GithubConfig} import io.circe.{Decoder, Encoder} import org.http4s.client.Client import org.scalamock.scalatest.MockFactory @@ -61,6 +60,18 @@ trait BaseSpec extends AnyFlatSpec with Matchers with TestData with MockFactory httpClientMock } + def httpClientMockGetWithoutResponse( + url: String, + response: IO[GHResponse[Unit]] + ): HttpClient[IO] = { + val httpClientMock = mock[HttpClientTest] + (httpClientMock + .getWithoutResponse(_: Option[String], _: String, _: Map[String, String])) + .expects(sampleToken, url, headerUserAgent) + .returns(response) + httpClientMock + } + def httpClientMockPost[In, Out]( url: String, req: In, diff --git a/microsite/docs/repository.md b/microsite/docs/repository.md index 3b50a34fa..cf69b5916 100644 --- a/microsite/docs/repository.md +++ b/microsite/docs/repository.md @@ -15,6 +15,7 @@ with Github4s, you can interact with: - [List user repositories](#list-user-repositories) - [List contributors](#list-contributors) - [List collaborators](#list-collaborators) + - [Check user is a repository collaborator](#check-if-a-user-is-a-repository-collaborator) - [Get repository permissions for a user](#get-repository-permissions-for-a-user) - [Commits](#commits) - [List commits on a repository](#list-commits-on-a-repository) @@ -182,6 +183,26 @@ The `result` on the right is the corresponding [List[User]][user-scala]. See [the API doc](https://developer.github.com/v3/repos/collaborators/#list-collaborators) for full reference. +### Check if a user is a repository collaborator + +Response `statusCode` when user is a collaborator is 204 (No Content) + +Response `statusCode` when user is not a collaborator is 404 (Not Found) + +```scala mdoc:compile-only +val userIsCollaborator = gh.repos.userIsCollaborator("47degrees", "github4s", "rafaparadela") +val response = userIsCollaborator.unsafeRunSync() +response.statusCode match { + case 204 => println("User is a collaborator") + case 404 => println("User is not a collaborator) +} +``` + +The `result` on the right is `Unit` + +See [the API doc](https://developer.github.com/v3/repos/collaborators/#check-if-a-user-is-a-repository-collaborator) +for full reference. + ### Get repository permissions for a user Checks the repository permission of a collaborator. From 215da17eba75675cda2e88ac980c9006914ff18f Mon Sep 17 00:00:00 2001 From: Zach Kirlew Date: Tue, 21 Jul 2020 17:00:32 +0200 Subject: [PATCH 02/12] Remove unnecessary delete method call --- github4s/src/main/scala/github4s/http/HttpClient.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/github4s/src/main/scala/github4s/http/HttpClient.scala b/github4s/src/main/scala/github4s/http/HttpClient.scala index c96a64d47..2e7749f41 100644 --- a/github4s/src/main/scala/github4s/http/HttpClient.scala +++ b/github4s/src/main/scala/github4s/http/HttpClient.scala @@ -58,7 +58,7 @@ class HttpClient[F[_]: Sync](client: Client[F], val config: GithubConfig) { headers: Map[String, String] = Map.empty ): F[GHResponse[Unit]] = run[Unit, Unit]( - RequestBuilder(buildURL(url)).deleteMethod.withHeaders(headers).withAuth(accessToken) + RequestBuilder(buildURL(url)).withHeaders(headers).withAuth(accessToken) ) def patch[Req: Encoder, Res: Decoder]( From cfd83074dfff7719537e0713b0974739617ff30e Mon Sep 17 00:00:00 2001 From: Zach Kirlew Date: Tue, 21 Jul 2020 17:06:13 +0200 Subject: [PATCH 03/12] Close string literal in docs --- microsite/docs/repository.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/microsite/docs/repository.md b/microsite/docs/repository.md index cf69b5916..0aec7244c 100644 --- a/microsite/docs/repository.md +++ b/microsite/docs/repository.md @@ -194,7 +194,7 @@ val userIsCollaborator = gh.repos.userIsCollaborator("47degrees", "github4s", "r val response = userIsCollaborator.unsafeRunSync() response.statusCode match { case 204 => println("User is a collaborator") - case 404 => println("User is not a collaborator) + case 404 => println("User is not a collaborator") } ``` From 5e4f7f0520fea515e7cc29e293f45dd6e5200858 Mon Sep 17 00:00:00 2001 From: Zach Kirlew Date: Tue, 21 Jul 2020 19:10:09 +0200 Subject: [PATCH 04/12] Return boolean result instead of unit --- .../github4s/algebras/Repositories.scala | 5 ++- .../RepositoriesInterpreter.scala | 31 ++++++++++++++----- .../github4s/integration/ReposSpec.scala | 27 +++++++++++++--- .../test/scala/github4s/unit/ReposSpec.scala | 2 +- microsite/docs/repository.md | 15 +++++---- 5 files changed, 59 insertions(+), 21 deletions(-) diff --git a/github4s/src/main/scala/github4s/algebras/Repositories.scala b/github4s/src/main/scala/github4s/algebras/Repositories.scala index b5dde9f1f..b85070eca 100644 --- a/github4s/src/main/scala/github4s/algebras/Repositories.scala +++ b/github4s/src/main/scala/github4s/algebras/Repositories.scala @@ -278,6 +278,9 @@ trait Repositories[F[_]] { * organization members that are direct collaborators, organization members with access through team memberships, * organization members with access through default organization permissions, and organization owners. * + * Expect 204 No Content response when user is a collaborator + * Expect 404 Not Found response when user not is a collaborator + * * @param owner of the repo * @param repo name of the repo * @param username Github username @@ -289,7 +292,7 @@ trait Repositories[F[_]] { repo: String, username: String, headers: Map[String, String] = Map() - ): F[GHResponse[Unit]] + ): F[GHResponse[Boolean]] /** * Get the repository permission of a collaborator diff --git a/github4s/src/main/scala/github4s/interpreters/RepositoriesInterpreter.scala b/github4s/src/main/scala/github4s/interpreters/RepositoriesInterpreter.scala index cd069a5d7..76cd62e7a 100644 --- a/github4s/src/main/scala/github4s/interpreters/RepositoriesInterpreter.scala +++ b/github4s/src/main/scala/github4s/interpreters/RepositoriesInterpreter.scala @@ -16,7 +16,9 @@ package github4s.interpreters +import cats.Functor import cats.data.NonEmptyList +import cats.syntax.functor._ import com.github.marklister.base64.Base64._ import github4s.Decoders._ import github4s.Encoders._ @@ -25,8 +27,10 @@ import github4s.algebras.Repositories import github4s.domain._ import github4s.http.HttpClient -class RepositoriesInterpreter[F[_]](implicit client: HttpClient[F], accessToken: Option[String]) - extends Repositories[F] { +class RepositoriesInterpreter[F[_]: Functor](implicit + client: HttpClient[F], + accessToken: Option[String] +) extends Repositories[F] { override def get( owner: String, repo: String, @@ -222,12 +226,14 @@ class RepositoriesInterpreter[F[_]](implicit client: HttpClient[F], accessToken: repo: String, username: String, headers: Map[String, String] - ): F[GHResponse[Unit]] = - client.getWithoutResponse( - accessToken, - s"repos/$owner/$repo/collaborators/$username", - headers - ) + ): F[GHResponse[Boolean]] = + client + .getWithoutResponse( + accessToken, + s"repos/$owner/$repo/collaborators/$username", + headers + ) + .map(handleIsCollaboratorResponse) override def getRepoPermissionForUser( owner: String, @@ -335,4 +341,13 @@ class RepositoriesInterpreter[F[_]](implicit client: HttpClient[F], accessToken: headers, NewStatusRequest(state, target_url, description, context) ) + + private def handleIsCollaboratorResponse(response: GHResponse[Unit]): GHResponse[Boolean] = + response.result match { + case Right(_) => response.copy(result = Right(true)) + case Left(_) if response.statusCode == 404 => + response.copy(result = Right(false)) + case Left(error) => GHResponse(Left(error), response.statusCode, response.headers) + } + } diff --git a/github4s/src/test/scala/github4s/integration/ReposSpec.scala b/github4s/src/test/scala/github4s/integration/ReposSpec.scala index 22cff6ba0..fc079ea0f 100644 --- a/github4s/src/test/scala/github4s/integration/ReposSpec.scala +++ b/github4s/src/test/scala/github4s/integration/ReposSpec.scala @@ -19,7 +19,7 @@ package github4s.integration import cats.data.NonEmptyList import cats.effect.{IO, Resource} import cats.implicits._ -import github4s.GHError.NotFoundError +import github4s.GHError.{NotFoundError, UnauthorizedError} import github4s.domain._ import github4s.utils.{BaseIntegrationSpec, Integration} import github4s.{GHResponse, Github} @@ -313,7 +313,7 @@ trait ReposSpec extends BaseIntegrationSpec { response.statusCode shouldBe notFoundStatusCode } - "Repos >> UserIsCollaborator" should "return an empty body with no content status code" taggedAs Integration in { + "Repos >> UserIsCollaborator" should "return true when response is successful" taggedAs Integration in { val response = clientResource .use { client => Github[IO](client, accessToken).repos @@ -326,11 +326,11 @@ trait ReposSpec extends BaseIntegrationSpec { } .unsafeRunSync() - testIsRight[Unit](response, r => r should be(())) + testIsRight[Boolean](response, r => r should be(true)) response.statusCode shouldBe noContentStatusCode } - it should "return a not found error when username is not a repo collaborator" taggedAs Integration in { + it should "return false when not found error occurs" taggedAs Integration in { val response = clientResource .use { client => Github[IO](client, accessToken).repos @@ -343,10 +343,27 @@ trait ReposSpec extends BaseIntegrationSpec { } .unsafeRunSync() - testIsLeft[NotFoundError, Unit](response) + testIsRight[Boolean](response, r => r should be(false)) response.statusCode shouldBe notFoundStatusCode } + it should "return error when other errors occur" taggedAs Integration in { + val response = clientResource + .use { client => + Github[IO](client, accessToken).repos + .userIsCollaborator( + validRepoName, + validRepoName, + invalidUsername, + headers = headerUserAgent + ) + } + .unsafeRunSync() + + testIsLeft[UnauthorizedError, Boolean](response) + response.statusCode shouldBe unauthorizedStatusCode + } + "Repos >> GetRepoPermissionForUser" should "return user repo permission" taggedAs Integration in { val response = clientResource .use { client => diff --git a/github4s/src/test/scala/github4s/unit/ReposSpec.scala b/github4s/src/test/scala/github4s/unit/ReposSpec.scala index dc5a35b85..cf72b89c7 100644 --- a/github4s/src/test/scala/github4s/unit/ReposSpec.scala +++ b/github4s/src/test/scala/github4s/unit/ReposSpec.scala @@ -344,7 +344,7 @@ class ReposSpec extends BaseSpec { "Repos.userIsCollaborator" should "call to httpClient.getWithoutResponse with the right parameters" in { val response: IO[GHResponse[Unit]] = - IO(GHResponse(().asRight, okStatusCode, Map.empty)) + IO(GHResponse(().asRight, noContentStatusCode, Map.empty)) implicit val httpClientMock = httpClientMockGetWithoutResponse( url = s"repos/$validRepoOwner/$validRepoName/collaborators/$validUsername", diff --git a/microsite/docs/repository.md b/microsite/docs/repository.md index 0aec7244c..4e41709cc 100644 --- a/microsite/docs/repository.md +++ b/microsite/docs/repository.md @@ -185,20 +185,23 @@ reference. ### Check if a user is a repository collaborator -Response `statusCode` when user is a collaborator is 204 (No Content) +Returns whether a given user is a repository collaborator or not. -Response `statusCode` when user is not a collaborator is 404 (Not Found) +204 No Content responses will produce a true result + +404 Not Found responses will produce a false result ```scala mdoc:compile-only val userIsCollaborator = gh.repos.userIsCollaborator("47degrees", "github4s", "rafaparadela") val response = userIsCollaborator.unsafeRunSync() -response.statusCode match { - case 204 => println("User is a collaborator") - case 404 => println("User is not a collaborator") +if (response.result) { + println("User is a collaborator") +} else { + println("User is not a collaborator") } ``` -The `result` on the right is `Unit` +The `result` on the right is `Boolean` See [the API doc](https://developer.github.com/v3/repos/collaborators/#check-if-a-user-is-a-repository-collaborator) for full reference. From 27d64720eaf67b815f3d84cc09c82f9b3d0f1cdf Mon Sep 17 00:00:00 2001 From: Zach Kirlew Date: Tue, 21 Jul 2020 19:12:49 +0200 Subject: [PATCH 05/12] Improve doc consistency --- github4s/src/main/scala/github4s/algebras/Repositories.scala | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/github4s/src/main/scala/github4s/algebras/Repositories.scala b/github4s/src/main/scala/github4s/algebras/Repositories.scala index b85070eca..5f4aae9bd 100644 --- a/github4s/src/main/scala/github4s/algebras/Repositories.scala +++ b/github4s/src/main/scala/github4s/algebras/Repositories.scala @@ -278,8 +278,8 @@ trait Repositories[F[_]] { * organization members that are direct collaborators, organization members with access through team memberships, * organization members with access through default organization permissions, and organization owners. * - * Expect 204 No Content response when user is a collaborator - * Expect 404 Not Found response when user not is a collaborator + * 204 No Content responses will produce a true result + * 404 Not Found responses will produce a false result * * @param owner of the repo * @param repo name of the repo From ad4cecfd21e54fb0a71aefd89a74c6befa0d52de Mon Sep 17 00:00:00 2001 From: Zach Kirlew Date: Tue, 21 Jul 2020 19:13:36 +0200 Subject: [PATCH 06/12] Fix typo --- github4s/src/main/scala/github4s/algebras/Repositories.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/github4s/src/main/scala/github4s/algebras/Repositories.scala b/github4s/src/main/scala/github4s/algebras/Repositories.scala index 5f4aae9bd..434c13b01 100644 --- a/github4s/src/main/scala/github4s/algebras/Repositories.scala +++ b/github4s/src/main/scala/github4s/algebras/Repositories.scala @@ -285,7 +285,7 @@ trait Repositories[F[_]] { * @param repo name of the repo * @param username Github username * @param headers optional user headers to include in the request - * @return a unit GHResponse + * @return a Boolean GHResponse */ def userIsCollaborator( owner: String, From 3116dc11c6c1cb2f7632ab2e2cbfbdb8eed06406 Mon Sep 17 00:00:00 2001 From: Zach Kirlew Date: Tue, 21 Jul 2020 19:17:14 +0200 Subject: [PATCH 07/12] Fix docs --- microsite/docs/repository.md | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/microsite/docs/repository.md b/microsite/docs/repository.md index 4e41709cc..77eca49d9 100644 --- a/microsite/docs/repository.md +++ b/microsite/docs/repository.md @@ -194,10 +194,9 @@ Returns whether a given user is a repository collaborator or not. ```scala mdoc:compile-only val userIsCollaborator = gh.repos.userIsCollaborator("47degrees", "github4s", "rafaparadela") val response = userIsCollaborator.unsafeRunSync() -if (response.result) { - println("User is a collaborator") -} else { - println("User is not a collaborator") +response.result match { + case Left(e) => println(s"Something went wrong: ${e.getMessage}") + case Right(r) => println(r) } ``` From 4a46ef31748ca5cfea4811dce88bd0a94472cce4 Mon Sep 17 00:00:00 2001 From: Zach Kirlew Date: Wed, 22 Jul 2020 10:00:00 +0200 Subject: [PATCH 08/12] Remove unnecessary comments in docs --- microsite/docs/repository.md | 4 ---- 1 file changed, 4 deletions(-) diff --git a/microsite/docs/repository.md b/microsite/docs/repository.md index 77eca49d9..454c25c26 100644 --- a/microsite/docs/repository.md +++ b/microsite/docs/repository.md @@ -187,10 +187,6 @@ reference. Returns whether a given user is a repository collaborator or not. -204 No Content responses will produce a true result - -404 Not Found responses will produce a false result - ```scala mdoc:compile-only val userIsCollaborator = gh.repos.userIsCollaborator("47degrees", "github4s", "rafaparadela") val response = userIsCollaborator.unsafeRunSync() From 30abb8cb6b388983391deb81e1a586cf602bdfa7 Mon Sep 17 00:00:00 2001 From: Zach Kirlew Date: Wed, 22 Jul 2020 11:26:30 +0200 Subject: [PATCH 09/12] Correct integration test params --- .../src/test/scala/github4s/integration/ReposSpec.scala | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/github4s/src/test/scala/github4s/integration/ReposSpec.scala b/github4s/src/test/scala/github4s/integration/ReposSpec.scala index fc079ea0f..c4db99345 100644 --- a/github4s/src/test/scala/github4s/integration/ReposSpec.scala +++ b/github4s/src/test/scala/github4s/integration/ReposSpec.scala @@ -335,7 +335,7 @@ trait ReposSpec extends BaseIntegrationSpec { .use { client => Github[IO](client, accessToken).repos .userIsCollaborator( - validRepoName, + validRepoOwner, validRepoName, invalidUsername, headers = headerUserAgent @@ -350,11 +350,11 @@ trait ReposSpec extends BaseIntegrationSpec { it should "return error when other errors occur" taggedAs Integration in { val response = clientResource .use { client => - Github[IO](client, accessToken).repos + Github[IO](client, "invalid-access-token".some).repos .userIsCollaborator( + validRepoOwner, validRepoName, - validRepoName, - invalidUsername, + validUsername, headers = headerUserAgent ) } From 79ea66882a66b7c1d268151eddd1bf0525cd3a8a Mon Sep 17 00:00:00 2001 From: Zach Kirlew Date: Thu, 23 Jul 2020 11:38:56 +0200 Subject: [PATCH 10/12] Allow requests expect empty body --- .../main/scala/github4s/http/HttpClient.scala | 30 +++++++++++++++---- 1 file changed, 25 insertions(+), 5 deletions(-) diff --git a/github4s/src/main/scala/github4s/http/HttpClient.scala b/github4s/src/main/scala/github4s/http/HttpClient.scala index 2e7749f41..e9ae59e47 100644 --- a/github4s/src/main/scala/github4s/http/HttpClient.scala +++ b/github4s/src/main/scala/github4s/http/HttpClient.scala @@ -17,7 +17,7 @@ package github4s.http import cats.data.EitherT -import cats.effect.Sync +import cats.effect.{Resource, Sync} import cats.instances.string._ import cats.syntax.either._ import cats.syntax.functor._ @@ -57,7 +57,7 @@ class HttpClient[F[_]: Sync](client: Client[F], val config: GithubConfig) { url: String, headers: Map[String, String] = Map.empty ): F[GHResponse[Unit]] = - run[Unit, Unit]( + runEmptyBody[Unit]( RequestBuilder(buildURL(url)).withHeaders(headers).withAuth(accessToken) ) @@ -154,6 +154,21 @@ class HttpClient[F[_]: Sync](client: Client[F], val config: GithubConfig) { private def buildURL(method: String): String = s"${config.baseUrl}$method" private def run[Req: Encoder, Res: Decoder](request: RequestBuilder[Req]): F[GHResponse[Res]] = + runRequest(request) + .use { response => + buildResponse(response).map(GHResponse(_, response.status.code, response.headers.toMap)) + } + + private def runEmptyBody[Req: Encoder](request: RequestBuilder[Req]): F[GHResponse[Unit]] = + runRequest( + request + ).use { response => + buildResponseFromEmpty(response).map( + GHResponse(_, response.status.code, response.headers.toMap) + ) + } + + private def runRequest[Req: Encoder](request: RequestBuilder[Req]): Resource[F, Response[F]] = client .run( Request[F]() @@ -162,9 +177,6 @@ class HttpClient[F[_]: Sync](client: Client[F], val config: GithubConfig) { .withHeaders((config.toHeaderList ++ request.toHeaderList): _*) .withJsonBody(request.data) ) - .use { response => - buildResponse(response).map(GHResponse(_, response.status.code, response.headers.toMap)) - } } object HttpClient { @@ -196,6 +208,14 @@ object HttpClient { _.leftMap[GHError](identity) ) + private[github4s] def buildResponseFromEmpty[F[_]: Sync]( + response: Response[F] + ): F[Either[GHError, Unit]] = + response.status.code match { + case i if Status(i).isSuccess => Sync[F].pure(().asRight) + case _ => buildResponse[F, Unit](response) + } + private def responseBody[F[_]: Sync](response: Response[F]): F[String] = response.bodyText.compile.foldMonoid } From 8a2d2881ca9cefa414e6503106dc2b1bebcb4967 Mon Sep 17 00:00:00 2001 From: Zach Kirlew Date: Thu, 23 Jul 2020 11:41:05 +0200 Subject: [PATCH 11/12] More consistent naming --- github4s/src/main/scala/github4s/http/HttpClient.scala | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/github4s/src/main/scala/github4s/http/HttpClient.scala b/github4s/src/main/scala/github4s/http/HttpClient.scala index e9ae59e47..058384faa 100644 --- a/github4s/src/main/scala/github4s/http/HttpClient.scala +++ b/github4s/src/main/scala/github4s/http/HttpClient.scala @@ -57,7 +57,7 @@ class HttpClient[F[_]: Sync](client: Client[F], val config: GithubConfig) { url: String, headers: Map[String, String] = Map.empty ): F[GHResponse[Unit]] = - runEmptyBody[Unit]( + runWithoutResponse[Unit]( RequestBuilder(buildURL(url)).withHeaders(headers).withAuth(accessToken) ) @@ -159,7 +159,7 @@ class HttpClient[F[_]: Sync](client: Client[F], val config: GithubConfig) { buildResponse(response).map(GHResponse(_, response.status.code, response.headers.toMap)) } - private def runEmptyBody[Req: Encoder](request: RequestBuilder[Req]): F[GHResponse[Unit]] = + private def runWithoutResponse[Req: Encoder](request: RequestBuilder[Req]): F[GHResponse[Unit]] = runRequest( request ).use { response => From 6f49b1c7b01497fbea80399c84b3ff396be3f672 Mon Sep 17 00:00:00 2001 From: Ben Fradet Date: Thu, 23 Jul 2020 12:46:40 +0200 Subject: [PATCH 12/12] Apply suggestions from code review --- github4s/src/main/scala/github4s/algebras/Repositories.scala | 2 -- github4s/src/test/scala/github4s/integration/ReposSpec.scala | 4 ++-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/github4s/src/main/scala/github4s/algebras/Repositories.scala b/github4s/src/main/scala/github4s/algebras/Repositories.scala index 434c13b01..8ebe18224 100644 --- a/github4s/src/main/scala/github4s/algebras/Repositories.scala +++ b/github4s/src/main/scala/github4s/algebras/Repositories.scala @@ -278,8 +278,6 @@ trait Repositories[F[_]] { * organization members that are direct collaborators, organization members with access through team memberships, * organization members with access through default organization permissions, and organization owners. * - * 204 No Content responses will produce a true result - * 404 Not Found responses will produce a false result * * @param owner of the repo * @param repo name of the repo diff --git a/github4s/src/test/scala/github4s/integration/ReposSpec.scala b/github4s/src/test/scala/github4s/integration/ReposSpec.scala index c4db99345..0e9347ee5 100644 --- a/github4s/src/test/scala/github4s/integration/ReposSpec.scala +++ b/github4s/src/test/scala/github4s/integration/ReposSpec.scala @@ -313,7 +313,7 @@ trait ReposSpec extends BaseIntegrationSpec { response.statusCode shouldBe notFoundStatusCode } - "Repos >> UserIsCollaborator" should "return true when response is successful" taggedAs Integration in { + "Repos >> UserIsCollaborator" should "return true when the user is a collaborator" taggedAs Integration in { val response = clientResource .use { client => Github[IO](client, accessToken).repos @@ -330,7 +330,7 @@ trait ReposSpec extends BaseIntegrationSpec { response.statusCode shouldBe noContentStatusCode } - it should "return false when not found error occurs" taggedAs Integration in { + it should "return false when the user is not a collaborator" taggedAs Integration in { val response = clientResource .use { client => Github[IO](client, accessToken).repos