search

CAVaccineInventory / vial

The Django application powering calltheshots.us
https://vial.calltheshots.us
MIT License
12 stars 1 forks source link

AssertionError in export_mapbox, caused by a corrupted merged location #696

Open sentry-io[bot] opened 3 years ago

sentry-io[bot] commented 3 years ago

Sentry Issue: VIAL-BR

AssertionError: 
(3 additional frame(s) were not displayed)
...
  File "api/utils.py", line 93, in replacement_view_function
    response = view_fn(request, *args, **kwargs)
  File "django/views/decorators/csrf.py", line 54, in wrapped_view
    return view_func(*args, **kwargs)
  File "api/export_mapbox.py", line 189, in export_mapbox
    _mapbox_geojson(location, expansion), option=orjson.OPT_APPEND_NEWLINE
  File "api/export_mapbox.py", line 82, in _mapbox_geojson
    "appointment_details": report.full_appointment_details(location),
  File "core/models.py", line 1127, in full_appointment_details
    assert location.id == self.location_id
simonw commented 3 years ago

Discord: https://discord.com/channels/799147121357881364/813861006718926848/858057432592941136

Ugaso tracked it to this report https://vial.calltheshots.us/admin/core/report/35224/change/ on this location: https://vial.calltheshots.us/admin/core/location/8234/history/

simonw commented 3 years ago

Here's the method with the failing assertion: https://github.com/CAVaccineInventory/vial/blob/cdaaab053a9cf1cef40104a2cdf480b7932d58f7/vaccinate/core/models.py#L1125-L1146

simonw commented 3 years ago

And here's where it's being called with a location that doesn't correctly match: https://github.com/CAVaccineInventory/vial/blob/cdaaab053a9cf1cef40104a2cdf480b7932d58f7/vaccinate/core/exporter/__init__.py#L248-L271

simonw commented 3 years ago

So it looks like latest = location.dn_latest_non_skip_report is somehow returning a report that points to a different location.

simonw commented 3 years ago 
select location.id, report.location_id
from location join report on location.dn_latest_non_skip_report_id = report.id
where location.id != report.location_id

This query should never return any results... but it's returning 318!

https://vial.calltheshots.us/dashboard/?sql=select+location.id%2C+report.location_id%0D%0Afrom+location+join+report+on+location.dn_latest_non_skip_report_id+%3D+report.id%0D%0Awhere+location.id+%21%3D+report.location_id%3Aj5QwL7F04zzyVyyLtWHruY9xzAi8SCegMq-poQIX_iY

simonw commented 3 years ago

Here's the full list of affected locations:

simonw commented 3 years ago

... and every single one of those pages is a 500 error right now.

simonw commented 3 years ago

All of those locations are soft-deleted: https://vial.calltheshots.us/dashboard/?sql=select+%27-+https%3A%2F%2Fvial.calltheshots.us%2Fadmin%2Fcore%2Flocation%2F%27+%7C%7C+location.id%2C+location.soft_deleted%0D%0Afrom+location+join+report+on+location.dn_latest_non_skip_report_id+%3D+report.id%0D%0Awhere+location.id+%21%3D+report.location_id%3A7RxSdnUXmC7u-ohRq3i29eX4EVlsch6eZ6d582VIMF4

select '- https://vial.calltheshots.us/admin/core/location/' || location.id, location.soft_deleted
from location join report on location.dn_latest_non_skip_report_id = report.id
where location.id != report.location_id
simonw commented 3 years ago

This was caused by merges. All but one of those 318 events has a populated duplicate_of_id column - so they were merged, but the dn_latest_non_skip_report_id column on the loser of the merge was not recalculated.

simonw commented 3 years ago

[ Need to figure out what's going on with the single location that wasn't the loser of a merge - https://vial.calltheshots.us/dashboard/?sql=select+location.id%2C+location.soft_deleted%2C+location.duplicate_of_id%2C+location.dn_latest_non_skip_report_id%2C+report.id+as+report_id%2C+report.location_id+as+report_location_id%0D%0Afrom+location+join+report+on+location.dn_latest_non_skip_report_id+%3D+report.id%0D%0Awhere+location.id+%21%3D+report.location_id%0D%0Aand+duplicate_of_id+is+null%3AIhGCoZ7Qpa2nlkCAxZT9ebxootYQihvKnHlXVqlPqa8 shows that to be 8038 which isn't soft-deleted either)

simonw commented 3 years ago

Looks like that one had a manual edit which broke the data model somehow: https://vial.calltheshots.us/admin/core/location/8038/history/compare/?version_id2=217907&version_id1=217877

simonw commented 3 years ago

Aha! The 317 locations here are a red-herring - the export script only considers not-soft-deleted locations, so they're not causing the error with the Mapbox export.

The location that is breaking things is the one @ugaso identified, https://vial.calltheshots.us/admin/core/location/8234/history/

simonw commented 3 years ago

It looks to me like https://vial.calltheshots.us/admin/core/location/8038/history/ is the corrupt location: it was merged into https://vial.calltheshots.us/admin/core/location/8234/ BUT a later edit in the admin tool reverted those soft_deleted and duplicate_of fields back to their pre-merged state - even though the reports had been copied across.

That edit happened within an hour of the merge. My best guess is that someone had left a tab open with the edit form for that location, then when they hit "save" later it over-wrote the data from the merge.

I think I can fix that manually now (I'll re-run the merge) - then I'll make the duplicate_of field read-only in the admin to avoid this happening again in the future.

simonw commented 3 years ago

Re-running the merge using https://vial.calltheshots.us/admin/merge-locations/?winner=ldbdr&loser=lczkk

simonw commented 3 years ago

There are now zero non-soft-deleted locations with mismatching location IDs against their denormalized report IDs: https://vial.calltheshots.us/dashboard/?sql=select+location.id%2C+location.soft_deleted%2C+location.duplicate_of_id%2C+location.dn_latest_non_skip_report_id%2C+report.id+as+report_id%2C+report.location_id+as+report_location_id%0D%0Afrom+location+join+report+on+location.dn_latest_non_skip_report_id+%3D+report.id%0D%0Awhere+location.id+%21%3D+report.location_id%0D%0Aand+duplicate_of_id+is+null%3AIhGCoZ7Qpa2nlkCAxZT9ebxootYQihvKnHlXVqlPqa8

simonw commented 3 years ago

Re-ran the mapbox export by clicking the button in https://console.cloud.google.com/cloudscheduler?project=django-vaccinateca

simonw commented 3 years ago

It ran successfully.

simonw commented 3 years ago

To avoid this happening in the future, I'm going to do two things:

ugotsoul commented 3 years ago

It looks to me like https://vial.calltheshots.us/admin/core/location/8038/history/ is the corrupt location: it was merged into https://vial.calltheshots.us/admin/core/location/8234/ BUT a later edit in the admin tool reverted those soft_deleted and duplicate_of fields back to their pre-merged state - even though the reports had been copied across.

TY for solving this! What an interesting bug.

How were you able to track the merge history of that location? I found it! Merged location dashboard details this well: https://vial.calltheshots.us/dashboard/merged-locations/