Validate each PagerDuty events separately

Validating events separately allows discarding only invalid messages
instead of the whole payload.

lib/pager_duty/validator/schemas/incident_trigger.json

-{
-  "type": "object",
-  "required": ["messages"],
-  "properties": {
-    "messages": {
-      "type": "array",
-      "items": {
-        "type": "object",
-        "required": ["event", "incident"],
-        "properties": {
-          "event": {
-            "type": "string",
-            "enum": [
-              "incident.trigger",
-              "incident.acknowledge",
-              "incident.unacknowledge",
-              "incident.resolve",
-              "incident.assign",
-              "incident.escalate",
-              "incident.delegate",
-              "incident.annotate"
-            ]
-          },
-          "incident": {
-            "type": "object",
-            "required": [
-              "html_url",
-              "incident_number",
-              "title",
-              "status",
-              "created_at",
-              "urgency",
-              "incident_key"
-            ],
-            "properties": {
-              "html_url": { "type": "string" },
-              "incindent_number": { "type": "integer" },
-              "title": { "type": "string" },
-              "status": { "type": "string" },
-              "created_at": { "type": "string" },
-              "urgency": { "type": "string", "enum": ["high", "low"] },
-              "incident_key": { "type": ["string", "null"] },
-              "assignments": {
-                "type": "array",
-                "items": {
-                  "assignee": {
-                    "type": "array",
-                    "items": {
-                      "summary": { "type": "string" },
-                      "html_url": { "type": "string" }
-                    }
-                  }
-                }
-              },
-              "impacted_services": {
-                "type": "array",
-                "items": {
-                  "summary": { "type": "string" },
-                  "html_url": { "type": "string" }
-                }
-              }
-            }
-          }
-        }
-      }
-    }
-  }
-}

lib/pager_duty/validator/schemas/message.json

+{
+  "type": "object",
+  "required": ["event", "incident"],
+  "properties": {
+    "event": { "type": "string" },
+    "incident": {
+      "type": "object",
+      "required": [
+        "html_url",
+        "incident_number",
+        "title",
+        "status",
+        "created_at",
+        "urgency",
+        "incident_key"
+      ],
+      "properties": {
+        "html_url": { "type": "string" },
+        "incindent_number": { "type": "integer" },
+        "title": { "type": "string" },
+        "status": { "type": "string" },
+        "created_at": { "type": "string" },
+        "urgency": { "type": "string", "enum": ["high", "low"] },
+        "incident_key": { "type": ["string", "null"] },
+        "assignments": {
+          "type": "array",
+          "items": {
+            "assignee": {
+              "type": "array",
+              "items": {
+                "summary": { "type": "string" },
+                "html_url": { "type": "string" }
+              }
+            }
+          }
+        },
+        "impacted_services": {
+          "type": "array",
+          "items": {
+            "summary": { "type": "string" },
+            "html_url": { "type": "string" }
+          }
+        }
+      }
+    }
+  }
+}

lib/pager_duty/webhook_payload_parser.rb

@@ -2,7 +2,7 @@
 
 module PagerDuty
   class WebhookPayloadParser
-    SCHEMA_PATH = File.join('lib', 'pager_duty', 'validator', 'schemas', 'incident_trigger.json')
+    SCHEMA_PATH = File.join('lib', 'pager_duty', 'validator', 'schemas', 'message.json')
 
     def initialize(payload)
       @payload = payload
@@ -13,7 +13,7 @@ module PagerDuty
     end
 
     def call
-      Array(payload['messages']).map { |msg| parse_message(msg) }
+      Array(payload['messages']).map { |msg| parse_message(msg) }.reject(&:empty?)
     end
 
     private
@@ -21,7 +21,7 @@ module PagerDuty
     attr_reader :payload
 
     def parse_message(message)
