feat: implement photo uploads with file validation - #324
Conversation
📝 WalkthroughWalkthroughAdds JPEG-only photo attachment support to the suggest endpoint (multipart parsing, validation, and notifier forwarding), enriches LocationValidationError logs with suggestion UUIDs and validation errors, bumps the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CoreAPI as Core API
participant Validator as PhotoAttachmentClass
participant Notifier
Client->>CoreAPI: POST /suggest (multipart/form-data with photo)
CoreAPI->>CoreAPI: parse multipart, read file bytes, infer MIME
CoreAPI->>Validator: create/validate attachment (filename, bytes, mime)
alt valid
Validator-->>CoreAPI: PhotoAttachment
CoreAPI->>Notifier: notifier_function(..., attachments=[PhotoAttachment])
Notifier-->>CoreAPI: ack
CoreAPI-->>Client: 200/201 Created
else invalid
Validator-->>CoreAPI: raises ValueError / returns error
CoreAPI-->>Client: 400 Bad Request (photo validation error)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
goodmap/goodmap.py (1)
81-85: MAX_CONTENT_LENGTH may be too restrictive for photo uploads.The 100KB limit will reject most photo uploads before they reach the application. Typical smartphone photos are 2-5MB. Consider increasing this limit to accommodate the new photo upload feature, or document that photos must be pre-compressed client-side.
🤖 Fix all issues with AI agents
In `@goodmap/core_api.py`:
- Around line 56-105: Run black on the file to fix formatting, then in
compress_photo make filename handling robust and adjust try/except flow: replace
the naive filename.rsplit logic with a safe split (e.g., use os.path.splitext or
check for '.' to produce base + ".jpg") so extensionless names become
"photo.jpg", and refactor the try/except so the function assigns
compressed_content, new_mime_type, new_filename inside the try and only performs
a single return at the end (use the except to set them back to original values
and log the warning) instead of returning from inside the except; this will
satisfy static analysis (TRY300) and keep logging intact.
- Around line 48-51: The compress_photo function is never invoked and the config
values conflict (PHOTO_COMPRESSION_THRESHOLD = 2MB vs MAX_CONTENT_LENGTH =
100KB), causing Flask to reject large uploads before compression; fix by calling
compress_photo in the upload flow before creating the attachment (locate the
upload handler that calls photo_attachment_class and invoke
compress_photo(file_bytes or file_stream) and use its returned bytes when
constructing photo_attachment_class), and reconcile the config by either
lowering PHOTO_COMPRESSION_THRESHOLD to be <= MAX_CONTENT_LENGTH or increasing
MAX_CONTENT_LENGTH so compression can run (adjust the constants
PHOTO_COMPRESSION_THRESHOLD and/or MAX_CONTENT_LENGTH accordingly).
🧹 Nitpick comments (3)
goodmap/core_api.py (1)
213-228: JPEG-only validation is good, but consider applying compression here.The photo validation flow correctly uses
photo_attachment_classto enforce JPEG-only uploads. However, sincecompress_photois defined but unused, consider whether compression should be applied to the uploaded photo content before creating the attachment (if the intent was to compress large uploads).If compression is not needed, consider removing the unused
compress_photofunction and related constants to reduce dead code.goodmap/goodmap.py (2)
64-73: Variable shadowing and handler accumulation concerns.
The loop variable
logger_nameis fine, but note thatlogging.getLogger(logger_name)in the loop creates/retrieves different loggers - this works correctly.Handler accumulation: If
create_app_from_configis called multiple times (e.g., in tests), handlers will accumulate on the loggers. Consider checking if handlers already exist:Suggested fix to prevent handler accumulation
if os.environ.get("FLASK_DEBUG") == "1": handler = logging.StreamHandler() handler.setLevel(logging.DEBUG) handler.setFormatter(logging.Formatter("%(name)s - %(levelname)s - %(message)s")) for logger_name in ("goodmap", "platzky"): - logging.getLogger(logger_name).setLevel(logging.DEBUG) - logging.getLogger(logger_name).addHandler(handler) + log = logging.getLogger(logger_name) + log.setLevel(logging.DEBUG) + if not log.handlers: + log.addHandler(handler) logger.info("Debug logging enabled")
125-143: Debug wrapper is functional but could address TRY300.The wrapper correctly logs notification attempts and errors. The static analysis hint (TRY300) suggests moving
return resultto anelseblock for cleaner exception flow, but this is optional given the re-raise pattern.Optional: Address TRY300
def debug_notify(message: str, attachments: list[AttachmentProtocol] | None = None): logger.debug("Notifier called with message: %s", message) if attachments: logger.debug( "Attachments: %s", [(a.filename, len(a.content), a.mime_type) for a in attachments], ) try: result = original_notify(message, attachments=attachments) logger.debug("Notifier succeeded, result type: %s", type(result).__name__) - return result except Exception as e: logger.error( "Notifier failed: %s: %s", type(e).__name__, str(e), exc_info=True, ) raise + else: + return result
🧪 E2E Test Results📊 View full workflow run
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@goodmap/core_api.py`:
- Around line 210-216: The warning currently logs potentially sensitive values
by interpolating e.validation_errors into the message in the suggest endpoint;
change the logger.warning call in the except LocationValidationError block to
avoid including raw validation values in the message string—use either str(e)
(which is implemented to hide details) or a sanitized summary (e.g., list of
field names or a count) for the human-readable message, and keep the full
e.validation_errors only in the extra={} payload for structured logging; ensure
the return behavior (make_response(jsonify({"message":
ERROR_INVALID_LOCATION_DATA}), 400)) remains unchanged.
🧪 E2E Test Results📊 View full workflow run
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@goodmap/core_api.py`:
- Around line 167-169: The return statement inside the exception handler (the
call to make_response/jsonify with str(e)) is incorrectly formatted for Black;
run the auto-formatter (black goodmap/core_api.py) or reformat the call to
follow Black's style so the call to make_response(jsonify({"message": str(e)}),
400) is formatted correctly (ensure parentheses and commas follow Black's
line-wrapping rules) so the Black CI check passes.
- Around line 208-209: The call notifier_function(notifier_message,
attachments=attachments) passes an unsupported kwarg and will raise a TypeError;
update the call in goodmap/core_api.py to call notifier_function only with the
message (notifier_function(notifier_message)) or, if you must support custom
notifiers that accept attachments, detect support first (e.g.,
inspect.signature/notifier_function.__code__.co_varnames or hasattr for a custom
wrapper) and only pass attachments when the notifier declares an attachments
parameter; ensure references include notifier_function, notifier_message and the
attachments variable so you modify the correct call site.
🧹 Nitpick comments (1)
goodmap/goodmap.py (1)
22-23: Unused logger initialization.The
loggeris initialized but never used in this file. Consider removing it, or if logging is planned for future use, add a TODO comment.♻️ Suggested removal
-logger = logging.getLogger(__name__) -Also remove the import if not needed elsewhere:
-import logging
🧪 E2E Test Results📊 View full workflow run
|
🧪 E2E Test Results📊 View full workflow run
|
🧪 E2E Test Results📊 View full workflow run 📊 E2E Stress Test Performance✅ Status: PASSED (13685.54ms max < 25000ms limit)
📈 Individual Run Times
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
goodmap/goodmap.py (1)
70-74: Configuration mismatch:MAX_CONTENT_LENGTHvsmax_size.
MAX_CONTENT_LENGTHis set to 100KB (line 74), butphoto_attachment_config.max_sizeis 5MB (line 110). Flask will reject any request exceeding 100KB before photo validation logic executes, making the 5MB limit unreachable.Either increase
MAX_CONTENT_LENGTHto accommodate 5MB photos plus overhead, or reducemax_sizeto fit within the 100KB limit.Proposed fix
# SECURITY: Set maximum request body size to 100KB (prevents memory exhaustion) # This protects against large file uploads and JSON payloads - # Based on calculation: ~6.5KB max legitimate payload + multipart overhead + # Based on calculation: 5MB photo + multipart overhead + form fields if "MAX_CONTENT_LENGTH" not in app.config: - app.config["MAX_CONTENT_LENGTH"] = 100 * 1024 # 100KB + app.config["MAX_CONTENT_LENGTH"] = 6 * 1024 * 1024 # 6MB (5MB photo + overhead)Also applies to: 107-111
tests/unit_tests/test_core_api.py (1)
1-14: Fix Black formatting to pass CI.The pipeline indicates Black formatting check failed on this file. Run
black tests/unit_tests/test_core_api.pyto fix.goodmap/core_api.py (2)
1-11: Fix Black formatting to pass CI.The pipeline indicates Black formatting check failed. Run
black goodmap/core_api.pyto fix.
78-87: Mutable default argumentfeature_flags={}.Using a mutable default (
{}) can cause unexpected behavior if the dict is modified. Replace withNoneand initialize inside the function.Proposed fix
def core_pages( database, languages: LanguagesMapping, notifier_function, csrf_generator, location_model, photo_attachment_class: type[AttachmentProtocol], photo_attachment_config: AttachmentConfig, - feature_flags={}, + feature_flags=None, ) -> Blueprint: core_api_blueprint = Blueprint("api", __name__, url_prefix="/api") + if feature_flags is None: + feature_flags = {}
🧹 Nitpick comments (3)
goodmap/goodmap.py (1)
22-23: Unused logger initialization.The
loggeris initialized at module level but never used in this file. Consider removing it if not needed, or this may be intentional for future use.tests/unit_tests/test_core_api.py (2)
403-419: MoveBytesIOimport to module level.
BytesIOis imported inside multiple test functions. Move it to the module-level imports for cleaner code.Proposed fix at top of file
import json +from io import BytesIO from unittest import mockThen remove the
from io import BytesIOlines from inside each test function.
505-528: Test assumes ordered suggestion retrieval.Line 528 accesses
suggestions[-1]assuming the newly added suggestion is last. This may be fragile if the database doesn't guarantee insertion order. Consider filtering by name or using a unique identifier instead.Proposed fix
# Verify suggestion was stored suggestions = db.get_suggestions({}) assert len(suggestions) == initial_count + 1 - assert suggestions[-1]["name"] == "Test Location With Photo" + # Find the newly added suggestion by name + new_suggestion = next( + (s for s in suggestions if s["name"] == "Test Location With Photo"), None + ) + assert new_suggestion is not None
|
raven-wing
changed the title
🧪 E2E Test Results📊 View full workflow run 📊 E2E Stress Test Performance✅ Status: PASSED (11176.24ms max < 25000ms limit)
📈 Individual Run Times
|
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.