Avoid job duplication by Github webhooks (#34)

* Avoid job duplication by Github webhooks

- closes #29

Signed-off-by: Tallys Martins <[email protected]>

* Apply changes from first review

Signed-off-by: Tallys Martins <[email protected]>

* Apply review changes

* Small fix in WebhooksControllerTest

Author: tallysmartins

lib/elixir_bench/benchmarks/benchmarks.ex

@@ -51,6 +51,16 @@ defmodule ElixirBench.Benchmarks do
     Repo.fetch(where(Job, uuid: ^uuid))
   end
 
+  defp fetch_job_by_repo_and_commit(repo, commit_sha) do
+    query =
+      Job
+      |> Job.filter_by_repo(repo.id)
+      |> where(commit_sha: ^commit_sha)
+      |> last()
+
+    Repo.fetch(query)
+  end
+
   def list_benchmarks_by_repo_id(repo_ids) do
     Repo.all(from(b in Benchmark, where: b.repo_id in ^repo_ids))
   end
@@ -63,6 +73,16 @@ defmodule ElixirBench.Benchmarks do
     Repo.all(Job)
   end
 
+  def get_or_create_job(repo, %{commit_sha: commit_sha} = attrs) do
+    case fetch_job_by_repo_and_commit(repo, commit_sha) do
+      {:ok, job} ->
+        {:ok, job}
+
+      {:error, :not_found} ->
+        create_job(repo, attrs)
+    end
+  end
+
   def create_job(repo, attrs) do
     changeset = Job.create_changeset(%Job{repo_id: repo.id}, attrs)
 

lib/elixir_bench/benchmarks/job.ex

@@ -2,6 +2,7 @@ defmodule ElixirBench.Benchmarks.Job do
   use Ecto.Schema
 
   import Ecto.Changeset
+  import Ecto.Query, only: [from: 2]
 
   alias ElixirBench.Repos
   alias ElixirBench.Benchmarks.{Runner, Job, Config}
@@ -66,4 +67,8 @@ defmodule ElixirBench.Benchmarks.Job do
     |> validate_required(@submit_fields)
     |> put_change(:completed_at, DateTime.utc_now())
   end
+
+  def filter_by_repo(query, repo_id) do
+    from(j in query, where: j.repo_id == ^repo_id)
+  end
 end

lib/elixir_bench_web/controllers/github/webhooks_controller.ex

@@ -45,7 +45,7 @@ defmodule ElixirBenchWeb.Github.WebHooks do
 
     with {:ok, %Repos.Repo{} = repo} <- Repos.fetch_repo_by_slug(slug),
          {:ok, job} <-
-           Benchmarks.create_job(repo, %{branch_name: branch_name, commit_sha: commit_sha}) do
+           Benchmarks.get_or_create_job(repo, %{branch_name: branch_name, commit_sha: commit_sha}) do
       conn
       |> render(ElixirBenchWeb.JobView, "show.json", job: job, repo: repo)
     else

test/elixir_bench/benchmarks/benchmarks_test.exs

@@ -101,15 +101,78 @@ defmodule ElixirBench.BenchmarksTest do
     end
   end
 
+  describe "get_or_create_job/2" do
+    test "insert new jobs if no job exist for a given commit-sha and repo" do
+      repo = insert(:repo)
+
+      assert_difference(Job, 2) do
+        assert {:ok, job} =
+                 Benchmarks.get_or_create_job(repo, %{
+                   branch_name: "master",
+                   commit_sha: "ABC123"
+                 })
+
+        assert {:ok, _other_job} =
+                 Benchmarks.get_or_create_job(repo, %{
+                   branch_name: "master",
+                   commit_sha: "dingdong"
+                 })
+      end
+
+      assert %Job{branch_name: "master", commit_sha: "ABC123"} = job
+    end
+
+    test "return last existent job for a given commit-sha and repo" do
+      repo = insert(:repo)
+      attrs = %{repo: repo, branch_name: "master", commit_sha: "ABC123"}
+
+      %{id: id} = insert(:job, attrs)
+      %{id: id2} = insert(:job, attrs)
+
+      refute id == id2
+
+      assert_difference(Job, 0) do
+        assert {:ok, job} = Benchmarks.get_or_create_job(repo, attrs)
+      end
+
+      assert %{id: ^id2, commit_sha: "ABC123"} = job
+    end
+
+    test "insert new job if exists a job with same commit-sha but its from another repo" do
+      repo = insert(:repo)
+      other_repo = insert(:repo)
+
+      assert_difference(Job, 2) do
+        assert {:ok, job} =
+                 Benchmarks.get_or_create_job(repo, %{
+                   branch_name: "master",
+                   commit_sha: "ABC123"
+                 })
+
+        assert {:ok, other_job} =
+                 Benchmarks.get_or_create_job(other_repo, %{
+                   branch_name: "master",
+                   commit_sha: "ABC123"
+                 })
+      end
+
+      assert %Job{branch_name: "master", commit_sha: "ABC123"} = job
+      assert %Job{branch_name: "master", commit_sha: "ABC123"} = other_job
+    end
+  end
+
   describe "create_job/2" do
-    test "insert new Job given correct params" do
+    test "insert new job given correct params" do
       repo = insert(:repo)
 
       assert_difference(Job, 1) do
         assert {:ok, job} =
-                 Benchmarks.create_job(repo, %{branch_name: "mm/benchee", commit_sha: "ABC123"})
+                 Benchmarks.create_job(repo, %{
+                   branch_name: "master",
+                   commit_sha: "ABC123"
+                 })
 
-        assert %Job{branch_name: "mm/benchee", commit_sha: "ABC123"} = job
+        assert %Job{branch_name: "master", commit_sha: "ABC123"} = job
       end
     end
 
@@ -171,7 +234,7 @@ defmodule ElixirBench.BenchmarksTest do
       assert @samples_count == ElixirBench.Repo.aggregate(query, :count, :id)
     end
 
-    test "do not duplicata benchmark in multiple runs" do
+    test "do not duplicate benchmark in multiple runs" do
       job = insert(:job)
 
       assert_difference(Benchmarks.Benchmark, 4) do

test/elixir_bench_web/controllers/github/webhooks_controller_test.exs

@@ -47,21 +47,9 @@ defmodule ElixirBenchWeb.Github.WebHooksControllerTest do
 
       payload = push_payload() |> Jason.decode!()
 
-      refs_head = %{payload | "ref" => "refs/heads/mybranch"}
       empty_string = %{payload | "ref" => ""}
       nil_value = %{payload | "ref" => nil}
-
-      assert_difference(Repo, 0) do
-        assert_difference(Job, 1) do
-          {:ok, %{"data" => data}} =
-            context.conn
-            |> set_headers("push")
-            |> post("/hooks/handle", %{"payload" => Jason.encode!(refs_head)})
-            |> decode_response_body
-        end
-      end
-
-      assert %{"branch_name" => "mybranch", "repo_slug" => "baxterthehacker/public-repo"} = data
+      refs_head = %{payload | "ref" => "refs/heads/mybranch"}
 
       {:ok, %{"errors" => errors}} =
         context.conn
@@ -78,6 +66,18 @@ defmodule ElixirBenchWeb.Github.WebHooksControllerTest do
         |> decode_response_body
 
       assert %{"branch_name" => ["can't be blank"]} = errors
+
+      assert_difference(Repo, 0) do
+        assert_difference(Job, 1) do
+          {:ok, %{"data" => data}} =
+            context.conn
+            |> set_headers("push")
+            |> post("/hooks/handle", %{"payload" => Jason.encode!(refs_head)})
+            |> decode_response_body
+        end
+      end
+
+      assert %{"branch_name" => "mybranch", "repo_slug" => "baxterthehacker/public-repo"} = data
     end
 
     test "respond to ping event", context do
@@ -90,6 +90,30 @@ defmodule ElixirBenchWeb.Github.WebHooksControllerTest do
       assert 200 = response.status
     end
 
+    # This scenario happens when there is a branch with an open pull request
+    # So Github sends requests for both events, pull request and push.
+    test "not duplicate job for same branch and commit references", context do
+      insert(:repo, @github_repo_attrs)
+      push_params = %{"payload" => push_payload()}
+      pull_request = %{"payload" => pull_request_payload()}
+
+      assert_difference(Job, 1) do
+        {:ok, %{"data" => data}} =
+          context.conn
+          |> set_headers("pull_request")
+          |> post("/hooks/handle", pull_request)
+          |> decode_response_body
+
+        {:ok, %{"data" => ^data}} =
+          context.conn
+          |> set_headers("push")
+          |> post("/hooks/handle", push_params)
+          |> decode_response_body
+      end
+
+      assert %{"branch_name" => "changes", "repo_slug" => "baxterthehacker/public-repo"} = data
+    end
+
     test "not break given unexpected pull request event payload scheme", context do
       insert(:repo, @github_repo_attrs)
       params = %{"payload" => ~s({"data": "some data"})}