Initial implementation #1

rslota · 2017-02-09T11:38:31Z

This PR introduces... well, this project's implementation. This is, I would say, the first draft. It works and it has integration tests (100% coverage, and I hope it will stay this way). There are also some docs (mostly README.md) and several handy integrations (travis, credo, coveralls.io etc.).

Even though there is Dockerfile on my TODO list, I think I'll skip it and we may add it later on. It's not needed, just very convenient.

TODO:

tests (100% coverage :))
coveralls.io integration
dialyxir integration
credo integration (linter)
travis build integration (coverage + dialyzer + code linter)
Code docs / specs
Write README.md
Dockerfile
Change SVG bages in README.md to point to master branch just before merging

michalwski

This is simple, indeed :) I have some comments to this PR.

Also in a following PR I think we should add some metrics counting number of requests from clients, number of failed and succeeded requests to FCM and APNS. Elapsed time measurements for the whole request and specific requests to providers may prove useful on production as well.

michalwski · 2017-02-13T11:13:41Z

config/prod.exs

+        using: :path
+    ],
+    https: [
+        ip: {127, 0, 0, 1},


How about {0, 0, 0, 0} by default?

I don't think its a good idea. In many cases this project will be used on same host as MongooseIM. Also if somebody uses it on different host, he will notice that he has to change configuration because REST API won't work without it. But with {0, 0, 0, 0} user may not notice that his service is open for attacks, because it will always work this way, even if such configuration is not needed.

Makes sense, thanks for the explanation.

michalwski · 2017-02-13T11:18:27Z

mix.exs

@@ -29,6 +42,19 @@ defmodule MongoosePush.Mixfile do
  #
  # Type "mix help deps" for more examples and options
  defp deps do
-    []
+    [
+     {:pigeon, git: "https://github.com/rslota/pigeon.git", tag: "6d1e4e3"},


I see rslota/pigeon is a fork from cstar/pigeon. Are you @rslota going to send a PR to upstream?

I can try, but there are some changes that the author may not like. Author of this project had much different plan when comes to pooling.

Try to convince him :)

michalwski · 2017-02-13T11:20:23Z

priv/apns/dev_cert.pem

@@ -0,0 +1,23 @@
+-----BEGIN CERTIFICATE-----


Can all these *.pem files be generated on demand? For example when the project is first compiled or run?

They can be generated but it adds one and only one additional step to building the project. If I leave those example certs in repo, there not need to have openssl while running the project and there not need for Makefile.

I believe openssl is required in order to provide HTTPs from Elixir node. This probably means that openssl will have to be installed anyway. My biggest concern is with such files included in the repo, users may start using them without regenerating. For such a simple project which most probably we'll be used internally fresh certs and keys may not be mission critical. It is still sth which lights a warning in my brain, though.

Ok, you can now generate those certs via mix certs.dev.

michalwski · 2017-02-13T11:22:55Z

tools/travis-setup.sh

+# Skip this setup for jobs that don't run exunit
+test "${PRESET}" == "exunit" || exit 0
+
+docker run -d -p 2197:2197 mobify/apns-http2-mock-server


Any chance for a specific version here and in the next container?

We want the latest. Always. If the API changes we need to adapt, because our code needs to be compatible with newest APNS / FCM API.

Fair enough :)

coveralls · 2017-02-14T12:01:31Z

Changes Unknown when pulling 1dae070 on initial_implementation into ** on master**.

mentels

Will continue the review from lib/mongoose_push/api/v1.ex tomorrow.

mentels · 2017-02-15T15:52:40Z

README.md

+
+#### Perquisites
+
+* Elixir (http://elixir-lang.org/install.html)


Which version of Elixir?

mentels · 2017-02-15T15:53:26Z

README.md

+
+## Configuration
+
+The whole configuration is contained in file `config/{prod|dev|test}.exs` depending on with `MIX_ENV` you will be using. You should use `MIX_ENV=prod` for production installations and `MIX_ENV=dev` for you development. Anyway, lets take a look on `config/prod.exs`, part by part.  


on witch -> on which

for you development -> ?

mentels · 2017-02-15T15:55:09Z

README.md

+After this step you may try to run the service via:
+```bash
+_build/prod/rel/mongoose_push/bin/mongoose_push console
+```


If we want to be serious, we really need to support releases. I won't accept an OS PR that has on support for releases :)

Could you elaborate? What do you mean by support for releases exactly? This README.md shows how to run this project from the release.

Damn...how could I have missed that? Sorry, of course you do support releases with distillery. Awesome :)

