diff --git a/allianceauth/authentication/managers.py b/allianceauth/authentication/managers.py index 3e16b3c2..2e91ed4d 100644 --- a/allianceauth/authentication/managers.py +++ b/allianceauth/authentication/managers.py @@ -23,9 +23,8 @@ def available_states_query(character): class CharacterOwnershipManager(Manager): def create_by_token(self, token): - if not EveCharacter.objects.filter(character_id=token.character_id).exists(): - EveCharacter.objects.create_character(token.character_id) - return self.create(character=EveCharacter.objects.get(character_id=token.character_id), user=token.user, owner_hash=token.character_owner_hash) + character = EveCharacter.objects.get_or_create_esi(token.character_id) + return self.create(character=character, user=token.user, owner_hash=token.character_owner_hash) class StateQuerySet(QuerySet): diff --git a/allianceauth/authentication/signals.py b/allianceauth/authentication/signals.py index 4ebd9651..a77d954c 100644 --- a/allianceauth/authentication/signals.py +++ b/allianceauth/authentication/signals.py @@ -95,11 +95,8 @@ def record_character_ownership(sender, instance, created, *args, **kwargs): query = Q(owner_hash=instance.character_owner_hash) # purge ownership records if the hash or auth user account has changed CharacterOwnership.objects.filter(character__character_id=instance.character_id).exclude(query).delete() - # create character if needed - if EveCharacter.objects.filter(character_id=instance.character_id).exists() is False: - logger.debug(f'Token is for a new character. Creating model for {instance.character_name} ({instance.character_id})') - EveCharacter.objects.create_character(instance.character_id) - char = EveCharacter.objects.get(character_id=instance.character_id) + # create character if needed (race-safe: a concurrent token save may create it first) + char = EveCharacter.objects.get_or_create_esi(instance.character_id) # check if we need to create ownership if instance.user and not CharacterOwnership.objects.filter( character__character_id=instance.character_id).exists(): diff --git a/allianceauth/corputils/views.py b/allianceauth/corputils/views.py index e91fcf63..1a900168 100644 --- a/allianceauth/corputils/views.py +++ b/allianceauth/corputils/views.py @@ -33,10 +33,7 @@ def corpstats_add(request, token) -> HttpResponseRedirect: corp_id = EveCharacter.objects.get(character_id=token.character_id).corporation_id else: corp_id = get_characters_character_id(token.character_id).corporation_id - try: - corp = EveCorporationInfo.objects.get(corporation_id=corp_id) - except EveCorporationInfo.DoesNotExist: - corp = EveCorporationInfo.objects.create_corporation(corp_id=corp_id, use_etag=False) + corp = EveCorporationInfo.objects.get_or_create_esi(corporation_id=corp_id) cs = CorpStats.objects.create(token=token, corp=corp) try: cs.update() diff --git a/allianceauth/eveonline/autogroups/models.py b/allianceauth/eveonline/autogroups/models.py index 232359f2..d12207f7 100644 --- a/allianceauth/eveonline/autogroups/models.py +++ b/allianceauth/eveonline/autogroups/models.py @@ -130,7 +130,7 @@ class AutogroupsConfig(models.Model): group = self.get_alliance_group(alliance) except EveAllianceInfo.DoesNotExist: logger.debug(f'User {user} main characters alliance does not exist in the database. Creating.') - alliance = EveAllianceInfo.objects.create_alliance(user.profile.main_character.alliance_id) + alliance = EveAllianceInfo.objects.get_or_create_esi(user.profile.main_character.alliance_id) group = self.get_alliance_group(alliance) except AttributeError: logger.warning(f'User {user} does not have a main character. Group membership not updated') @@ -151,7 +151,7 @@ class AutogroupsConfig(models.Model): group = self.get_corp_group(corp) except EveCorporationInfo.DoesNotExist: logger.debug(f'User {user} main characters corporation does not exist in the database. Creating.') - corp = EveCorporationInfo.objects.create_corporation( + corp = EveCorporationInfo.objects.get_or_create_esi( corporation_id=user.profile.main_character.corporation_id ) group = self.get_corp_group(corp) diff --git a/allianceauth/eveonline/autogroups/tests/test_models.py b/allianceauth/eveonline/autogroups/tests/test_models.py index 1c8d1d2f..daa31c1e 100644 --- a/allianceauth/eveonline/autogroups/tests/test_models.py +++ b/allianceauth/eveonline/autogroups/tests/test_models.py @@ -105,25 +105,28 @@ class AutogroupsConfigTestCase(TestCase): self.assertNotIn(group, self.member.groups.all()) - # todo: this test case currently does not work, because it forces - # an exception during a transaction, which is not easily testable - # the production code itself should be fine though - # I therefore commented out the test case for now - """ @patch('.models.EveAllianceInfo.objects.create_alliance') - def test_update_alliance_group_membership_no_alliance_model( + def test_update_alliance_group_membership_creates_missing_alliance( self, mock_create_alliance ): - def mock_create_alliance_side_effect(*args, **kwargs): - return EveAllianceInfo.objects.create( - alliance_id='3459', - alliance_name='alliance name', - alliance_ticker='alliance_ticker', - executor_corp_id='2345' - ) + # The character's alliance is not yet in the database, so accessing the + # alliance property raises DoesNotExist and the membership update must + # create it via the race-safe get_or_create_esi() helper before + # assigning the group. This previously could not be tested because the + # create happened outside a savepoint; get_or_create_esi() fixes that. + created_alliance = EveAllianceInfo( + alliance_id='3459', + alliance_name='created alliance', + alliance_ticker='NEW', + executor_corp_id='2345', + ) - mock_create_alliance.side_effect = mock_create_alliance_side_effect + def create_side_effect(*args, **kwargs): + created_alliance.save() + return created_alliance + + mock_create_alliance.side_effect = create_side_effect obj = AutogroupsConfig.objects.create(alliance_groups=True) obj.states.add(AuthUtils.get_member_state()) @@ -134,7 +137,7 @@ class AutogroupsConfigTestCase(TestCase): corporation_name='test corp', corporation_ticker='tickr', alliance_id='3459', - alliance_name='alliance name', + alliance_name='created alliance', ) self.member.profile.main_character = char self.member.profile.save() @@ -142,10 +145,10 @@ class AutogroupsConfigTestCase(TestCase): # Act obj.update_alliance_group_membership(self.member) - group = obj.get_alliance_group(self.alliance) - - self.assertNotIn(group, self.member.groups.all()) - """ + # The missing alliance was created and the group assigned. + mock_create_alliance.assert_called_once() + group = obj.get_alliance_group(created_alliance) + self.assertIn(group, self.member.groups.all()) def test_update_corp_group_membership(self): obj = AutogroupsConfig.objects.create(corp_groups=True) diff --git a/allianceauth/eveonline/managers.py b/allianceauth/eveonline/managers.py index 06c0b0f7..ce3ea159 100644 --- a/allianceauth/eveonline/managers.py +++ b/allianceauth/eveonline/managers.py @@ -63,24 +63,15 @@ class EveCharacterManager(Manager["EveCharacter"]): faction_model = _eve_faction_model() corporation_id = character.corporation_id - try: - corporation_obj = corporation_model.objects.get(corporation_id=corporation_id) - except corporation_model.DoesNotExist: - corporation_obj = corporation_model.objects.create_corporation(corporation_id=corporation_id) + corporation_obj = corporation_model.objects.get_or_create_esi(corporation_id=corporation_id) if character.alliance_id: - try: - alliance_obj = alliance_model.objects.get(alliance_id=character.alliance_id) - except alliance_model.DoesNotExist: - alliance_obj = alliance_model.objects.create_alliance(alliance_id=character.alliance_id) + alliance_obj = alliance_model.objects.get_or_create_esi(alliance_id=character.alliance_id) else: alliance_obj = None if character.faction_id: - try: - faction_obj = faction_model.objects.get(faction_id=character.faction_id) - except faction_model.DoesNotExist: - faction_obj = faction_model.objects.create_faction(faction_id=character.faction_id) + faction_obj = faction_model.objects.get_or_create_esi(faction_id=character.faction_id) else: faction_obj = None @@ -106,6 +97,16 @@ class EveCharacterManager(Manager["EveCharacter"]): last_updated_other=datetime.strptime(character_response.headers.get("Last-Modified"), "%a, %d %b %Y %H:%M:%S GMT").replace(tzinfo=timezone.utc) ) + def get_or_create_esi(self, character_id: "CharacterID") -> "EveCharacter": + try: + return self.get(character_id=character_id) + except self.model.DoesNotExist: + try: + with transaction.atomic(): + return self.create_character(character_id=character_id) + except IntegrityError: + return self.get(character_id=character_id) + def update_character(self, character_id) -> "EveCharacter": return self.get(character_id=character_id).update_character() @@ -131,10 +132,7 @@ class EveAllianceManager(Manager["EveAllianceInfo"]): faction_model = _eve_faction_model() if alliance.faction_id: - try: - faction_obj = faction_model.objects.get(faction_id=alliance.faction_id) - except faction_model.DoesNotExist: - faction_obj = faction_model.objects.create_faction(faction_id=alliance.faction_id) + faction_obj = faction_model.objects.get_or_create_esi(faction_id=alliance.faction_id) else: faction_obj = None @@ -184,18 +182,12 @@ class EveCorporationManager(Manager["EveCorporationInfo"]): faction_model = _eve_faction_model() if corporation.alliance_id: - try: - alliance_obj = alliance_model.objects.get(alliance_id=corporation.alliance_id) - except alliance_model.DoesNotExist: - alliance_obj = alliance_model.objects.create_alliance(alliance_id=corporation.alliance_id) + alliance_obj = alliance_model.objects.get_or_create_esi(alliance_id=corporation.alliance_id) else: alliance_obj = None if corporation.faction_id: - try: - faction_obj = faction_model.objects.get(faction_id=corporation.faction_id) - except faction_model.DoesNotExist: - faction_obj = faction_model.objects.create_faction(faction_id=corporation.faction_id) + faction_obj = faction_model.objects.get_or_create_esi(faction_id=corporation.faction_id) else: faction_obj = None diff --git a/allianceauth/eveonline/models.py b/allianceauth/eveonline/models.py index a048daed..b5b5ee95 100644 --- a/allianceauth/eveonline/models.py +++ b/allianceauth/eveonline/models.py @@ -239,8 +239,7 @@ class EveAllianceInfo(models.Model): return self for corp_id in corp_ids: - if not EveCorporationInfo.objects.filter(corporation_id=corp_id).exists(): - EveCorporationInfo.objects.create_corporation(corporation_id=corp_id) + EveCorporationInfo.objects.get_or_create_esi(corporation_id=corp_id) EveCorporationInfo.objects.filter( corporation_id__in=corp_ids ).update( @@ -266,10 +265,7 @@ class EveAllianceInfo(models.Model): return self if alliance.faction_id: - try: - self.faction = EveFactionInfo.objects.get(faction_id=alliance.faction_id) - except EveFactionInfo.DoesNotExist: - self.faction = EveFactionInfo.objects.create_faction(faction_id=alliance.faction_id) + self.faction = EveFactionInfo.objects.get_or_create_esi(faction_id=alliance.faction_id) else: self.faction = None @@ -402,18 +398,12 @@ class EveCorporationInfo(models.Model): return self if corporation.alliance_id: - try: - self.alliance = EveAllianceInfo.objects.get(alliance_id=corporation.alliance_id) - except EveAllianceInfo.DoesNotExist: - self.alliance = EveAllianceInfo.objects.create_alliance(alliance_id=corporation.alliance_id) + self.alliance = EveAllianceInfo.objects.get_or_create_esi(alliance_id=corporation.alliance_id) else: self.alliance = None if corporation.faction_id: - try: - self.faction = EveFactionInfo.objects.get(faction_id=corporation.faction_id) - except EveFactionInfo.DoesNotExist: - self.faction = EveFactionInfo.objects.create_faction(faction_id=corporation.faction_id) + self.faction = EveFactionInfo.objects.get_or_create_esi(faction_id=corporation.faction_id) else: self.faction = None diff --git a/allianceauth/eveonline/tests/test_managers.py b/allianceauth/eveonline/tests/test_managers.py index cc5cb939..ab63003d 100644 --- a/allianceauth/eveonline/tests/test_managers.py +++ b/allianceauth/eveonline/tests/test_managers.py @@ -362,6 +362,41 @@ class EveCharacterManagerTestCase(TestCase): self.assertEqual(result.character_name, "character.name") self.assertIsNone(EveCharacter.objects.get_character_by_id(9999)) + def test_get_or_create_esi_returns_existing_character(self) -> None: + character = EveCharacter.objects.create( + character_id=1234, + character_name="Existing Character", + corporation_id=2345, + corporation_name="Existing Corp", + corporation_ticker="EXC", + ) + + result = EveCharacter.objects.get_or_create_esi(1234) + + self.assertEqual(result, character) + + def test_get_or_create_esi_handles_integrity_error_for_character(self) -> None: + existing = EveCharacter.objects.create( + character_id=1234, + character_name="Existing Character", + corporation_id=2345, + corporation_name="Existing Corp", + corporation_ticker="EXC", + ) + + with mock.patch.object( + EveCharacter.objects, + "get", + side_effect=[EveCharacter.DoesNotExist, existing], + ), mock.patch.object( + EveCharacter.objects, + "create_character", + side_effect=IntegrityError, + ): + result = EveCharacter.objects.get_or_create_esi(1234) + + self.assertEqual(result, existing) + class EveAllianceManagerTestCase(TestCase): @mock.patch("allianceauth.eveonline.managers.open_api_provider.get_alliance") diff --git a/allianceauth/eveonline/tests/test_models.py b/allianceauth/eveonline/tests/test_models.py index 6d4b0b51..ed1ca0a9 100644 --- a/allianceauth/eveonline/tests/test_models.py +++ b/allianceauth/eveonline/tests/test_models.py @@ -3,6 +3,7 @@ from types import SimpleNamespace from unittest.mock import MagicMock, PropertyMock, patch from django.core.exceptions import ObjectDoesNotExist +from django.db.utils import IntegrityError from django.test import TestCase from esi.exceptions import HTTPNotModified @@ -363,6 +364,75 @@ class EveCharacterTestCase(TestCase): self.assertEqual(my_character.alliance_name, 'Created Alliance') self.assertEqual(my_character.faction_name, 'Created Faction') + @patch('allianceauth.eveonline.models.open_api_provider.get_affiliations') + def test_update_character_survives_alliance_creation_race(self, mock_get_affiliations) -> None: + """Regression test for the multi-worker race condition. + + Two update tasks can run for the same character concurrently. Both see the + alliance as missing, both call ``create_alliance()``, and the loser's INSERT + raises ``IntegrityError`` on the unique ``alliance_id`` constraint + (``1062, "Duplicate entry ... for key + 'eveonline_eveallianceinfo_alliance_id_fbfbea3c_uniq'"``). + ``update_character()`` must recover by returning the alliance the winning + worker committed instead of propagating the error. + """ + my_character = EveCharacter.objects.create( + character_id=1001, + character_name='Race Victim', + corporation_id=2001, + corporation_name='Dummy Corp 1', + corporation_ticker='DC1', + alliance_id=None, + ) + # Corporation already exists so only the alliance path exercises the race. + EveCorporationInfo.objects.create( + corporation_id=2002, + corporation_name='Dummy Corp 2', + corporation_ticker='DC2', + member_count=1, + ceo_id=3, + ) + # The alliance the *other* worker wins the race to create. + winning_alliance = EveAllianceInfo.objects.create( + alliance_id=3001, + alliance_name='Winner Alliance', + alliance_ticker='WIN', + executor_corp_id=2002, + ) + mock_get_affiliations.return_value = ( + [ + SimpleNamespace( + character_id=1001, + corporation_id=2002, + alliance_id=3001, + faction_id=None, + ) + ], + response_stub(), + ) + + # Simulate the race: the first get() misses, create_alliance() then loses + # the unique-constraint race (IntegrityError), and the follow-up get() + # finds the row the winning worker committed. + with patch.object( + EveAllianceInfo.objects, + 'get', + side_effect=[EveAllianceInfo.DoesNotExist, winning_alliance], + ), patch.object( + EveAllianceInfo.objects, + 'create_alliance', + side_effect=IntegrityError( + "(1062, \"Duplicate entry '3001' for key " + "'eveonline_eveallianceinfo_alliance_id_fbfbea3c_uniq'\")" + ), + ): + result = my_character.update_character() + + result.refresh_from_db() + self.assertEqual(result.alliance_id, 3001) + self.assertEqual(result.alliance_name, 'Winner Alliance') + self.assertEqual(result.alliance_ticker, 'WIN') + def test_image_url(self) -> None: self.assertEqual( EveCharacter.generic_portrait_url(42),