Fix SQLite row conversion error in toggle-favourite endpoint#1211
Closed
Riddhi8077 wants to merge 10 commits intoAOSSIE-Org:mainfrom
Closed
Fix SQLite row conversion error in toggle-favourite endpoint#1211Riddhi8077 wants to merge 10 commits intoAOSSIE-Org:mainfrom
Riddhi8077 wants to merge 10 commits intoAOSSIE-Org:mainfrom
Conversation
Contributor
WalkthroughDatabase connection infrastructure enhanced with PRAGMA configurations and row_factory settings. New image retrieval function added alongside simplified metadata parsing logic. Test infrastructure refactored with temporary database fixture and comprehensive test coverage for connection and metadata operations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 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 |
Contributor
|
|
Author
|
@coderabbitai Review |
Contributor
✅ Actions performedReview triggered.
|
1 similar comment
Contributor
✅ Actions performedReview triggered.
|
Contributor
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
backend/app/database/images.py (1)
424-444: Add return type hint.The function lacks a return type annotation. Consider adding
-> dict | Noneor-> Optional[dict]for better type safety.♻️ Suggested fix
-def db_get_image_by_id(image_id: str): +def db_get_image_by_id(image_id: str) -> dict | None: """ Retrieve a single image by its ID. """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/database/images.py` around lines 424 - 444, The function db_get_image_by_id is missing a return type annotation; update its signature to include a precise return type like -> dict | None (or -> Optional[dict] with from typing import Optional) so callers and type checkers know it returns either a dict or None; modify the def db_get_image_by_id(image_id: str): line to def db_get_image_by_id(image_id: str) -> dict | None: (or use Optional[dict]) and add any necessary import.backend/tests/conftest.py (2)
7-13: Unused imports can be removed.These table creation function imports (
db_create_faces_table,db_create_images_table, etc.) appear to be remnants from the removedsetup_before_all_tests()fixture and are no longer used in the currenttemp_db_pathfixture.♻️ Suggested cleanup
import pytest import os import tempfile import sqlite3 - -# Import database table creation functions -from app.database.faces import db_create_faces_table -from app.database.images import db_create_images_table -from app.database.face_clusters import db_create_clusters_table -from app.database.yolo_mapping import db_create_YOLO_classes_table -from app.database.albums import db_create_albums_table, db_create_album_images_table -from app.database.folders import db_create_folders_table -from app.database.metadata import db_create_metadata_table🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/conftest.py` around lines 7 - 13, Remove the unused table-creation imports from conftest: delete the imports of db_create_faces_table, db_create_images_table, db_create_clusters_table, db_create_YOLO_classes_table, db_create_albums_table, db_create_album_images_table, db_create_folders_table, and db_create_metadata_table since they are no longer referenced by the temp_db_path fixture or anywhere in the file; keep only the imports actually used by the current fixtures/tests to clean up dead code.
48-48: Trailing whitespace.Line 48 contains trailing whitespace that should be removed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/conftest.py` at line 48, Open backend/tests/conftest.py and remove the trailing whitespace from the blank line that currently contains only whitespace (the empty line around the test fixtures setup) so that the file has no extra spaces at the end of that line; this is a simple whitespace cleanup—no code logic changes required.backend/tests/database/test_connection.py (1)
1-4: Unused imports.
osandtempfileare imported but not used in this test file.♻️ Suggested cleanup
-import os import sqlite3 -import tempfile import pytest🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/database/test_connection.py` around lines 1 - 4, The file imports unused modules os and tempfile which should be removed to clean up imports; update the top of backend/tests/database/test_connection.py to only import the actually used symbols (keep sqlite3 and pytest) and delete the unused os and tempfile imports so there are no unused-import warnings.
🤖 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/app/database/images.py`:
- Line 21: The file imports get_db_connection twice causing a duplicate import;
remove the redundant import statement that references get_db_connection (from
app.database.connection) so only the original import at the earlier location
remains; search for the symbol get_db_connection in
backend/app/database/images.py and delete the duplicate import line while
keeping the existing import used by any functions or classes in that module.
In `@backend/app/utils/images.py`:
- Around line 496-508: The function image_util_parse_metadata currently returns
the raw truthy metadata (which may be a JSON string) instead of a dict; update
image_util_parse_metadata to detect if metadata is a str and json.loads it
(import json), handle json.JSONDecodeError by logging or falling back to the
default dict, and if metadata is already a dict return it; keep the existing
default-return behavior when metadata is falsy so callers like db_get_all_images
and Pydantic's MetadataModel receive a proper dict.
---
Nitpick comments:
In `@backend/app/database/images.py`:
- Around line 424-444: The function db_get_image_by_id is missing a return type
annotation; update its signature to include a precise return type like -> dict |
None (or -> Optional[dict] with from typing import Optional) so callers and type
checkers know it returns either a dict or None; modify the def
db_get_image_by_id(image_id: str): line to def db_get_image_by_id(image_id: str)
-> dict | None: (or use Optional[dict]) and add any necessary import.
In `@backend/tests/conftest.py`:
- Around line 7-13: Remove the unused table-creation imports from conftest:
delete the imports of db_create_faces_table, db_create_images_table,
db_create_clusters_table, db_create_YOLO_classes_table, db_create_albums_table,
db_create_album_images_table, db_create_folders_table, and
db_create_metadata_table since they are no longer referenced by the temp_db_path
fixture or anywhere in the file; keep only the imports actually used by the
current fixtures/tests to clean up dead code.
- Line 48: Open backend/tests/conftest.py and remove the trailing whitespace
from the blank line that currently contains only whitespace (the empty line
around the test fixtures setup) so that the file has no extra spaces at the end
of that line; this is a simple whitespace cleanup—no code logic changes
required.
In `@backend/tests/database/test_connection.py`:
- Around line 1-4: The file imports unused modules os and tempfile which should
be removed to clean up imports; update the top of
backend/tests/database/test_connection.py to only import the actually used
symbols (keep sqlite3 and pytest) and delete the unused os and tempfile imports
so there are no unused-import warnings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 317c6bb4-fd50-4ac1-b601-56bada863c7f
📒 Files selected for processing (7)
backend/.gitignorebackend/app/database/connection.pybackend/app/database/images.pybackend/app/utils/images.pybackend/tests/conftest.pybackend/tests/database/test_connection.pybackend/tests/database/test_metadata.py
Contributor
|
|
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
backend/app/database/images.py (1)
21-21: 🛠️ Refactor suggestion | 🟠 MajorDuplicate import.
get_db_connectionis already imported at line 9. This has been flagged in a previous review.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/database/images.py` at line 21, The file contains a duplicate import of get_db_connection; remove the redundant import line so only the single import of get_db_connection remains (keep the original import at its first occurrence and delete the later one referencing get_db_connection in images.py), ensuring any references in functions like those in this module still use the existing imported get_db_connection.backend/app/utils/images.py (1)
496-508:⚠️ Potential issue | 🔴 CriticalCritical: JSON parsing removed breaks existing functionality.
This has already been flagged in a previous review. The function now returns the raw metadata string instead of parsing the JSON. Callers like
db_get_all_images(line 183) and the routes expect adict, but with metadata stored as TEXT (JSON string) in SQLite, this will cause Pydantic validation failures.The JSON parsing logic must be preserved.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/utils/images.py` around lines 496 - 508, The function image_util_parse_metadata currently returns the raw metadata (likely a JSON string) which breaks callers expecting a dict (e.g., db_get_all_images and various routes); update image_util_parse_metadata to detect if metadata is a string and parse it with json.loads, handle ValueError/JSONDecodeError by logging or swallowing and returning the existing default dict (same shape as the current default), and ensure the function always returns a dict; also add or ensure the json import is present and preserve existing default keys ("name","date_created","width","height","file_location","file_size","item_type") so Pydantic validation downstream continues to work.
🧹 Nitpick comments (5)
backend/tests/database/test_connection.py (1)
1-4: Remove unused imports.
osandtempfileare imported but not used in this file.♻️ Proposed fix
-import os import sqlite3 -import tempfile import pytest🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/database/test_connection.py` around lines 1 - 4, Remove the unused imports in test_connection.py by deleting the import statements for os and tempfile so only the actually used modules (sqlite3 and pytest) remain; update the top-of-file import list to no longer reference the unused symbols `os` and `tempfile` to satisfy linting and avoid unused-import warnings.backend/tests/conftest.py (2)
3-4: Remove unused import.
sqlite3is imported but not used in this file.♻️ Proposed fix
import tempfile -import sqlite3🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/conftest.py` around lines 3 - 4, Remove the unused top-level import "sqlite3" from the file (delete the line importing sqlite3) so only necessary imports remain (e.g., keep "import tempfile"); ensure no other references to sqlite3 exist in functions or fixtures such as any conftest fixtures before committing.
48-48: Minor: Trailing whitespace on line 48.There's a stray space character before the empty line at the end of the file.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/tests/conftest.py` at line 48, Remove the trailing whitespace on line 48 of backend/tests/conftest.py by deleting the stray space character at the end of that line (ensure the file ends with a clean newline only); no code logic changes required—just trim the extra space so the file ends with an empty line without trailing whitespace.backend/app/database/connection.py (1)
11-12: Consider improving the comment to explain the "why".The comment "VERY IMPORTANT" doesn't explain why
row_factoryis critical. A brief explanation would help future maintainers understand its purpose.💡 Suggested improvement
- # VERY IMPORTANT + # Enable dict-like access to rows (required for dict(row) conversions) conn.row_factory = sqlite3.Row🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/database/connection.py` around lines 11 - 12, Replace the ambiguous comment "VERY IMPORTANT" before setting conn.row_factory = sqlite3.Row with a short explanatory comment that states why it's required (e.g., to make query results sqlite3.Row objects so callers can access columns by name like row["col"] or convert rows to dicts), and mention any downstream code that depends on named-column access (reference conn.row_factory and sqlite3.Row) so future maintainers understand the rationale.backend/app/database/images.py (1)
424-444: Add return type hint for consistency with codebase patterns.The function lacks a return type annotation. Adding it improves code clarity and IDE support.
💡 Suggested improvement
-def db_get_image_by_id(image_id: str): +def db_get_image_by_id(image_id: str) -> dict | None: """ Retrieve a single image by its ID. """The function implementation correctly uses the context-managed connection with
sqlite3.Rowsupport, enablingdict(row)conversion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/app/database/images.py` around lines 424 - 444, Add a return type annotation to db_get_image_by_id to match project patterns (e.g., def db_get_image_by_id(image_id: str) -> Optional[dict]): update imports to include Optional (and Dict/Any if you prefer a more specific type) and ensure the function still returns None or dict(row) as before; reference db_get_image_by_id and get_db_connection to locate the function and adjust the typing imports at the top of the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@backend/app/database/images.py`:
- Line 21: The file contains a duplicate import of get_db_connection; remove the
redundant import line so only the single import of get_db_connection remains
(keep the original import at its first occurrence and delete the later one
referencing get_db_connection in images.py), ensuring any references in
functions like those in this module still use the existing imported
get_db_connection.
In `@backend/app/utils/images.py`:
- Around line 496-508: The function image_util_parse_metadata currently returns
the raw metadata (likely a JSON string) which breaks callers expecting a dict
(e.g., db_get_all_images and various routes); update image_util_parse_metadata
to detect if metadata is a string and parse it with json.loads, handle
ValueError/JSONDecodeError by logging or swallowing and returning the existing
default dict (same shape as the current default), and ensure the function always
returns a dict; also add or ensure the json import is present and preserve
existing default keys
("name","date_created","width","height","file_location","file_size","item_type")
so Pydantic validation downstream continues to work.
---
Nitpick comments:
In `@backend/app/database/connection.py`:
- Around line 11-12: Replace the ambiguous comment "VERY IMPORTANT" before
setting conn.row_factory = sqlite3.Row with a short explanatory comment that
states why it's required (e.g., to make query results sqlite3.Row objects so
callers can access columns by name like row["col"] or convert rows to dicts),
and mention any downstream code that depends on named-column access (reference
conn.row_factory and sqlite3.Row) so future maintainers understand the
rationale.
In `@backend/app/database/images.py`:
- Around line 424-444: Add a return type annotation to db_get_image_by_id to
match project patterns (e.g., def db_get_image_by_id(image_id: str) ->
Optional[dict]): update imports to include Optional (and Dict/Any if you prefer
a more specific type) and ensure the function still returns None or dict(row) as
before; reference db_get_image_by_id and get_db_connection to locate the
function and adjust the typing imports at the top of the file.
In `@backend/tests/conftest.py`:
- Around line 3-4: Remove the unused top-level import "sqlite3" from the file
(delete the line importing sqlite3) so only necessary imports remain (e.g., keep
"import tempfile"); ensure no other references to sqlite3 exist in functions or
fixtures such as any conftest fixtures before committing.
- Line 48: Remove the trailing whitespace on line 48 of
backend/tests/conftest.py by deleting the stray space character at the end of
that line (ensure the file ends with a clean newline only); no code logic
changes required—just trim the extra space so the file ends with an empty line
without trailing whitespace.
In `@backend/tests/database/test_connection.py`:
- Around line 1-4: Remove the unused imports in test_connection.py by deleting
the import statements for os and tempfile so only the actually used modules
(sqlite3 and pytest) remain; update the top-of-file import list to no longer
reference the unused symbols `os` and `tempfile` to satisfy linting and avoid
unused-import warnings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d91ad0b1-2dcd-40ec-894a-9b4409388f88
📒 Files selected for processing (7)
backend/.gitignorebackend/app/database/connection.pybackend/app/database/images.pybackend/app/utils/images.pybackend/tests/conftest.pybackend/tests/database/test_connection.pybackend/tests/database/test_metadata.py
Removed unused import statement for database connection.
Contributor
|
|
Contributor
|
|
Contributor
|
|
|
Closing this as unreviewed issue |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes an issue where the
/images/toggle-favouriteendpoint raised an Internal Server Error when attempting to convert SQLite query results into dictionaries.The problem occurred because SQLite rows were returned as tuples, which caused an error when using
dict(row).The fix sets the SQLite connection's
row_factorytosqlite3.Row, allowing rows to behave like dictionary objects and enabling safe conversion usingdict(row).Problem
The
/images/toggle-favouriteendpoint attempted to convert database rows to dictionaries using:dict(row)
However, the SQLite connection did not use a row factory that supports dictionary-style access. As a result, rows were returned as tuples, which caused the following error:
dictionary update sequence element #0 has length 1; 2 is required
This resulted in the endpoint returning an Internal Server Error.
Solution
To resolve this issue, the SQLite connection configuration was updated to include:
conn.row_factory = sqlite3.Row
This change ensures that query results are returned as dictionary-like objects instead of tuples.
As a result, rows can now be safely converted using
dict(row)without raising errors, and the endpoint functions as expected.Changes Made
sqlite3.Rowas the row factorydict(row)/images/toggle-favouriteendpointTesting
The fix was verified by running the backend server locally and testing the
/images/toggle-favouriteendpoint.The endpoint now returns successful responses without raising errors.
Basic functionality of the favourite toggle feature was confirmed after the change.
Before
The
/images/toggle-favouriteendpoint returned an Internal Server Error due to failure when converting SQLite rows to dictionaries.Example error:
dictionary update sequence element #0 has length 1; 2 is required
After
The database connection now uses:
conn.row_factory = sqlite3.Row
Rows are returned as dictionary-like objects, allowing safe conversion using
dict(row).The
/images/toggle-favouriteendpoint now works correctly and returns successful responses.Notes
Please let me know if any changes or improvements are required. I would be happy to update the PR accordingly.
Closes #1200
Summary by CodeRabbit
New Features
Chores