StarPunk/docs/reviews/error-handling-rest-vs-web-patterns.md

Architectural Review: Error Handling in Web Routes

Review Date: 2025-11-18 Reviewer: Architect Agent Status: Analysis Complete - Recommendation Provided Related Test Failure: test_update_nonexistent_note_404 in tests/test_routes_admin.py:386

Executive Summary

A test expects POST /admin/edit/99999 (updating a nonexistent note) to return HTTP 404, but the current implementation returns HTTP 302 (redirect). This mismatch reveals an inconsistency in error handling patterns between GET and POST routes.

Recommendation: Fix the implementation to match the test expectation. The POST route should return 404 when the resource doesn't exist, consistent with the GET route behavior.

Problem Statement

The Test Failure

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  # EXPECTED: 404
                                         # ACTUAL: 302

Current Implementation Behavior

The update_note_submit() function in /home/phil/Projects/starpunk/starpunk/routes/admin.py (lines 127-164) does not check if the note exists before attempting to update it. When update_note() raises NoteNotFoundError, the exception is caught by the generic Exception handler, which:

1. Flashes an error message
2. Redirects to the edit form: redirect(url_for("admin.edit_note_form", note_id=note_id))
3. Returns HTTP 302

This redirect then fails (since the note doesn't exist), but the initial response is still 302, not 404.

Root Cause Analysis

Pattern Inconsistency

The codebase has inconsistent error handling between GET and POST routes:

1. GET /admin/edit/<note_id> (lines 100-124): Explicitly checks for note existence

   note = get_note(id=note_id)
   if not note:
       flash("Note not found", "error")
       return redirect(url_for("admin.dashboard")), 404  # ✓ Returns 404
   

2. POST /admin/edit/<note_id> (lines 127-164): Does NOT check for note existence

   try:
       note = update_note(id=note_id, content=content, published=published)
       # ... success handling
   except ValueError as e:  # ← Catches InvalidNoteDataError
       flash(f"Error updating note: {e}", "error")
       return redirect(url_for("admin.edit_note_form", note_id=note_id))  # ✗ Returns 302
   except Exception as e:  # ← Would catch NoteNotFoundError
       flash(f"Unexpected error updating note: {e}", "error")
       return redirect(url_for("admin.edit_note_form", note_id=note_id))  # ✗ Returns 302
   

Why This Matters

The update_note() function in starpunk/notes.py raises NoteNotFoundError (lines 605-607) when the note doesn't exist:

existing_note = get_note(slug=slug, id=id, load_content=False)
if existing_note is None:
    identifier = slug if slug is not None else id
    raise NoteNotFoundError(identifier)  # ← This exception is raised

Since NoteNotFoundError is a subclass of NoteError (which extends Exception), it gets caught by the generic except Exception handler in the route, resulting in a redirect instead of a 404.

Existing Pattern Analysis

Pattern 1: GET Route for Edit Form (CORRECT)

File: starpunk/routes/admin.py lines 100-124

@bp.route("/edit/<int:note_id>", methods=["GET"])
@require_auth
def edit_note_form(note_id: int):
    note = get_note(id=note_id)

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

    return render_template("admin/edit.html", note=note)

Status Code: 404 User Experience: Redirects to dashboard with flash message Test: test_edit_nonexistent_note_404 (line 376) - PASSES

Pattern 2: DELETE Route (INCONSISTENT)

File: starpunk/routes/admin.py lines 167-200

The delete route does NOT explicitly check if the note exists. It relies on delete_note() which is idempotent and returns successfully even if the note doesn't exist (see starpunk/notes.py lines 774-778).

Test: test_delete_nonexistent_note_shows_error (line 443)

response = authenticated_client.post(
    "/admin/delete/99999",
    data={"confirm": "yes"},
    follow_redirects=True
)
assert response.status_code == 200  # ← Expects redirect + success (200 after following redirect)
assert b"error" in response.data.lower() or b"not found" in response.data.lower()

This test shows a different expectation: it expects a redirect (200 after following) with an error message, NOT a 404.

However, looking at the delete_note() implementation, it's idempotent - it returns successfully even if the note doesn't exist. This means the delete route won't flash an error for nonexistent notes unless we add explicit checking.

REST vs Web Form Patterns

Two Valid Approaches

Approach A: REST-Style (Strict HTTP Semantics)

• 404 for all operations on nonexistent resources
• Applies to both GET and POST
• More "API-like" behavior
• Better for programmatic clients

Approach B: Web-Form-Friendly (User Experience First)

• 404 for GET (can't show the form)
• 302 redirect for POST (show error message to user)
• More common in traditional web applications
• Better user experience (shows error in context)

Which Approach for StarPunk?

Looking at the test suite:

1. GET route test (line 376): Expects 404 ✓
2. POST route test (line 381): Expects 404 ✓
3. DELETE route test (line 443): Expects 200 (redirect + error message) ✗

The test suite is inconsistent. However, the edit tests (test_edit_nonexistent_note_404 and test_update_nonexistent_note_404) both expect 404, suggesting the intent is Approach A: REST-Style.

Architectural Decision

Recommendation: Approach A (REST-Style)

All operations on nonexistent resources should return 404, regardless of HTTP method.

Rationale

1. Consistency: GET already returns 404, POST should match
2. Test Intent: Both tests expect 404
3. API Future: StarPunk will eventually have Micropub API - REST patterns will be needed
4. Correctness: HTTP 404 is the semantically correct response for "resource not found"
5. Debugging: Clearer error signaling for developers and future API consumers

Trade-offs

Pros:

• Consistent HTTP semantics
• Easier to reason about
• Better for future API development
• Test suite alignment

Cons:

• Slightly worse UX (user sees error page instead of flash message)
• Requires custom 404 error handler for good UX
• More routes need explicit existence checks

Mitigation: Implement custom 404 error handler that shows user-friendly message with navigation back to dashboard.

Implementation Plan

Changes Required

1. Fix update_note_submit() in starpunk/routes/admin.py

Current (lines 127-164):

@bp.route("/edit/<int:note_id>", methods=["POST"])
@require_auth
def update_note_submit(note_id: int):
    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))

