feat(media): Add comprehensive logging for media uploads - v1.4.1
Implements media upload logging per docs/design/v1.4.1/media-logging-design.md Changes: - Add logging to save_media() in starpunk/media.py: * INFO: Successful uploads with file details * WARNING: Validation/optimization/variant failures * ERROR: Unexpected system errors - Remove duplicate logging in Micropub media endpoint - Add 5 comprehensive logging tests in TestMediaLogging class - Bump version to 1.4.1 - Update CHANGELOG.md All media upload operations now logged for debugging and observability. Validation errors, optimization failures, and variant generation issues are tracked at appropriate log levels. Original functionality unchanged. Test results: 28/28 media tests pass, 5 new logging tests pass 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
193
docs/design/v1.4.1/developer-questions.md
Normal file
193
docs/design/v1.4.1/developer-questions.md
Normal file
@@ -0,0 +1,193 @@
|
||||
# Developer Questions - v1.4.1 Media Upload Logging
|
||||
|
||||
**Developer**: Developer Agent
|
||||
**Date**: 2025-12-16
|
||||
**Status**: Addressed - See architect-responses.md
|
||||
|
||||
## Overview
|
||||
|
||||
I have reviewed the design document at `docs/design/v1.4.1/media-logging-design.md` and the existing code. Below are my questions and concerns before implementation.
|
||||
|
||||
## Questions
|
||||
|
||||
### 1. Variant Generation Error Handling and Re-raising
|
||||
|
||||
**Issue**: The design shows variant generation failures should be logged at WARNING level but NOT fail the overall upload (line 109: "Variant generation failure should NOT fail the overall upload"). However, the example code shows:
|
||||
|
||||
```python
|
||||
try:
|
||||
variants = generate_all_variants(...)
|
||||
except Exception as e:
|
||||
current_app.logger.warning(...)
|
||||
# Continue - original image is still usable
|
||||
```
|
||||
|
||||
**Current behavior**: Looking at the existing `save_media()` function (lines 444-456 in `starpunk/media.py`), variant generation is called **without** explicit error handling. If `generate_all_variants()` raises an exception, it will propagate up and fail the entire upload.
|
||||
|
||||
**Question**: Should I:
|
||||
- A) Add a try/except around `generate_all_variants()` to catch all exceptions, log them at WARNING, and continue (making it non-fatal)?
|
||||
- B) Only catch specific expected exceptions and let unexpected errors propagate?
|
||||
|
||||
If option A, what should be returned for `variants` in the return dict when generation fails? Empty list?
|
||||
|
||||
### 2. Unexpected Error Logging and Exception Re-raising
|
||||
|
||||
**Issue**: The design specifies an "Unexpected Error (ERROR)" log format (lines 111-122), but the pseudocode doesn't show where this would be caught.
|
||||
|
||||
**Question**: Should I add a top-level try/except in `save_media()` that catches any unexpected exception (not ValueError), logs it at ERROR level with the format shown, and then re-raises it? Or is this log format meant for callers to use (though you said not to add logging to callers)?
|
||||
|
||||
**Current behavior**: Unexpected errors (like `OSError` for disk full) will currently propagate without being logged.
|
||||
|
||||
### 3. Logging Import
|
||||
|
||||
**Minor detail**: Should I add `import logging` at the top of `starpunk/media.py`? I see the design uses `current_app.logger` which doesn't require importing `logging`, but I want to confirm there are no other logging-related imports needed.
|
||||
|
||||
**Current state**: The file already imports `from flask import current_app` (line 20), so `current_app.logger` is available.
|
||||
|
||||
### 4. Test File Naming and Location
|
||||
|
||||
**Issue**: The design references `tests/test_media.py` for unit tests (line 225), but the actual test file is named `tests/test_media_upload.py`.
|
||||
|
||||
**Question**: Should I:
|
||||
- A) Add the logging tests to the existing `tests/test_media_upload.py` file?
|
||||
- B) Create a new `tests/test_media.py` file as specified in the design?
|
||||
- C) Ask the architect to clarify/update the design?
|
||||
|
||||
**Recommendation**: Option A seems more practical since the existing file already tests `save_media()` and related functions.
|
||||
|
||||
### 5. Integration Test Scope
|
||||
|
||||
**Issue**: The design says to add integration tests to `tests/test_admin_routes.py` and `tests/test_micropub.py` (lines 244-250), but:
|
||||
- The actual file is named `tests/test_routes_admin.py` (not `test_admin_routes.py`)
|
||||
- The design says "Verify that upload failures are logged (check caplog)" but also says not to add logging to these routes
|
||||
|
||||
**Question**: Since we're removing the duplicate logging from the Micropub route and not adding any to the admin route, should these integration tests:
|
||||
- A) Test that `save_media()` logging works when called from these routes (using caplog)?
|
||||
- B) Only test that flash messages and error responses are unchanged (no logging assertions)?
|
||||
- C) Be skipped entirely since unit tests will cover the logging behavior?
|
||||
|
||||
**My understanding**: Option A - verify that when an upload fails through the admin or micropub route, the logging from `save_media()` is captured. This ensures the logging is working end-to-end.
|
||||
|
||||
### 6. Optimized Image Bytes Variable Scope
|
||||
|
||||
**Minor implementation detail**: Looking at the existing code (lines 400-401 in `starpunk/media.py`):
|
||||
|
||||
```python
|
||||
optimized_img, width, height, optimized_bytes = optimize_image(file_data, file_size)
|
||||
```
|
||||
|
||||
The variable `optimized_bytes` is already in scope and used later in the function. The success log needs to check if optimization occurred:
|
||||
|
||||
```python
|
||||
was_optimized = len(optimized_bytes) < input_size
|
||||
```
|
||||
|
||||
**Question**: Should `input_size` be captured at the start of the function (before validation) to compare against final size? The design pseudocode shows this (line 136: `input_size = len(file_data)`), and this makes sense.
|
||||
|
||||
**Current state**: The function currently has `file_size = len(file_data)` at line 398. Should I rename this to `input_size` for consistency with the design, or add a separate `input_size` variable?
|
||||
|
||||
### 7. Media ID Availability for Variant Failure Logging
|
||||
|
||||
**Issue**: The design shows that variant generation failures should log the `media_id` (line 106-108):
|
||||
|
||||
```python
|
||||
Media upload variant generation failed: filename="{original_filename}", media_id={id}, error="{error_message}"
|
||||
```
|
||||
|
||||
**Current code flow**: In `save_media()`, the `media_id` is available at line 442 after the database insert. However, variant generation happens immediately after (line 447), so the media_id is in scope. This should work fine.
|
||||
|
||||
**Confirmation**: Just confirming this is correct - we'll have the media_id available when catching variant generation exceptions. No issue here, just documenting my understanding.
|
||||
|
||||
## Edge Cases and Concerns
|
||||
|
||||
### 8. Empty File Handling
|
||||
|
||||
**Current behavior**: `validate_image()` is called first, which will fail on empty files with "Invalid or corrupted image" at line 101.
|
||||
|
||||
**Concern**: The validation failure log message includes `size={input_size}b` which would show `size=0b` for empty files. This is correct behavior, just noting it for completeness.
|
||||
|
||||
### 9. Animated GIF Rejection Logging
|
||||
|
||||
**Current behavior**: Animated GIFs >10MB are rejected in `validate_image()` with a ValueError at lines 128-132.
|
||||
|
||||
**Question**: This will be logged as a validation failure with the error message. Is this the intended behavior? The error message is quite detailed for users.
|
||||
|
||||
**My understanding**: Yes, this is correct - it's a validation failure and should be logged at WARNING level with the full error message.
|
||||
|
||||
### 10. GIF Optimization Path
|
||||
|
||||
**Current behavior**: Animated GIFs return early from `optimize_image()` at lines 175-177, returning the original `image_data` bytes.
|
||||
|
||||
**Concern**: For animated GIFs, the success log will show:
|
||||
- `optimized=False` (since `len(image_data) == input_size`)
|
||||
- `variants=1` (only the 'original' variant is created, per line 357)
|
||||
|
||||
This is correct, just documenting expected behavior.
|
||||
|
||||
## Testing Concerns
|
||||
|
||||
### 11. Mocking and Application Context
|
||||
|
||||
**Issue**: The existing tests in `tests/test_media_upload.py` use fixtures like `app` and `tmp_path`. Looking at the test structure (lines 48-50), tests are organized in classes.
|
||||
|
||||
**Question**: For the logging tests with `caplog`, should I:
|
||||
- A) Add them to existing test classes (e.g., `TestImageValidation`, `TestMediaSave`)?
|
||||
- B) Create a new test class `TestMediaLogging`?
|
||||
|
||||
**Recommendation**: Option B for clarity - a dedicated test class for logging behavior.
|
||||
|
||||
### 12. Caplog Level Setting
|
||||
|
||||
**From existing tests**: I see examples using both:
|
||||
- `caplog.at_level(logging.INFO)` (context manager)
|
||||
- `caplog.set_level(logging.WARNING)` (method call)
|
||||
|
||||
**Question**: Which pattern should I follow? The design example shows `caplog.at_level(logging.INFO)` (line 237), so I'll use that pattern unless directed otherwise.
|
||||
|
||||
## Implementation Approach
|
||||
|
||||
Based on my understanding, here's my planned implementation approach:
|
||||
|
||||
1. **Add input_size tracking** at the start of `save_media()` (rename or alias `file_size`)
|
||||
2. **Add try/except for validation** with WARNING logging (wrap existing validate call)
|
||||
3. **Add try/except for optimization** with WARNING logging (wrap existing optimize call)
|
||||
4. **Add try/except for variant generation** with WARNING logging (new error handling)
|
||||
5. **Add success logging** at the end before return
|
||||
6. **Remove duplicate logging** from `starpunk/routes/micropub.py` (line 202)
|
||||
7. **Add unit tests** to `tests/test_media_upload.py` in new `TestMediaLogging` class
|
||||
8. **Add integration tests** to existing route test files
|
||||
9. **Update version** to 1.4.1 in `starpunk/__init__.py`
|
||||
10. **Update CHANGELOG.md** with the provided entry
|
||||
|
||||
## Clarifications Needed Before Implementation
|
||||
|
||||
**Priority 1 (Blocking)**:
|
||||
- Question 1: Variant generation error handling strategy
|
||||
- Question 2: Unexpected error logging location and strategy
|
||||
|
||||
**Priority 2 (Important)**:
|
||||
- Question 4: Test file naming
|
||||
- Question 5: Integration test scope
|
||||
|
||||
**Priority 3 (Nice to have)**:
|
||||
- Question 6: Variable naming consistency
|
||||
- Questions 9, 10: Confirmation of edge case behavior
|
||||
|
||||
## Summary
|
||||
|
||||
The design is generally clear and well-specified. My main concerns are around:
|
||||
|
||||
1. The variant generation error handling strategy (fatal vs non-fatal)
|
||||
2. Where/how to log unexpected errors (OSError, etc.)
|
||||
3. Integration test scope and what to verify
|
||||
|
||||
Once these are clarified, implementation should be straightforward. The logging patterns are consistent with existing code, and the test infrastructure (caplog) is already in use.
|
||||
|
||||
## References
|
||||
|
||||
- Design document: `/home/phil/Projects/starpunk/docs/design/v1.4.1/media-logging-design.md`
|
||||
- Existing media code: `/home/phil/Projects/starpunk/starpunk/media.py`
|
||||
- Existing micropub route: `/home/phil/Projects/starpunk/starpunk/routes/micropub.py`
|
||||
- Existing admin route: `/home/phil/Projects/starpunk/starpunk/routes/admin.py`
|
||||
- Existing tests: `/home/phil/Projects/starpunk/tests/test_media_upload.py`
|
||||
- Logging examples: `/home/phil/Projects/starpunk/starpunk/media.py` (delete_media function, line 639)
|
||||
Reference in New Issue
Block a user