Skip to content
This repository has been archived by the owner on Jun 1, 2022. It is now read-only.

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

Open
sentry-io bot opened this issue Jun 25, 2021 · 19 comments
Open

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

sentry-io bot opened this issue Jun 25, 2021 · 19 comments
Labels
bug Something isn't working exporters Tools for exporting data

Comments

@sentry-io
Copy link

sentry-io bot commented Jun 25, 2021

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
Copy link
Collaborator

simonw commented Jun 25, 2021

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
Copy link
Collaborator

simonw commented Jun 25, 2021

Here's the method with the failing assertion:

vial/vaccinate/core/models.py

Lines 1125 to 1146 in cdaaab0

def full_appointment_details(self, location: Optional[Location] = None):
# We often call this from contexts where the report was
# prefetched off of a location, and fetching self.location
# would be another DB query within a tight loop; support
# passing it in as an extra arg.
if location is not None:
assert location.id == self.location_id
else:
location = self.location
# Do not access self.location below; use location instead.
if self.appointment_details:
return self.appointment_details
elif location.county and self.appointment_tag.slug == "county_website":
return location.county.vaccine_reservations_url
elif self.appointment_tag.slug == "myturn_ca_gov":
return "https://myturn.ca.gov/"
elif location.website:
return location.website
elif location.provider and location.provider.appointments_url:
return location.provider.appointments_url
return None

@simonw
Copy link
Collaborator

simonw commented Jun 25, 2021

And here's where it's being called with a location that doesn't correctly match:

vial/vaccinate/core/exporter/__init__.py

Lines 248 to 271 in cdaaab0

latest = location.dn_latest_non_skip_report
if latest:
is_yes = any(
[t for t in latest.availability_tags.all() if t.group == "yes"]
)
public_notes = [latest.public_notes or None] if is_yes else ""
tags = [
t.previous_names[0] if t.previous_names else t.name
for t in latest.availability_tags.all()
]
result[-1].update(
{
"Has Report": 1,
"Appointment scheduling instructions": [
latest.full_appointment_details(location)
],
"Availability Info": tags,
"Latest report": latest.created_at.strftime(
"%Y-%m-%dT%H:%M:%S.000Z"
),
"Latest report notes": public_notes,
"Latest report yes?": 1 if is_yes else 0,
}
)

@simonw
Copy link
Collaborator

simonw commented Jun 25, 2021

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

@simonw
Copy link
Collaborator

simonw commented Jun 25, 2021 

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
Copy link
Collaborator

simonw commented Jun 25, 2021

Here's the full list of affected locations:

@simonw
Copy link
Collaborator

simonw commented Jun 25, 2021

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

@simonw
Copy link
Collaborator

simonw commented Jun 25, 2021 •
edited
Loading

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
Copy link
Collaborator

simonw commented Jun 25, 2021

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
Copy link
Collaborator

simonw commented Jun 25, 2021 •
edited
Loading

[ 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
Copy link
Collaborator

simonw commented Jun 25, 2021

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
Copy link
Collaborator

simonw commented Jun 25, 2021

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 simonw added bug Something isn't working exporters Tools for exporting data labels Jun 25, 2021
@simonw
Copy link
Collaborator

simonw commented Jun 25, 2021

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
Copy link
Collaborator

simonw commented Jun 25, 2021

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

@simonw
Copy link
Collaborator

simonw commented Jun 25, 2021

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
Copy link
Collaborator

simonw commented Jun 25, 2021

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

@simonw
Copy link
Collaborator

simonw commented Jun 25, 2021

It ran successfully.

@simonw
Copy link
Collaborator

simonw commented Jun 25, 2021

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

  • Make duplicate_of a read-only column, which can only be edited using the merge locations tool
  • Introduce a validation rule that says you can't un-check the soft_deleted column for locations that have a duplicate_of value.

@simonw simonw changed the title AssertionError in export_mapbox AssertionError in export_mapbox, caused by a corrupted merged location Jun 25, 2021
@ugotsoul
Copy link
Contributor

ugotsoul commented Jun 25, 2021 •
edited
Loading

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/

@MoralCode MoralCode mentioned this issue Jun 27, 2021
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
bug Something isn't working exporters Tools for exporting data
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@simonw @ugotsoul