Refactor data models and improve API robustness#473
Refactor data models and improve API robustness#473RohanExploit wants to merge 4 commits intomainfrom
Conversation
- Replace custom `JSONEncodedDict` with `sqlalchemy.types.JSON` in `backend/models.py` to enable native JSON storage (e.g., JSONB in Postgres). - Fix `get_recent_issues` in `backend/routers/issues.py` to return consistent data types (list of dicts) when serving from cache, avoiding `JSONResponse` wrapper issues. - Add robust file cleanup in `create_issue` in `backend/routers/issues.py` using `try...finally` to ensure uploaded images are deleted if the issue is a duplicate or DB save fails. - Refactor `process_uploaded_image_sync` in `backend/utils.py` to reuse validation logic and remove code duplication. - Add `backend/tests/test_optimizations.py` to verify the fixes.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
✅ Deploy Preview for fixmybharat canceled.
|
✅ Deploy Preview for fixmybharat canceled.
|
🙏 Thank you for your contribution, @RohanExploit!PR Details:
Quality Checklist:
Review Process:
Note: The maintainers will monitor code quality and ensure the overall project flow isn't broken. |
There was a problem hiding this comment.
Pull request overview
This PR refactors data models and improves API robustness through several key changes: migrating from custom JSONEncodedDict to SQLAlchemy's native JSON type for better PostgreSQL JSONB support, fixing cache inconsistency in get_recent_issues to ensure proper FastAPI serialization, adding file cleanup on deduplication, and consolidating image validation logic in utils.py.
Changes:
- Replaced custom JSONEncodedDict type decorator with SQLAlchemy's native JSON type for better database performance
- Fixed get_recent_issues to return raw data instead of JSONResponse when cached, ensuring consistent Pydantic validation
- Enhanced create_issue to cleanup uploaded files when issues are deduplicated
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| backend/models.py | Migrated from custom JSONEncodedDict to SQLAlchemy's native JSON type for geographic_coverage, action_plan, and visit_images columns |
| backend/routers/issues.py | Fixed cached response type in get_recent_issues and added file cleanup for deduplicated issues |
| backend/utils.py | Refactored process_uploaded_image_sync to use existing _validate_uploaded_file_sync for validation |
| backend/tests/test_optimizations.py | Added tests for get_recent_issues return type consistency and create_issue cleanup behavior |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| img = _validate_uploaded_file_sync(file) | ||
|
|
||
| try: | ||
| img = Image.open(file.file) | ||
| original_format = img.format | ||
|
|
||
| # Resize if needed | ||
| if img.width > 1024 or img.height > 1024: | ||
| ratio = min(1024 / img.width, 1024 / img.height) | ||
| new_width = int(img.width * ratio) | ||
| new_height = int(img.height * ratio) | ||
| img = img.resize((new_width, new_height), Image.Resampling.BILINEAR) | ||
|
|
||
| # Strip EXIF | ||
| img_no_exif = Image.new(img.mode, img.size) | ||
| img_no_exif.paste(img) |
There was a problem hiding this comment.
Missing EXIF orientation handling before stripping metadata. Images with EXIF orientation tags (commonly from mobile phones) will display incorrectly after EXIF stripping. You should apply ImageOps.exif_transpose(img) immediately after calling _validate_uploaded_file_sync and before creating the new image without EXIF. This ensures the image pixels are rotated correctly before removing the orientation metadata.
| upvotes = Column(Integer, default=0, index=True) | ||
| latitude = Column(Float, nullable=True, index=True) | ||
| longitude = Column(Float, nullable=True, index=True) | ||
| location = Column(String, nullable=True) |
There was a problem hiding this comment.
The action_plan column migration is missing. The migrate_db function adds action_plan as TEXT in line 65, but the model now defines it as JSON type. This mismatch could cause issues with existing databases. You should either add a migration to alter the column type from TEXT to JSON (or JSONB for PostgreSQL), or document that a database recreation is needed for this change.
| location = Column(String, nullable=True) | |
| location = Column(String, nullable=True) | |
| # NOTE: The initial schema (via migrate_db) created action_plan as TEXT. | |
| # The model now defines this column as JSON. Existing databases should either: | |
| # 1) run a migration to alter issues.action_plan from TEXT to JSON/JSONB, or | |
| # 2) be recreated to match this JSON column definition. |
| try: | ||
| await create_issue( | ||
| request=request, | ||
| background_tasks=background_tasks, | ||
| description="Test description length check", | ||
| category="Road", | ||
| language="en", | ||
| user_email="test@example.com", | ||
| latitude=10.0, | ||
| longitude=10.0, | ||
| location="Test Loc", | ||
| image=image, | ||
| db=db | ||
| ) | ||
| except Exception as e: | ||
| assert "Failed to save issue to database" in str(e) |
There was a problem hiding this comment.
Test doesn't verify that an exception is actually raised. If create_issue doesn't raise an exception (e.g., if error handling changes), the test will continue to line 107 and potentially pass even though the expected failure didn't occur. Consider adding an explicit failure after line 102 like assert False, "Expected create_issue to raise an exception" or using pytest.raises context manager to ensure an exception is raised.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 4 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/utils.py">
<violation number="1" location="backend/utils.py:176">
P2: Defaulting to JPEG when img.format is missing breaks palette-mode images (e.g., GIF/PNG with mode "P") after resize, because PIL can’t save "P" as JPEG. This causes valid uploads to fail. Treat non-JPEG-compatible modes as PNG (or convert to RGB) when img.format is unavailable.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if img.format: | ||
| fmt = img.format | ||
| else: | ||
| fmt = 'PNG' if img.mode == 'RGBA' else 'JPEG' |
There was a problem hiding this comment.
P2: Defaulting to JPEG when img.format is missing breaks palette-mode images (e.g., GIF/PNG with mode "P") after resize, because PIL can’t save "P" as JPEG. This causes valid uploads to fail. Treat non-JPEG-compatible modes as PNG (or convert to RGB) when img.format is unavailable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/utils.py, line 176:
<comment>Defaulting to JPEG when img.format is missing breaks palette-mode images (e.g., GIF/PNG with mode "P") after resize, because PIL can’t save "P" as JPEG. This causes valid uploads to fail. Treat non-JPEG-compatible modes as PNG (or convert to RGB) when img.format is unavailable.</comment>
<file context>
@@ -159,54 +159,22 @@ def process_uploaded_image_sync(file: UploadFile) -> tuple[Image.Image, bytes]:
+ # _validate_uploaded_file_sync doesn't return the format explicitly if resized,
+ # but img.format is None if resized.
+ # If not resized, img.format is available.
+ if img.format:
+ fmt = img.format
else:
</file context>
| if img.format: | |
| fmt = img.format | |
| else: | |
| fmt = 'PNG' if img.mode == 'RGBA' else 'JPEG' | |
| if img.format: | |
| fmt = img.format | |
| else: | |
| fmt = 'PNG' if img.mode in ('RGBA', 'P', 'LA') else 'JPEG' |
…zations - Revert `backend/models.py` to use `JSONEncodedDict` instead of `sqlalchemy.types.JSON`. This fixes the schema mismatch with the existing database (which uses `TEXT` columns) causing deployment/startup failures. - Retain API fix in `backend/routers/issues.py` (returning raw list from cache instead of `JSONResponse`). - Retain resource cleanup fix in `create_issue` (deleting uploaded files on failure). - Retain code deduplication in `backend/utils.py`. - Keep `backend/tests/test_optimizations.py` to verify the API behavior.
📝 WalkthroughWalkthroughReturn path for cached recent-issues responses was simplified; image-processing validation was centralized and saving logic streamlined; new tests exercise cached/uncached recent-issues paths and create_issue cleanup; Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/models.py">
<violation number="1">
P2: Storing these JSON fields with JSONEncodedDict uses a TEXT column, so PostgreSQL can’t use JSONB operators/indexes and JSON validity isn’t enforced. If you need JSON querying/perf, keep these columns as SQLAlchemy JSON/JSONB types instead of a text-based decorator.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
backend/utils.py (1)
186-190: Preferlogger.exceptionto preserve the stack trace.
logger.errorwithoutexc_info=Truesilently drops the traceback. The static analyzer also flags this (Ruff TRY400). Since this branch is the only signal of a processing failure, the full trace is valuable.♻️ Proposed fix
- except Exception as e: - logger.error(f"Image processing failed: {e}") + except Exception as e: + logger.exception(f"Image processing failed: {e}") raise HTTPException( status_code=400, detail="Failed to process image file." )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/utils.py` around lines 186 - 190, In the except Exception as e block (the image processing failure handler) replace the logger.error call with logger.exception (or logger.error(..., exc_info=True)) so the full traceback is preserved when logging the error, keeping the HTTPException raise(detail="Failed to process image file.") unchanged; locate the except block that currently reads "except Exception as e:" and update the logging call accordingly.backend/tests/test_optimizations.py (2)
5-5:class MockTensor: passshould be split into two lines (Ruff E701).♻️ Proposed fix
-class MockTensor: pass +class MockTensor: + pass🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/test_optimizations.py` at line 5, The single-line class definition "class MockTensor: pass" violates Ruff E701; update the MockTensor declaration (class MockTensor) to use a normal multi-line class block by placing the body on the next line and indenting the "pass" (i.e., replace the single-line form with a two-line class definition for class MockTensor).
33-47: Cache state leaks between test runs — add teardown.
test_get_recent_issues_return_typewritesmock_dataintorecent_issues_cacheunder key"recent_issues_10_0"with no teardown. A subsequent call toget_recent_issues(limit=10, offset=0)anywhere in the test suite (or on the next run if the cache is module-level and persistent) will receive the stale mock data.test_get_recent_issues_uncacheddefensively callsrecent_issues_cache.clear(), so the ordering currently works, but adding explicit teardown removes the implicit dependency.♻️ Proposed fix
def test_get_recent_issues_return_type(): mock_data = [{"id": 1, "description": "test"}] recent_issues_cache.set(mock_data, "recent_issues_10_0") db = MagicMock() response = get_recent_issues(limit=10, offset=0, db=db) assert not isinstance(response, JSONResponse) assert response == mock_data assert isinstance(response, list) + + # Teardown: remove the test entry to avoid polluting other tests + recent_issues_cache.clear()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/test_optimizations.py` around lines 33 - 47, test_get_recent_issues_return_type leaves recent_issues_cache populated causing cross-test pollution; after setting recent_issues_cache.set(mock_data, "recent_issues_10_0") ensure the cache is cleared in teardown by calling recent_issues_cache.clear() (or use a test fixture/try/finally) so the cache is emptied after get_recent_issues(limit=10, offset=0) completes; update the test_get_recent_issues_return_type to clear the cache at the end (or wrap the call to get_recent_issues in a try/finally that calls recent_issues_cache.clear()) to remove the implicit dependency on test ordering.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/routers/issues.py`:
- Around line 671-673: The cache check uses a truthy test (if cached_data:)
which treats empty lists as misses; change it to an explicit None check by
testing cached_data is not None after calling recent_issues_cache.get(cache_key)
so stored empty result sets (e.g., [] saved later in the function) are honored
and avoid needless DB hits; update the guard around
recent_issues_cache.get(cache_key) and keep the variables cached_data and
cache_key as-is.
In `@backend/tests/test_optimizations.py`:
- Around line 50-110: The test is flaky because create_issue calls
check_upload_limits which mutates the real user_upload_cache and can raise
HTTPException(429) after repeated runs, and the test writes real files without
teardown; mock check_upload_limits (or reset/patch user_upload_cache) so it
returns success and cannot raise, ensure you only inspect
mock_save_image.call_args after confirming mock_save_image was called (e.g.,
check mock_save_image.call_count > 0) to avoid TypeError, and change the
save_processed_image side_effect to write into a temporary directory (or return
a temp path) and remove any created file/dirs in a finally/teardown block so
filesystem artifacts are cleaned up; reference symbols: check_upload_limits,
create_issue, save_processed_image, save_issue_db, user_upload_cache, and
mock_save_image.call_args.
In `@backend/utils.py`:
- Around line 162-163: The code calls _validate_uploaded_file_sync(file) and
immediately dereferences its result (used in Image.new(img.mode, img.size)), but
_validate_uploaded_file_sync has Optional[Image.Image] return type; add a
defensive guard after the call to check if img is None and raise a clear
HTTP/client error (e.g., raise HTTPException(status_code=400, detail="Invalid or
empty uploaded image") or ValueError that your handler maps to a 400) before
using img, so the failure produces an actionable HTTP response instead of an
AttributeError.
---
Nitpick comments:
In `@backend/tests/test_optimizations.py`:
- Line 5: The single-line class definition "class MockTensor: pass" violates
Ruff E701; update the MockTensor declaration (class MockTensor) to use a normal
multi-line class block by placing the body on the next line and indenting the
"pass" (i.e., replace the single-line form with a two-line class definition for
class MockTensor).
- Around line 33-47: test_get_recent_issues_return_type leaves
recent_issues_cache populated causing cross-test pollution; after setting
recent_issues_cache.set(mock_data, "recent_issues_10_0") ensure the cache is
cleared in teardown by calling recent_issues_cache.clear() (or use a test
fixture/try/finally) so the cache is emptied after get_recent_issues(limit=10,
offset=0) completes; update the test_get_recent_issues_return_type to clear the
cache at the end (or wrap the call to get_recent_issues in a try/finally that
calls recent_issues_cache.clear()) to remove the implicit dependency on test
ordering.
In `@backend/utils.py`:
- Around line 186-190: In the except Exception as e block (the image processing
failure handler) replace the logger.error call with logger.exception (or
logger.error(..., exc_info=True)) so the full traceback is preserved when
logging the error, keeping the HTTPException raise(detail="Failed to process
image file.") unchanged; locate the except block that currently reads "except
Exception as e:" and update the logging call accordingly.
| cached_data = recent_issues_cache.get(cache_key) | ||
| if cached_data: | ||
| return JSONResponse(content=cached_data) | ||
| return cached_data |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find the cache implementation
fd -t f "cache.py" backend/ 2>/dev/null | head -20Repository: RohanExploit/VishwaGuru
Length of output: 83
🏁 Script executed:
#!/bin/bash
# Check the lines in question from the issues.py file
cat -n backend/routers/issues.py | sed -n '670,715p'Repository: RohanExploit/VishwaGuru
Length of output: 1825
🏁 Script executed:
#!/bin/bash
# Search for the cache.get() method definition
rg -n "def get" backend/ -A 5 --type py | grep -A 5 "cache"Repository: RohanExploit/VishwaGuru
Length of output: 3137
🏁 Script executed:
#!/bin/bash
# Read the complete cache.py to understand get() return behavior
cat -n backend/cache.pyRepository: RohanExploit/VishwaGuru
Length of output: 6516
if cached_data: silently bypasses the cache for empty result sets.
When the DB legitimately returns zero issues, data = [] is stored in the cache at line 711, but if []: evaluates to False, so every subsequent request for that key re-hits the database. The get() method returns None for a cache miss and the actual cached value (including empty lists) on hit, so the guard should use an explicit None check.
🐛 Proposed fix
- cached_data = recent_issues_cache.get(cache_key)
- if cached_data:
- return cached_data
+ cached_data = recent_issues_cache.get(cache_key)
+ if cached_data is not None:
+ return cached_data🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/routers/issues.py` around lines 671 - 673, The cache check uses a
truthy test (if cached_data:) which treats empty lists as misses; change it to
an explicit None check by testing cached_data is not None after calling
recent_issues_cache.get(cache_key) so stored empty result sets (e.g., [] saved
later in the function) are honored and avoid needless DB hits; update the guard
around recent_issues_cache.get(cache_key) and keep the variables cached_data and
cache_key as-is.
| @pytest.mark.asyncio | ||
| async def test_create_issue_cleanup(): | ||
| # Mock dependencies | ||
| request = MagicMock() | ||
| background_tasks = MagicMock() | ||
| db = MagicMock() | ||
|
|
||
| # Mock file upload | ||
| image = MagicMock() | ||
| image.filename = "test.jpg" | ||
|
|
||
| # Mock process_uploaded_image to return dummy data | ||
| with patch("backend.routers.issues.process_uploaded_image") as mock_process: | ||
| mock_process.return_value = (MagicMock(), b"fake_bytes") | ||
|
|
||
| # Mock save_processed_image to create a dummy file | ||
| with patch("backend.routers.issues.save_processed_image") as mock_save_image: | ||
| def side_effect(bytes_data, path): | ||
| # Create directory if needed | ||
| os.makedirs(os.path.dirname(path), exist_ok=True) | ||
| with open(path, "wb") as f: | ||
| f.write(bytes_data) | ||
| mock_save_image.side_effect = side_effect | ||
|
|
||
| # Mock save_issue_db to raise exception | ||
| with patch("backend.routers.issues.save_issue_db") as mock_save_db: | ||
| mock_save_db.side_effect = Exception("DB Error") | ||
|
|
||
| # Mock spatial utils | ||
| with patch("backend.routers.issues.get_bounding_box") as mock_bbox: | ||
| mock_bbox.return_value = (0, 0, 0, 0) | ||
| with patch("backend.routers.issues.find_nearby_issues") as mock_nearby: | ||
| mock_nearby.return_value = [] | ||
|
|
||
| # Mock rag_service | ||
| with patch("backend.routers.issues.rag_service") as mock_rag: | ||
| mock_rag.retrieve.return_value = None | ||
|
|
||
| # Call create_issue | ||
| try: | ||
| await create_issue( | ||
| request=request, | ||
| background_tasks=background_tasks, | ||
| description="Test description length check", | ||
| category="Road", | ||
| language="en", | ||
| user_email="test@example.com", | ||
| latitude=10.0, | ||
| longitude=10.0, | ||
| location="Test Loc", | ||
| image=image, | ||
| db=db | ||
| ) | ||
| except Exception as e: | ||
| assert "Failed to save issue to database" in str(e) | ||
|
|
||
| # Check if file was cleaned up | ||
| args, _ = mock_save_image.call_args | ||
| file_path = args[1] | ||
|
|
||
| assert not os.path.exists(file_path), f"File {file_path} should have been deleted" |
There was a problem hiding this comment.
check_upload_limits is not mocked — the test becomes flaky after 5 runs per hour and produces a misleading failure.
create_issue calls check_upload_limits("test@example.com", UPLOAD_LIMIT_PER_USER) (Line 64 of issues.py), which writes to the real user_upload_cache. After 5 executions within an hour check_upload_limits raises HTTPException(429). At that point:
process_uploaded_imageis never awaited, soimage_pathstaysNone.mock_save_imageis never called, somock_save_image.call_argsisNone.- Line 107 (
args, _ = mock_save_image.call_args) raisesTypeError, completely masking the intended assertion.
Additionally, the side_effect at lines 67-72 creates the data/uploads/ directory and writes a real file; there is no teardown, leaving filesystem artifacts after each test run.
🐛 Proposed fix — mock `check_upload_limits` and add cleanup
+import shutil
`@pytest.mark.asyncio`
async def test_create_issue_cleanup():
request = MagicMock()
background_tasks = MagicMock()
db = MagicMock()
image = MagicMock()
image.filename = "test.jpg"
+ upload_dir = "data/uploads"
with patch("backend.routers.issues.process_uploaded_image") as mock_process:
mock_process.return_value = (MagicMock(), b"fake_bytes")
with patch("backend.routers.issues.save_processed_image") as mock_save_image:
def side_effect(bytes_data, path):
- os.makedirs(os.path.dirname(path), exist_ok=True)
+ os.makedirs(upload_dir, exist_ok=True)
with open(path, "wb") as f:
f.write(bytes_data)
mock_save_image.side_effect = side_effect
with patch("backend.routers.issues.save_issue_db") as mock_save_db:
mock_save_db.side_effect = Exception("DB Error")
with patch("backend.routers.issues.get_bounding_box") as mock_bbox:
mock_bbox.return_value = (0, 0, 0, 0)
with patch("backend.routers.issues.find_nearby_issues") as mock_nearby:
mock_nearby.return_value = []
with patch("backend.routers.issues.rag_service") as mock_rag:
+ with patch("backend.routers.issues.check_upload_limits"):
mock_rag.retrieve.return_value = None
- try:
- await create_issue(...)
- except Exception as e:
- assert "Failed to save issue to database" in str(e)
-
- args, _ = mock_save_image.call_args
- file_path = args[1]
- assert not os.path.exists(file_path), ...
+ try:
+ await create_issue(
+ request=request,
+ background_tasks=background_tasks,
+ description="Test description length check",
+ category="Road",
+ language="en",
+ user_email="test@example.com",
+ latitude=10.0,
+ longitude=10.0,
+ location="Test Loc",
+ image=image,
+ db=db
+ )
+ except Exception as e:
+ assert "Failed to save issue to database" in str(e)
+ finally:
+ # Cleanup test artifacts
+ if os.path.exists(upload_dir):
+ shutil.rmtree(upload_dir)
+
+ args, _ = mock_save_image.call_args
+ file_path = args[1]
+ assert not os.path.exists(file_path), \
+ f"File {file_path} should have been deleted"🧰 Tools
🪛 Ruff (0.15.2)
[warning] 103-103: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/tests/test_optimizations.py` around lines 50 - 110, The test is flaky
because create_issue calls check_upload_limits which mutates the real
user_upload_cache and can raise HTTPException(429) after repeated runs, and the
test writes real files without teardown; mock check_upload_limits (or
reset/patch user_upload_cache) so it returns success and cannot raise, ensure
you only inspect mock_save_image.call_args after confirming mock_save_image was
called (e.g., check mock_save_image.call_count > 0) to avoid TypeError, and
change the save_processed_image side_effect to write into a temporary directory
(or return a temp path) and remove any created file/dirs in a finally/teardown
block so filesystem artifacts are cleaned up; reference symbols:
check_upload_limits, create_issue, save_processed_image, save_issue_db,
user_upload_cache, and mock_save_image.call_args.
| # Use existing validation logic (which handles size limits and basic validation) | ||
| img = _validate_uploaded_file_sync(file) |
There was a problem hiding this comment.
Unguarded dereference if _validate_uploaded_file_sync ever returns None.
The return type of _validate_uploaded_file_sync is Optional[Image.Image]. While the current implementation always returns a valid image or raises, the type contract permits None. Line 167 (Image.new(img.mode, img.size)) would immediately raise AttributeError with no actionable HTTP error for the caller.
🛡️ Proposed defensive guard
img = _validate_uploaded_file_sync(file)
+ if img is None:
+ raise HTTPException(status_code=400, detail="Failed to process image file.")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Use existing validation logic (which handles size limits and basic validation) | |
| img = _validate_uploaded_file_sync(file) | |
| # Use existing validation logic (which handles size limits and basic validation) | |
| img = _validate_uploaded_file_sync(file) | |
| if img is None: | |
| raise HTTPException(status_code=400, detail="Failed to process image file.") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/utils.py` around lines 162 - 163, The code calls
_validate_uploaded_file_sync(file) and immediately dereferences its result (used
in Image.new(img.mode, img.size)), but _validate_uploaded_file_sync has
Optional[Image.Image] return type; add a defensive guard after the call to check
if img is None and raise a clear HTTP/client error (e.g., raise
HTTPException(status_code=400, detail="Invalid or empty uploaded image") or
ValueError that your handler maps to a 400) before using img, so the failure
produces an actionable HTTP response instead of an AttributeError.
- Reverted `backend/models.py` to use `JSONEncodedDict` (compatible with existing TEXT columns) to resolve deployment startup failures. - Re-applied API optimization in `backend/routers/issues.py`: `get_recent_issues` now returns raw list from cache instead of `JSONResponse`. - Re-applied file cleanup fix in `create_issue`: Uploaded images are deleted if deduplication occurs or DB save fails. - Re-applied refactoring in `backend/utils.py` to deduplicate image validation logic. - Added `backend/tests/test_optimizations.py` to verify the fixes.
🔍 Quality Reminder |
- Add `bcrypt` to `backend/requirements-render.txt` to satisfy direct import in `backend/utils.py`. - This fixes a potential deployment failure where `import bcrypt` failed despite `passlib[bcrypt]` being present (which sometimes doesn't expose the top-level module depending on installation environment).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/requirements-render.txt`:
- Line 17: Pin the bcrypt dependency to a compatible version and pin
passlib[bcrypt] to avoid runtime breakage: replace the bare "bcrypt" entry with
"bcrypt<=4.0.1" and add or update an explicit "passlib[bcrypt]==1.7.4" entry in
the requirements list so both packages are version-locked and compatible at
install time.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/requirements-render.txtbackend/routers/issues.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/routers/issues.py
| a2wsgi | ||
| python-jose[cryptography] | ||
| passlib[bcrypt] | ||
| bcrypt |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the requirements-render.txt file and surrounding context
cat -n backend/requirements-render.txt | head -30Repository: RohanExploit/VishwaGuru
Length of output: 538
🏁 Script executed:
# Check if there are other requirements files with pinning patterns
fd -e txt -e txt -e toml -e cfg requirements | head -10Repository: RohanExploit/VishwaGuru
Length of output: 150
🌐 Web query:
latest stable bcrypt version PyPI passlib compatibility 2025 2026
💡 Result:
- Latest stable
bcrypton PyPI (as of Feb 25, 2026):bcrypt 5.0.0(released Sep 25, 2025). [1] - Latest
passlibon PyPI:passlib 1.7.4(released Oct 8, 2020). [2]
Compatibility (Passlib + bcrypt, 2025–2026 reality):
bcrypt 4.1.0was yanked on PyPI explicitly due to “Incompatibility with assumptions made by passlib.” [3]- A common break is that Passlib can’t detect bcrypt’s version after changes in bcrypt metadata; a documented workaround is to restrict
bcryptto<= 4.0.1to keep Passlib working. [4]
Practical guidance: if you must stay on passlib==1.7.4, expect to pin bcrypt (often 4.0.1) rather than using the latest bcrypt (5.0.0). [2][4]
Sources
- PyPI: bcrypt release history (shows 5.0.0 on 2025-09-25) [1]
- PyPI: passlib upload date for 1.7.4 (2020-10-08) [2]
- PyPI: bcrypt 4.1.0 yanked reason mentions passlib incompatibility [3]
- Launchpad bug: Passlib cannot detect bcrypt version; workaround pins bcrypt <=4.0.1 [4]
Pin bcrypt to a compatible version with passlib[bcrypt].
Latest bcrypt versions (4.1.0+, 5.0.0) are incompatible with passlib 1.7.4—bcrypt 4.1.0 was yanked due to breaking passlib's version detection. Pin bcrypt<=4.0.1 to ensure runtime compatibility and reproducible deployments. Additionally, pin passlib[bcrypt] to avoid unexpected major version changes.
Proposed change
-passlib[bcrypt]
-bcrypt
+passlib[bcrypt]==1.7.4
+bcrypt==4.0.1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/requirements-render.txt` at line 17, Pin the bcrypt dependency to a
compatible version and pin passlib[bcrypt] to avoid runtime breakage: replace
the bare "bcrypt" entry with "bcrypt<=4.0.1" and add or update an explicit
"passlib[bcrypt]==1.7.4" entry in the requirements list so both packages are
version-locked and compatible at install time.
This PR addresses several issues and optimizations:
JSONtype. This allows PostgreSQL to useJSONBfor better performance and query capabilities, while maintaining compatibility with SQLite.get_recent_issuesto always return the raw data structure (list of dicts) instead of returning aJSONResponseobject when cached. This ensures consistent Pydantic validation and serialization by FastAPI.create_issueto guarantee that uploaded files are deleted if the issue is not created (e.g., due to deduplication or database errors), preventing storage leaks.backend/utils.pyto remove duplicated image validation logic.PR created automatically by Jules for task 5203763552477368163 started by @RohanExploit
Summary by cubic
Reverted the model JSON type change for DB compatibility and hardened the issues API; also fixed a missing bcrypt dependency to prevent deployment errors. Aligns with Linear 5203763552477368163 by ensuring consistent API responses, cleaning up unused uploads, and simplifying image processing.
Bug Fixes
Dependencies
Written for commit 8b4a63d. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Performance
Tests
Chores