-      return {} unless valid_payload?
+      return {} unless valid_message?(message)
 
       {
         'event' => message['event'],
@@ -65,8 +65,8 @@ module PagerDuty
       Array(entities).reject { |e| e['summary'].blank? && e['url'].blank? }
     end
 
-    def valid_payload?
-      ::JSONSchemer.schema(Pathname.new(SCHEMA_PATH)).valid?(payload)
+    def valid_message?(message)
+      ::JSONSchemer.schema(Pathname.new(SCHEMA_PATH)).valid?(message)
     end
   end
 end

spec/lib/pager_duty/webhook_payload_parser_spec.rb

@@ -9,36 +9,36 @@ RSpec.describe PagerDuty::WebhookPayloadParser do
       File.read(File.join(File.dirname(__FILE__), '../../fixtures/pager_duty/webhook_incident_trigger.json'))
     end
 
+    let(:triggered_event) do
+      {
+        'event' => 'incident.trigger',
+        'incident' => {
+          'url' => 'https://webdemo.pagerduty.com/incidents/PRORDTY',
+          'incident_number' => 33,
+          'title' => 'My new incident',
+          'status' => 'triggered',
+          'created_at' => '2017-09-26T15:14:36Z',
+          'urgency' => 'high',
+          'incident_key' => nil,
+          'assignees' => [{
+            'summary' => 'Laura Haley',
+            'url' => 'https://webdemo.pagerduty.com/users/P553OPV'
+          }],
+          'impacted_services' => [{
+            'summary' => 'Production XDB Cluster',
+            'url' => 'https://webdemo.pagerduty.com/services/PN49J75'
+          }]
+        }
+      }
+    end
+
     subject(:parse) { described_class.call(payload) }
 
     context 'when payload is a correct PagerDuty payload' do
       let(:payload) { Gitlab::Json.parse(fixture_file) }
 
       it 'returns parsed payload' do
-        is_expected.to eq(
-          [
-            {
-              'event' => 'incident.trigger',
-              'incident' => {
-                'url' => 'https://webdemo.pagerduty.com/incidents/PRORDTY',
-                'incident_number' => 33,
-                'title' => 'My new incident',
-                'status' => 'triggered',
-                'created_at' => '2017-09-26T15:14:36Z',
-                'urgency' => 'high',
-                'incident_key' => nil,
-                'assignees' => [{
-                  'summary' => 'Laura Haley',
-                  'url' => 'https://webdemo.pagerduty.com/users/P553OPV'
-                }],
-                'impacted_services' => [{
-                  'summary' => 'Production XDB Cluster',
-                  'url' => 'https://webdemo.pagerduty.com/services/PN49J75'
-                }]
-              }
-            }
-          ]
-        )
+        is_expected.to eq([triggered_event])
       end
 
       context 'when assignments summary and html_url are blank' do
@@ -74,7 +74,38 @@ RSpec.describe PagerDuty::WebhookPayloadParser do
       let(:payload) { { 'messages' => [{ 'event' => 'incident.trigger' }] } }
 
       it 'returns payload with blank incidents' do
-        is_expected.to eq([{}])
+        is_expected.to eq([])
+      end
+    end
+
+    context 'when payload consists of two messages' do
+      context 'when one of the messages has no incident data' do
+        let(:payload) do
+          valid_payload = Gitlab::Json.parse(fixture_file)
+          event = { 'event' => 'incident.trigger' }
+          valid_payload['messages'] = valid_payload['messages'].append(event)
+          valid_payload
+        end
+
+        it 'returns parsed payload with valid events only' do
+          is_expected.to eq([triggered_event])
+        end
+      end
+
+      context 'when one of the messages has unknown event' do
+        let(:payload) do
+          valid_payload = Gitlab::Json.parse(fixture_file)
+          event = { 'event' => 'incident.unknown', 'incident' => valid_payload['messages'].first['incident'] }
+          valid_payload['messages'] = valid_payload['messages'].append(event)
+          valid_payload
+        end
+
+        it 'returns parsed payload' do
+          unknown_event = triggered_event.dup
+          unknown_event['event'] = 'incident.unknown'
+
+          is_expected.to contain_exactly(triggered_event, unknown_event)
+        end
       end
     end
   end