From ecc2029dbd2961b37621598003d3553139337e6b Mon Sep 17 00:00:00 2001 From: Henry Popp Date: Sun, 6 Jun 2021 12:44:32 -0500 Subject: [PATCH 1/5] refactor: on_response moved inside notification meta --- lib/pigeon.ex | 39 +++--- lib/pigeon/adapter.ex | 8 +- lib/pigeon/adm.ex | 57 ++++---- lib/pigeon/adm/notification.ex | 3 +- lib/pigeon/adm/result_parser.ex | 28 ++-- lib/pigeon/apns.ex | 14 +- lib/pigeon/apns/config.ex | 3 +- lib/pigeon/apns/jwt_config.ex | 3 +- lib/pigeon/apns/notification.ex | 3 +- lib/pigeon/apns/shared.ex | 20 +-- lib/pigeon/configurable.ex | 2 +- lib/pigeon/dispatcher.ex | 4 +- lib/pigeon/fcm.ex | 18 +-- lib/pigeon/fcm/config.ex | 36 +++--- lib/pigeon/fcm/notification.ex | 3 +- lib/pigeon/legacy_fcm.ex | 14 +- lib/pigeon/legacy_fcm/config.ex | 65 +++++----- lib/pigeon/legacy_fcm/notification.ex | 3 +- lib/pigeon/legacy_fcm/result_parser.ex | 14 +- lib/pigeon/metadata.ex | 7 + lib/pigeon/notification_queue.ex | 20 +-- lib/pigeon/sandbox.ex | 10 +- lib/pigeon/tasks.ex | 4 +- test/pigeon/adm/result_parser_test.exs | 21 ++- test/pigeon/legacy_fcm/result_parser_test.exs | 122 ++++++++---------- test/pigeon/sandbox_test.exs | 7 +- 26 files changed, 258 insertions(+), 270 deletions(-) create mode 100644 lib/pigeon/metadata.ex diff --git a/lib/pigeon.ex b/lib/pigeon.ex index b4be87ca..ade47ee7 100644 --- a/lib/pigeon.ex +++ b/lib/pigeon.ex @@ -25,6 +25,8 @@ defmodule Pigeon do See `Pigeon.Adapter` for instructions. """ + alias Pigeon.Tasks + @default_timeout 5_000 @typedoc ~S""" @@ -81,7 +83,10 @@ defmodule Pigeon do def push(pid, notifications, opts) when is_list(notifications) do if Keyword.has_key?(opts, :on_response) do - push_async(pid, notifications, opts[:on_response]) + on_response = Keyword.get(opts, :on_response) + notifications = Enum.map(notifications, fn n -> put_on_response(n, on_response) end) + + push_async(pid, notifications) else timeout = Keyword.get(opts, :timeout, @default_timeout) @@ -102,7 +107,8 @@ defmodule Pigeon do timeout = Keyword.get(opts, :timeout, @default_timeout) if Keyword.has_key?(opts, :on_response) do - push_async(pid, notification, opts[:on_response]) + notification = put_on_response(notification, Keyword.get(opts, :on_response)) + push_async(pid, notification) else push_sync(pid, notification, timeout) end @@ -112,8 +118,9 @@ defmodule Pigeon do myself = self() ref = :erlang.make_ref() on_response = fn x -> send(myself, {:"$push", ref, x}) end + notification = put_on_response(notification, on_response) - push_async(pid, notification, on_response) + push_async(pid, notification) receive do {:"$push", ^ref, x} -> x @@ -122,30 +129,30 @@ defmodule Pigeon do end end - defp push_async(pid, notifications, on_response) - when is_list(notifications) do - for n <- notifications, do: push_async(pid, n, on_response) - end - - defp push_async(pid, notification, nil) do - GenServer.cast(pid, {:push, notification, nil}) + defp push_async(pid, notifications) when is_list(notifications) do + for n <- notifications, do: push_async(pid, n) end - defp push_async(pid, notification, on_response) when is_pid(pid) do + defp push_async(pid, notification) when is_pid(pid) do if Process.alive?(pid) do - GenServer.cast(pid, {:push, notification, on_response}) + GenServer.cast(pid, {:push, notification}) else - on_response.(%{notification | response: :not_started}) + Tasks.process_on_response(%{notification | response: :not_started}) end end - defp push_async(pid, notification, on_response) do + defp push_async(pid, notification) do case Process.whereis(pid) do nil -> - on_response.(%{notification | response: :not_started}) + Tasks.process_on_response(%{notification | response: :not_started}) _pid -> - GenServer.cast(pid, {:push, notification, on_response}) + GenServer.cast(pid, {:push, notification}) end end + + defp put_on_response(notification, on_response) do + meta = %{notification.__meta__ | on_response: on_response} + %{notification | __meta__: meta} + end end diff --git a/lib/pigeon/adapter.ex b/lib/pigeon/adapter.ex index e70b746f..47637058 100644 --- a/lib/pigeon/adapter.ex +++ b/lib/pigeon/adapter.ex @@ -9,7 +9,7 @@ defmodule Pigeon.Adapter do ``` defmodule Pigeon.Sandbox do - import Pigeon.Tasks, only: [process_on_response: 2] + import Pigeon.Tasks, only: [process_on_response: 1] @behaviour Pigeon.Adapter @@ -53,11 +53,7 @@ defmodule Pigeon.Adapter do @doc """ Invoked to handle push notifications. """ - @callback handle_push( - notification :: struct | [struct], - on_response :: Pigeon.on_response(), - state :: term - ) :: + @callback handle_push(notification :: struct | [struct], state :: term) :: {:noreply, new_state :: term} | {:stop, reason :: term, new_state :: term} end diff --git a/lib/pigeon/adm.ex b/lib/pigeon/adm.ex index c67a28fe..903c186e 100644 --- a/lib/pigeon/adm.ex +++ b/lib/pigeon/adm.ex @@ -140,7 +140,7 @@ defmodule Pigeon.ADM do @behaviour Pigeon.Adapter - import Pigeon.Tasks, only: [process_on_response: 2] + import Pigeon.Tasks, only: [process_on_response: 1] alias Pigeon.ADM.{Config, ResultParser} require Logger @@ -167,15 +167,17 @@ defmodule Pigeon.ADM do end @impl true - def handle_push(notification, on_response, state) do + def handle_push(notification, state) do case refresh_access_token_if_needed(state) do {:ok, state} -> - :ok = do_push(notification, state, on_response) + :ok = do_push(notification, state) {:noreply, state} {:error, reason} -> - notification = %{notification | response: reason} - process_on_response(on_response, notification) + notification + |> Map.put(:response, reason) + |> process_on_response() + {:noreply, state} end end @@ -281,11 +283,11 @@ defmodule Pigeon.ADM do [{"Content-Type", "application/x-www-form-urlencoded;charset=UTF-8"}] end - defp do_push(notification, state, on_response) do + defp do_push(notification, state) do request = {notification.registration_id, encode_payload(notification)} response = - case on_response do + case notification.__meta__.on_response do nil -> fn {reg_id, payload} -> HTTPoison.post(adm_uri(reg_id), payload, adm_headers(state)) @@ -296,11 +298,12 @@ defmodule Pigeon.ADM do case HTTPoison.post(adm_uri(reg_id), payload, adm_headers(state)) do {:ok, %HTTPoison.Response{status_code: status, body: body}} -> notification = %{notification | registration_id: reg_id} - process_response(status, body, notification, on_response) + process_response(status, body, notification) {:error, %HTTPoison.Error{reason: :connect_timeout}} -> - notification = %{notification | response: :timeout} - process_on_response(on_response, notification) + notification + |> Map.put(:response, :timeout) + |> process_on_response() end end end @@ -349,25 +352,31 @@ defmodule Pigeon.ADM do payload |> Map.put("md5", md5) end - defp process_response(200, body, notification, on_response), - do: handle_200_status(body, notification, on_response) + defp process_response(200, body, notification), + do: handle_200_status(body, notification) - defp process_response(status, body, notification, on_response), - do: handle_error_status_code(status, body, notification, on_response) + defp process_response(status, body, notification), + do: handle_error_status_code(status, body, notification) - defp handle_200_status(body, notification, on_response) do + defp handle_200_status(body, notification) do {:ok, json} = Pigeon.json_library().decode(body) - parse_result(notification, json, on_response) + + notification + |> ResultParser.parse(json) + |> process_on_response() end - defp handle_error_status_code(status, body, notification, on_response) do + defp handle_error_status_code(status, body, notification) do case Pigeon.json_library().decode(body) do {:ok, %{"reason" => _reason} = result_json} -> - parse_result(notification, result_json, on_response) + notification + |> ResultParser.parse(result_json) + |> process_on_response() {:error, _} -> - n = %{notification | response: generic_error_reason(status)} - process_on_response(on_response, n) + notification + |> Map.put(:response, generic_error_reason(status)) + |> process_on_response() end end @@ -375,12 +384,4 @@ defmodule Pigeon.ADM do defp generic_error_reason(401), do: :authentication_error defp generic_error_reason(500), do: :internal_server_error defp generic_error_reason(_), do: :unknown_error - - # no on_response callback, ignore - @doc false - def parse_result(_, _, nil), do: :ok - - def parse_result(notification, response, on_response) do - ResultParser.parse(notification, response, on_response) - end end diff --git a/lib/pigeon/adm/notification.ex b/lib/pigeon/adm/notification.ex index 829fe9f0..3562d1a0 100644 --- a/lib/pigeon/adm/notification.ex +++ b/lib/pigeon/adm/notification.ex @@ -3,7 +3,8 @@ defmodule Pigeon.ADM.Notification do Defines Amazon ADM notification struct and convenience constructor functions. """ - defstruct consolidation_key: nil, + defstruct __meta__: %Pigeon.Metadata{}, + consolidation_key: nil, expires_after: 604_800, md5: nil, payload: %{}, diff --git a/lib/pigeon/adm/result_parser.ex b/lib/pigeon/adm/result_parser.ex index 8880b47f..21cbdd3d 100644 --- a/lib/pigeon/adm/result_parser.ex +++ b/lib/pigeon/adm/result_parser.ex @@ -6,33 +6,31 @@ defmodule Pigeon.ADM.ResultParser do ## Examples - iex> parse(%Pigeon.ADM.Notification{}, %{}, fn(x) -> x end) + iex> parse(%Pigeon.ADM.Notification{}, %{}) %Pigeon.ADM.Notification{response: :success} - iex> parse(%Pigeon.ADM.Notification{}, %{"registrationID" => "test"}, - ...> fn(x) -> x end) + iex> parse(%Pigeon.ADM.Notification{}, %{"registrationID" => "test"}) %Pigeon.ADM.Notification{response: :update, updated_registration_id: "test"} - iex> parse(%Pigeon.ADM.Notification{}, %{"reason" => "InvalidRegistrationId"}, - ...> fn(x) -> x end) + iex> parse(%Pigeon.ADM.Notification{}, %{"reason" => "InvalidRegistrationId"}) %Pigeon.ADM.Notification{response: :invalid_registration_id} """ # Handle RegID updates - def parse(notification, %{"registrationID" => new_regid}, on_response) do - n = %{notification | response: :update, updated_registration_id: new_regid} - on_response.(n) + def parse(notification, %{"registrationID" => new_regid}) do + notification + |> Map.put(:response, :update) + |> Map.put(:updated_registration_id, new_regid) end - def parse(notification, %{"reason" => error}, on_response) do - error = error |> to_error_atom - n = %{notification | response: error} - on_response.(n) + def parse(notification, %{"reason" => error}) do + notification + |> Map.put(:response, to_error_atom(error)) end - def parse(notification, %{}, on_response) do - n = %{notification | response: :success} - on_response.(n) + def parse(notification, %{}) do + notification + |> Map.put(:response, :success) end defp to_error_atom("InvalidRegistrationId"), do: :invalid_registration_id diff --git a/lib/pigeon/apns.ex b/lib/pigeon/apns.ex index 4697c723..aab7db68 100644 --- a/lib/pigeon/apns.ex +++ b/lib/pigeon/apns.ex @@ -178,19 +178,13 @@ defmodule Pigeon.APNS do end @impl true - def handle_push(notification, on_response, %{config: config, queue: queue} = state) do + def handle_push(notification, %{config: config, queue: queue} = state) do headers = Configurable.push_headers(config, notification, []) payload = Configurable.push_payload(config, notification, []) Client.default().send_request(state.socket, headers, payload) - new_q = - NotificationQueue.add( - queue, - state.stream_id, - notification, - on_response - ) + new_q = NotificationQueue.add(queue, state.stream_id, notification) state = state @@ -246,8 +240,8 @@ defmodule Pigeon.APNS do # Do nothing if no queued item for stream {:noreply, %{state | queue: new_queue}} - {{notif, on_response}, new_queue} -> - Configurable.handle_end_stream(config, stream, notif, on_response) + {notif, new_queue} -> + Configurable.handle_end_stream(config, stream, notif) {:noreply, %{state | queue: new_queue}} end end diff --git a/lib/pigeon/apns/config.ex b/lib/pigeon/apns/config.ex index b8ed94c9..3d0f3e60 100644 --- a/lib/pigeon/apns/config.ex +++ b/lib/pigeon/apns/config.ex @@ -134,8 +134,7 @@ defimpl Pigeon.Configurable, for: Pigeon.APNS.Config do defdelegate push_payload(config, notification, opts), to: Shared - defdelegate handle_end_stream(config, stream, notification, on_response), - to: Shared + defdelegate handle_end_stream(config, stream, notification), to: Shared defdelegate schedule_ping(any), to: Shared diff --git a/lib/pigeon/apns/jwt_config.ex b/lib/pigeon/apns/jwt_config.ex index b5b357e0..f81891ba 100644 --- a/lib/pigeon/apns/jwt_config.ex +++ b/lib/pigeon/apns/jwt_config.ex @@ -145,8 +145,7 @@ defimpl Pigeon.Configurable, for: Pigeon.APNS.JWTConfig do defdelegate push_payload(config, notification, opts), to: Shared - defdelegate handle_end_stream(config, stream, notification, on_response), - to: Shared + defdelegate handle_end_stream(config, stream, notification), to: Shared defdelegate schedule_ping(any), to: Shared diff --git a/lib/pigeon/apns/notification.ex b/lib/pigeon/apns/notification.ex index 0dcbb2ff..6024ea15 100644 --- a/lib/pigeon/apns/notification.ex +++ b/lib/pigeon/apns/notification.ex @@ -3,7 +3,8 @@ defmodule Pigeon.APNS.Notification do Defines APNS notification struct and constructor functions. """ - defstruct collapse_id: nil, + defstruct __meta__: %Pigeon.Metadata{}, + collapse_id: nil, device_token: nil, expiration: nil, priority: nil, diff --git a/lib/pigeon/apns/shared.ex b/lib/pigeon/apns/shared.ex index e9a58c36..6c01c710 100644 --- a/lib/pigeon/apns/shared.ex +++ b/lib/pigeon/apns/shared.ex @@ -1,7 +1,7 @@ defmodule Pigeon.APNS.Shared do @moduledoc false - import Pigeon.Tasks, only: [process_on_response: 2] + import Pigeon.Tasks, only: [process_on_response: 1] alias Pigeon.APNS.{Config, Error, JWTConfig, Notification} @@ -45,23 +45,23 @@ defmodule Pigeon.APNS.Shared do Pigeon.json_library().encode!(notification.payload) end - def handle_end_stream(_config, stream, notification, on_response) do + def handle_end_stream(_config, stream, notification) do %{headers: headers, body: body, status: status} = stream case status do 200 -> - n = - notification - |> Map.put(:id, get_header(headers, @apns_id)) - |> Map.put(:response, :success) - - process_on_response(on_response, n) + notification + |> Map.put(:id, get_header(headers, @apns_id)) + |> Map.put(:response, :success) + |> process_on_response() _error -> reason = Error.parse(body) Error.log(reason, notification) - notification = %{notification | response: reason} - process_on_response(on_response, notification) + + notification + |> Map.put(:response, reason) + |> process_on_response() end end diff --git a/lib/pigeon/configurable.ex b/lib/pigeon/configurable.ex index 3e2fa2b2..cb4f383c 100644 --- a/lib/pigeon/configurable.ex +++ b/lib/pigeon/configurable.ex @@ -10,7 +10,7 @@ defprotocol Pigeon.Configurable do def push_payload(config, notification, opts) - def handle_end_stream(config, stream, notification, on_response) + def handle_end_stream(config, stream, notification) @doc ~S""" Schedules connection ping if necessary. diff --git a/lib/pigeon/dispatcher.ex b/lib/pigeon/dispatcher.ex index 8c9c3900..47922b21 100644 --- a/lib/pigeon/dispatcher.ex +++ b/lib/pigeon/dispatcher.ex @@ -123,8 +123,8 @@ defmodule Pigeon.Dispatcher do end @impl true - def handle_cast({:push, notification, on_response}, %{adapter: adapter, state: state}) do - case adapter.handle_push(notification, on_response, state) do + def handle_cast({:push, notification}, %{adapter: adapter, state: state}) do + case adapter.handle_push(notification, state) do {:noreply, new_state} -> {:noreply, %{adapter: adapter, state: new_state}} {:stop, reason, new_state} -> {:stop, reason, %{adapter: adapter, state: new_state}} end diff --git a/lib/pigeon/fcm.ex b/lib/pigeon/fcm.ex index 456ad35c..04cb9349 100644 --- a/lib/pigeon/fcm.ex +++ b/lib/pigeon/fcm.ex @@ -129,23 +129,13 @@ defmodule Pigeon.FCM do end @impl true - def handle_push( - notification, - on_response, - %{config: config, queue: queue, token: token} = state - ) do + def handle_push(notification, %{config: config, queue: queue, token: token} = state) do headers = Configurable.push_headers(config, notification, token: token) payload = Configurable.push_payload(config, notification, []) Client.default().send_request(state.socket, headers, payload) - new_q = - NotificationQueue.add( - queue, - state.stream_id, - notification, - on_response - ) + new_q = NotificationQueue.add(queue, state.stream_id, notification) state = state @@ -234,8 +224,8 @@ defmodule Pigeon.FCM do # Do nothing if no queued item for stream {:noreply, %{state | queue: new_queue}} - {{notif, on_response}, new_queue} -> - Configurable.handle_end_stream(config, stream, notif, on_response) + {notif, new_queue} -> + Configurable.handle_end_stream(config, stream, notif) {:noreply, %{state | queue: new_queue}} end end diff --git a/lib/pigeon/fcm/config.ex b/lib/pigeon/fcm/config.ex index e95f64e4..3924a970 100644 --- a/lib/pigeon/fcm/config.ex +++ b/lib/pigeon/fcm/config.ex @@ -6,8 +6,6 @@ defmodule Pigeon.FCM.Config do service_account_json: nil, uri: 'fcm.googleapis.com' - import Pigeon.Tasks, only: [process_on_response: 2] - @type t :: %__MODULE__{ port: pos_integer, project_id: binary, @@ -65,9 +63,9 @@ defimpl Pigeon.Configurable, for: Pigeon.FCM.Config do require Logger - import Pigeon.Tasks, only: [process_on_response: 2] + import Pigeon.Tasks, only: [process_on_response: 1] - alias Pigeon.Encodable + alias Pigeon.{Encodable, Metadata} alias Pigeon.FCM.{Config, Error} @type sock :: {:sslsocket, any, pid | {any, any}} @@ -117,21 +115,25 @@ defimpl Pigeon.Configurable, for: Pigeon.FCM.Config do Encodable.binary_payload(notification) end - def handle_end_stream(_config, _stream, _notif, nil), do: :ok - - def handle_end_stream(_config, %{error: nil} = stream, notif, on_response) do - stream.body - |> Pigeon.json_library().decode!() - |> case do - %{"name" => name} -> - process_on_response(on_response, %{notif | name: name, response: :success}) + def handle_end_stream(_config, %{error: nil} = stream, notif) do + if Metadata.on_response?(notif) do + stream.body + |> Pigeon.json_library().decode!() + |> case do + %{"name" => name} -> + notif + |> Map.put(:name, name) + |> Map.put(:response, :success) + |> process_on_response() - %{"error" => error} -> - process_on_response(on_response, %{ + %{"error" => error} -> notif - | error: error, - response: Error.parse(error) - }) + |> Map.put(:error, error) + |> Map.put(:response, Error.parse(error)) + |> process_on_response() + end + else + :ok end end diff --git a/lib/pigeon/fcm/notification.ex b/lib/pigeon/fcm/notification.ex index 84c2c9df..cf227fe5 100644 --- a/lib/pigeon/fcm/notification.ex +++ b/lib/pigeon/fcm/notification.ex @@ -3,7 +3,8 @@ defmodule Pigeon.FCM.Notification do Defines FCM notification struct and convenience constructor functions. """ - defstruct android: nil, + defstruct __meta__: %Pigeon.Metadata{}, + android: nil, apns: nil, data: nil, error: nil, diff --git a/lib/pigeon/legacy_fcm.ex b/lib/pigeon/legacy_fcm.ex index dd98399f..7734ee35 100644 --- a/lib/pigeon/legacy_fcm.ex +++ b/lib/pigeon/legacy_fcm.ex @@ -212,19 +212,13 @@ defmodule Pigeon.LegacyFCM do end @impl true - def handle_push(notification, on_response, %{config: config, queue: queue} = state) do + def handle_push(notification, %{config: config, queue: queue} = state) do headers = Configurable.push_headers(config, notification, []) payload = Configurable.push_payload(config, notification, []) Client.default().send_request(state.socket, headers, payload) - new_q = - NotificationQueue.add( - queue, - state.stream_id, - notification, - on_response - ) + new_q = NotificationQueue.add(queue, state.stream_id, notification) state = state @@ -280,8 +274,8 @@ defmodule Pigeon.LegacyFCM do # Do nothing if no queued item for stream {:noreply, %{state | queue: new_queue}} - {{notif, on_response}, new_queue} -> - Configurable.handle_end_stream(config, stream, notif, on_response) + {notif, new_queue} -> + Configurable.handle_end_stream(config, stream, notif) {:noreply, %{state | queue: new_queue}} end end diff --git a/lib/pigeon/legacy_fcm/config.ex b/lib/pigeon/legacy_fcm/config.ex index e92bd8ae..d6eeccab 100644 --- a/lib/pigeon/legacy_fcm/config.ex +++ b/lib/pigeon/legacy_fcm/config.ex @@ -41,7 +41,7 @@ defimpl Pigeon.Configurable, for: Pigeon.LegacyFCM.Config do require Logger - import Pigeon.Tasks, only: [process_on_response: 2] + import Pigeon.Tasks, only: [process_on_response: 1] alias Pigeon.Encodable alias Pigeon.LegacyFCM.{Config, ResultParser} @@ -89,46 +89,56 @@ defimpl Pigeon.Configurable, for: Pigeon.LegacyFCM.Config do Encodable.binary_payload(notification) end - def handle_end_stream(_config, %{error: nil} = stream, notif, on_response) do - do_handle_end_stream(stream.status, stream.body, notif, on_response) + def handle_end_stream(_config, %{error: nil} = stream, notif) do + do_handle_end_stream(stream.status, stream.body, notif) end - def handle_end_stream(_config, %{error: _error}, _notif, nil), do: :ok - - def handle_end_stream(_config, _stream, {_regids, notif}, on_response) do - notif = %{notif | status: :unavailable} - process_on_response(on_response, notif) + def handle_end_stream(_config, _stream, {_regids, notif}) do + notif + |> Map.put(:status, :unavailable) + |> process_on_response() end - defp do_handle_end_stream(200, body, notif, on_response) do + defp do_handle_end_stream(200, body, notif) do result = Pigeon.json_library().decode!(body) notif = %{notif | status: :success} - parse_result(notif.registration_id, result, on_response, notif) + + notif.registration_id + |> parse_result(result, notif) + |> process_on_response() end - defp do_handle_end_stream(400, _body, notif, on_response) do + defp do_handle_end_stream(400, _body, notif) do log_error("400", "Malformed JSON") - notif = %{notif | status: :malformed_json} - process_on_response(on_response, notif) + + notif + |> Map.put(:status, :malformed_json) + |> process_on_response() end - defp do_handle_end_stream(401, _body, notif, on_response) do + defp do_handle_end_stream(401, _body, notif) do log_error("401", "Unauthorized") - notif = %{notif | status: :unauthorized} - process_on_response(on_response, notif) + + notif + |> Map.put(:status, :unauthorized) + |> process_on_response() end - defp do_handle_end_stream(500, _body, notif, on_response) do + defp do_handle_end_stream(500, _body, notif) do log_error("500", "Internal server error") - notif = %{notif | status: :internal_server_error} - process_on_response(on_response, notif) + + notif + |> Map.put(:status, :internal_server_error) + |> process_on_response() end - defp do_handle_end_stream(code, body, notif, on_response) do + defp do_handle_end_stream(code, body, notif) do reason = parse_error(body) log_error(code, reason) - notif = %{notif | response: reason} - process_on_response(on_response, notif) + + notif + |> Map.put(:response, reason) + |> process_on_response() end def schedule_ping(_config), do: :ok @@ -152,16 +162,13 @@ defimpl Pigeon.Configurable, for: Pigeon.LegacyFCM.Config do defp redact(config), do: config - # no on_response callback, ignore - def parse_result(_, _, nil, _notif), do: :ok - - def parse_result(ids, %{"results" => results}, on_response, notification) do - ResultParser.parse(ids, results, on_response, notification) + def parse_result(ids, %{"results" => results}, notification) do + ResultParser.parse(ids, results, notification) end - def parse_result(id, %{"message_id" => _} = result, on_response, notification) + def parse_result(id, %{"message_id" => _} = result, notification) when is_binary(id) do - parse_result([id], %{"results" => [result]}, on_response, notification) + parse_result([id], %{"results" => [result]}, notification) end def parse_error(data) do diff --git a/lib/pigeon/legacy_fcm/notification.ex b/lib/pigeon/legacy_fcm/notification.ex index a2dd8831..b2c2b84e 100644 --- a/lib/pigeon/legacy_fcm/notification.ex +++ b/lib/pigeon/legacy_fcm/notification.ex @@ -3,7 +3,8 @@ defmodule Pigeon.LegacyFCM.Notification do Defines `Pigeon.LegacyFCM.Notification` struct and convenience constructor functions. """ - defstruct collapse_key: nil, + defstruct __meta__: %Pigeon.Metadata{}, + collapse_key: nil, condition: nil, content_available: false, dry_run: false, diff --git a/lib/pigeon/legacy_fcm/result_parser.ex b/lib/pigeon/legacy_fcm/result_parser.ex index e769b4c2..e97527b4 100644 --- a/lib/pigeon/legacy_fcm/result_parser.ex +++ b/lib/pigeon/legacy_fcm/result_parser.ex @@ -1,17 +1,15 @@ defmodule Pigeon.LegacyFCM.ResultParser do @moduledoc false - import Pigeon.Tasks, only: [process_on_response: 2] - - def parse([], [], on_response, notif) do - process_on_response(on_response, notif) + def parse([], [], notif) do + notif end - def parse(regid, results, on_response, notif) when is_binary(regid) do - parse([regid], results, on_response, notif) + def parse(regid, results, notif) when is_binary(regid) do + parse([regid], results, notif) end - def parse([regid | reg_res], [result | rest_results], on_response, notif) do + def parse([regid | reg_res], [result | rest_results], notif) do updated_notif = case result do %{"message_id" => id, "registration_id" => new_regid} -> @@ -29,7 +27,7 @@ defmodule Pigeon.LegacyFCM.ResultParser do |> put_error(regid, error) end - parse(reg_res, rest_results, on_response, updated_notif) + parse(reg_res, rest_results, updated_notif) end defp put_update(%{response: resp} = notif, regid, new_regid) do diff --git a/lib/pigeon/metadata.ex b/lib/pigeon/metadata.ex new file mode 100644 index 00000000..5621c641 --- /dev/null +++ b/lib/pigeon/metadata.ex @@ -0,0 +1,7 @@ +defmodule Pigeon.Metadata do + defstruct on_response: nil + + def on_response?(notification) do + notification.__meta__.on_response != nil + end +end diff --git a/lib/pigeon/notification_queue.ex b/lib/pigeon/notification_queue.ex index ef0375d8..b1d14bbf 100644 --- a/lib/pigeon/notification_queue.ex +++ b/lib/pigeon/notification_queue.ex @@ -1,7 +1,7 @@ defmodule Pigeon.NotificationQueue do @moduledoc false - @type queue :: %{required(pos_integer) => {term, (... -> no_return)}} + @type queue :: %{required(pos_integer) => term} @doc ~S""" Returns a new empty queue. @@ -19,12 +19,12 @@ defmodule Pigeon.NotificationQueue do ## Examples - iex> add(%{}, 1, %Pigeon.APNS.Notification{}, nil) - %{1 => {%Pigeon.APNS.Notification{}, nil}} + iex> add(%{}, 1, %Pigeon.APNS.Notification{}) + %{1 => %Pigeon.APNS.Notification{}} """ - @spec add(queue, pos_integer, term, term) :: queue - def add(queue, stream_id, notification, on_response) do - Map.put(queue, stream_id, {notification, on_response}) + @spec add(queue, pos_integer, term) :: queue + def add(queue, stream_id, notification) do + Map.put(queue, stream_id, notification) end @doc ~S""" @@ -32,13 +32,13 @@ defmodule Pigeon.NotificationQueue do ## Examples - iex> queue = %{1 => {%Pigeon.APNS.Notification{}, nil}} + iex> queue = %{1 => %Pigeon.APNS.Notification{}} iex> pop(queue, 1) - {{%Pigeon.APNS.Notification{}, nil}, %{}} + {%Pigeon.APNS.Notification{}, %{}} - iex> queue = %{1 => {%Pigeon.APNS.Notification{}, nil}} + iex> queue = %{1 => %Pigeon.APNS.Notification{}} iex> pop(queue, 3) - {nil, %{1 => {%Pigeon.APNS.Notification{}, nil}}} + {nil, %{1 => %Pigeon.APNS.Notification{}}} """ @spec pop(queue, pos_integer) :: {nil | {term, term}, queue} def pop(queue, stream_id) do diff --git a/lib/pigeon/sandbox.ex b/lib/pigeon/sandbox.ex index 1fb65218..c1767c5b 100644 --- a/lib/pigeon/sandbox.ex +++ b/lib/pigeon/sandbox.ex @@ -7,7 +7,7 @@ defmodule Pigeon.Sandbox do exactly as given. """ - import Pigeon.Tasks, only: [process_on_response: 2] + import Pigeon.Tasks, only: [process_on_response: 1] @behaviour Pigeon.Adapter @@ -22,13 +22,13 @@ defmodule Pigeon.Sandbox do end @impl true - def handle_push(%{response: nil} = notification, on_response, state) do - process_on_response(on_response, %{notification | response: :success}) + def handle_push(%{response: nil} = notification, state) do + process_on_response(%{notification | response: :success}) {:noreply, state} end - def handle_push(notification, on_response, state) do - process_on_response(on_response, notification) + def handle_push(notification, state) do + process_on_response(notification) {:noreply, state} end end diff --git a/lib/pigeon/tasks.ex b/lib/pigeon/tasks.ex index ca19133f..0fc8b5b9 100644 --- a/lib/pigeon/tasks.ex +++ b/lib/pigeon/tasks.ex @@ -1,9 +1,9 @@ defmodule Pigeon.Tasks do @moduledoc false - def process_on_response(nil, _notif), do: :ok + def process_on_response(%{__meta__: %{on_response: nil}}), do: :ok - def process_on_response(on_response, notif) do + def process_on_response(%{__meta__: %{on_response: on_response}} = notif) do Task.Supervisor.start_child(Pigeon.Tasks, fn -> on_response.(notif) end) end end diff --git a/test/pigeon/adm/result_parser_test.exs b/test/pigeon/adm/result_parser_test.exs index e8ce8e9b..8e3356f0 100644 --- a/test/pigeon/adm/result_parser_test.exs +++ b/test/pigeon/adm/result_parser_test.exs @@ -4,17 +4,16 @@ defmodule Pigeon.ADM.ResultParserTest do test "parses known error reasons without crashing" do n = Pigeon.ADM.Notification.new("test") - onr = fn _ -> :ok end - Pigeon.ADM.ResultParser.parse(n, %{"reason" => "InvalidRegistrationId"}, onr) - Pigeon.ADM.ResultParser.parse(n, %{"reason" => "InvalidData"}, onr) - Pigeon.ADM.ResultParser.parse(n, %{"reason" => "InvalidConsolidationKey"}, onr) - Pigeon.ADM.ResultParser.parse(n, %{"reason" => "InvalidExpiration"}, onr) - Pigeon.ADM.ResultParser.parse(n, %{"reason" => "InvalidChecksum"}, onr) - Pigeon.ADM.ResultParser.parse(n, %{"reason" => "InvalidType"}, onr) - Pigeon.ADM.ResultParser.parse(n, %{"reason" => "Unregistered"}, onr) - Pigeon.ADM.ResultParser.parse(n, %{"reason" => "AccessTokenExpired"}, onr) - Pigeon.ADM.ResultParser.parse(n, %{"reason" => "MessageTooLarge"}, onr) - Pigeon.ADM.ResultParser.parse(n, %{"reason" => "MaxRateExceeded"}, onr) + Pigeon.ADM.ResultParser.parse(n, %{"reason" => "InvalidRegistrationId"}) + Pigeon.ADM.ResultParser.parse(n, %{"reason" => "InvalidData"}) + Pigeon.ADM.ResultParser.parse(n, %{"reason" => "InvalidConsolidationKey"}) + Pigeon.ADM.ResultParser.parse(n, %{"reason" => "InvalidExpiration"}) + Pigeon.ADM.ResultParser.parse(n, %{"reason" => "InvalidChecksum"}) + Pigeon.ADM.ResultParser.parse(n, %{"reason" => "InvalidType"}) + Pigeon.ADM.ResultParser.parse(n, %{"reason" => "Unregistered"}) + Pigeon.ADM.ResultParser.parse(n, %{"reason" => "AccessTokenExpired"}) + Pigeon.ADM.ResultParser.parse(n, %{"reason" => "MessageTooLarge"}) + Pigeon.ADM.ResultParser.parse(n, %{"reason" => "MaxRateExceeded"}) end end diff --git a/test/pigeon/legacy_fcm/result_parser_test.exs b/test/pigeon/legacy_fcm/result_parser_test.exs index c836f4be..e3f3dbb6 100644 --- a/test/pigeon/legacy_fcm/result_parser_test.exs +++ b/test/pigeon/legacy_fcm/result_parser_test.exs @@ -8,88 +8,80 @@ defmodule Pigeon.LegacyFCM.ResultParserTest do end test "parse_result with success" do - expected = [success: "regid"] - - ResultParser.parse( - ["regid"], - [%{"message_id" => "1:0408"}], - &assert_response(&1, expected), - %Notification{} - ) + notif = + ResultParser.parse( + ["regid"], + [%{"message_id" => "1:0408"}], + %Notification{} + ) + + assert notif.response == [success: "regid"] end test "parse_result with single non-list regid" do - expected = [success: "regid"] - - ResultParser.parse( - "regid", - [%{"message_id" => "1:0408"}], - &assert_response(&1, expected), - %Notification{} - ) + notif = + ResultParser.parse( + "regid", + [%{"message_id" => "1:0408"}], + %Notification{} + ) + + assert notif.response == [success: "regid"] end test "parse_result with success and new registration_id" do - pid = self() - resp = fn resp -> send(pid, resp) end - - ResultParser.parse( - ["regid"], - [%{"message_id" => "1:2342", "registration_id" => "32"}], - &resp.(&1), - %Notification{} - ) - - receive do - notif -> - assert notif.response == [update: {"regid", "32"}] - assert notif.message_id == "1:2342" - after - 5_000 -> flunk("No response received.") - end + notif = + ResultParser.parse( + ["regid"], + [%{"message_id" => "1:2342", "registration_id" => "32"}], + %Notification{} + ) + + assert notif.response == [update: {"regid", "32"}] + assert notif.message_id == "1:2342" end test "parse_result with error Unavailable" do - expected = [unavailable: "regid"] - - ResultParser.parse( - ["regid"], - [%{"error" => "Unavailable"}], - &assert_response(&1, expected), - %Notification{} - ) + notif = + ResultParser.parse( + ["regid"], + [%{"error" => "Unavailable"}], + %Notification{} + ) + + assert notif.response == [unavailable: "regid"] end test "parse_result with error NotRegistered" do - expected = [not_registered: "regid"] - - ResultParser.parse( - ["regid"], - [%{"error" => "NotRegistered"}], - &assert_response(&1, expected), - %Notification{} - ) + notif = + ResultParser.parse( + ["regid"], + [%{"error" => "NotRegistered"}], + %Notification{} + ) + + assert notif.response == [not_registered: "regid"] end test "parse_result with error InvalidRegistration" do - expected = [invalid_registration: "regid"] - - ResultParser.parse( - ["regid"], - [%{"error" => "InvalidRegistration"}], - &assert_response(&1, expected), - %Notification{} - ) + notif = + ResultParser.parse( + ["regid"], + [%{"error" => "InvalidRegistration"}], + %Notification{} + ) + + assert notif.response == [invalid_registration: "regid"] end test "parse_result with custom error" do - expected = [unknown_error: "regid"] - - ResultParser.parse( - ["regid"], - [%{"error" => "CustomError"}], - &assert_response(&1, expected), - %Notification{} - ) + notif = + ResultParser.parse( + ["regid"], + [%{"error" => "CustomError"}], + %Notification{} + ) + + assert notif.response == [unknown_error: "regid"] end end diff --git a/test/pigeon/sandbox_test.exs b/test/pigeon/sandbox_test.exs index 69eca115..43c65880 100644 --- a/test/pigeon/sandbox_test.exs +++ b/test/pigeon/sandbox_test.exs @@ -20,8 +20,9 @@ defmodule Pigeon.SandboxTest do assert n.response == :timeout end - test "handles any kind of term" do - n = PigeonTest.Sandbox.push("unexpected") - assert n == "unexpected" + test "handles any kind of map with __meta__" do + n = %{__meta__: %Pigeon.Metadata{}, expect_the: "unexpected"} + PigeonTest.Sandbox.push([n, n], on_response: nil) + # Didn't crash, nothing to test. end end From 0422fe960197c292a2722e5e280a6df965787c02 Mon Sep 17 00:00:00 2001 From: Henry Popp Date: Sun, 6 Jun 2021 14:32:45 -0500 Subject: [PATCH 2/5] feat: removed debug_log Also: - Sending a list of synchronous pushes now sends them one at a time in order. - Removed APNS debug log error description messages. - Added tests for all top-level Pigeon push options. --- CHANGELOG.md | 3 + config/test.exs | 3 - lib/pigeon.ex | 33 ++++----- lib/pigeon/apns.ex | 3 +- lib/pigeon/apns/error.ex | 122 -------------------------------- lib/pigeon/apns/shared.ex | 5 +- lib/pigeon/dispatcher.ex | 5 +- lib/pigeon/legacy_fcm/config.ex | 13 +--- test/pigeon_test.exs | 61 ++++++++++++++++ 9 files changed, 82 insertions(+), 166 deletions(-) create mode 100644 test/pigeon_test.exs diff --git a/CHANGELOG.md b/CHANGELOG.md index 35d4e941..0fa7a598 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,8 @@ ([#183](https://github.com/codedge-llc/pigeon/pull/183)) - Kadabra bumped to v0.5.0, and now a required dependency. ([#184](https://github.com/codedge-llc/pigeon/pull/184)) +- Sending a list of pushes synchronously now actually sends them one at a time. For production + workloads, using the async `:on_response` callback is strongly suggested. **Fixed** @@ -21,6 +23,7 @@ - `:certfile` and `:keyfile` are no longer valid options for APNS configurations. Instead, read the file before loading (e.g. `cert: File.read!("cert.pem")`) ([#183](https://github.com/codedge-llc/pigeon/pull/183)) +- `:debug_log` removed. ## v1.6.0 diff --git a/config/test.exs b/config/test.exs index e0c70b57..9bf18170 100644 --- a/config/test.exs +++ b/config/test.exs @@ -8,9 +8,6 @@ config :pigeon, :test, apns_key: File.read!("key_unencrypted.pem"), apns_topic: System.get_env("APNS_TOPIC") -config :pigeon, - debug_log: true - config :pigeon, PigeonTest.ADM, adapter: Pigeon.ADM, client_id: System.get_env("ADM_OAUTH2_CLIENT_ID"), diff --git a/lib/pigeon.ex b/lib/pigeon.ex index ade47ee7..5948e8ef 100644 --- a/lib/pigeon.ex +++ b/lib/pigeon.ex @@ -73,33 +73,20 @@ defmodule Pigeon do Application.get_env(:pigeon, :json_library, Jason) end - def debug_log?, do: Application.get_env(:pigeon, :debug_log, false) - - @spec push(pid | atom, notification :: struct | [struct], push_opts) :: - {:ok, notification :: struct} - | {:error, notification :: struct} - | :ok + @spec push(pid | atom, notification :: struct, push_opts) :: + notification :: struct | no_return + @spec push(pid | atom, notifications :: [struct, ...], push_opts) :: + notifications :: [struct, ...] | no_return def push(pid, notifications, opts \\ []) def push(pid, notifications, opts) when is_list(notifications) do if Keyword.has_key?(opts, :on_response) do on_response = Keyword.get(opts, :on_response) notifications = Enum.map(notifications, fn n -> put_on_response(n, on_response) end) - push_async(pid, notifications) else timeout = Keyword.get(opts, :timeout, @default_timeout) - - notifications - |> Enum.map(&Task.async(fn -> push_sync(pid, &1, timeout) end)) - |> Task.yield_many(timeout + 500) - |> Enum.map(fn {task, response} -> - case response do - nil -> Task.shutdown(task, :brutal_kill) - {:ok, resp} -> resp - _error -> nil - end - end) + for n <- notifications, do: push_sync(pid, n, timeout), into: [] end end @@ -131,11 +118,12 @@ defmodule Pigeon do defp push_async(pid, notifications) when is_list(notifications) do for n <- notifications, do: push_async(pid, n) + :ok end defp push_async(pid, notification) when is_pid(pid) do if Process.alive?(pid) do - GenServer.cast(pid, {:push, notification}) + send_push(pid, notification) else Tasks.process_on_response(%{notification | response: :not_started}) end @@ -147,10 +135,15 @@ defmodule Pigeon do Tasks.process_on_response(%{notification | response: :not_started}) _pid -> - GenServer.cast(pid, {:push, notification}) + send_push(pid, notification) end end + defp send_push(pid, notification) do + send(pid, {:"$push", notification}) + :ok + end + defp put_on_response(notification, on_response) do meta = %{notification.__meta__ | on_response: on_response} %{notification | __meta__: meta} diff --git a/lib/pigeon/apns.ex b/lib/pigeon/apns.ex index aab7db68..7e34a915 100644 --- a/lib/pigeon/apns.ex +++ b/lib/pigeon/apns.ex @@ -158,11 +158,12 @@ defmodule Pigeon.APNS do @behaviour Pigeon.Adapter alias Pigeon.{Configurable, NotificationQueue} + alias Pigeon.APNS.ConfigParser alias Pigeon.Http2.{Client, Stream} @impl true def init(opts) do - config = Pigeon.APNS.ConfigParser.parse(opts) + config = ConfigParser.parse(opts) Configurable.validate!(config) state = %__MODULE__{config: config} diff --git a/lib/pigeon/apns/error.ex b/lib/pigeon/apns/error.ex index cfb27230..16ef153d 100644 --- a/lib/pigeon/apns/error.ex +++ b/lib/pigeon/apns/error.ex @@ -5,16 +5,6 @@ defmodule Pigeon.APNS.Error do alias Pigeon.APNS.Notification - @doc ~S""" - If enabled, logs a notification and its error response. - """ - @spec log(Notification.error_response(), Notification.t()) :: :ok - def log(reason, notification) do - if Pigeon.debug_log?() do - Logger.error("#{reason}: #{msg(reason)}\n#{inspect(notification)}") - end - end - @doc false @spec parse(binary) :: Notification.error_response() def parse(data) do @@ -54,116 +44,4 @@ defmodule Pigeon.APNS.Error do defp parse_response("ServiceUnavailable"), do: :service_unavailable defp parse_response("Shutdown"), do: :shutdown defp parse_response(_), do: :unknown_error - - @doc false - @spec msg(Notification.error_response()) :: String.t() - # 400 - def msg(:bad_collapse_id) do - "The collapse identifier exceeds the maximum allowed size" - end - - def msg(:bad_device_token) do - """ - The specified device token was bad. Verify that the request contains a - valid token and that the token matches the environment. - """ - end - - def msg(:bad_expiration_date), do: "The apns-expiration value is bad." - def msg(:bad_message_id), do: "The apns-id value is bad." - def msg(:bad_priority), do: "The apns-priority value is bad." - def msg(:bad_topic), do: "The apns-topic was invalid." - - def msg(:device_token_not_for_topic) do - "The device token does not match the specified topic." - end - - def msg(:duplicate_headers), do: "One or more headers were repeated." - def msg(:idle_timeout), do: "Idle time out." - - def msg(:missing_device_token) do - """ - The device token is not specified in the request :path. Verify that the - :path header contains the device token. - """ - end - - def msg(:missing_topic) do - """ - The apns-topic header of the request was not specified and was required. - The apns-topic header is mandatory when the client is connected using a - certificate that supports multiple topics. - """ - end - - def msg(:payload_empty), do: "The message payload was empty." - def msg(:topic_disallowed), do: "Pushing to this topic is not allowed." - - # 403 - def msg(:bad_certificate), do: "The certificate was bad." - - def msg(:bad_certificate_environment) do - "The client certificate was for the wrong environment." - end - - def msg(:expired_provider_token) do - "The provider token is stale and a new token should be generated." - end - - def msg(:forbidden) do - "The specified action is not allowed." - end - - def msg(:invalid_provider_token) do - """ - The provider token is not valid or the token signature could not - be verified. - """ - end - - def msg(:missing_provider_token) do - """ - No provider certificate was used to connect to APNs and Authorization - header was missing or no provider token was specified. - """ - end - - # 404 - def msg(:bad_path), do: "The request contained a bad :path value." - - # 405 - def msg(:method_not_allowed), do: "The specified :method was not POST." - - # 410 - def msg(:unregistered) do - "The device token is inactive for the specified topic." - end - - # 413 - def msg(:payload_too_large) do - """ - The message payload was too large. The maximum payload size is - 4096 bytes. - """ - end - - # 429 - def msg(:too_many_provider_token_updates) do - "The provider token is being updated too often." - end - - def msg(:too_many_requests) do - "Too many requests were made consecutively to the same device token." - end - - # 500 - def msg(:internal_server_error), do: "An internal server error occurred." - - # 503 - def msg(:service_unavailable), do: "The service is unavailable." - def msg(:shutdown), do: "The server is shutting down." - - # Misc - def msg(:timeout), do: "The SSL connection timed out." - def msg(_else), do: "" end diff --git a/lib/pigeon/apns/shared.ex b/lib/pigeon/apns/shared.ex index 6c01c710..f20a99a2 100644 --- a/lib/pigeon/apns/shared.ex +++ b/lib/pigeon/apns/shared.ex @@ -56,11 +56,8 @@ defmodule Pigeon.APNS.Shared do |> process_on_response() _error -> - reason = Error.parse(body) - Error.log(reason, notification) - notification - |> Map.put(:response, reason) + |> Map.put(:response, Error.parse(body)) |> process_on_response() end end diff --git a/lib/pigeon/dispatcher.ex b/lib/pigeon/dispatcher.ex index 47922b21..9667d1e6 100644 --- a/lib/pigeon/dispatcher.ex +++ b/lib/pigeon/dispatcher.ex @@ -111,7 +111,6 @@ defmodule Pigeon.Dispatcher do GenServer.start_link(__MODULE__, opts, name: opts[:name]) end - @impl true def init(opts) do case opts[:adapter].init(opts) do {:ok, state} -> @@ -122,15 +121,13 @@ defmodule Pigeon.Dispatcher do end end - @impl true - def handle_cast({:push, notification}, %{adapter: adapter, state: state}) do + def handle_info({:"$push", notification}, %{adapter: adapter, state: state}) do case adapter.handle_push(notification, state) do {:noreply, new_state} -> {:noreply, %{adapter: adapter, state: new_state}} {:stop, reason, new_state} -> {:stop, reason, %{adapter: adapter, state: new_state}} end end - @impl true def handle_info(msg, %{adapter: adapter, state: state}) do case adapter.handle_info(msg, state) do {:noreply, new_state} -> {:noreply, %{adapter: adapter, state: new_state}} diff --git a/lib/pigeon/legacy_fcm/config.ex b/lib/pigeon/legacy_fcm/config.ex index d6eeccab..89da6614 100644 --- a/lib/pigeon/legacy_fcm/config.ex +++ b/lib/pigeon/legacy_fcm/config.ex @@ -109,32 +109,25 @@ defimpl Pigeon.Configurable, for: Pigeon.LegacyFCM.Config do end defp do_handle_end_stream(400, _body, notif) do - log_error("400", "Malformed JSON") - notif |> Map.put(:status, :malformed_json) |> process_on_response() end defp do_handle_end_stream(401, _body, notif) do - log_error("401", "Unauthorized") - notif |> Map.put(:status, :unauthorized) |> process_on_response() end defp do_handle_end_stream(500, _body, notif) do - log_error("500", "Internal server error") - notif |> Map.put(:status, :internal_server_error) |> process_on_response() end - defp do_handle_end_stream(code, body, notif) do + defp do_handle_end_stream(_code, body, notif) do reason = parse_error(body) - log_error(code, reason) notif |> Map.put(:response, reason) @@ -181,8 +174,4 @@ defimpl Pigeon.Configurable, for: Pigeon.LegacyFCM.Config do |> Logger.error() end end - - defp log_error(code, reason) do - if Pigeon.debug_log?(), do: Logger.error("#{reason}: #{code}") - end end diff --git a/test/pigeon_test.exs b/test/pigeon_test.exs new file mode 100644 index 00000000..b3d90eda --- /dev/null +++ b/test/pigeon_test.exs @@ -0,0 +1,61 @@ +defmodule PigeonTest do + use ExUnit.Case + + @notif %Pigeon.FCM.Notification{target: {:token, "test"}} + + test "json_library/0 defaults to Jason" do + assert Pigeon.json_library() == Jason + end + + describe "push/3" do + test "synchronously sends a push by default" do + n = Pigeon.push(PigeonTest.Sandbox, @notif) + assert n.response == :success + end + + test "synchronously sends a list of notifications" do + notifs = Pigeon.push(PigeonTest.Sandbox, [@notif, @notif]) + for n <- notifs, do: assert(n.response == :success) + end + + test "response: :timeout if timed out" do + n = Pigeon.push(PigeonTest.Sandbox, @notif, timeout: 0) + assert n.response == :timeout + + notifs = Pigeon.push(PigeonTest.Sandbox, [@notif, @notif], timeout: 0) + for n <- notifs, do: assert(n.response == :timeout) + end + + test "response: :not_started if pid not alive" do + pid = Process.spawn(& &1, []) + Process.exit(pid, :stop) + n = Pigeon.push(pid, @notif) + assert n.response == :not_started + end + end + + describe "push/3 with on_response" do + test "asynchronously sends a push" do + pid = self() + on_response = fn x -> send(pid, x) end + + assert Pigeon.push(PigeonTest.Sandbox, @notif, on_response: on_response) == :ok + assert_receive(%Pigeon.FCM.Notification{response: :success}, 5_000) + end + + test "asynchronously sends a list of notifications" do + pid = self() + on_response = fn x -> send(pid, x) end + + assert Pigeon.push(PigeonTest.Sandbox, [@notif, @notif], on_response: on_response) == + :ok + + assert_receive(%Pigeon.FCM.Notification{response: :success}, 5_000) + assert_receive(%Pigeon.FCM.Notification{response: :success}, 5_000) + end + + test "blackholes the response if nil" do + assert Pigeon.push(PigeonTest.Sandbox, @notif, on_response: nil) == :ok + end + end +end From 7382237f4a1f582627864d0cede98eb45a926b00 Mon Sep 17 00:00:00 2001 From: Henry Popp Date: Sun, 6 Jun 2021 14:41:56 -0500 Subject: [PATCH 3/5] fix: adm tweaks --- lib/pigeon/adm.ex | 31 +++++++++++-------------------- 1 file changed, 11 insertions(+), 20 deletions(-) diff --git a/lib/pigeon/adm.ex b/lib/pigeon/adm.ex index 903c186e..479811c7 100644 --- a/lib/pigeon/adm.ex +++ b/lib/pigeon/adm.ex @@ -286,27 +286,18 @@ defmodule Pigeon.ADM do defp do_push(notification, state) do request = {notification.registration_id, encode_payload(notification)} - response = - case notification.__meta__.on_response do - nil -> - fn {reg_id, payload} -> - HTTPoison.post(adm_uri(reg_id), payload, adm_headers(state)) - end - - _ -> - fn {reg_id, payload} -> - case HTTPoison.post(adm_uri(reg_id), payload, adm_headers(state)) do - {:ok, %HTTPoison.Response{status_code: status, body: body}} -> - notification = %{notification | registration_id: reg_id} - process_response(status, body, notification) - - {:error, %HTTPoison.Error{reason: :connect_timeout}} -> - notification - |> Map.put(:response, :timeout) - |> process_on_response() - end - end + response = fn {reg_id, payload} -> + case HTTPoison.post(adm_uri(reg_id), payload, adm_headers(state)) do + {:ok, %HTTPoison.Response{status_code: status, body: body}} -> + notification = %{notification | registration_id: reg_id} + process_response(status, body, notification) + + {:error, %HTTPoison.Error{reason: :connect_timeout}} -> + notification + |> Map.put(:response, :timeout) + |> process_on_response() end + end Task.Supervisor.start_child(Pigeon.Tasks, fn -> response.(request) end) :ok From b5181f5b88018f260ae149e74f912230f9655179 Mon Sep 17 00:00:00 2001 From: Henry Popp Date: Sun, 6 Jun 2021 22:01:18 -0500 Subject: [PATCH 4/5] feat: dispatcher pooling Also: - on_response supports {m, f} and {m, f, a} tuples - Formatted to 80 character line length --- .formatter.exs | 2 +- .gitignore | 4 ++ lib/pigeon.ex | 41 +++++++++++------ lib/pigeon/adapter.ex | 4 +- lib/pigeon/apns/config.ex | 4 -- lib/pigeon/apns/config_parser.ex | 9 +++- lib/pigeon/apns/error.ex | 35 +++++++++++++- lib/pigeon/apns/jwt_config.ex | 6 ++- lib/pigeon/apns/shared.ex | 6 --- lib/pigeon/application.ex | 1 + lib/pigeon/dispatcher.ex | 36 ++++++--------- lib/pigeon/dispatcher_worker.ex | 41 +++++++++++++++++ lib/pigeon/fcm.ex | 7 ++- lib/pigeon/fcm/config.ex | 48 +++++++++++--------- lib/pigeon/fcm/notification.ex | 12 +++-- lib/pigeon/http2/client.ex | 6 ++- lib/pigeon/legacy_fcm/result_parser.ex | 20 +++++++- lib/pigeon/metadata.ex | 10 ++-- lib/pigeon/registry.ex | 28 ++++++++++++ lib/pigeon/tasks.ex | 17 ++++++- mix.exs | 11 +++-- test/pigeon/fcm_test.exs | 8 +++- test/pigeon/legacy_fcm/notification_test.exs | 6 ++- test/pigeon_test.exs | 42 +++++++++++++---- 24 files changed, 297 insertions(+), 107 deletions(-) create mode 100644 lib/pigeon/dispatcher_worker.ex create mode 100644 lib/pigeon/registry.ex diff --git a/.formatter.exs b/.formatter.exs index 899581ae..4a7f4bb0 100644 --- a/.formatter.exs +++ b/.formatter.exs @@ -1,4 +1,4 @@ [ inputs: ["{mix,.formatter}.exs", "{config,lib,test}/**/*.{ex,exs}"], - line_length: 90 + line_length: 80 ] diff --git a/.gitignore b/.gitignore index dadc11fe..10f04855 100644 --- a/.gitignore +++ b/.gitignore @@ -25,6 +25,10 @@ pigeon-*.tar # Temporary files for e.g. tests. /tmp +# Ignore dialyzer files +/priv/plts/*.plt +/priv/plts/*.plt.hash + # Misc. cert.pem key_unencrypted.pem diff --git a/lib/pigeon.ex b/lib/pigeon.ex index 5948e8ef..d06e2439 100644 --- a/lib/pigeon.ex +++ b/lib/pigeon.ex @@ -48,7 +48,12 @@ defmodule Pigeon do n = Pigeon.ADM.Notification.new("token", %{"message" => "test"}) Pigeon.ADM.push(n, on_response: handler) """ - @type on_response :: (Notification.t() -> no_return) + @type on_response :: + (notification -> no_return) + | {module, atom} + | {module, atom, [any]} + + @type notification :: %{__meta__: Pigeon.Metadata.t()} @typedoc ~S""" Options for sending push notifications. @@ -62,6 +67,17 @@ defmodule Pigeon do timeout: non_neg_integer ] + @doc """ + Returns the configured default pool size for Pigeon dispatchers. + To customize this value, include the following in your config/config.exs: + + config :pigeon, :default_pool_size, 5 + """ + @spec default_pool_size :: pos_integer + def default_pool_size() do + Application.get_env(:pigeon, :default_pool_size, 5) + end + @doc """ Returns the configured JSON encoding library for Pigeon. To customize the JSON library, include the following in your config/config.exs: @@ -82,7 +98,12 @@ defmodule Pigeon do def push(pid, notifications, opts) when is_list(notifications) do if Keyword.has_key?(opts, :on_response) do on_response = Keyword.get(opts, :on_response) - notifications = Enum.map(notifications, fn n -> put_on_response(n, on_response) end) + + notifications = + Enum.map(notifications, fn n -> + put_on_response(n, on_response) + end) + push_async(pid, notifications) else timeout = Keyword.get(opts, :timeout, @default_timeout) @@ -94,7 +115,9 @@ defmodule Pigeon do timeout = Keyword.get(opts, :timeout, @default_timeout) if Keyword.has_key?(opts, :on_response) do - notification = put_on_response(notification, Keyword.get(opts, :on_response)) + notification = + put_on_response(notification, Keyword.get(opts, :on_response)) + push_async(pid, notification) else push_sync(pid, notification, timeout) @@ -121,20 +144,12 @@ defmodule Pigeon do :ok end - defp push_async(pid, notification) when is_pid(pid) do - if Process.alive?(pid) do - send_push(pid, notification) - else - Tasks.process_on_response(%{notification | response: :not_started}) - end - end - defp push_async(pid, notification) do - case Process.whereis(pid) do + case Pigeon.Registry.next(pid) do nil -> Tasks.process_on_response(%{notification | response: :not_started}) - _pid -> + pid -> send_push(pid, notification) end end diff --git a/lib/pigeon/adapter.ex b/lib/pigeon/adapter.ex index 47637058..84295e06 100644 --- a/lib/pigeon/adapter.ex +++ b/lib/pigeon/adapter.ex @@ -41,9 +41,9 @@ defmodule Pigeon.Adapter do Invoked when the server is started. Return value should be `{:ok, state}` for the `Pigeon.Dispatcher` state, - or `{:error, atom}` if started with invalid configuration options. + or `{:stop, atom}` if started with invalid configuration options. """ - @callback init(opts :: Keyword.t()) :: {:ok, term} | {:error, atom} + @callback init(opts :: Keyword.t()) :: {:ok, any} | {:stop, any} @doc """ Invoked to handle all other messages. diff --git a/lib/pigeon/apns/config.ex b/lib/pigeon/apns/config.ex index 3d0f3e60..33b38519 100644 --- a/lib/pigeon/apns/config.ex +++ b/lib/pigeon/apns/config.ex @@ -118,10 +118,6 @@ defimpl Pigeon.Configurable, for: Pigeon.APNS.Config do # Configurable Callbacks - defdelegate worker_name(any), to: Shared - - defdelegate max_demand(any), to: Shared - @spec connect(any) :: {:ok, sock} | {:error, String.t()} def connect(%{uri: uri} = config) do uri = to_charlist(uri) diff --git a/lib/pigeon/apns/config_parser.ex b/lib/pigeon/apns/config_parser.ex index 46adf5de..e6948486 100644 --- a/lib/pigeon/apns/config_parser.ex +++ b/lib/pigeon/apns/config_parser.ex @@ -24,8 +24,13 @@ defmodule Pigeon.APNS.ConfigParser do @spec parse(atom | config_opts) :: config | {:error, :invalid_config} def parse(opts) when is_list(opts) do case config_type(Enum.into(opts, %{})) do - :error -> raise Pigeon.ConfigError, reason: "configuration is invalid", config: opts - type -> type.new(opts) + :error -> + raise Pigeon.ConfigError, + reason: "configuration is invalid", + config: opts + + type -> + type.new(opts) end end diff --git a/lib/pigeon/apns/error.ex b/lib/pigeon/apns/error.ex index 16ef153d..956cee74 100644 --- a/lib/pigeon/apns/error.ex +++ b/lib/pigeon/apns/error.ex @@ -15,33 +15,64 @@ defmodule Pigeon.APNS.Error do end defp parse_response("BadCollapseId"), do: :bad_collapse_id + defp parse_response("BadDeviceToken"), do: :bad_device_token + defp parse_response("BadExpirationDate"), do: :bad_expiration_date + defp parse_response("BadMessageId"), do: :bad_message_id + defp parse_response("BadPriority"), do: :bad_priority + defp parse_response("BadTopic"), do: :bad_topic + defp parse_response("DeviceTokenNotForTopic"), do: :device_token_not_for_topic + defp parse_response("DuplicateHeaders"), do: :duplicate_headers + defp parse_response("IdleTimeout"), do: :idle_timeout + defp parse_response("InvalidPushType"), do: :invalid_push_type + defp parse_response("MissingDeviceToken"), do: :missing_device_token + defp parse_response("MissingTopic"), do: :missing_topic + defp parse_response("PayloadEmpty"), do: :payload_empty + defp parse_response("TopicDisallowed"), do: :topic_disallowed + defp parse_response("BadCertificate"), do: :bad_certificate - defp parse_response("BadCertificateEnvironment"), do: :bad_certificate_environment + + defp parse_response("BadCertificateEnvironment"), + do: :bad_certificate_environment + defp parse_response("ExpiredProviderToken"), do: :expired_provider_token + defp parse_response("Forbidden"), do: :forbidden + defp parse_response("InvalidProviderToken"), do: :invalid_provider_token + defp parse_response("MissingProviderToken"), do: :missing_provider_token + defp parse_response("BadPath"), do: :bad_path + defp parse_response("MethodNotAllowed"), do: :method_not_allowed + defp parse_response("Unregistered"), do: :unregistered + defp parse_response("PayloadTooLarge"), do: :payload_too_large - defp parse_response("TooManyProviderTokenUpdates"), do: :too_many_provider_token_updates + + defp parse_response("TooManyProviderTokenUpdates"), + do: :too_many_provider_token_updates + defp parse_response("TooManyRequests"), do: :too_many_requests + defp parse_response("InternalServerError"), do: :internal_server_error + defp parse_response("ServiceUnavailable"), do: :service_unavailable + defp parse_response("Shutdown"), do: :shutdown + defp parse_response(_), do: :unknown_error end diff --git a/lib/pigeon/apns/jwt_config.ex b/lib/pigeon/apns/jwt_config.ex index f81891ba..cd2dea57 100644 --- a/lib/pigeon/apns/jwt_config.ex +++ b/lib/pigeon/apns/jwt_config.ex @@ -136,7 +136,8 @@ defimpl Pigeon.Configurable, for: Pigeon.APNS.JWTConfig do Pigeon.Http2.Client.default().connect(uri, :https, options) end - @spec push_headers(JWTConfig.t(), Notification.t(), Keyword.t()) :: headers | no_return + @spec push_headers(JWTConfig.t(), Notification.t(), Keyword.t()) :: + headers | no_return def push_headers(%JWTConfig{} = config, notification, opts) do config |> Shared.push_headers(notification, opts) @@ -205,7 +206,8 @@ defimpl Pigeon.Configurable, for: Pigeon.APNS.JWTConfig do key = %{"pem" => config.key} now = :os.system_time(:seconds) - signer = Joken.Signer.create("ES256", key, %{"kid" => config.key_identifier}) + signer = + Joken.Signer.create("ES256", key, %{"kid" => config.key_identifier}) {:ok, token, _claims} = default_claims(iss: config.team_id, iat: now) diff --git a/lib/pigeon/apns/shared.ex b/lib/pigeon/apns/shared.ex index f20a99a2..b916d52e 100644 --- a/lib/pigeon/apns/shared.ex +++ b/lib/pigeon/apns/shared.ex @@ -17,12 +17,6 @@ defmodule Pigeon.APNS.Shared do @apns_expiration "apns-expiration" @apns_collapse_id "apns-collapse-id" - @spec worker_name(any) :: atom | nil - def worker_name(%{name: name}), do: name - - @spec max_demand(any) :: non_neg_integer - def max_demand(_config), do: 1_000 - @spec push_headers(config, Notification.t(), opts) :: headers() def push_headers(_config, notification, _opts) do json = Pigeon.json_library().encode!(notification.payload) diff --git a/lib/pigeon/application.ex b/lib/pigeon/application.ex index a4cce5de..f03b0669 100644 --- a/lib/pigeon/application.ex +++ b/lib/pigeon/application.ex @@ -10,6 +10,7 @@ defmodule Pigeon.Application do Client.default().start children = [ + Pigeon.Registry, {APNS.Token, %{}}, {Task.Supervisor, name: Pigeon.Tasks} ] diff --git a/lib/pigeon/dispatcher.ex b/lib/pigeon/dispatcher.ex index 9667d1e6..d41f73b6 100644 --- a/lib/pigeon/dispatcher.ex +++ b/lib/pigeon/dispatcher.ex @@ -75,7 +75,7 @@ defmodule Pigeon.Dispatcher do ``` """ - use GenServer + use Supervisor @doc false defmacro __using__(opts) do @@ -86,7 +86,7 @@ defmodule Pigeon.Dispatcher do config_opts = Application.get_env(@otp_app, __MODULE__, []) opts = - [name: __MODULE__] + [name: __MODULE__, pool_size: Pigeon.default_pool_size()] |> Keyword.merge(config_opts) |> Keyword.merge(opts) @@ -108,30 +108,20 @@ defmodule Pigeon.Dispatcher do def start_link(opts) do opts[:adapter] || raise "adapter is not specified" - GenServer.start_link(__MODULE__, opts, name: opts[:name]) + Supervisor.start_link(__MODULE__, opts, name: opts[:name]) end def init(opts) do - case opts[:adapter].init(opts) do - {:ok, state} -> - {:ok, %{adapter: opts[:adapter], state: state}} - - {:error, reason} -> - {:error, reason} - end - end - - def handle_info({:"$push", notification}, %{adapter: adapter, state: state}) do - case adapter.handle_push(notification, state) do - {:noreply, new_state} -> {:noreply, %{adapter: adapter, state: new_state}} - {:stop, reason, new_state} -> {:stop, reason, %{adapter: adapter, state: new_state}} - end - end + opts = + opts + |> Keyword.put(:supervisor, opts[:name] || self()) + |> Keyword.delete(:name) + + children = + for index <- 1..(opts[:pool_size] || Pigeon.default_pool_size()) do + Supervisor.child_spec({Pigeon.DispatcherWorker, opts}, id: index) + end - def handle_info(msg, %{adapter: adapter, state: state}) do - case adapter.handle_info(msg, state) do - {:noreply, new_state} -> {:noreply, %{adapter: adapter, state: new_state}} - {:stop, reason, new_state} -> {:stop, reason, %{adapter: adapter, state: new_state}} - end + Supervisor.init(children, strategy: :one_for_one) end end diff --git a/lib/pigeon/dispatcher_worker.ex b/lib/pigeon/dispatcher_worker.ex new file mode 100644 index 00000000..b208f7e1 --- /dev/null +++ b/lib/pigeon/dispatcher_worker.ex @@ -0,0 +1,41 @@ +defmodule Pigeon.DispatcherWorker do + @moduledoc false + + use GenServer + + def start_link(opts) do + opts[:adapter] || raise "adapter is not specified" + GenServer.start_link(__MODULE__, opts) + end + + def init(opts) do + case opts[:adapter].init(opts) do + {:ok, state} -> + Pigeon.Registry.register(opts[:supervisor]) + {:ok, %{adapter: opts[:adapter], state: state}} + + {:error, reason} -> + {:error, reason} + end + end + + def handle_info({:"$push", notification}, %{adapter: adapter, state: state}) do + case adapter.handle_push(notification, state) do + {:noreply, new_state} -> + {:noreply, %{adapter: adapter, state: new_state}} + + {:stop, reason, new_state} -> + {:stop, reason, %{adapter: adapter, state: new_state}} + end + end + + def handle_info(msg, %{adapter: adapter, state: state}) do + case adapter.handle_info(msg, state) do + {:noreply, new_state} -> + {:noreply, %{adapter: adapter, state: new_state}} + + {:stop, reason, new_state} -> + {:stop, reason, %{adapter: adapter, state: new_state}} + end + end +end diff --git a/lib/pigeon/fcm.ex b/lib/pigeon/fcm.ex index 04cb9349..33ac67a0 100644 --- a/lib/pigeon/fcm.ex +++ b/lib/pigeon/fcm.ex @@ -129,7 +129,8 @@ defmodule Pigeon.FCM do end @impl true - def handle_push(notification, %{config: config, queue: queue, token: token} = state) do + def handle_push(notification, state) do + %{config: config, queue: queue, token: token} = state headers = Configurable.push_headers(config, notification, token: token) payload = Configurable.push_payload(config, notification, []) @@ -175,7 +176,9 @@ defmodule Pigeon.FCM do Process.send_after(self(), @refresh, @retry_after) {:noreply, %{state | retries: state.retries - 1}} else - raise "too many failed attempts to refresh, last error: #{inspect(exception)}" + raise "too many failed attempts to refresh, last error: #{ + inspect(exception) + }" end end end diff --git a/lib/pigeon/fcm/config.ex b/lib/pigeon/fcm/config.ex index 3924a970..72b5717d 100644 --- a/lib/pigeon/fcm/config.ex +++ b/lib/pigeon/fcm/config.ex @@ -30,10 +30,20 @@ defmodule Pigeon.FCM.Config do } """ def new(opts) when is_list(opts) do + project_id = + opts + |> Keyword.get(:project_id) + |> decode_bin() + + service_account_json = + opts + |> Keyword.get(:service_account_json) + |> decode_json() + %__MODULE__{ port: Keyword.get(opts, :port, 443), - project_id: opts |> Keyword.get(:project_id) |> decode_bin(), - service_account_json: opts |> Keyword.get(:service_account_json) |> decode_json(), + project_id: project_id, + service_account_json: service_account_json, uri: Keyword.get(opts, :uri, 'fcm.googleapis.com') } end @@ -65,7 +75,7 @@ defimpl Pigeon.Configurable, for: Pigeon.FCM.Config do import Pigeon.Tasks, only: [process_on_response: 1] - alias Pigeon.{Encodable, Metadata} + alias Pigeon.Encodable alias Pigeon.FCM.{Config, Error} @type sock :: {:sslsocket, any, pid | {any, any}} @@ -116,24 +126,20 @@ defimpl Pigeon.Configurable, for: Pigeon.FCM.Config do end def handle_end_stream(_config, %{error: nil} = stream, notif) do - if Metadata.on_response?(notif) do - stream.body - |> Pigeon.json_library().decode!() - |> case do - %{"name" => name} -> - notif - |> Map.put(:name, name) - |> Map.put(:response, :success) - |> process_on_response() - - %{"error" => error} -> - notif - |> Map.put(:error, error) - |> Map.put(:response, Error.parse(error)) - |> process_on_response() - end - else - :ok + stream.body + |> Pigeon.json_library().decode!() + |> case do + %{"name" => name} -> + notif + |> Map.put(:name, name) + |> Map.put(:response, :success) + |> process_on_response() + + %{"error" => error} -> + notif + |> Map.put(:error, error) + |> Map.put(:response, Error.parse(error)) + |> process_on_response() end end diff --git a/lib/pigeon/fcm/notification.ex b/lib/pigeon/fcm/notification.ex index cf227fe5..89bd65cb 100644 --- a/lib/pigeon/fcm/notification.ex +++ b/lib/pigeon/fcm/notification.ex @@ -44,8 +44,10 @@ defmodule Pigeon.FCM.Notification do FCM notification target. Must be one of the following: - `{:token, "string"}` - Registration token to send a message to. - - `{:topic, "string"}` - Topic name to send a message to, e.g. "weather". Note: "/topics/" prefix should not be provided. - - `{:condition, "string"}` - Condition to send a message to, e.g. "'foo' in topics && 'bar' in topics". + - `{:topic, "string"}` - Topic name to send a message to, e.g. "weather". + Note: "/topics/" prefix should not be provided. + - `{:condition, "string"}` - Condition to send a message to, e.g. "'foo' + in topics && 'bar' in topics". """ @type target :: {:token, binary} | {:topic, binary} | {:condition, binary} @@ -88,7 +90,11 @@ defmodule Pigeon.FCM.Notification do def new({type, _} = target, notification, data) when type in [:token, :topic, :condition] do - %Pigeon.FCM.Notification{target: target, notification: notification, data: data} + %Pigeon.FCM.Notification{ + target: target, + notification: notification, + data: data + } end end diff --git a/lib/pigeon/http2/client.ex b/lib/pigeon/http2/client.ex index ab185847..1054e76b 100644 --- a/lib/pigeon/http2/client.ex +++ b/lib/pigeon/http2/client.ex @@ -24,7 +24,11 @@ defmodule Pigeon.Http2.Client do @callback send_ping(pid) :: :ok - @callback send_request(pid, headers :: [{binary, binary}, ...], data :: String.t()) :: + @callback send_request( + pid, + headers :: [{binary, binary}, ...], + data :: String.t() + ) :: :ok @callback handle_end_stream(msg :: term, state :: term) :: diff --git a/lib/pigeon/legacy_fcm/result_parser.ex b/lib/pigeon/legacy_fcm/result_parser.ex index e97527b4..73598d99 100644 --- a/lib/pigeon/legacy_fcm/result_parser.ex +++ b/lib/pigeon/legacy_fcm/result_parser.ex @@ -45,19 +45,35 @@ defmodule Pigeon.LegacyFCM.ResultParser do %{notif | response: [{error, regid} | resp]} end - def parse_error("DeviceMessageRateExceeded"), do: :device_message_rate_exceeded + def parse_error("DeviceMessageRateExceeded"), + do: :device_message_rate_exceeded + def parse_error("InternalServerError"), do: :internal_server_error + def parse_error("InvalidApnsCredential"), do: :invalid_apns_credential + def parse_error("InvalidDataKey"), do: :invalid_data_key + def parse_error("InvalidPackageName"), do: :invalid_package_name + def parse_error("InvalidParameters"), do: :invalid_parameters + def parse_error("InvalidRegistration"), do: :invalid_registration + def parse_error("InvalidTtl"), do: :invalid_ttl + def parse_error("MessageTooBig"), do: :message_too_big + def parse_error("MissingRegistration"), do: :missing_registration + def parse_error("MismatchSenderId"), do: :mismatch_sender_id + def parse_error("NotRegistered"), do: :not_registered - def parse_error("TopicsMessageRateExceeded"), do: :topics_message_rate_exceeded + + def parse_error("TopicsMessageRateExceeded"), + do: :topics_message_rate_exceeded + def parse_error("Unavailable"), do: :unavailable + def parse_error(_), do: :unknown_error end diff --git a/lib/pigeon/metadata.ex b/lib/pigeon/metadata.ex index 5621c641..3a80aec2 100644 --- a/lib/pigeon/metadata.ex +++ b/lib/pigeon/metadata.ex @@ -1,7 +1,9 @@ defmodule Pigeon.Metadata do - defstruct on_response: nil + @moduledoc false + + @type t :: %__MODULE__{ + on_response: Pigeon.on_response() + } - def on_response?(notification) do - notification.__meta__.on_response != nil - end + defstruct on_response: nil end diff --git a/lib/pigeon/registry.ex b/lib/pigeon/registry.ex new file mode 100644 index 00000000..64f76f73 --- /dev/null +++ b/lib/pigeon/registry.ex @@ -0,0 +1,28 @@ +defmodule Pigeon.Registry do + @moduledoc false + + def child_spec(_opts \\ []) do + %{ + id: __MODULE__, + start: {Registry, :start_link, [[keys: :duplicate, name: __MODULE__]]}, + type: :supervisor + } + end + + def register(pid) do + Registry.register(__MODULE__, pid, nil) + end + + def unregister(pid) do + Registry.unregister(__MODULE__, pid) + end + + def next(pid) do + __MODULE__ + |> Registry.lookup(pid) + |> case do + [] -> nil + pids -> pids |> Enum.random() |> elem(0) + end + end +end diff --git a/lib/pigeon/tasks.ex b/lib/pigeon/tasks.ex index 0fc8b5b9..0819549e 100644 --- a/lib/pigeon/tasks.ex +++ b/lib/pigeon/tasks.ex @@ -3,7 +3,20 @@ defmodule Pigeon.Tasks do def process_on_response(%{__meta__: %{on_response: nil}}), do: :ok - def process_on_response(%{__meta__: %{on_response: on_response}} = notif) do - Task.Supervisor.start_child(Pigeon.Tasks, fn -> on_response.(notif) end) + def process_on_response(%{__meta__: %{on_response: {m, f}}} = notif) do + Task.Supervisor.start_child(Pigeon.Tasks, fn -> + :erlang.apply(m, f, [notif]) + end) + end + + def process_on_response(%{__meta__: %{on_response: {m, f, a}}} = notif) do + Task.Supervisor.start_child(Pigeon.Tasks, fn -> + :erlang.apply(m, f, [notif] ++ a) + end) + end + + def process_on_response(%{__meta__: %{on_response: fun}} = notif) + when is_function(fun, 1) do + Task.Supervisor.start_child(Pigeon.Tasks, fn -> fun.(notif) end) end end diff --git a/mix.exs b/mix.exs index 95782c29..b3778fd5 100644 --- a/mix.exs +++ b/mix.exs @@ -10,10 +10,7 @@ defmodule Pigeon.Mixfile do build_embedded: Mix.env() == :prod, deps: deps(), description: description(), - dialyzer: [ - plt_add_apps: [:kadabra], - ignore_warnings: "config/dialyzer.ignore-warnings" - ], + dialyzer: dialyzer(), docs: docs(), elixir: "~> 1.6", elixirc_options: [warnings_as_errors: true], @@ -93,4 +90,10 @@ defmodule Pigeon.Mixfile do maintainers: ["Henry Popp", "Tyler Hurst"] ] end + + defp dialyzer do + [ + plt_file: {:no_warn, "priv/plts/dialyzer.plt"} + ] + end end diff --git a/test/pigeon/fcm_test.exs b/test/pigeon/fcm_test.exs index cfdfe209..98f7617f 100644 --- a/test/pigeon/fcm_test.exs +++ b/test/pigeon/fcm_test.exs @@ -58,7 +58,9 @@ defmodule Pigeon.FCMTest do pid = self() {:ok, dispatcher} = - Pigeon.Dispatcher.start_link(Application.get_env(:pigeon, PigeonTest.FCM)) + Pigeon.Dispatcher.start_link( + Application.get_env(:pigeon, PigeonTest.FCM) + ) Pigeon.push(dispatcher, n, on_response: fn x -> send(pid, x) end) @@ -84,7 +86,9 @@ defmodule Pigeon.FCMTest do n = Notification.new(target, %{}, @data) pid = self() - Pigeon.push(PigeonTest.NotStarted, n, on_response: fn x -> send(pid, x) end) + Pigeon.push(PigeonTest.NotStarted, n, + on_response: fn x -> send(pid, x) end + ) assert_receive(n = %Notification{target: ^target}, 5000) refute n.name diff --git a/test/pigeon/legacy_fcm/notification_test.exs b/test/pigeon/legacy_fcm/notification_test.exs index 6b8085f9..5c2fcb14 100644 --- a/test/pigeon/legacy_fcm/notification_test.exs +++ b/test/pigeon/legacy_fcm/notification_test.exs @@ -48,7 +48,8 @@ defmodule Pigeon.LegacyFCM.NotificationTest do payload: %{"data" => data} } - assert Pigeon.LegacyFCM.Notification.new(@reg_id, %{}, data) == expected_result + assert Pigeon.LegacyFCM.Notification.new(@reg_id, %{}, data) == + expected_result end test "LegacyFCM new with notification and data maps" do @@ -67,6 +68,7 @@ defmodule Pigeon.LegacyFCM.NotificationTest do payload: %{"notification" => n, "data" => data} } - assert Pigeon.LegacyFCM.Notification.new(@reg_id, n, data) == expected_result + assert Pigeon.LegacyFCM.Notification.new(@reg_id, n, data) == + expected_result end end diff --git a/test/pigeon_test.exs b/test/pigeon_test.exs index b3d90eda..4e0b2d50 100644 --- a/test/pigeon_test.exs +++ b/test/pigeon_test.exs @@ -1,35 +1,39 @@ defmodule PigeonTest do use ExUnit.Case - @notif %Pigeon.FCM.Notification{target: {:token, "test"}} + @n %Pigeon.FCM.Notification{target: {:token, "test"}} test "json_library/0 defaults to Jason" do assert Pigeon.json_library() == Jason end + test "default_pool_size/0 defaults to 5" do + assert Pigeon.default_pool_size() == 5 + end + describe "push/3" do test "synchronously sends a push by default" do - n = Pigeon.push(PigeonTest.Sandbox, @notif) + n = Pigeon.push(PigeonTest.Sandbox, @n) assert n.response == :success end test "synchronously sends a list of notifications" do - notifs = Pigeon.push(PigeonTest.Sandbox, [@notif, @notif]) + notifs = Pigeon.push(PigeonTest.Sandbox, [@n, @n]) for n <- notifs, do: assert(n.response == :success) end test "response: :timeout if timed out" do - n = Pigeon.push(PigeonTest.Sandbox, @notif, timeout: 0) + n = Pigeon.push(PigeonTest.Sandbox, @n, timeout: 0) assert n.response == :timeout - notifs = Pigeon.push(PigeonTest.Sandbox, [@notif, @notif], timeout: 0) + notifs = Pigeon.push(PigeonTest.Sandbox, [@n, @n], timeout: 0) for n <- notifs, do: assert(n.response == :timeout) end test "response: :not_started if pid not alive" do pid = Process.spawn(& &1, []) Process.exit(pid, :stop) - n = Pigeon.push(pid, @notif) + n = Pigeon.push(pid, @n) assert n.response == :not_started end end @@ -39,7 +43,9 @@ defmodule PigeonTest do pid = self() on_response = fn x -> send(pid, x) end - assert Pigeon.push(PigeonTest.Sandbox, @notif, on_response: on_response) == :ok + assert Pigeon.push(PigeonTest.Sandbox, @n, on_response: on_response) == + :ok + assert_receive(%Pigeon.FCM.Notification{response: :success}, 5_000) end @@ -47,7 +53,7 @@ defmodule PigeonTest do pid = self() on_response = fn x -> send(pid, x) end - assert Pigeon.push(PigeonTest.Sandbox, [@notif, @notif], on_response: on_response) == + assert Pigeon.push(PigeonTest.Sandbox, [@n, @n], on_response: on_response) == :ok assert_receive(%Pigeon.FCM.Notification{response: :success}, 5_000) @@ -55,7 +61,25 @@ defmodule PigeonTest do end test "blackholes the response if nil" do - assert Pigeon.push(PigeonTest.Sandbox, @notif, on_response: nil) == :ok + assert Pigeon.push(PigeonTest.Sandbox, @n, on_response: nil) == :ok + end + + test "accepts {m, f} tuple" do + on_response = {PigeonTest, :assert_success} + + assert Pigeon.push(PigeonTest.Sandbox, @n, on_response: on_response) == + :ok end + + test "accepts {m, f, a} tuple" do + on_response = {PigeonTest, :assert_success, [:other_data]} + + assert Pigeon.push(PigeonTest.Sandbox, @n, on_response: on_response) == + :ok + end + end + + def assert_success(notification, _opts \\ []) do + assert notification.response == :success end end From 8b4b148b92ecc4d4728991bcb75bdad4ed3d248f Mon Sep 17 00:00:00 2001 From: Henry Popp Date: Sun, 6 Jun 2021 22:27:59 -0500 Subject: [PATCH 5/5] refactor: simplified Pigeon.push/3 --- lib/pigeon.ex | 35 ++++++----------------------------- test/pigeon_test.exs | 2 +- 2 files changed, 7 insertions(+), 30 deletions(-) diff --git a/lib/pigeon.ex b/lib/pigeon.ex index d06e2439..178f97b4 100644 --- a/lib/pigeon.ex +++ b/lib/pigeon.ex @@ -96,30 +96,16 @@ defmodule Pigeon do def push(pid, notifications, opts \\ []) def push(pid, notifications, opts) when is_list(notifications) do - if Keyword.has_key?(opts, :on_response) do - on_response = Keyword.get(opts, :on_response) - - notifications = - Enum.map(notifications, fn n -> - put_on_response(n, on_response) - end) - - push_async(pid, notifications) - else - timeout = Keyword.get(opts, :timeout, @default_timeout) - for n <- notifications, do: push_sync(pid, n, timeout), into: [] - end + for n <- notifications, do: push(pid, n, opts) end def push(pid, notification, opts) do - timeout = Keyword.get(opts, :timeout, @default_timeout) - if Keyword.has_key?(opts, :on_response) do - notification = - put_on_response(notification, Keyword.get(opts, :on_response)) - + on_response = Keyword.get(opts, :on_response) + notification = put_on_response(notification, on_response) push_async(pid, notification) else + timeout = Keyword.get(opts, :timeout, @default_timeout) push_sync(pid, notification, timeout) end end @@ -139,26 +125,17 @@ defmodule Pigeon do end end - defp push_async(pid, notifications) when is_list(notifications) do - for n <- notifications, do: push_async(pid, n) - :ok - end - defp push_async(pid, notification) do case Pigeon.Registry.next(pid) do nil -> Tasks.process_on_response(%{notification | response: :not_started}) pid -> - send_push(pid, notification) + send(pid, {:"$push", notification}) + :ok end end - defp send_push(pid, notification) do - send(pid, {:"$push", notification}) - :ok - end - defp put_on_response(notification, on_response) do meta = %{notification.__meta__ | on_response: on_response} %{notification | __meta__: meta} diff --git a/test/pigeon_test.exs b/test/pigeon_test.exs index 4e0b2d50..2fae4992 100644 --- a/test/pigeon_test.exs +++ b/test/pigeon_test.exs @@ -54,7 +54,7 @@ defmodule PigeonTest do on_response = fn x -> send(pid, x) end assert Pigeon.push(PigeonTest.Sandbox, [@n, @n], on_response: on_response) == - :ok + [:ok, :ok] assert_receive(%Pigeon.FCM.Notification{response: :success}, 5_000) assert_receive(%Pigeon.FCM.Notification{response: :success}, 5_000)