Proposed:

@bp.route("/edit/<int:note_id>", methods=["POST"])
@require_auth
def update_note_submit(note_id: int):
    # CHECK IF NOTE EXISTS FIRST
    from starpunk.notes import NoteNotFoundError

    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

    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))

2. Fix DELETE route consistency (OPTIONAL)

The delete route should also check for existence:

Add to delete_note_submit() before deletion:

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

    # CHECK IF NOTE EXISTS
    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

    try:
        delete_note(id=note_id, soft=False)
        flash("Note deleted successfully", "success")
    except ValueError as e:
        flash(f"Error deleting note: {e}", "error")
    except Exception as e:
        flash(f"Unexpected error deleting note: {e}", "error")

    return redirect(url_for("admin.dashboard"))

However: The test test_delete_nonexistent_note_shows_error expects 200 (redirect), not 404. This test may need updating, or we accept the inconsistency for delete operations (which are idempotent).

Recommendation: Update the delete test to expect 404 for consistency.

Testing Strategy

After implementing the fix:

1. Run test_update_nonexistent_note_404 - should PASS
2. Run test_edit_nonexistent_note_404 - should still PASS
3. Run full test suite to check for regressions
4. Consider updating test_delete_nonexistent_note_shows_error to expect 404

Consistency Matrix

Route	Method	Resource Missing	Current Behavior	Expected Behavior	Status
/admin/edit/<id>	GET	Returns 404	404	404	✓ CORRECT
/admin/edit/<id>	POST	Returns 302	302	404	✗ FIX NEEDED
/admin/delete/<id>	POST	Returns 302	302	404?	⚠ INCONSISTENT TEST

Additional Recommendations

1. Create Architecture Decision Record

Document this decision in /home/phil/Projects/starpunk/docs/decisions/ADR-012-error-handling-http-status-codes.md

2. Create Error Handling Standard

Document error handling patterns in /home/phil/Projects/starpunk/docs/standards/http-error-handling.md:

• When to return 404 vs redirect
• How to handle validation errors
• Flash message patterns
• Custom error pages

3. Exception Hierarchy Review

The exception handling in routes could be more specific:

except NoteNotFoundError as e:  # ← Should have been caught earlier
    # This shouldn't happen now that we check first
    flash("Note not found", "error")
    return redirect(url_for("admin.dashboard")), 404
except InvalidNoteDataError as e:  # ← More specific than ValueError
    flash(f"Invalid data: {e}", "error")
    return redirect(url_for("admin.edit_note_form", note_id=note_id))
except NoteSyncError as e:  # ← File/DB sync issues
    flash(f"System error: {e}", "error")
    return redirect(url_for("admin.dashboard")), 500
except Exception as e:  # ← Truly unexpected
    current_app.logger.error(f"Unexpected error in update_note_submit: {e}")
    flash("An unexpected error occurred", "error")
    return redirect(url_for("admin.dashboard")), 500

However, with the existence check at the start, NoteNotFoundError should never be raised from update_note().

Decision Summary

The Fix

Change /home/phil/Projects/starpunk/starpunk/routes/admin.py line 129-154:

Add existence check before processing form data:

# Add after function definition, before form processing
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

Why This is the Right Approach

1. Matches existing pattern: GET route already does this (line 118-122)
2. Matches test expectations: Both edit tests expect 404
3. HTTP correctness: 404 is the right status for missing resources
4. Future-proof: Will work correctly when Micropub API is added
5. Simple fix: Minimal code change, high consistency gain

What NOT to Do

Do NOT change the test to expect 302. The test is correct; the implementation is wrong.

Reason:

• Redirecting on POST to a nonexistent resource is semantically incorrect
• Makes debugging harder (did the update fail, or does the resource not exist?)
• Inconsistent with GET behavior
• Bad pattern for future API development

Conclusion

This is a bug in the implementation, not the test. The fix is straightforward: add an existence check at the start of update_note_submit(), matching the pattern used in edit_note_form().

This architectural pattern should be applied consistently across all routes:

1. Check resource existence first
2. Return 404 if not found (with user-friendly flash message)
3. Validate input
4. Perform operation
5. Handle expected exceptions
6. Return appropriate status codes

Next Steps:

1. Implement the fix in update_note_submit()
2. Run tests to verify fix
3. Consider fixing delete route for consistency
4. Document pattern in standards
5. Create ADR for HTTP error handling policy