Event already exists: return 400; implement with one less query

events/models.py

@@ -1,7 +1,9 @@
+import re
 import json
 import uuid
 
 from django.db import models
+from django.db.utils import IntegrityError
 
 from projects.models import Project
 from compat.timestamp import parse_timestamp
@@ -174,39 +176,43 @@ class Event(models.Model):
         # below at least puts the parsed_data in the right place, and does some of the basic object set up (FKs to other
         # objects etc).
 
-        event, created = cls.objects.get_or_create(  # NOTE immediate creation... is this what we want?
-            event_id=parsed_data["event_id"],
-            project=ingested_event.project,
-            defaults={
-                'ingested_event': ingested_event,  # <= thoughts about defaults v.s. check-as-part-of-get go here :-)
-                'issue': issue,
-                'server_side_timestamp': ingested_event.timestamp,
-                'data': json.dumps(parsed_data),
+        try:
+            event = cls.objects.create(
+                event_id=parsed_data["event_id"],
+                project=ingested_event.project,
+                ingested_event=ingested_event,  # <= thoughts about defaults v.s. check-as-part-of-get go here...
+                issue=issue,
+                server_side_timestamp=ingested_event.timestamp,
+                data=json.dumps(parsed_data),
 
-                'timestamp': parse_timestamp(parsed_data["timestamp"]),
-                'platform': parsed_data["platform"],
+                timestamp=parse_timestamp(parsed_data["timestamp"]),
+                platform=parsed_data["platform"],
 
-                'level': maybe_empty(parsed_data.get("level", "")),
-                'logger': maybe_empty(parsed_data.get("logger", "")),
-                # 'transaction': maybe_empty(parsed_data.get("transaction", "")), passed as part of denormalized_fields
+                level=maybe_empty(parsed_data.get("level", "")),
+                logger=maybe_empty(parsed_data.get("logger", "")),
+                # transaction=maybe_empty(parsed_data.get("transaction", "")), part of denormalized_fields
 
-                'server_name': maybe_empty(parsed_data.get("server_name", "")),
-                'release': maybe_empty(parsed_data.get("release", "")),
-                'dist': maybe_empty(parsed_data.get("dist", "")),
+                server_name=maybe_empty(parsed_data.get("server_name", "")),
+                release=maybe_empty(parsed_data.get("release", "")),
+                dist=maybe_empty(parsed_data.get("dist", "")),
 
-                'environment': maybe_empty(parsed_data.get("environment", "")),
+                environment=maybe_empty(parsed_data.get("environment", "")),
 
-                'sdk_name': maybe_empty(parsed_data.get("", {}).get("name", "")),
-                'sdk_version': maybe_empty(parsed_data.get("", {}).get("version", "")),
+                sdk_name=maybe_empty(parsed_data.get("", {}).get("name", "")),
+                sdk_version=maybe_empty(parsed_data.get("", {}).get("version", "")),
 
-                'has_exception': "exception" in parsed_data,
-                'has_logentry': "logentry" in parsed_data,
+                has_exception="exception" in parsed_data,
+                has_logentry="logentry" in parsed_data,
 
-                'debug_info': ingested_event.debug_info,
+                debug_info=ingested_event.debug_info,
 
-                'ingest_order': ingest_order,
+                ingest_order=ingest_order,
 
                 **denormalized_fields,
-            }
-        )
-        return event, created
+            )
+            created = True
+            return event, created
+        except IntegrityError as e:
+            assert re.match(
+                r".*unique constraint failed.*events_event.*project_id.*events_event.*event_id", str(e).lower())
+            return None, False

ingest/tests.py

@@ -6,6 +6,8 @@ from django.test import TestCase
 from django.utils import timezone
 from django.test.client import RequestFactory
 
+from rest_framework.exceptions import ValidationError
+
 from projects.models import Project
 from events.factories import create_event_data
 from issues.factories import get_or_create_issue
@@ -17,9 +19,9 @@ from .views import BaseIngestAPIView
 
 
 class IngestViewTestCase(TestCase):
-    # For now this test focusses on alert-sending, but it may evolve more generally to be about the IngestView.
 
     def setUp(self):
+        # the existence of loud/quiet reflect that parts of this test focusses on alert-sending
         self.request_factory = RequestFactory()
         self.loud_project = Project.objects.create(
             name="loud",
@@ -188,6 +190,28 @@ class IngestViewTestCase(TestCase):
             )
             self.assertEquals(expected_called, send_unmute_alert.delay.called)
 
+    def test_deal_with_double_event_ids(self):
+        request = self.request_factory.post("/api/1/store/")
+
+        project = self.quiet_project
+
+        event_data = create_event_data()
+
+        # first time
+        BaseIngestAPIView().process_event(
+            event_data,
+            project,
+            request,
+        )
+
+        with self.assertRaises(ValidationError):
+            # second time
+            BaseIngestAPIView().process_event(
+                event_data,
+                project,
+                request,
+            )
+
 
 class TimeZoneTesCase(TestCase):
     """This class contains some tests that formalize my understanding of how Django works; they are not strictly tests

ingest/views.py

@@ -9,6 +9,7 @@ from rest_framework import permissions, status
 from rest_framework.response import Response
 from rest_framework.views import APIView
 from rest_framework import exceptions
+from rest_framework.exceptions import ValidationError
 
 # from projects.models import Project
 from compat.auth import parse_auth_header_value
@@ -75,6 +76,8 @@ class BaseIngestAPIView(APIView):
         # before proceeding because it may be useful for debugging errors in the digest process.
         ingested_event = cls.ingest_event(now, event_data, request, project)
         if settings.BUGSINK_DIGEST_IMMEDIATELY:
+            # NOTE once we implement the no-immediate case, we should do so in a way that catches ValidationErrors
+            # raised by digest_event
             cls.digest_event(ingested_event, event_data)
 
     @classmethod
@@ -170,7 +173,10 @@ class BaseIngestAPIView(APIView):
                 issue.delete()
                 raise ViolatedExpectation("no event created, but issue created")
 
-            return
+            # Validating by letting the DB raise an exception, and only after taking some other actions already, is not
+            # "by the book" (some book), but it's the most efficient way of doing it when your basic expectation is that
+            # multiple events with the same event_id "don't happen" (i.e. are the result of badly misbehaving clients)
+            raise ValidationError("Event already exists", code="event_already_exists")
 
         create_release_if_needed(ingested_event.project, event.release)