diff --git a/src/bpp/newsfragments/+pbn-polonuuid-kolizja.bugfix.rst b/src/bpp/newsfragments/+pbn-polonuuid-kolizja.bugfix.rst new file mode 100644 index 000000000..ff3731ff7 --- /dev/null +++ b/src/bpp/newsfragments/+pbn-polonuuid-kolizja.bugfix.rst @@ -0,0 +1,8 @@ +Import osób z API instytucji PBN radzi sobie z osobami, którym PBN zmienił +identyfikator. ``polonUuid`` (identyfikator z POL-onu) jest stabilną +tożsamością osoby, a ``personId`` PBN potrafi zmienić — np. po scaleniu +zdublowanych profili. Import dopasowywał wpis wyłącznie po ``personId``, +więc taka osoba rozbijała się o unikalność ``polonUuid`` i była pomijana; +teraz jej wpis jest przepinany na nowy identyfikator. Osoby bez +``polonUuid`` są pomijane z czytelnym komunikatem w logu, zamiast trafiać +do monitoringu jako „konflikt tożsamości". diff --git a/src/pbn_integrator/tests/test_osoba_z_instytucji.py b/src/pbn_integrator/tests/test_osoba_z_instytucji.py new file mode 100644 index 000000000..0cd709fcf --- /dev/null +++ b/src/pbn_integrator/tests/test_osoba_z_instytucji.py @@ -0,0 +1,140 @@ +"""Testy zapisu osób z API instytucji PBN (`_zapisz_osobe_z_instytucji`). + +Kontekst: ``OsobaZInstytucji`` ma DWA klucze unikalne — ``personId`` +(OneToOne na ``Scientist``) oraz ``polonUuid``. PBN potrafi wystawić tę samą +fizyczną osobę pod NOWYM ``personId``, zachowując jej ``polonUuid`` z POL-onu +(np. po scaleniu zdublowanych profili). Import dopasowywał wiersz wyłącznie po +``personId``, więc taka osoba leciała na INSERT i rozbijała się o unikalność +``polonUuid``. +""" + +import uuid + +import pytest +from model_bakery import baker + +from pbn_api.models import Scientist +from pbn_api.models.institution import Institution +from pbn_api.models.osoba_z_instytucji import OsobaZInstytucji +from pbn_integrator.utils.scientists import _zapisz_osobe_z_instytucji + + +@pytest.fixture +def instytucja(db): + return baker.make(Institution) + + +def _person(scientist, instytucja, polon_uuid, **kw): + dane = { + "personId": scientist.pk, + "institutionId": instytucja.pk, + "firstName": "Jan", + "lastName": "Kowalski", + "institutionName": "Instytut Testowy", + "polonUuid": str(polon_uuid), + "phdStudent": False, + "title": "dr hab.", + "from": "2020-01-01", + "to": "2030-12-31", + } + dane.update(kw) + return dane + + +@pytest.mark.django_db +def test_zapisz_osobe_z_instytucji_zapisuje_wszystkie_pola(instytucja): + scientist = baker.make(Scientist) + polon_uuid = uuid.uuid4() + + assert _zapisz_osobe_z_instytucji(_person(scientist, instytucja, polon_uuid)) + + osoba = OsobaZInstytucji.objects.get() + assert osoba.personId_id == scientist.pk + assert osoba.institutionId_id == instytucja.pk + assert str(osoba.polonUuid) == str(polon_uuid) + assert osoba.firstName == "Jan" + assert osoba.lastName == "Kowalski" + assert osoba.institutionName == "Instytut Testowy" + assert osoba.title == "dr hab." + assert osoba.phdStudent is False + assert str(osoba._from) == "2020-01-01" + assert str(osoba._to) == "2030-12-31" + + +@pytest.mark.django_db +def test_zapisz_osobe_bez_polonuuid_pomija_bez_raportu(instytucja, mocker): + """Brak ``polonUuid`` (kolumna NOT NULL) to nie „konflikt tożsamości". + + Wcześniej dowiadywaliśmy się o tym okrężnie — przez IntegrityError na + NOT NULL, raportowany do Rollbara pod mylącą etykietą konfliktu. + """ + report = mocker.patch("pbn_integrator.utils.scientists.rollbar.report_exc_info") + scientist = baker.make(Scientist) + + dane = _person(scientist, instytucja, uuid.uuid4()) + del dane["polonUuid"] + + assert not _zapisz_osobe_z_instytucji(dane) + assert not report.called + assert not OsobaZInstytucji.objects.exists() + + +@pytest.mark.django_db +def test_zapisz_osobe_przepina_wpis_gdy_pbn_zmienil_personId(instytucja): + """Ten sam ``polonUuid`` pod nowym ``personId`` PRZEPINA istniejący wiersz. + + Regresja (Rollbar #1523): leciał ``IntegrityError`` na + ``pbn_api_osobazinstytucji_polonUuid_key``, a osoba była pomijana — + czyli nowa tożsamość PBN nigdy nie trafiała do bazy i błąd wracał + przy każdym kolejnym imporcie. + """ + polon_uuid = uuid.uuid4() + stary = baker.make(Scientist) + nowy = baker.make(Scientist) + nowa_instytucja = baker.make(Institution) + + assert _zapisz_osobe_z_instytucji(_person(stary, instytucja, polon_uuid)) + pierwotny_pk = OsobaZInstytucji.objects.get().pk + + assert _zapisz_osobe_z_instytucji( + _person(nowy, nowa_instytucja, polon_uuid, firstName="Janina") + ) + + # Nadal JEDEN wiersz — ``polonUuid`` jest tożsamością osoby... + osoba = OsobaZInstytucji.objects.get() + # ...i to TEN SAM wiersz: przepięty, nie skasowany i odtworzony. FK z + # deduplikator_autorow (``main_osoba_z_instytucji``, on_delete=SET_NULL) + # przeżyłby podmianę jako NULL, więc pilnujemy pk. + assert osoba.pk == pierwotny_pk + assert osoba.personId_id == nowy.pk + assert osoba.firstName == "Janina" + # ``institutionId`` MUSI się odświeżyć — na nim stoi scoping per-uczelnia + # (patrz bpp/views/autocomplete/authors.py: institutionId_id == pbn_uid). + assert osoba.institutionId_id == nowa_instytucja.pk + + +@pytest.mark.django_db +def test_zapisz_osobe_pomija_gdy_tozsamosci_nie_da_sie_pogodzic(instytucja, mocker): + """Dwie osobne tożsamości PBN nie dają się scalić — pomijamy, nie wywalamy. + + Gdy nowy ``personId`` ma JUŻ swój wiersz (z innym ``polonUuid``), + przepięcie zderza się z unikalnością ``personId``. Scalenie to decyzja + o danych, nie poprawka techniczna: raportujemy do Rollbara i idziemy + dalej, zamiast przerywać cały import. + """ + report = mocker.patch("pbn_integrator.utils.scientists.rollbar.report_exc_info") + pierwsza = baker.make(Scientist) + druga = baker.make(Scientist) + uuid_a, uuid_b = uuid.uuid4(), uuid.uuid4() + + assert _zapisz_osobe_z_instytucji(_person(pierwsza, instytucja, uuid_a)) + assert _zapisz_osobe_z_instytucji(_person(druga, instytucja, uuid_b)) + + # Osoba spod uuid_a przychodzi teraz pod personId, który ma już swój wiersz: + assert not _zapisz_osobe_z_instytucji(_person(druga, instytucja, uuid_a)) + + assert report.called + # Raport musi nieść treść naruszonego ograniczenia — bez tego kolizja + # personId, NOT NULL i każdy inny IntegrityError wyglądają identycznie. + assert "constraint" in report.call_args.kwargs["extra_data"] + assert OsobaZInstytucji.objects.count() == 2 diff --git a/src/pbn_integrator/utils/scientists.py b/src/pbn_integrator/utils/scientists.py index c72857265..8fb610885 100644 --- a/src/pbn_integrator/utils/scientists.py +++ b/src/pbn_integrator/utils/scientists.py @@ -71,54 +71,97 @@ def pobierz_i_zapisz_dane_jednej_osoby( def _zapisz_osobe_z_instytucji(person): """Save a person from institution to OsobaZInstytucji model. - Handles IntegrityError for polonUuid conflicts gracefully. + ``OsobaZInstytucji`` ma DWA klucze unikalne: ``personId`` (OneToOne na + ``Scientist``) i ``polonUuid``. To ``polonUuid`` — identyfikator z POL-onu + — jest stabilną tożsamością fizycznej osoby; ``personId`` PBN potrafi + zmienić (np. po scaleniu zdublowanych profili). Dlatego dopasowujemy + wiersz NAJPIERW po ``polonUuid``, a ``personId`` traktujemy jako zwykłe + pole do zaktualizowania. Odwrotna kolejność (match wyłącznie po + ``personId``) leciała na INSERT i rozbijała się o unikalność + ``polonUuid``, a osoba była pomijana — czyli nowa tożsamość PBN nigdy nie + trafiała do bazy i ten sam błąd wracał przy każdym kolejnym imporcie. Args: person: Person data dictionary from PBN API. Returns: - True if saved successfully, False if skipped due to polonUuid conflict. + True if saved successfully, False if the person was skipped — brak + ``polonUuid`` (tylko log) albo błąd integralności, np. kolizja + ``personId`` (log + Rollbar). Import całej kadry leci dalej. """ from pbn_api.models.institution import Institution from pbn_api.models.osoba_z_instytucji import OsobaZInstytucji + polon_uuid = person.get("polonUuid") + if not polon_uuid: + # ``polonUuid`` jest NOT NULL, więc bez niego i tak nie ma czego + # zapisać. Mówimy to wprost, zamiast dowiadywać się tego okrężnie + # przez IntegrityError i raportować jako "konflikt tożsamości". + logger.info( + f"Pomijam osobę {person.get('personId')} — PBN nie podał polonUuid." + ) + return False + + dane = { + "firstName": person.get("firstName", ""), + "lastName": person.get("lastName", ""), + "institutionName": person.get("institutionName", ""), + "title": person.get("title") or "", + "phdStudent": person.get("phdStudent", False), + "_from": person.get("from"), + "_to": person.get("to"), + } + try: with transaction.atomic(): - OsobaZInstytucji.objects.update_or_create( - personId=Scientist.objects.get(pk=person["personId"]), - defaults={ - "firstName": person.get("firstName", ""), - "lastName": person.get("lastName", ""), - "institutionId": Institution.objects.get( - pk=person["institutionId"] - ), - "institutionName": person.get("institutionName", ""), - "title": person.get("title") or "", - "polonUuid": person.get("polonUuid"), - "phdStudent": person.get("phdStudent", False), - "_from": person.get("from"), - "_to": person.get("to"), - }, - ) + scientist = Scientist.objects.get(pk=person["personId"]) + instytucja = Institution.objects.get(pk=person["institutionId"]) + + osoba = OsobaZInstytucji.objects.filter(polonUuid=polon_uuid).first() + + if osoba is not None: + # Ta sama osoba z POL-onu — przepnij wiersz na (być może + # nowy) identyfikator PBN i odśwież dane. + osoba.personId = scientist + osoba.institutionId = instytucja + for pole, wartosc in dane.items(): + setattr(osoba, pole, wartosc) + osoba.save() + else: + OsobaZInstytucji.objects.update_or_create( + personId=scientist, + defaults={ + **dane, + "institutionId": instytucja, + "polonUuid": polon_uuid, + }, + ) return True except IntegrityError as e: - if "polonUuid" in str(e): - # Loguj konflikt polonUuid do Rollbar jako ostrzeżenie - rollbar.report_exc_info( - sys.exc_info(), - extra_data={ - "personId": person.get("personId"), - "polonUuid": person.get("polonUuid"), - "firstName": person.get("firstName"), - "lastName": person.get("lastName"), - }, - ) - logger.info( - f"UWAGA: Konflikt polonUuid dla osoby {person.get('personId')}: " - f"{person.get('polonUuid')}. Pomijam wpis (zalogowano do Rollbar)." - ) - return False - raise # Inne błędy IntegrityError propaguj + # Świadomie SZEROKO: import całej kadry uczelni nie ma padać przez + # jedną wadliwą osobę. Trafia tu m.in.: + # - kolizja personId (nowy personId ma już swój wiersz z innym + # polonUuid — scalenie dwóch tożsamości PBN to decyzja o danych, + # nie poprawka techniczna), + # - NOT NULL na polach, które PBN przysłał jako null, + # - każdy inny błąd integralności. + # Dlatego do Rollbara idzie treść naruszonego ograniczenia — bez niej + # wszystkie te przypadki wyglądają w raporcie identycznie. + rollbar.report_exc_info( + sys.exc_info(), + extra_data={ + "personId": person.get("personId"), + "polonUuid": polon_uuid, + "firstName": person.get("firstName"), + "lastName": person.get("lastName"), + "constraint": str(e), + }, + ) + logger.info( + f"UWAGA: Błąd integralności dla osoby {person.get('personId')} " + f"(polonUuid {polon_uuid}): {e}. Pomijam wpis (zalogowano do Rollbar)." + ) + return False def _get_max_workers():