Merge pull request #3507 from segiddins/segiddins/webhook-notify-job

Send webhook notifications through an ActiveJob

app/controllers/api/base_controller.rb

@@ -23,7 +23,6 @@ def enqueue_web_hook_jobs(version)
       job.fire(
         request.protocol.delete("://"),
         request.host_with_port,
-        version.rubygem,
         version
       )
     end

app/controllers/api/v1/web_hooks_controller.rb

@@ -32,7 +32,7 @@ def fire
     webhook = @api_key.user.web_hooks.new(url: @url)
     @rubygem ||= Rubygem.find_by_name("gemcutter")
 
-    if webhook.fire(request.protocol.delete("://"), request.host_with_port, @rubygem,
+    if webhook.fire(request.protocol.delete("://"), request.host_with_port,
       @rubygem.versions.most_recent, delayed: false)
       render plain: webhook.deployed_message(@rubygem)
     else

app/jobs/notify_web_hook_job.rb

@@ -0,0 +1,50 @@
+class NotifyWebHookJob < ApplicationJob
+  extend StatsD::Instrument
+  include TraceTagger
+
+  queue_as :default
+  self.queue_adapter = :good_job
+
+  before_perform { @kwargs = arguments.last.then { _1 if Hash.ruby2_keywords_hash?(_1) } }
+  before_perform { @webhook = @kwargs.fetch(:webhook) }
+  before_perform { @protocol = @kwargs.fetch(:protocol) }
+  before_perform { @host_with_port = @kwargs.fetch(:host_with_port) }
+  before_perform { @version = @kwargs.fetch(:version) }
+  before_perform { @rubygem = @version.rubygem }
+
+  attr_reader :webhook, :protocol, :host_with_port, :version, :rubygem
+
+  def perform(*)
+    url = webhook.url
+    set_tag "gemcutter.notifier.url", url
+    set_tag "gemcutter.notifier.webhook_id", webhook.id
+
+    timeout(5) do
+      RestClient.post url,
+        payload,
+        :timeout        => 5,
+        :open_timeout   => 5,
+        "Content-Type"  => "application/json",
+        "Authorization" => authorization
+    end
+    true
+  rescue *(HTTP_ERRORS + [RestClient::Exception, SocketError, SystemCallError]) => _e
+    webhook.increment! :failure_count
+    false
+  end
+  statsd_count_success :perform, "Webhook.perform"
+
+  def payload
+    rubygem.payload(version, protocol, host_with_port).to_json
+  end
+
+  def authorization
+    Digest::SHA2.hexdigest([rubygem.name, version.number, webhook.api_key].compact.join)
+  end
+
+  private
+
+  def timeout(sec, &)
+    Timeout.timeout(sec, &)
+  end
+end

app/models/web_hook.rb

@@ -18,13 +18,13 @@ def self.specific
     where.not(rubygem_id: nil)
   end
 
-  def fire(protocol, host_with_port, deploy_gem, version, delayed: true)
-    job = Notifier.new(url, protocol, host_with_port, deploy_gem, version, api_key)
+  def fire(protocol, host_with_port, version, delayed: true)
+    job = NotifyWebHookJob.new(webhook: self, protocol:, host_with_port:, version:)
 
     if delayed
-      Delayed::Job.enqueue job, priority: PRIORITIES[:web_hook]
+      job.enqueue
     else
-      job.perform
+      job.perform_now
     end
   end
 

test/functional/api/v1/deletions_controller_test.rb

@@ -1,6 +1,8 @@
 require "test_helper"
 
 class Api::V1::DeletionsControllerTest < ActionController::TestCase
+  include ActiveJob::TestHelper
+
   context "with yank rubygem api key scope" do
     setup do
       @api_key = create(:api_key, key: "12345", yank_rubygem: true)
@@ -385,7 +387,7 @@ class Api::V1::DeletionsControllerTest < ActionController::TestCase
                                         number: @v1.number).first
         end
         should "have enqueued a webhook" do
-          assert_instance_of Notifier, Delayed::Job.last.payload_object
+          assert_enqueued_jobs 1, only: NotifyWebHookJob
         end
       end
 

test/functional/api/v1/rubygems_controller_test.rb

@@ -312,9 +312,11 @@ def self.should_respond_to(format)
           assert_equal "Successfully registered gem: test (1.0.0)", @response.body
         end
         should "enqueue jobs" do
-          assert_difference "Delayed::Job.count", 4 do
+          assert_difference "Delayed::Job.count", 3 do
             assert_enqueued_jobs 4, only: FastlyPurgeJob do
-              post :create, body: gem_file("test-1.0.0.gem").read
+              assert_enqueued_jobs 1, only: NotifyWebHookJob do
+                post :create, body: gem_file("test-1.0.0.gem").read
+              end
             end
           end
         end

test/jobs/notify_web_hook_job_test.rb

