StarPunk/docs/design/phase-4-error-handling-fix.md

Phase 4: Error Handling Fix - Implementation Guide

Created: 2025-11-18 Status: Ready for Implementation Related ADR: ADR-012 HTTP Error Handling Policy Related Review: /home/phil/Projects/starpunk/docs/reviews/error-handling-rest-vs-web-patterns.md Test Failure: test_update_nonexistent_note_404

Problem Summary

The POST route for updating notes (/admin/edit/<id>) returns HTTP 302 (redirect) when the note doesn't exist, but the test expects HTTP 404. The GET route for the edit form already returns 404 correctly, so this is an inconsistency in the implementation.

Solution

Add an existence check at the start of update_note_submit() in /home/phil/Projects/starpunk/starpunk/routes/admin.py, matching the pattern used in edit_note_form().

Implementation Steps

Step 1: Modify update_note_submit() Function

File: /home/phil/Projects/starpunk/starpunk/routes/admin.py Lines: 127-164 Function: update_note_submit(note_id: int)

Add the following code after the function definition and decorator, before processing form data:

@bp.route("/edit/<int:note_id>", methods=["POST"])
@require_auth
def update_note_submit(note_id: int):
    """
    Handle note update submission

    Updates existing note with submitted form data.
    Requires authentication.

    Args:
        note_id: Database ID of note to update

    Form data:
        content: Updated markdown content (required)
        published: Checkbox for published status (optional)

    Returns:
        Redirect to dashboard on success, back to form on error

    Decorator: @require_auth
    """
    # CHECK IF NOTE EXISTS FIRST (ADDED)
    existing_note = get_note(id=note_id, load_content=False)
    if not existing_note:
        flash("Note not found", "error")
        return redirect(url_for("admin.dashboard")), 404

    # Rest of the function remains the same
    content = request.form.get("content", "").strip()
    published = "published" in request.form

    if not content:
        flash("Content cannot be empty", "error")
        return redirect(url_for("admin.edit_note_form", note_id=note_id))

    try:
        note = update_note(id=note_id, content=content, published=published)
        flash(f"Note updated: {note.slug}", "success")
        return redirect(url_for("admin.dashboard"))
    except ValueError as e:
        flash(f"Error updating note: {e}", "error")
        return redirect(url_for("admin.edit_note_form", note_id=note_id))
    except Exception as e:
        flash(f"Unexpected error updating note: {e}", "error")
        return redirect(url_for("admin.edit_note_form", note_id=note_id))

Step 2: Verify Fix with Tests

Run the failing test to verify it now passes:

uv run pytest tests/test_routes_admin.py::TestEditNote::test_update_nonexistent_note_404 -v

Expected output:

tests/test_routes_admin.py::TestEditNote::test_update_nonexistent_note_404 PASSED

Step 3: Run Full Admin Route Test Suite

Verify no regressions:

uv run pytest tests/test_routes_admin.py -v

All tests should pass.

Step 4: Verify Existing GET Route Still Works

The GET route should still return 404:

uv run pytest tests/test_routes_admin.py::TestEditNote::test_edit_nonexistent_note_404 -v

Should still pass (no changes to this route).

Code Changes Summary

File: /home/phil/Projects/starpunk/starpunk/routes/admin.py

Location: After line 129 (after function docstring, before form processing)

Add:

# Check if note exists first
existing_note = get_note(id=note_id, load_content=False)
if not existing_note:
    flash("Note not found", "error")
    return redirect(url_for("admin.dashboard")), 404

No other changes needed - the import for get_note already exists (line 15).

Why This Fix Works

Pattern Consistency

This matches the existing pattern in edit_note_form() (lines 118-122):

note = get_note(id=note_id)

if not note:
    flash("Note not found", "error")
    return redirect(url_for("admin.dashboard")), 404

Prevents Exception Handling

Without this check, the code would:

1. Try to call update_note(id=note_id, ...)
2. update_note() calls get_note() internally (line 603)
3. get_note() returns None for missing notes (line 368)
4. update_note() raises NoteNotFoundError (line 607)
5. Exception caught by except Exception (line 162)
6. Returns redirect with 302 status

With this check, the code:

1. Calls get_note(id=note_id) first
2. Returns 404 immediately if not found
3. Never calls update_note() for nonexistent notes

HTTP Semantic Correctness

• 404 Not Found: The correct HTTP status for "resource does not exist"
• 302 Found (Redirect): Used for successful operations that redirect elsewhere
• The test expects 404, which is semantically correct

User Experience

While returning 404, we still:

1. Flash an error message ("Note not found")
2. Redirect to the dashboard (safe location)
3. User sees the error in context

Flask allows returning both: return redirect(...), 404

Testing Strategy

Unit Test Coverage

This test should now pass:

def test_update_nonexistent_note_404(self, authenticated_client):
    """Test that updating a nonexistent note returns 404"""
    response = authenticated_client.post(
        "/admin/edit/99999",
        data={"content": "Updated content", "published": "on"},
        follow_redirects=False,
    )
    assert response.status_code == 404  # ✓ Should pass now

Manual Testing (Optional)

1. Start the development server
2. Log in as admin
3. Try to access /admin/edit/99999 (GET)
   • Should redirect to dashboard with "Note not found" message
   • Network tab shows 404 status
4. Try to POST to /admin/edit/99999 with form data
   • Should redirect to dashboard with "Note not found" message
   • Network tab shows 404 status

Additional Considerations

Performance Impact

Minimal: The existence check adds one database query:

• Query: SELECT * FROM notes WHERE id = ? AND deleted_at IS NULL
• With load_content=False: No file I/O
• SQLite with index: ~0.1ms
• Acceptable for single-user system

Alternative Approaches Rejected

1. Catch NoteNotFoundError specifically: Possible, but less explicit than checking first
2. Let error handler deal with it: Less flexible for per-route flash messages
3. Change test to expect 302: Wrong - test is correct, implementation is buggy

Future Improvements

Consider adding a similar check to delete_note_submit() for consistency:

@bp.route("/delete/<int:note_id>", methods=["POST"])
@require_auth
def delete_note_submit(note_id: int):
    if request.form.get("confirm") != "yes":
        flash("Deletion cancelled", "info")
        return redirect(url_for("admin.dashboard"))

    # ADD EXISTENCE CHECK
    existing_note = get_note(id=note_id, load_content=False)
    if not existing_note:
        flash("Note not found", "error")
        return redirect(url_for("admin.dashboard")), 404

    # Rest of delete logic...

However, this requires updating the test test_delete_nonexistent_note_shows_error to expect 404 instead of 200.

Expected Outcome

After implementing this fix:

1. ✓ test_update_nonexistent_note_404 passes
2. ✓ test_edit_nonexistent_note_404 still passes
3. ✓ All other admin route tests pass
4. ✓ GET and POST routes have consistent behavior
5. ✓ HTTP semantics are correct (404 for missing resources)

References

• Architectural review: /home/phil/Projects/starpunk/docs/reviews/error-handling-rest-vs-web-patterns.md
• ADR: /home/phil/Projects/starpunk/docs/decisions/ADR-012-http-error-handling-policy.md
• Current implementation: /home/phil/Projects/starpunk/starpunk/routes/admin.py
• Test file: /home/phil/Projects/starpunk/tests/test_routes_admin.py