mentels · 2017-02-15T15:56:52Z

README.md

-Documentation can be generated with [ExDoc](https://github.com/elixir-lang/ex_doc)
-and published on [HexDocs](https://hexdocs.pm). Once published, the docs can
-be found at [https://hexdocs.pm/mongoose_push](https://hexdocs.pm/mongoose_push).
+The first level keyword list is a pool definition. You may have several named pools of different sizes and with different configurations. Currently the only reason you may want to do this is that, the `REST` client may switch between them by specifying matching `:mode` in their push request.


"The first level keyword list is a pool definition." - this is not very understandable. Maybe start with "to setup a pool...".

mentels · 2017-02-15T16:02:36Z

lib/mix/task_certs_dev.ex

+  use Mix.Task
+
+  @shortdoc "Generate fake certs (placeholders) for HTTPS endpoint and APNS"
+  @moduledoc """


@moduledoc is usually put at the very top.

mentels · 2017-02-15T16:05:22Z

lib/mix/task_certs_dev.ex

+  end
+
+  defp maybe_gen_cert(cert_file, key_file, common_name) do
+    unless File.exists?(cert_file) and File.exists?(key_file) do


This condition is not readable. If I need to think what it means it has to be structured differently. One option would be:

case {File.exists?(cert_file), File.exists?(key_file)} do {false, false} -> ... _ -> ... end

I think that readability is very subjective, especially in this case (huh, case :)). This case pattern may be easier to read for erlang devs since in erlang this is the only sane way to write such condition. Elixir makes it easier and in my opinion more readable, so I wanted to exploit that.

But anyway, this is so minor that I'm just gonna change it, I don't really like unless that much :)

Maybe you can simply write if File.exists? ... and ... do ok else do_stuff() end

mentels · 2017-02-15T16:12:51Z

lib/mongoose_push.ex

-  @doc """
-  Hello world.
+  require Logger
+  import MongoosePush.Application


No, I really discourage these import. Because of that it takes you time to find functions like pools_by_name

Thats true, alias seems to be a better idea.

mentels · 2017-02-15T16:18:02Z

lib/mongoose_push.ex

+  @spec push(String.t, request) :: :ok | {:error, term}
+  def push(device_id, %{:service => service} = request) do
+      mode = Map.get(request, :mode, :prod)
+      [pool | _] = pools_by_mode(service, mode)


It looks like the functions on lines 44 and 45 are consructing a worker name. For me this level of abstraction doesn't belong to this function.

mentels · 2017-02-15T16:19:06Z

lib/mongoose_push.ex

+
+  @spec push(service, atom | pid, String.t, request) :: :ok | {:error, term}
+  defp push(:apns, worker, device_id, request) do
+    raw_notification = prepare_notification(:apns, request)


This type of code can be rewritten as:

raw_notification = prepare_notification(:apns, request) |> APNS.Notification.new(device_id, request[:topic]) |> APNS.push([name: worker]) |> normalize_response(:apns, device_id)

Yeah, I did write it this way, but credo does not agree with you. Its says that each pipe operator has to start with variable, not expression.

mentels · 2017-02-15T16:22:04Z

lib/mongoose_push.ex

+      "category" => request[:click_action]
+    }
+  end
+


For me, this module should not distinguish between the two types of the backends further than :fcm and :apns. In other words, I believe that the two deserve their own modules like MongoosePush.FCM and MongoosePush.APNS and the specifics of these services should sit in them. Maybe the next PR would address that but I simply don't like it.

coveralls · 2017-02-15T20:09:11Z

Changes Unknown when pulling 4443b46bd7f8ab43fb0e87daa7d0023fff0d4199 on initial_implementation into ** on master**.

coveralls · 2017-02-15T20:26:06Z

Changes Unknown when pulling a4a88a5 on initial_implementation into ** on master**.

coveralls · 2017-02-15T20:37:48Z

Changes Unknown when pulling 2dbe39d on initial_implementation into ** on master**.

mentels · 2017-02-16T08:12:57Z

lib/mongoose_push/api/v1.ex

+          {:error, reason} ->
+            conn
+            |> put_status(500)
+            |> json(%{:details => inspect reason})


The module is beautiful. Simple and readable. One issue though, the inspect from the Kernel module is not to be used to format data that you return to the client. You have to format you data differently, without inspect.

mentels · 2017-02-16T08:23:49Z

lib/mongoose_push/application.ex

+    config = env(service)
+
+    config
+    |> Enum.group_by(&(mode(elem(&1, 1))), &(elem &1, 0))


This is not really readable.

mentels · 2017-02-16T08:25:05Z

lib/mongoose_push/application.ex

+    end)
+
+    workers
+    |> List.flatten


Instead of flattening the list afterwards, you could use Enum.flat_map/2.

mentels · 2017-02-16T08:26:59Z

lib/mongoose_push/router.ex

+          true  -> :error
+          false -> :info
+        end
+    Logger.log log_level, inspect e


Again, you shouldn't be using inspect to format your output data.

mentels · 2017-02-16T08:27:22Z

lib/mongoose_push/service.ex

@@ -0,0 +1,9 @@
+defmodule MongoosePush.Service do


mentels · 2017-02-16T08:47:41Z

test/rest_v1_test.exs

+
+  defp get(path) do
+    %Response{status_code: status_code} =
+      HTTPoison.get!("https://localhost:8443" <> path, [], hackney: [:insecure])


Why are you using the insecure hackney option?

Because this is connection to HTTPS endpoint that is running on self-signed certs generated for tests. Without this option connection would fail due to unknown_ca error.

mentels · 2017-02-16T08:49:20Z

lib/mix/task_certs_dev.ex

+  end
+
+  defp maybe_gen_cert(cert_file, key_file, common_name) do
+    unless File.exists?(cert_file) and File.exists?(key_file) do


Maybe you can simply write if File.exists? ... and ... do ok else do_stuff() end

mentels · 2017-02-16T08:51:12Z

lib/mongoose_push.ex

-  @doc """
-  Hello world.
+  require Logger
+  alias MongoosePush.Application


I don't like this alias. It's misleading as at first glance you don't know if it refers to Elixir.Application or to Elixir.MngoosePush.Application.

Ok, you're right.

mentels · 2017-02-16T08:56:38Z

lib/mongoose_push/application.ex

@@ -4,19 +4,126 @@ defmodule MongoosePush.Application do
  @moduledoc false


For me, the whole pools management doesn't belong to this module. If would prefer just having it separated out to sth like MongoosePush.Pools which would provide some API to get workers names to start then under supervisors and then the other one to select a woker. Even better, I believe, is a solution in which you have a separate supervisors for each type of pool and it knows more details and the the "pools" module that know even more details.

Pools have been refactored. Please review new implementation :)

mentels · 2017-02-16T08:57:01Z

lib/mongoose_push/application.ex

+      pool_config = translate_worker_config(:fcm, pool_config)
+
+      Enum.map(1..pool_size(:fcm, pool_name), fn(id) ->
+        worker_name = worker_name(:fcm, pool_name, id)


To deep... doesn't credo complain about that?

No it does not. This is only 2 level in, so I guess its maximum allowed.

coveralls · 2017-02-16T12:19:15Z

Changes Unknown when pulling ea9383e42f2b47bb95f0e5746f6ee004c361f651 on initial_implementation into ** on master**.

coveralls · 2017-02-16T12:35:34Z

Changes Unknown when pulling fdef967 on initial_implementation into ** on master**.

coveralls · 2017-02-16T13:40:56Z

Changes Unknown when pulling fdef967 on initial_implementation into ** on master**.

mentels · 2017-02-17T10:45:16Z

lib/mongoose_push/pools.ex

@@ -0,0 +1,44 @@
+defmodule MongoosePush.Pools do


Now this is awesome

mentels · 2017-02-17T10:48:28Z

test/mongoose_push_application_test.exs

+  setup do
+    # Validate config/text.exs that is need for this test suite
+    apns_pools = Keyword.keys(Application.get_env(:mongoose_push, :apns))
+    4 = length(apns_pools)


That could be reduced to [:prod1, :prod2, :dev1, :dev2] = apns_pools

But is so minor that you don't have to bother yourself.

coveralls · 2017-02-17T12:58:05Z

Changes Unknown when pulling aa07594 on initial_implementation into ** on master**.

rslota added 3 commits February 9, 2017 12:35

Initial working draft

e74b8c8

Add tests

629fc8a

Add even more tests

4a11b02

rslota force-pushed the initial_implementation branch 2 times, most recently from fd2316c to b2865c3 Compare February 9, 2017 14:13

Add .travis.yml

edfd3b7

rslota force-pushed the initial_implementation branch from fc57e8e to 502aa2a Compare February 9, 2017 14:37

rslota added 4 commits February 9, 2017 15:40

Fix files reverted by rebase

502aa2a

Add coveralls.io badge

29c75c1

Add travis-ci badge

596675e

Add credo static code analysis tool

948098d

rslota force-pushed the initial_implementation branch 3 times, most recently from cd2fe3c to 5cf4941 Compare February 9, 2017 15:51

rslota added 6 commits February 9, 2017 16:54

Add dialyzer

5cf4941

Fix all credo warnings

ead5325

Swagger integration

f1a38fb

Update README.md

2de6d39

Add missing file

a84344a

Update mock docker repository name

e684083

michalwski reviewed Feb 13, 2017

View reviewed changes

rslota force-pushed the initial_implementation branch from ad7818b to 2c61d6b Compare February 14, 2017 11:00

Remove fake certs from repo

2c61d6b

rslota mentioned this pull request Feb 14, 2017

Push Notification For P2P and MUC esl/MongooseIM#902

Closed

rslota force-pushed the initial_implementation branch from 545f104 to 1dae070 Compare February 14, 2017 11:45

Fix credo warnings

1dae070

mentels reviewed Feb 15, 2017

View reviewed changes

rslota force-pushed the initial_implementation branch from 4443b46 to a4a88a5 Compare February 15, 2017 20:10

rslota added 2 commits February 15, 2017 21:14

Several documentation fixes

a4a88a5

Add MongoosePush.Service behaviour

2dbe39d

mentels reviewed Feb 16, 2017

View reviewed changes

rslota force-pushed the initial_implementation branch from 823f545 to ea9383e Compare February 16, 2017 12:02

rslota force-pushed the initial_implementation branch from ea9383e to fdef967 Compare February 16, 2017 12:21

Refactor pool handling

fdef967

mentels approved these changes Feb 17, 2017

View reviewed changes

Minor test setup improvement

aa07594

michalwski merged commit 35b33c0 into master Feb 17, 2017

michalwski deleted the initial_implementation branch February 17, 2017 13:04

Mo33n mentioned this pull request Oct 12, 2017

The specified device token was bad. Verify that the request contains a valid token and that the token matches the environment. #45

Closed


		#### Perquisites

		* Elixir (http://elixir-lang.org/install.html)


		## Configuration

		The whole configuration is contained in file `config/{prod\|dev\|test}.exs` depending on with `MIX_ENV` you will be using. You should use `MIX_ENV=prod` for production installations and `MIX_ENV=dev` for you development. Anyway, lets take a look on `config/prod.exs`, part by part.

		@@ -4,19 +4,126 @@ defmodule MongoosePush.Application do
		@moduledoc false

Initial implementation #1

Initial implementation #1

Conversation

rslota commented Feb 9, 2017 • edited

michalwski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Feb 14, 2017

mentels left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Feb 15, 2017

coveralls commented Feb 15, 2017

coveralls commented Feb 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Feb 16, 2017

coveralls commented Feb 16, 2017

coveralls commented Feb 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Feb 17, 2017

rslota commented Feb 9, 2017 •

edited