@@ -0,0 +1,40 @@
+require "test_helper"
+
+class NotifyWebHookJobTest < ActiveJob::TestCase
+  context "with a rubygem and version" do
+    setup do
+      @rubygem = create(:rubygem, name: "foogem", downloads: 42)
+      @version = create(:version,
+        rubygem: @rubygem,
+        number: "3.2.1",
+        authors: %w[AUTHORS],
+        description: "DESC")
+      @hook    = create(:web_hook, rubygem: @rubygem)
+      @job     = NotifyWebHookJob.new(webhook: @hook, protocol: "http", host_with_port: "localhost:1234", version: @version)
+    end
+
+    should "have gem properties encoded in JSON" do
+      payload = @job.run_callbacks(:perform) { JSON.parse(@job.payload) }
+      assert_equal "foogem",    payload["name"]
+      assert_equal "3.2.1",     payload["version"]
+      assert_equal "ruby",      payload["platform"]
+      assert_equal "DESC",      payload["info"]
+      assert_equal "AUTHORS",   payload["authors"]
+      assert_equal 42,          payload["downloads"]
+      assert_equal "http://localhost:1234/gems/foogem", payload["project_uri"]
+      assert_equal "http://localhost:1234/gems/foogem-3.2.1.gem", payload["gem_uri"]
+    end
+
+    should "send the right version out even for older gems" do
+      new_version = create(:version, number: "2.0.0", rubygem: @rubygem)
+      new_hook    = create(:web_hook)
+      job         = NotifyWebHookJob.new(webhook: new_hook, protocol: "http", host_with_port: "localhost:1234", version: new_version)
+      payload     = job.run_callbacks(:perform) { JSON.parse(job.payload) }
+
+      assert_equal "foogem", payload["name"]
+      assert_equal "2.0.0",  payload["version"]
+      assert_equal "http://localhost:1234/gems/foogem", payload["project_uri"]
+      assert_equal "http://localhost:1234/gems/foogem-2.0.0.gem", payload["gem_uri"]
+    end
+  end
+end

test/models/web_hook_test.rb

@@ -137,43 +137,6 @@ class WebHookTest < ActiveSupport::TestCase
     end
   end
 
-  context "with a rubygem and version" do
-    setup do
-      @rubygem = create(:rubygem, name: "foogem", downloads: 42)
-      @version = create(:version,
-        rubygem: @rubygem,
-        number: "3.2.1",
-        authors: %w[AUTHORS],
-        description: "DESC")
-      @hook    = create(:web_hook, rubygem: @rubygem)
-      @job     = Notifier.new(@hook.url, "http", "localhost:1234", @rubygem, @version)
-    end
-
-    should "have gem properties encoded in JSON" do
-      payload = JSON.load(@job.payload)
-      assert_equal "foogem",    payload["name"]
-      assert_equal "3.2.1",     payload["version"]
-      assert_equal "ruby",      payload["platform"]
-      assert_equal "DESC",      payload["info"]
-      assert_equal "AUTHORS",   payload["authors"]
-      assert_equal 42,          payload["downloads"]
-      assert_equal "http://localhost:1234/gems/foogem", payload["project_uri"]
-      assert_equal "http://localhost:1234/gems/foogem-3.2.1.gem", payload["gem_uri"]
-    end
-
-    should "send the right version out even for older gems" do
-      new_version = create(:version, number: "2.0.0", rubygem: @rubygem)
-      new_hook    = create(:web_hook)
-      job         = Notifier.new(new_hook.url, "http", "localhost:1234", @rubygem, new_version)
-      payload     = JSON.load(job.payload)
-
-      assert_equal "foogem", payload["name"]
-      assert_equal "2.0.0",  payload["version"]
-      assert_equal "http://localhost:1234/gems/foogem", payload["project_uri"]
-      assert_equal "http://localhost:1234/gems/foogem-2.0.0.gem", payload["gem_uri"]
-    end
-  end
-
   context "with a non-global hook job" do
     setup do
       @url     = "http://example.com/gemcutter"
@@ -186,15 +149,15 @@ class WebHookTest < ActiveSupport::TestCase
       authorization = Digest::SHA2.hexdigest(@rubygem.name + @version.number + @hook.user.api_key)
       RestClient.expects(:post).with(anything, anything, has_entries("Authorization" => authorization))
 
-      @hook.fire("https", "rubygems.org", @rubygem, @version, delayed: false)
+      @hook.fire("https", "rubygems.org", @version)
     end
 
     should "include an Authorization header for a user with no API key" do
       @hook.user.update(api_key: nil)
       authorization = Digest::SHA2.hexdigest(@rubygem.name + @version.number)
       RestClient.expects(:post).with(anything, anything, has_entries("Authorization" => authorization))
 
-      @hook.fire("https", "rubygems.org", @rubygem, @version, delayed: false)
+      @hook.fire("https", "rubygems.org", @version)
     end
 
     should "include an Authorization header for a user with many API keys" do
@@ -203,11 +166,11 @@ class WebHookTest < ActiveSupport::TestCase
       authorization = Digest::SHA2.hexdigest(@rubygem.name + @version.number + @hook.user.api_keys.first.hashed_key)
       RestClient.expects(:post).with(anything, anything, has_entries("Authorization" => authorization))
 
-      @hook.fire("https", "rubygems.org", @rubygem, @version, delayed: false)
+      @hook.fire("https", "rubygems.org", @version)
     end
 
     should "not increment failure count for hook" do
-      @hook.fire("https", "rubygems.org", @rubygem, @version, delayed: false)
+      @hook.fire("https", "rubygems.org", @version)
 
       assert_predicate @hook.failure_count, :zero?
     end
@@ -233,7 +196,7 @@ class WebHookTest < ActiveSupport::TestCase
        Net::ProtocolError].each_with_index do |exception, index|
         RestClient.stubs(:post).raises(exception)
 
-        @hook.fire("https", "rubygems.org", @rubygem, @version, delayed: false)
+        @hook.fire("https", "rubygems.org", @version)
 
         assert_equal index + 1, @hook.reload.failure_count
         assert_predicate @hook, :global?