From b2c347e0b9f6b6794f035a489e05f8fb267b9302 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pasternak?= Date: Fri, 24 Jul 2026 23:40:08 +0200 Subject: [PATCH 1/2] =?UTF-8?q?fix(pbn):=20przepinaj=20osob=C4=99=20gdy=20?= =?UTF-8?q?PBN=20zmieni=C5=82=20personId,=20zamiast=20j=C4=85=20pomija?= =?UTF-8?q?=C4=87?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. Import dopasowywał wiersz wyłącznie po personId, więc osoba wracająca pod nowym personId leciała na INSERT i rozbijała się o unikalność polonUuid. Handler łapał IntegrityError, raportował do Rollbara i POMIJAŁ osobę — czyli nowa tożsamość PBN nigdy nie trafiała do bazy, a ten sam błąd wracał przy każdym kolejnym imporcie (Rollbar #1523, bpp.ihit.waw.pl). Teraz dopasowanie idzie najpierw po polonUuid, a personId jest zwykłym polem do zaktualizowania. Zostaje raportowany tylko przypadek naprawdę niejednoznaczny: nowy personId ma już swój wiersz z innym polonUuid — scalenie dwóch tożsamości to decyzja o danych, nie poprawka techniczna. Przy okazji znika kruche dopasowanie po treści komunikatu wyjątku ("polonUuid" in str(e)), które łapało też naruszenie NOT NULL dla osób bez polonUuid i raportowało je jako "konflikt". Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NcAqeqyqBzNEkkVnhpHDaH --- .../+pbn-polonuuid-kolizja.bugfix.rst | 7 ++ .../tests/test_osoba_z_instytucji.py | 99 +++++++++++++++++ src/pbn_integrator/utils/scientists.py | 101 ++++++++++++------ 3 files changed, 172 insertions(+), 35 deletions(-) create mode 100644 src/bpp/newsfragments/+pbn-polonuuid-kolizja.bugfix.rst create mode 100644 src/pbn_integrator/tests/test_osoba_z_instytucji.py 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..f44023feb --- /dev/null +++ b/src/bpp/newsfragments/+pbn-polonuuid-kolizja.bugfix.rst @@ -0,0 +1,7 @@ +Import osób z API instytucji PBN nie pomija już osób, 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ł wiersz wyłącznie po ``personId``, +więc taka osoba rozbijała się o unikalność ``polonUuid`` i była pomijana, +a błąd wracał przy każdym kolejnym imporcie. Teraz wiersz jest przepinany +na nowy identyfikator. 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..f0030b298 --- /dev/null +++ b/src/pbn_integrator/tests/test_osoba_z_instytucji.py @@ -0,0 +1,99 @@ +"""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, + } + dane.update(kw) + return dane + + +@pytest.mark.django_db +def test_zapisz_osobe_z_instytucji_tworzy_wpis(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 str(osoba.polonUuid) == str(polon_uuid) + + +@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) + + assert _zapisz_osobe_z_instytucji(_person(stary, instytucja, polon_uuid)) + assert _zapisz_osobe_z_instytucji( + _person(nowy, instytucja, polon_uuid, firstName="Janina") + ) + + # Nadal JEDEN wiersz — ``polonUuid`` jest tożsamością osoby... + osoba = OsobaZInstytucji.objects.get() + # ...przepiętą na nowy identyfikator PBN, z odświeżonymi danymi. + assert osoba.personId_id == nowy.pk + assert osoba.firstName == "Janina" + + +@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 + assert OsobaZInstytucji.objects.count() == 2 diff --git a/src/pbn_integrator/utils/scientists.py b/src/pbn_integrator/utils/scientists.py index c72857265..014cbd895 100644 --- a/src/pbn_integrator/utils/scientists.py +++ b/src/pbn_integrator/utils/scientists.py @@ -71,54 +71,85 @@ 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 skipped due to an unresolvable + identity conflict (reported to Rollbar). """ from pbn_api.models.institution import Institution from pbn_api.models.osoba_z_instytucji import OsobaZInstytucji + polon_uuid = person.get("polonUuid") + 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 polon_uuid + else None ) + + 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 + except IntegrityError: + # Zostaje przypadek naprawdę niejednoznaczny: nowy personId ma JUŻ + # swój wiersz z innym polonUuid, więc przepięcie zderza się z + # unikalnością personId. Scalenie dwóch tożsamości PBN to decyzja o + # danych, nie poprawka techniczna — raportujemy i pomijamy. + rollbar.report_exc_info( + sys.exc_info(), + extra_data={ + "personId": person.get("personId"), + "polonUuid": polon_uuid, + "firstName": person.get("firstName"), + "lastName": person.get("lastName"), + }, + ) + logger.info( + f"UWAGA: Niejednoznaczna tożsamość osoby {person.get('personId')} " + f"(polonUuid {polon_uuid}). Pomijam wpis (zalogowano do Rollbar)." + ) + return False def _get_max_workers(): From 742dd318ed09fa9952c9599ad301840c4484a171 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Pasternak?= Date: Sat, 25 Jul 2026 00:05:42 +0200 Subject: [PATCH 2/2] =?UTF-8?q?fix(pbn):=20popraw=20diagnostyk=C4=99=20po?= =?UTF-8?q?=20self-review=20(constraint=20w=20raporcie,=20brak=20polonUuid?= =?UTF-8?q?)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Uwagi z self-review poprzedniego commita: - Rozszerzenie `except IntegrityError` na gołe łapanie było celowe (import całej kadry nie ma padać przez jedną osobę), ale komentarz twierdził, że trafia tam wyłącznie kolizja personId. Trafia tam też NOT NULL na polach przysłanych przez PBN jako null i każdy inny błąd integralności — a w Rollbarze wszystkie wyglądały identycznie. Dokładamy treść naruszonego ograniczenia do extra_data i prostujemy komentarz. - Osoba bez polonUuid: zamiast dowiadywać się o tym okrężnie przez IntegrityError na NOT NULL i raportować jako "konflikt tożsamości", wychodzimy od razu z czytelnym logiem. Przy okazji znika ścieżka, w której pusty string leciał ValueError-em z UUIDField i wywracał cały import (call site nie jest osłonięty). - Testy: dołożone asercje na przepięcie institutionId (na nim stoi scoping per-uczelnia), na stabilność pk (przepięcie, nie delete+create — FK deduplikator_autorow.main_osoba_z_instytucji ma SET_NULL) oraz round-trip wszystkich 9 pól. Obie asercje zweryfikowane mutacją kodu. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01NcAqeqyqBzNEkkVnhpHDaH --- .../+pbn-polonuuid-kolizja.bugfix.rst | 11 +++-- .../tests/test_osoba_z_instytucji.py | 47 +++++++++++++++++-- src/pbn_integrator/utils/scientists.py | 40 ++++++++++------ 3 files changed, 76 insertions(+), 22 deletions(-) diff --git a/src/bpp/newsfragments/+pbn-polonuuid-kolizja.bugfix.rst b/src/bpp/newsfragments/+pbn-polonuuid-kolizja.bugfix.rst index f44023feb..ff3731ff7 100644 --- a/src/bpp/newsfragments/+pbn-polonuuid-kolizja.bugfix.rst +++ b/src/bpp/newsfragments/+pbn-polonuuid-kolizja.bugfix.rst @@ -1,7 +1,8 @@ -Import osób z API instytucji PBN nie pomija już osób, którym PBN zmienił +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ł wiersz wyłącznie po ``personId``, -więc taka osoba rozbijała się o unikalność ``polonUuid`` i była pomijana, -a błąd wracał przy każdym kolejnym imporcie. Teraz wiersz jest przepinany -na nowy identyfikator. +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 index f0030b298..0cd709fcf 100644 --- a/src/pbn_integrator/tests/test_osoba_z_instytucji.py +++ b/src/pbn_integrator/tests/test_osoba_z_instytucji.py @@ -33,13 +33,16 @@ def _person(scientist, instytucja, polon_uuid, **kw): "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_tworzy_wpis(instytucja): +def test_zapisz_osobe_z_instytucji_zapisuje_wszystkie_pola(instytucja): scientist = baker.make(Scientist) polon_uuid = uuid.uuid4() @@ -47,7 +50,33 @@ def test_zapisz_osobe_z_instytucji_tworzy_wpis(instytucja): 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 @@ -62,17 +91,26 @@ def test_zapisz_osobe_przepina_wpis_gdy_pbn_zmienil_personId(instytucja): 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, instytucja, polon_uuid, firstName="Janina") + _person(nowy, nowa_instytucja, polon_uuid, firstName="Janina") ) # Nadal JEDEN wiersz — ``polonUuid`` jest tożsamością osoby... osoba = OsobaZInstytucji.objects.get() - # ...przepiętą na nowy identyfikator PBN, z odświeżonymi danymi. + # ...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 @@ -96,4 +134,7 @@ def test_zapisz_osobe_pomija_gdy_tozsamosci_nie_da_sie_pogodzic(instytucja, mock 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 014cbd895..8fb610885 100644 --- a/src/pbn_integrator/utils/scientists.py +++ b/src/pbn_integrator/utils/scientists.py @@ -85,13 +85,23 @@ def _zapisz_osobe_z_instytucji(person): person: Person data dictionary from PBN API. Returns: - True if saved successfully, False if skipped due to an unresolvable - identity conflict (reported to Rollbar). + 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", ""), @@ -107,11 +117,7 @@ def _zapisz_osobe_z_instytucji(person): scientist = Scientist.objects.get(pk=person["personId"]) instytucja = Institution.objects.get(pk=person["institutionId"]) - osoba = ( - OsobaZInstytucji.objects.filter(polonUuid=polon_uuid).first() - if polon_uuid - else None - ) + 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 @@ -131,11 +137,16 @@ def _zapisz_osobe_z_instytucji(person): }, ) return True - except IntegrityError: - # Zostaje przypadek naprawdę niejednoznaczny: nowy personId ma JUŻ - # swój wiersz z innym polonUuid, więc przepięcie zderza się z - # unikalnością personId. Scalenie dwóch tożsamości PBN to decyzja o - # danych, nie poprawka techniczna — raportujemy i pomijamy. + except IntegrityError as e: + # Ś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={ @@ -143,11 +154,12 @@ def _zapisz_osobe_z_instytucji(person): "polonUuid": polon_uuid, "firstName": person.get("firstName"), "lastName": person.get("lastName"), + "constraint": str(e), }, ) logger.info( - f"UWAGA: Niejednoznaczna tożsamość osoby {person.get('personId')} " - f"(polonUuid {polon_uuid}). Pomijam wpis (zalogowano do Rollbar)." + f"UWAGA: Błąd integralności dla osoby {person.get('personId')} " + f"(polonUuid {polon_uuid}): {e}. Pomijam wpis (zalogowano do Rollbar)." ) return False