Fixed issues and inconsistencies with pagination api

begedin · joshsmith · commit 84c543508644 · 2017-11-07T13:45:33.000-08:00
- added API.Errors namespace, to move other errors into
- added API.Errors.PaginationError
- rewrote some specs that were breaking contracts
- standardized get_all response to match request response
- corrected several type definitions
- added support for recovering from failed individual page requests during get_all
- added support for recovering from failed initial head request during get_all
- added support for marshalling a failed and succesful head response (body is "")
- rewrote ResultAggregator docs and argument names so they reflect the fact that it can be used for aggregating any standard response list, not just db commit
- wrote tests to cover new behaviors
diff --git a/lib/code_corps/accounts/accounts.ex b/lib/code_corps/accounts/accounts.ex
@@ -71,7 +71,7 @@ defmodule CodeCorps.Accounts do
     end
   end
 
-  @spec do_update_with_github_user(Usert.t, GithubUser.t) :: {:ok, User.t} | {:error, Changeset.t}
+  @spec do_update_with_github_user(User.t, GithubUser.t) :: {:ok, User.t} | {:error, Changeset.t}
   defp do_update_with_github_user(%User{} = user, %GithubUser{} = github_user) do
     user
     |> Changesets.update_with_github_user_changeset(github_user |> Adapters.User.to_user_attrs())
