Crash in classification test
- Dominant language
- Python
- Stars
- 30
- Forks
- 3
- Avg merge
- 9h 28m
- Merged PRs (30d)
- 42
Description
Running unit test - this is the same as SACGF/variantgrid_private/issues/3740
```
python3 manage.py test --keepdb classification.tests.views
```
I saw this stack trace - which I think is a [new thing in Django 5](https://docs.djangoproject.com/en/5.0/releases/5.0/)
> Passing unsaved model instances to related filters is no longer allowed.
```
.Traceback (most recent call last):
File "/home/dlawrence/localwork/variantgrid/classification/models/classification_inserter.py", line 362, in insert
json_data = record.as_json(ClassificationJsonParams(
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/dlawrence/localwork/variantgrid/classification/models/classification.py", line 1964, in as_json
return populate_classification_json(self, params)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/dlawrence/localwork/variantgrid/classification/models/classification_json.py", line 90, in populate_classification_json
last_published_version = classification.last_published_version
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/dlawrence/localwork/variantgrid/classification/models/classification.py", line 1841, in last_published_version
return ClassificationModification.objects.filter(classification=self, is_last_published=True) \
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/dlawrence/localwork/variantgrid/.venv/lib/python3.11/site-packages/django/db/models/manager.py", line 87, in manager_method
return getattr(self.get_queryset(), name)(*args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/dlawrence/localwork/variantgrid/.venv/lib/python3.11/site-packages/django/db/models/query.py", line 1495, in filter
return self._filter_or_exclude(False, args, kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/dlawrence/localwork/variantgrid/.venv/lib/python3.11/site-packages/django/db/models/query.py", line 1513, in _filter_or_exclude
clone._filter_or_exclude_inplace(negate, args, kwargs)
File "/home/dlawrence/localwork/variantgrid/.venv/lib/python3.11/site-packages/django/db/models/query.py", line 1523, in _filter_or_exclude_inplace
self._query.add_q(Q(*args, **kwargs))
File "/home/dlawrence/localwork/variantgrid/.venv/lib/python3.11/site-packages/django/db/models/sql/query.py", line 1646, in add_q
clause, _ = self._add_q(q_object, can_reuse)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/dlawrence/localwork/variantgrid/.venv/lib/python3.11/site-packages/django/db/models/sql/query.py", line 1678, in _add_q
child_clause, needed_inner = self.build_filter(
^^^^^^^^^^^^^^^^^^
File "/home/dlawrence/localwork/variantgrid/.venv/lib/python3.11/site-packages/django/db/models/sql/query.py", line 1588, in build_filter
condition = self.build_lookup(lookups, col, value)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/dlawrence/localwork/variantgrid/.venv/lib/python3.11/site-packages/django/db/models/sql/query.py", line 1415, in build_lookup
lookup = lookup_class(lhs, rhs)
^^^^^^^^^^^^^^^^^^^^^^
File "/home/dlawrence/localwork/variantgrid/.venv/lib/python3.11/site-packages/django/db/models/lookups.py", line 38, in __init__
self.rhs = self.get_prep_lookup()
^^^^^^^^^^^^^^^^^^^^^^
File "/home/dlawrence/localwork/variantgrid/.venv/lib/python3.11/site-packages/django/db/models/fields/related_lookups.py", line 103, in get_prep_lookup
self.rhs = get_normalized_value(self.rhs, self.lhs)[0]
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/dlawrence/localwork/variantgrid/.venv/lib/python3.11/site-packages/django/db/models/fields/related_lookups.py", line 20, in get_normalized_value
raise ValueError("Model instances passed to related filters must be saved.")
ValueError: Model instances passed to related filters must be saved.
```
Pasted it into claude to make issue:
# Bug: `last_published_version` crashes on unsaved Classification
## Error
```
ValueError: Model instances passed to related filters must be saved.
```
## Stack Trace
```
classification_inserter.py:362 → record.as_json(ClassificationJsonParams(...))
classification.py:1964 → populate_classification_json(self, params)
classification_json.py:90 → classification.last_published_version
classification.py:1841 → ClassificationModification.objects.filter(classification=self, is_last_published=True)
```
## Root Cause
`Classification.last_published_version` is a `@property` that filters `ClassificationModification` by `classification=self`. Django resolves this FK filter using the instance's primary key. If the `Classification` has not been saved yet (`pk=None`), Django raises:
> `ValueError: Model instances passed to related filters must be saved.`
```python
# classification.py:1839
@property
def last_published_version(self) -> 'ClassificationModification':
return ClassificationModification.objects.filter(classification=self, is_last_published=True) \ # ← crashes if self.pk is None
.select_related('classification', 'classification__lab', 'classification__lab__organization') \
.first()
```
## How It's Triggered
In `classification_inserter.py`, after attempting to patch a record, `as_json()` is called unconditionally at line 362 regardless of whether the record was saved:
```python
# Lines 355-362
if not patch_response.saved and (import_run or new_source_id):
# Only saves here if BOTH conditions hold
record.update_modified = False
record.save()
# Called regardless — if record is unsaved (no pk), this crashes:
json_data = record.as_json(ClassificationJsonParams(...))
```
If `patch_response.saved` is `False` **and** neither `import_run` nor `new_source_id` is set, the record is not saved before `as_json()` is called, so `record.pk` is `None` when `last_published_version` is accessed.
## Fix Options
**Option A (defensive guard in the property):** Handle unsaved instances gracefully:
```python
@property
def last_published_version(self) -> 'ClassificationModification':
if not self.pk:
return None
return ClassificationModification.objects.filter(classification=self, is_last_published=True) \
.select_related('classification', 'classification__lab', 'classification__lab__organization') \
.first()
```
**Option B (fix the inserter):** Ensure `record` is always saved before `as_json()` is called — i.e., review the logic at lines 355–362 and save unconditionally if the record has no pk.
Option A is lower risk since it makes the property safe to call at any time, consistent with how `last_published_sync_records` already null-checks `last_published_version` before using it.
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.