Public submission is demoted before the PATCH is validated, leaving zero public submissions on HTTP 400 - apps/jobs/views.py:538
- Dominant language
- Python
- Stars
- 2k
- Forks
- 984
- Avg merge
- 2h 54m
- Merged PRs (30d)
- 14
Description
`change_submission_data_and_visibility()` demotes the existing public submission **before** the main serializer validates the request. If any other field in the same PATCH is invalid, the demotion has already committed while the request returns HTTP 400 — leaving the participant team with **zero** public submissions in a restricted phase.
## Offending code
The demotion saves at line 538:
https://github.com/Cloud-CV/EvalAI/blob/f8aff86a7e613c61fae3e4b0e5c0382fe3b52a25/apps/jobs/views.py#L534-L538
```python
if submission_serializer.is_valid():
submission_serializer.save() # <-- commits here
except KeyError:
pass
```
The request as a whole is not validated until line 553, and rejected at line 558:
https://github.com/Cloud-CV/EvalAI/blob/f8aff86a7e613c61fae3e4b0e5c0382fe3b52a25/apps/jobs/views.py#L544-L561
```python
serializer = SubmissionSerializer(
submission,
data=request.data,
...
)
if serializer.is_valid():
serializer.save()
response_data = serializer.data
return Response(response_data, status=status.HTTP_200_OK)
else:
return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST)
```
Nothing wraps the two writes in a transaction, so the first one stands when the second never happens.
## Why it is wrong
The demotion is a side effect of a request that may still be rejected. The sequence for a PATCH that sets `is_public` **and** carries one invalid field is:
```
line 538: demotion .save() -> COMMITS (old submission is now private)
line 553: serializer.is_valid() -> False
line 558: return HTTP 400
```
The caller is told the request failed, but a write already landed. The new submission is not public (its save never ran) and the old one is no longer public either, so a phase with `is_restricted_to_select_one_submission=True` ends up with **zero** public submissions — a state the user cannot reach intentionally and did not ask for.
`project_url` and `publication_url` are `max_length=1000`, so triggering this needs nothing exotic:
```
>>> serializers.CharField(max_length=1000, allow_blank=True).run_validation("x"*1001)
ValidationError: ['Ensure this field has no more than 1000 characters.']
```
## Steps to reproduce
1. Create a challenge phase with `is_restricted_to_select_one_submission=True` and one public submission (call it A).
2. As a participant, `PATCH` a second submission B at
`/api/jobs/challenge//challenge_phase//submission/`
with **both**:
```json
{"is_public": true, "project_url": "<1001 characters>"}
```
3. The response is HTTP 400 (`project_url` too long).
4. Query the phase: **A is now private** and B is still private. The team has no public submission, despite the request having failed.
## Expected behaviour
- A PATCH that returns HTTP 400 causes no visibility change at all.
- The request is validated in full before any submission is demoted.
- The demotion and the target's save either both apply or neither does.
## Relationship to #4830 and #5227
This is a third distinct defect in the same block, and I want to be clear about how they differ so they are not collapsed into one:
- **#4830** (open, PR #4831): two requests racing can both demote the same submission and end with *two* public ones. Concurrency.
- **#5227** (my PR #5228): `is_public is True` never matches form-encoded input, so the demotion is skipped entirely. Type coercion.
- **This issue**: the demotion commits even when the request is subsequently rejected, ending with *zero* public ones. Ordering/atomicity.
The ordering problem here is independent of concurrency and reproduces single-threaded. Note that fixing #5227 makes this path reachable for form-encoded clients, where the demotion previously never fired at all — so this becomes easier to hit once that lands, which is why I am reporting it separately rather than leaving it implicit.
A fix likely wants the full request validated first, and the demotion plus the target's save wrapped in `transaction.atomic()`. That overlaps with the locking work already proposed in PR #4831, so it may make sense to address both there, or to sequence this after it.
---
Reported while addressing review feedback on #5228. I have not opened a PR for this one, to avoid conflicting with the open PR #4831 that touches the same lines.
Contributor guide
Research direction
Start in apps/jobs/views.py at change_submission_data_and_visibility(), especially the demotion around lines 534-538 and the main SubmissionSerializer validation around lines 544-561. Reproduce the PATCH against the submission endpoint with is_public=true and an overlong project_url, then add regression coverage showing that an HTTP 400 leaves both submissions' visibility unchanged and successful updates apply the visibility change consistently.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- django, python
- Domain
- api, backend, database
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 68/100