From 8ea8e8644e7d579d6c681a9ba237df81fb586543 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonatan=20K=C5=82osko?= Date: Fri, 6 Sep 2024 15:16:39 +0700 Subject: [PATCH] Refactor HTTP response --- lib/livebook/file_system/s3/client.ex | 70 +++++++++++++------------ lib/livebook/notebook/content_loader.ex | 2 +- lib/livebook/runtime/dependencies.ex | 2 +- lib/livebook/update_check.ex | 2 +- lib/livebook/utils/http.ex | 26 ++++----- lib/livebook_cli/server.ex | 2 +- rel/app/standalone.exs | 2 +- 7 files changed, 52 insertions(+), 54 deletions(-) diff --git a/lib/livebook/file_system/s3/client.ex b/lib/livebook/file_system/s3/client.ex index 8c3a59518..e355fa964 100644 --- a/lib/livebook/file_system/s3/client.ex +++ b/lib/livebook/file_system/s3/client.ex @@ -12,7 +12,7 @@ defmodule Livebook.FileSystem.S3.Client do query = %{"list-type" => "2", "prefix" => prefix, "delimiter" => delimiter} case get(file_system, "/", query: query) do - {:ok, 200, _headers, %{"ListBucketResult" => result}} -> + {:ok, %{status: 200, body: %{"ListBucketResult" => result}}} -> file_keys = xml_get_list(result, "Contents", "Key") prefix_keys = xml_get_list(result, "CommonPrefixes", "Prefix") @@ -37,7 +37,7 @@ defmodule Livebook.FileSystem.S3.Client do # with an upper limit of 0 and retrieve the bucket name. case get(file_system, "/", query: %{"list-type" => "2", "max-keys" => "0"}) do - {:ok, 200, _headers, %{"ListBucketResult" => result}} -> {:ok, result["Name"]} + {:ok, %{status: 200, body: %{"ListBucketResult" => result}}} -> {:ok, result["Name"]} other -> request_response_to_error(other) end end @@ -48,8 +48,8 @@ defmodule Livebook.FileSystem.S3.Client do @spec get_object(S3.t(), String.t()) :: {:ok, map()} | {:error, String.t()} def get_object(file_system, key) do case get(file_system, "/" <> encode_key(key), long: true, decode: false) do - {:ok, 200, _headers, body} -> {:ok, body} - {:ok, 404, _headers, _body} -> FileSystem.Utils.posix_error(:enoent) + {:ok, %{status: 200, body: body}} -> {:ok, body} + {:ok, %{status: 404}} -> FileSystem.Utils.posix_error(:enoent) other -> request_response_to_error(other) end end @@ -73,7 +73,7 @@ defmodule Livebook.FileSystem.S3.Client do @spec put_object(S3.t(), String.t(), String.t() | nil) :: :ok | {:error, String.t()} def put_object(file_system, key, content) do case put(file_system, "/" <> encode_key(key), body: content, long: true) do - {:ok, 200, _headers, _body} -> :ok + {:ok, %{status: 200}} -> :ok other -> request_response_to_error(other) end end @@ -83,11 +83,11 @@ defmodule Livebook.FileSystem.S3.Client do """ @spec head_object(S3.t(), String.t()) :: {:ok, map()} | {:error, String.t()} def head_object(file_system, key) do - with {:ok, 200, headers, _body} <- head(file_system, "/" <> encode_key(key)), - {:ok, etag} <- Livebook.Utils.HTTP.fetch_header(headers, "etag") do + with {:ok, %{status: 200, headers: headers}} <- head(file_system, "/" <> encode_key(key)), + {:ok, [etag]} <- Map.fetch(headers, "etag") do {:ok, %{etag: etag}} else - {:ok, 404, _headers, _body} -> FileSystem.Utils.posix_error(:enoent) + {:ok, %{status: 404}} -> FileSystem.Utils.posix_error(:enoent) other -> request_response_to_error(other) end end @@ -101,8 +101,8 @@ defmodule Livebook.FileSystem.S3.Client do headers = [{"x-amz-copy-source", copy_source}] case put(file_system, "/" <> encode_key(destination_key), headers: headers) do - {:ok, 200, _headers, _body} -> :ok - {:ok, 404, _headers, _body} -> FileSystem.Utils.posix_error(:enoent) + {:ok, %{status: 200}} -> :ok + {:ok, %{status: 404}} -> FileSystem.Utils.posix_error(:enoent) other -> request_response_to_error(other) end end @@ -113,8 +113,8 @@ defmodule Livebook.FileSystem.S3.Client do @spec delete_object(S3.t(), String.t()) :: :ok | {:error, String.t()} def delete_object(file_system, key) do case delete(file_system, "/" <> encode_key(key)) do - {:ok, 204, _headers, _body} -> :ok - {:ok, 404, _headers, _body} -> :ok + {:ok, %{status: 204}} -> :ok + {:ok, %{status: 404}} -> :ok other -> request_response_to_error(other) end end @@ -135,8 +135,8 @@ defmodule Livebook.FileSystem.S3.Client do headers = [{"Content-MD5", Base.encode64(body_md5)}] case post(file_system, "/", query: %{"delete" => ""}, headers: headers, body: body) do - {:ok, 200, _headers, %{"Error" => _}} = result -> request_response_to_error(result) - {:ok, 200, _headers, _body} -> :ok + {:ok, %{status: 200, body: %{"Error" => _}}} = result -> request_response_to_error(result) + {:ok, %{status: 200}} -> :ok other -> request_response_to_error(other) end end @@ -168,7 +168,7 @@ defmodule Livebook.FileSystem.S3.Client do query = %{"uploads" => ""} case post(file_system, "/" <> encode_key(key), query: query, body: "") do - {:ok, 200, _headers, %{"InitiateMultipartUploadResult" => result}} -> + {:ok, %{status: 200, body: %{"InitiateMultipartUploadResult" => result}}} -> {:ok, result["UploadId"]} other -> @@ -185,8 +185,9 @@ defmodule Livebook.FileSystem.S3.Client do query = %{"uploadId" => upload_id, "partNumber" => part_number} opts = [query: query, body: content, long: true] - with {:ok, 200, headers, _body} <- put(file_system, "/" <> encode_key(key), opts), - {:ok, etag} <- Livebook.Utils.HTTP.fetch_header(headers, "etag") do + with {:ok, %{status: 200, headers: headers}} <- + put(file_system, "/" <> encode_key(key), opts), + {:ok, [etag]} <- Map.fetch(headers, "etag") do {:ok, %{etag: etag}} else other -> request_response_to_error(other) @@ -208,7 +209,7 @@ defmodule Livebook.FileSystem.S3.Client do |> IO.iodata_to_binary() case post(file_system, "/" <> encode_key(key), query: query, body: body) do - {:ok, 200, _headers, _body} -> :ok + {:ok, %{status: 200}} -> :ok other -> request_response_to_error(other) end end @@ -221,7 +222,7 @@ defmodule Livebook.FileSystem.S3.Client do query = %{"uploadId" => upload_id} case delete(file_system, "/" <> encode_key(key), query: query) do - {:ok, 204, _headers, _body} -> :ok + {:ok, %{status: 204}} -> :ok other -> request_response_to_error(other) end end @@ -279,8 +280,7 @@ defmodule Livebook.FileSystem.S3.Client do url, headers, body || "", - uri_encode_path: false, - session_token: credentials.token + uri_encode_path: false ) end @@ -303,18 +303,20 @@ defmodule Livebook.FileSystem.S3.Client do if decode?, do: decode(result), else: result end - defp decode({:ok, status, headers, body}) do - if xml?(headers, body), - do: {:ok, status, headers, S3.XML.decode!(body)}, - else: {:ok, status, headers, body} + defp decode({:ok, response}) do + if xml?(response) do + {:ok, update_in(response.body, &S3.XML.decode!/1)} + else + {:ok, response} + end end defp decode({:error, _} = error), do: error - defp xml?(headers, body) do - guess_xml? = String.starts_with?(body, " true # Apparently some requests return XML without content-type :error when guess_xml? -> true @@ -322,24 +324,26 @@ defmodule Livebook.FileSystem.S3.Client do end end - defp request_response_to_error({:ok, 403, _headers, %{"Error" => %{"Message" => message}}}) do + defp request_response_to_error( + {:ok, %{status: 403, body: %{"Error" => %{"Message" => message}}}} + ) do {:error, "access denied, " <> Livebook.Utils.downcase_first(message)} end - defp request_response_to_error({:ok, 403, _headers, _body}) do + defp request_response_to_error({:ok, %{status: 403}}) do {:error, "access denied"} end - defp request_response_to_error({:ok, _status, _headers, %{"Error" => %{"Message" => message}}}) do + defp request_response_to_error({:ok, %{body: %{"Error" => %{"Message" => message}}}}) do {:error, Livebook.Utils.downcase_first(message)} end - defp request_response_to_error({:ok, _status, _headers, %{"Error" => [_ | _] = errors}}) do + defp request_response_to_error({:ok, %{body: %{"Error" => [_ | _] = errors}}}) do [%{"Message" => message} | errors] = errors {:error, Livebook.Utils.downcase_first(message) <> ", and #{length(errors)} more errors"} end - defp request_response_to_error({:ok, _status, _headers, _body}) do + defp request_response_to_error({:ok, _response}) do {:error, "unexpected response"} end diff --git a/lib/livebook/notebook/content_loader.ex b/lib/livebook/notebook/content_loader.ex index b1ceccdc0..6bdaff6e6 100644 --- a/lib/livebook/notebook/content_loader.ex +++ b/lib/livebook/notebook/content_loader.ex @@ -54,7 +54,7 @@ defmodule Livebook.Notebook.ContentLoader do @spec fetch_content(String.t()) :: {:ok, String.t()} | {:error, String.t()} def fetch_content(url) do case HTTP.request(:get, url) do - {:ok, 200, headers, body} -> + {:ok, %{status: 200, headers: headers, body: body}} -> valid_content? = case HTTP.fetch_content_type(headers) do {:ok, content_type} -> diff --git a/lib/livebook/runtime/dependencies.ex b/lib/livebook/runtime/dependencies.ex index b4fe3737b..5acb94200 100644 --- a/lib/livebook/runtime/dependencies.ex +++ b/lib/livebook/runtime/dependencies.ex @@ -284,7 +284,7 @@ defmodule Livebook.Runtime.Dependencies do url = api_url <> "/packages?" <> URI.encode_query(params) case Livebook.Utils.HTTP.request(:get, url) do - {:ok, status, _headers, body} -> + {:ok, %{status: status, body: body}} -> with 200 <- status, {:ok, packages} <- Jason.decode(body) do packages = packages diff --git a/lib/livebook/update_check.ex b/lib/livebook/update_check.ex index a23ffd608..3c4fb8ef0 100644 --- a/lib/livebook/update_check.ex +++ b/lib/livebook/update_check.ex @@ -132,7 +132,7 @@ defmodule Livebook.UpdateCheck do headers = [{"accept", "application/vnd.github.v3+json"}] case Livebook.Utils.HTTP.request(:get, url, headers: headers) do - {:ok, status, _headers, body} -> + {:ok, %{status: status, body: body}} -> with 200 <- status, {:ok, release} <- Jason.decode(body) do {:ok, release} diff --git a/lib/livebook/utils/http.ex b/lib/livebook/utils/http.ex index dad7f1911..07e67ab01 100644 --- a/lib/livebook/utils/http.ex +++ b/lib/livebook/utils/http.ex @@ -1,26 +1,17 @@ defmodule Livebook.Utils.HTTP do @type status :: non_neg_integer() - @type headers :: list(header()) + @type headers :: %{String.t() => list(String.t())} @type header :: {String.t(), String.t()} - @doc """ - Retrieves the header value from response headers. - """ - @spec fetch_header(headers(), String.t()) :: {:ok, String.t()} | :error - def fetch_header(headers, key) do - case Enum.find(headers, &match?({^key, _}, &1)) do - {_, value} -> {:ok, value} - _ -> :error - end - end - @doc """ Retrieves content type from response headers. """ @spec fetch_content_type(headers()) :: {:ok, String.t()} | :error def fetch_content_type(headers) do - with {:ok, value} <- fetch_header(headers, "content-type") do + with {:ok, [value]} <- Map.fetch(headers, "content-type") do {:ok, value |> String.split(";") |> hd()} + else + _ -> :error end end @@ -37,7 +28,7 @@ defmodule Livebook.Utils.HTTP do """ @spec request(atom(), String.t(), keyword()) :: - {:ok, status(), headers(), binary()} | {:error, term()} + {:ok, %{status: status(), headers: headers(), body: binary()}} | {:error, term()} def request(method, url, opts \\ []) when is_atom(method) and is_binary(url) and is_list(opts) do headers = build_headers(opts[:headers] || []) @@ -59,7 +50,7 @@ defmodule Livebook.Utils.HTTP do case :httpc.request(method, request, http_opts, opts) do {:ok, {{_, status, _}, headers, body}} -> - {:ok, status, parse_headers(headers), body} + {:ok, %{status: status, headers: parse_headers(headers), body: body}} {:error, error} -> {:error, error} @@ -76,9 +67,12 @@ defmodule Livebook.Utils.HTTP do end defp parse_headers(headers) do - Enum.map(headers, fn {key, val} -> + headers + |> Enum.map(fn {key, val} -> {String.downcase(to_string(key)), to_string(val)} end) + |> Enum.group_by(&elem(&1, 0), &elem(&1, 1)) + |> Map.new() end @doc """ diff --git a/lib/livebook_cli/server.ex b/lib/livebook_cli/server.ex index 25489e467..cbb101351 100644 --- a/lib/livebook_cli/server.ex +++ b/lib/livebook_cli/server.ex @@ -130,7 +130,7 @@ defmodule LivebookCLI.Server do health_url = set_path(base_url, "/public/health") case Livebook.Utils.HTTP.request(:get, health_url) do - {:ok, status, _headers, body} -> + {:ok, %{status: status, body: body}} -> with 200 <- status, {:ok, body} <- Jason.decode(body), %{"application" => "livebook"} <- body do diff --git a/rel/app/standalone.exs b/rel/app/standalone.exs index 04184cc26..0a03e84bd 100644 --- a/rel/app/standalone.exs +++ b/rel/app/standalone.exs @@ -143,7 +143,7 @@ defmodule Standalone do Logger.debug("Downloading #{url}") case Livebook.Utils.HTTP.request(:get, url, timeout: :infinity) do - {:ok, 200, _headers, body} -> + {:ok, %{status: 200, body: body}} -> body {:error, error} ->