diff --git a/lib/code_corps/github/adapters/utils/body_decorator.ex b/lib/code_corps/github/adapters/utils/body_decorator.ex
@@ -5,6 +5,7 @@ defmodule CodeCorps.GitHub.Adapters.Utils.BodyDecorator do
 
   alias CodeCorps.{
     Comment,
+    GithubIssue,
     Task,
     User,
     WebClient
@@ -13,7 +14,7 @@ defmodule CodeCorps.GitHub.Adapters.Utils.BodyDecorator do
   @separator "\r\n\r\n[//]: # (Please type your edits below this line)\r\n\r\n---"
   @linebreak "\r\n\r\n"
 
-  @spec add_code_corps_header(map, Comment.t | Task.t) :: map
+  @spec add_code_corps_header(map, Comment.t | Task.t | GithubIssue.t) :: map
   def add_code_corps_header(%{"body" => body} = attrs, %Comment{user: %User{github_id: nil}} = comment) do
     modified_body = build_header(comment) <> @separator <> @linebreak <> body
     attrs |> Map.put("body", modified_body)
diff --git a/lib/code_corps/github/api/api.ex b/lib/code_corps/github/api/api.ex
@@ -2,36 +2,43 @@ defmodule CodeCorps.GitHub.API do
   alias CodeCorps.{
     GithubAppInstallation,
     GitHub,
+    GitHub.API.Errors.PaginationError,
     GitHub.API.Pagination,
     GitHub.APIError,
     GitHub.HTTPClientError,
+    GitHub.Utils.ResultAggregator,
     User
   }
 
   alias HTTPoison.{Error, Response}
 
   def gateway(), do: Application.get_env(:code_corps, :github)
 
-  @spec request(GitHub.method, String.t, GitHub.headers, GitHub.body, list) :: GitHub.response
+  @typep raw_body :: String.t
+  @typep raw_headers :: list({String.t, String.t})
+  @typep raw_options :: Keyword.t
+
+  @spec request(GitHub.method, String.t, raw_body, raw_headers, raw_options) :: GitHub.response
   def request(method, url, body, headers, options) do
     gateway().request(method, url, body, headers, options)
     |> marshall_response()
   end
 
-  @spec get_all(String.t, GitHub.headers, list) :: GitHub.response
+  @spec get_all(String.t, raw_headers, raw_options) :: {:ok, list(map)} | {:error, PaginationError.t} | {:error, GitHub.api_error_struct}
   def get_all(url, headers, options) do
-    {:ok, %Response{request_url: request_url, headers: response_headers}} =
-      gateway().request(:head, url, "", headers, options)
-
-    response_headers
-    |> Pagination.retrieve_total_pages()
-    |> Pagination.to_page_numbers()
-    |> Enum.map(&Pagination.add_page_param(options, &1))
-    |> Enum.map(&gateway().request(:get, url, "", headers, &1))
-    |> Enum.map(&marshall_response/1)
-    |> Enum.map(&Tuple.to_list/1)
-    |> Enum.map(&List.last/1)
-    |> List.flatten
+    case gateway().request(:head, url, "", headers, options) do
+      {:ok, %Response{headers: response_headers, status_code: code}} when code in 200..399 ->
+        response_headers
+        |> Pagination.retrieve_total_pages()
+        |> Pagination.to_page_numbers()
+        |> Enum.map(&Pagination.add_page_param(options, &1))
+        |> Enum.map(&gateway().request(:get, url, "", headers, &1))
+        |> Enum.map(&marshall_response/1)
+        |> ResultAggregator.aggregate
+        |> marshall_paginated_response()
+      other
+        -> other |> marshall_response()
+    end
   end
 
   @doc """
@@ -69,6 +76,9 @@ defmodule CodeCorps.GitHub.API do
   @typep http_failure :: {:error, term}
 
   @spec marshall_response(http_success | http_failure) :: GitHub.response
+  defp marshall_response({:ok, %Response{body: "", status_code: status}}) when status in 200..299 do
+    {:ok, %{}}
+  end
   defp marshall_response({:ok, %Response{body: body, status_code: status}}) when status in 200..299 do
     case body |> Poison.decode do
       {:ok, json} ->
@@ -80,6 +90,9 @@ defmodule CodeCorps.GitHub.API do
   defp marshall_response({:ok, %Response{body: body, status_code: 404}}) do
     {:error, APIError.new({404, %{"message" => body}})}
   end
+  defp marshall_response({:ok, %Response{body: "", status_code: status}}) when status in 400..599 do
+    {:error, APIError.new({status, %{"message" => "API Error during HEAD request"}})}
+  end
   defp marshall_response({:ok, %Response{body: body, status_code: status}}) when status in 400..599 do
     case body |> Poison.decode do
       {:ok, json} ->
@@ -94,4 +107,8 @@ defmodule CodeCorps.GitHub.API do
   defp marshall_response({:error, reason}) do
     {:error, HTTPClientError.new(reason: reason)}
   end
+
+  @spec marshall_paginated_response(tuple) :: tuple
+  defp marshall_paginated_response({:ok, pages}), do: {:ok, pages |> List.flatten}
+  defp marshall_paginated_response({:error, responses}), do: {:error, responses |> PaginationError.new}
 end
diff --git a/lib/code_corps/github/api/errors/pagination_error.ex b/lib/code_corps/github/api/errors/pagination_error.ex
@@ -0,0 +1,30 @@
+defmodule CodeCorps.GitHub.API.Errors.PaginationError do
+  alias CodeCorps.GitHub.{HTTPClientError, APIError}
+
+  @type t :: %__MODULE__{
+    message: String.t,
+    api_errors: list,
+    client_errors: list,
+    retrieved_pages: list
+  }
+
+  defstruct [
+    message: "One or more pages failed to retrieve during a GitHub API Pagination Request",
+    retrieved_pages: [],
+    client_errors: [],
+    api_errors: []
+  ]
+
+  def new({pages, errors}) do
+    %__MODULE__{
+      retrieved_pages: pages,
+      client_errors: errors |> Enum.filter(&is_client_error?/1),
+      api_errors: errors |> Enum.filter(&is_api_error?/1)
+    }
+  end
+
+  defp is_client_error?(%HTTPClientError{}), do: true
+  defp is_client_error?(_), do: false
+  defp is_api_error?(%APIError{}), do: true
+  defp is_api_error?(_), do: false
+end
diff --git a/lib/code_corps/github/api/installation.ex b/lib/code_corps/github/api/installation.ex
@@ -18,30 +18,27 @@ defmodule CodeCorps.GitHub.API.Installation do
 
   https://developer.github.com/v3/apps/installations/#list-repositories
   """
-  @spec repositories(GithubAppInstallation.t) :: {:ok, list(map)} | {:error, GitHub.api_error_struct}
+  @spec repositories(GithubAppInstallation.t) :: {:ok, list(map)} | {:error, GitHub.paginated_endpoint_error}
   def repositories(%GithubAppInstallation{} = installation) do
     with {:ok, access_token} <- installation |> get_access_token(),
-         {:ok, responses} <- fetch_repositories(access_token),
-         repositories <- extract_repositories(responses) do
-      {:ok, repositories}
+         {:ok, responses} <- access_token |> fetch_repositories() do
+      {:ok, responses |> extract_repositories}
     else
       {:error, error} -> {:error, error}
     end
   end
 
+  @spec fetch_repositories(String.t) :: {:ok, list(map)} | {:error, GitHub.paginated_endpoint_error}
   defp fetch_repositories(access_token) do
     "installation/repositories"
     |> GitHub.get_all(%{}, [access_token: access_token, params: [per_page: 100]])
-    |> (&{:ok, &1}).()
   end
 
+  @spec extract_repositories(list(map)) :: list(map)
   defp extract_repositories(responses) do
     responses
-    |> Enum.reduce([], &merge_repositories/2)
-  end
-
-  defp merge_repositories(response, acc) do
-    acc |> Enum.concat(response |> Map.get("repositories"))
+    |> Enum.map(&Map.get(&1, "repositories"))
+    |> List.flatten
   end
 
   @doc """
diff --git a/lib/code_corps/github/api/pagination.ex b/lib/code_corps/github/api/pagination.ex
@@ -70,7 +70,7 @@ defmodule CodeCorps.GitHub.API.Pagination do
   @doc ~S"""
   From the specified page count, generates a list of integers, `1..count`
   """
-  @spec to_page_numbers(integer) :: list(integer)
+  @spec to_page_numbers(integer) :: Range.t
   def to_page_numbers(total) when is_integer(total), do: 1..total
 
   @doc ~S"""
diff --git a/lib/code_corps/github/api/repository.ex b/lib/code_corps/github/api/repository.ex
@@ -17,7 +17,7 @@ defmodule CodeCorps.GitHub.API.Repository do
   All pages of records are retrieved.
   Closed issues are included.
   """
-  @spec issues(GithubRepo.t) :: {:ok, list(map)} | {:error, GitHub.api_error_struct}
+  @spec issues(GithubRepo.t) :: {:ok, list(map)} | {:error, GitHub.paginated_endpoint_error}
   def issues(%GithubRepo{
     github_app_installation: %GithubAppInstallation{
       github_account_login: owner
@@ -27,7 +27,6 @@ defmodule CodeCorps.GitHub.API.Repository do
     with {:ok, access_token} <- API.Installation.get_access_token(installation) do
       "repos/#{owner}/#{repo}/issues"
       |> GitHub.get_all(%{}, [access_token: access_token, params: [per_page: 100, state: "all"]])
-      |> (&{:ok, &1}).()
     else
       {:error, error} -> {:error, error}
     end
@@ -38,7 +37,7 @@ defmodule CodeCorps.GitHub.API.Repository do
 
   All pages of records are retrieved.
   """
-  @spec pulls(GithubRepo.t) :: {:ok, list(map)} | {:error, GitHub.api_error_struct}
+  @spec pulls(GithubRepo.t) :: {:ok, list(map)} | {:error, GitHub.paginated_endpoint_error}
   def pulls(%GithubRepo{
     github_app_installation: %GithubAppInstallation{
       github_account_login: owner
@@ -48,7 +47,6 @@ defmodule CodeCorps.GitHub.API.Repository do
     with {:ok, access_token} <- API.Installation.get_access_token(installation) do
       "repos/#{owner}/#{repo}/pulls"
       |> GitHub.get_all(%{}, [access_token: access_token, params: [per_page: 100, state: "all"]])
-      |> (&{:ok, &1}).()
     else
       {:error, error} -> {:error, error}
     end
@@ -57,7 +55,7 @@ defmodule CodeCorps.GitHub.API.Repository do
   @doc ~S"""
   Retrieves comments from all issues in a github repository.
   """
-  @spec issue_comments(GithubRepo.t) :: {:ok, list(map)} | {:error, GitHub.api_error_struct}
+  @spec issue_comments(GithubRepo.t) :: {:ok, list(map)} | {:error, GitHub.paginated_endpoint_error}
   def issue_comments(%GithubRepo{
     github_app_installation: %GithubAppInstallation{
       github_account_login: owner
@@ -67,7 +65,6 @@ defmodule CodeCorps.GitHub.API.Repository do
     with {:ok, access_token} <- API.Installation.get_access_token(installation) do
       "repos/#{owner}/#{repo}/issues/comments"
       |> GitHub.get_all(%{}, [access_token: access_token, params: [per_page: 100]])
-      |> (&{:ok, &1}).()
     else
       {:error, error} -> {:error, error}
     end
diff --git a/lib/code_corps/github/event/installation/installation.ex b/lib/code_corps/github/event/installation/installation.ex
@@ -90,7 +90,7 @@ defmodule CodeCorps.GitHub.Event.Installation do
   defp marshall_result({:error, :installation, :unexpected_installation_payload, _steps}), do: {:error, :unexpected_payload}
   defp marshall_result({:error, :installation, %Changeset{}, _steps}), do: {:error, :validation_error_on_syncing_installation}
   defp marshall_result({:error, :installation, :too_many_unprocessed_installations, _steps}), do: {:error, :multiple_unprocessed_installations_found}
-  defp marshall_result({:error, :api_response, %CodeCorps.GitHub.APIError{}, _steps}), do: {:error, :github_api_error_on_syncing_repos}
+  defp marshall_result({:error, :api_response, %CodeCorps.GitHub.API.Errors.PaginationError{}, _steps}), do: {:error, :github_api_error_on_syncing_repos}
   defp marshall_result({:error, :deleted_repos, {_results, _changesets}, _steps}), do: {:error, :validation_error_on_deleting_removed_repos}
   defp marshall_result({:error, :synced_repos, {_results, _changesets}, _steps}), do: {:error, :validation_error_on_syncing_existing_repos}
   defp marshall_result({:error, :processed_installation, %Changeset{}, _steps}), do: {:error, :validation_error_on_marking_installation_processed}
diff --git a/lib/code_corps/github/event/installation/repos.ex b/lib/code_corps/github/event/installation/repos.ex
@@ -43,7 +43,7 @@ defmodule CodeCorps.GitHub.Event.Installation.Repos do
   end
 
   # transaction step 1
-  @spec fetch_api_repo_list(map) :: {:ok, map} | {:error, GitHub.api_error_struct}
+  @spec fetch_api_repo_list(map) :: {:ok, list(map)} | {:error, GitHub.paginated_endpoint_error}
   defp fetch_api_repo_list(%{processing_installation: %GithubAppInstallation{} = installation}) do
     installation |> Installation.repositories()
   end
diff --git a/lib/code_corps/github/github.ex b/lib/code_corps/github/github.ex
@@ -84,14 +84,28 @@ defmodule CodeCorps.GitHub do
     end
   end
 
-
   @type method :: :get | :post | :put | :delete | :patch | :head
 
   @type body :: {:multipart, list} | map
   @type headers :: %{String.t => String.t} | %{}
   @type response :: {:ok, map} | {:error, api_error_struct}
   @type api_error_struct :: APIError.t | HTTPClientError.t
 
+  @typedoc ~S"""
+  Potential errors which can happen when retrieving data from a paginated
+  endpoint.
+
+  If a new access token is required, then it is regenerated and stored into an
+  installation, which can result in any of
+    - `Ecto.Changeset.t`
+    - `CodeCorps.GitHub.APIError.t`
+    - `CodeCorps.GitHub.HTTPClientError.t`
+
+  Once that is done, the actual request is made, which can error out with
+    - `CodeCorps.GitHub.Errors.PaginationError.t`
+  """
+  @type paginated_endpoint_error :: Ecto.Changeset.t | APIError.t | HTTPClientError.t | API.Errors.PaginationError.t
+
   @doc """
   A low level utility function to make a direct request to the GitHub API.
   """
@@ -110,6 +124,20 @@ defmodule CodeCorps.GitHub do
     end
   end
 
+  @doc ~S"""
+  A low level utility function to make an authenticated request to a GitHub API
+  endpoint which supports pagination, and fetch all the pages from that endpoint
+  at once, by making parallel requests to each page and aggregating the results.
+  """
+  @spec get_all(String.t, headers, list) :: {:ok, list(map)} | {:error, API.Errors.PaginationError.t} | {:error, api_error_struct}
+  def get_all(endpoint, headers, options) do
+    API.get_all(
+      api_url_for(endpoint),
+      headers |> Headers.user_request(options),
+      options
+    )
+  end
+
   @doc """
   A low level utility function to make an authenticated request to the
   GitHub API on behalf of a GitHub App or integration
@@ -129,14 +157,6 @@ defmodule CodeCorps.GitHub do
     end
   end
 
-  def get_all(endpoint, headers, options) do
-    API.get_all(
-      api_url_for(endpoint),
-      headers |> Headers.user_request(options),
-      options
-    )
-  end
-
   @token_url "https://github.com/login/oauth/access_token"
 
   @doc """
diff --git a/lib/code_corps/github/utils/result_aggregator.ex b/lib/code_corps/github/utils/result_aggregator.ex
@@ -1,19 +1,22 @@
 defmodule CodeCorps.GitHub.Utils.ResultAggregator do
   @moduledoc ~S"""
-  Module used for the purpose of aggregating results from multiple repository commit actions.
+  Used for aggregating a list of results.
   """
 
-  alias Ecto.Changeset
-
   @doc ~S"""
-  Aggregates a list of database commit results into an `:ok`, or `:error` tuple.
+  Aggregates a list of result tuples into a single result tuple.
+
+  A result tuple is a two-element tuple where the first element is `:ok`,
+  or `:error`, while the second element is the resulting data.
+
+  This function goes through a list of such tuples and aggregates the list into
+  a single tuple where
 
-  All list members are assumed to be either an `{:ok, committed_record}` or
-  `{:error, changeset}`.
+  - if all tuples in the list are `:ok` tuples, returns `{:ok, results}`
+  - if any tuple is an `:error` tuple, returns `{:error, {results, errors}}`
 
-  The aggregate is an `{:ok, committed_records}` if all results are
-  `{:ok, committed_record}`, or an `{:error, {committed_records, changesets}}`
-  if any of the results is an `{:error, changeset}`.
+  - `results` and `errors` are lists of second tuple elements in their
+    respective tuples
   """
   @spec aggregate(list) :: {:ok, list} | {:error, {list, list}}
   def aggregate(results) when is_list(results) do
@@ -22,15 +25,15 @@ defmodule CodeCorps.GitHub.Utils.ResultAggregator do
 
   @spec collect(list, list, list) :: tuple
   defp collect(results, recods \\ [], changesets \\ [])
-  defp collect([{:ok, record} | tail], records, changesets) do
-    collect(tail, records ++ [record], changesets)
+  defp collect([{:ok, record} | tail], records, errors) do
+    collect(tail, records ++ [record], errors)
   end
-  defp collect([{:error, %Changeset{} = changeset} | tail], records, changesets) do
-    collect(tail, records, changesets ++ [changeset])
+  defp collect([{:error, error} | tail], records, errors) do
+    collect(tail, records, errors ++ [error])
   end
-  defp collect([], records, changesets), do: {records, changesets}
+  defp collect([], records, errors), do: {records, errors}
 
   @spec summarize(tuple) :: tuple
   defp summarize({records, []}), do: {:ok, records}
-  defp summarize({records, changesets}), do: {:error, {records, changesets}}
+  defp summarize({records, errors}), do: {:error, {records, errors}}
 end
diff --git a/lib/code_corps/model/github_user.ex b/lib/code_corps/model/github_user.ex
@@ -2,6 +2,8 @@ defmodule CodeCorps.GithubUser do
   use Ecto.Schema
   import Ecto.Changeset
 
+  @type t :: %__MODULE__{}
+
   schema "github_users" do
     field :avatar_url, :string
     field :email, :string
diff --git a/priv/repo/structure.sql b/priv/repo/structure.sql
@@ -2,7 +2,7 @@
 -- PostgreSQL database dump
 --
 
--- Dumped from database version 10.0
+-- Dumped from database version 9.5.9
 -- Dumped by pg_dump version 10.0
 
 SET statement_timeout = 0;
diff --git a/test/lib/code_corps/github/api/api_test.exs b/test/lib/code_corps/github/api/api_test.exs
diff --git a/test/lib/code_corps/github/api/installation_test.exs b/test/lib/code_corps/github/api/installation_test.exs
