diff --git a/src/registrar/models/domain.py b/src/registrar/models/domain.py index f7e76cbc4..2ace5f22d 100644 --- a/src/registrar/models/domain.py +++ b/src/registrar/models/domain.py @@ -1847,10 +1847,9 @@ class Domain(TimeStampedModel, DomainHelper): missingSecurity = True missingTech = True - # Potential collision - mismatch between _contacts and contacts? - # But the ID wouldnt match in this case because the default is being grabbed? - if len(cleaned.get("_contacts")) < 3: - for contact in cleaned.get("_contacts"): + contacts = cleaned.get("_contacts", []) + if len(contacts) < 3: + for contact in contacts: if contact.type == PublicContact.ContactTypeChoices.ADMINISTRATIVE: missingAdmin = False if contact.type == PublicContact.ContactTypeChoices.SECURITY: @@ -1868,9 +1867,9 @@ class Domain(TimeStampedModel, DomainHelper): if missingTech: technical_contact = self.get_default_technical_contact() technical_contact.save() - + logger.info( - "_add_missing_contacts_if_unknown => In function. Values are " + "_add_missing_contacts_if_unknown => Adding contacts. Values are " f"missingAdmin: {missingAdmin}, missingSecurity: {missingSecurity}, missingTech: {missingTech}" ) @@ -2052,7 +2051,7 @@ class Domain(TimeStampedModel, DomainHelper): if contacts and isinstance(contacts, list) and len(contacts) > 0: cleaned_contacts = self._fetch_contacts(contacts) return cleaned_contacts - + def _fetch_contacts(self, contact_data): """Fetch contact info.""" choices = PublicContact.ContactTypeChoices @@ -2086,13 +2085,11 @@ class Domain(TimeStampedModel, DomainHelper): def _get_or_create_public_contact(self, public_contact: PublicContact): """Tries to find a PublicContact object in our DB. If it can't, it'll create it. Returns PublicContact""" - logger.info(f"in function") db_contact = PublicContact.objects.filter( registry_id=public_contact.registry_id, contact_type=public_contact.contact_type, domain=self, ) - logger.info(f"db_contact {db_contact}") # If we find duplicates, log it and delete the oldest ones. if db_contact.count() > 1: @@ -2119,13 +2116,10 @@ class Domain(TimeStampedModel, DomainHelper): with transaction.atomic(): public_contact.save(skip_epp_save=True) logger.info(f"Created a new PublicContact: {public_contact}") - # In rare cases, _add_missing_contacts_if_unknown will cause a race condition with this function. - # This is because it calls .save(), which is called here. - # except IntegrityError as err: logger.error( - "_get_or_create_public_contact() => tried to create a duplicate public contact: " - f"{err}", exc_info=True + "_get_or_create_public_contact() => tried to create a duplicate public contact: " f"{err}", + exc_info=True, ) return PublicContact.objects.get( registry_id=public_contact.registry_id, @@ -2142,7 +2136,7 @@ class Domain(TimeStampedModel, DomainHelper): if existing_contact.email != public_contact.email or existing_contact.registry_id != public_contact.registry_id: existing_contact.delete() public_contact.save() - logger.warning("Requested PublicContact is out of sync " "with DB.") + logger.warning("Requested PublicContact is out of sync with our DB.") return public_contact # If it already exists, we can assume that the DB instance was updated during set, so we should just use that. diff --git a/src/registrar/tests/test_models_domain.py b/src/registrar/tests/test_models_domain.py index 8fde89fd0..083725a55 100644 --- a/src/registrar/tests/test_models_domain.py +++ b/src/registrar/tests/test_models_domain.py @@ -350,27 +350,28 @@ class TestDomainCreation(MockEppLib): """Rule: An approved domain request must result in a domain""" @less_console_noise_decorator - def test_get_security_email_race_condition(self): + def test_get_or_create_public_contact_race_condition(self): """ Scenario: Two processes try to create the same security contact simultaneously Given a domain in UNKNOWN state When a race condition occurs during contact creation Then no IntegrityError is raised And only one security contact exists in database - And the correct security email is returned + And the correct public contact is returned + + CONTEXT: We ran into an intermittent but somewhat rare issue where IntegrityError + was raised when creating PublicContact. + Per our logs, this seemed to appear during periods of high app activity. """ domain, _ = Domain.objects.get_or_create(name="defaultsecurity.gov") - - # Store original method - original_filter = PublicContact.objects.filter - self.first_call = True - def mock_filter(*args, **kwargs): - """ Simulates the race condition by creating a - duplicate contact between filter check and save - """ - result = original_filter(*args, **kwargs) - # Return empty queryset for first call. Otherwise just proceed as normal. + self.first_call = True + + def mock_filter(*args, **kwargs): + """Simulates a race condition by creating a + duplicate contact between the first filter and save. + """ + # Return an empty queryset for the first call. Otherwise just proceed as normal. if self.first_call: self.first_call = False duplicate = PublicContact( @@ -378,21 +379,21 @@ class TestDomainCreation(MockEppLib): contact_type=PublicContact.ContactTypeChoices.SECURITY, registry_id="defaultSec", email="dotgov@cisa.dhs.gov", - name="Registry Customer Service" + name="Registry Customer Service", ) duplicate.save(skip_epp_save=True) return PublicContact.objects.none() - return result - - with patch.object(PublicContact.objects, 'filter', side_effect=mock_filter): + return PublicContact.objects.filter(*args, **kwargs) + + with patch.object(PublicContact.objects, "filter", side_effect=mock_filter): try: public_contact = PublicContact( domain=domain, contact_type=PublicContact.ContactTypeChoices.SECURITY, registry_id="defaultSec", email="dotgov@cisa.dhs.gov", - name="Registry Customer Service" + name="Registry Customer Service", ) returned_public_contact = domain._get_or_create_public_contact(public_contact) except IntegrityError: @@ -403,13 +404,14 @@ class TestDomainCreation(MockEppLib): "Expected behavior: gracefully handle duplicate creation and return existing contact." ) - # Verify only one contact exists + # Verify that only one contact exists and its correctness security_contacts = PublicContact.objects.filter( - domain=domain, - contact_type=PublicContact.ContactTypeChoices.SECURITY + domain=domain, contact_type=PublicContact.ContactTypeChoices.SECURITY ) self.assertEqual(security_contacts.count(), 1) self.assertEqual(returned_public_contact, security_contacts.get()) + self.assertEqual(returned_public_contact.registry_id, "defaultSec") + self.assertEqual(returned_public_contact.email, "dotgov@cisa.dhs.gov") @boto3_mocking.patching def test_approved_domain_request_creates_domain_locally(self):