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

```python
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
   ```python
   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
   ```python
   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:

```python
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

```python
@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)
```python
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):
```python
@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**:
```python
@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**:
```python
@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:

```python
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:

```python
# 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