Skip to content

Fix: Return metadata as dict instead of string in db_get_images_by_cluster_id #707

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
VasuS609 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix: Return metadata as dict instead of string in db_get_images_by_cluster_id #707

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion backend/app/database/face_clusters.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import sqlite3
from typing import Optional, List, Dict, TypedDict, Union
from app.config.settings import DATABASE_PATH
from app.utils.images import image_util_parse_metadata

# Type definitions
ClusterId = str
Expand Down Expand Up @@ -306,12 +307,15 @@ def db_get_images_by_cluster_id(

bbox = json.loads(bbox_json)

# Normalize metadata to a dict; downstream schemas expect an object
metadata_dict = image_util_parse_metadata(metadata)

images.append(
{
"image_id": image_id,
"image_path": image_path,
"thumbnail_path": thumbnail_path,
"metadata": metadata,
"metadata": metadata_dict,
"face_id": face_id,
"confidence": confidence,
"bbox": bbox,
Expand Down
28 changes: 15 additions & 13 deletions backend/app/logging/setup_logging.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,19 +235,21 @@ def emit(self, record: logging.LogRecord) -> None:
Args:
record: The log record to process
"""
# Get the appropriate module name
module_name = record.name
if "." in module_name:
module_name = module_name.split(".")[-1]

# Create a message that includes the original module in the format
msg = record.getMessage()

# Find the appropriate logger
logger = get_logger(module_name)

# Log the message with our custom formatting
logger.log(record.levelno, f"[uvicorn] {msg}")
# Prevent infinite recursion
if getattr(record, "_in_intercept_handler", False):
return

record._in_intercept_handler = True

try:
# Just print directly to avoid recursion issues
msg = self.format(record)
print(msg, file=sys.stdout)
except Exception as e:
print(f"Logging error: {e}", file=sys.stderr)
finally:
if hasattr(record, "_in_intercept_handler"):
delattr(record, "_in_intercept_handler")
Comment on lines +238 to +252
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check for InterceptHandler implementations across the codebase
rg -n "class InterceptHandler" --type py -A 10

Repository: AOSSIE-Org/PictoPy

Length of output: 180

🏁 Script executed:

#!/bin/bash
# Also check if there's a sync-microservice version as mentioned in learnings
find . -type f -name "setup_logging.py" | head -20

Repository: AOSSIE-Org/PictoPy

Length of output: 180

🏁 Script executed:

#!/bin/bash
# Get full context of the emit method and surrounding code in backend version
sed -n '200,260p' backend/app/logging/setup_logging.py | cat -n

Repository: AOSSIE-Org/PictoPy

Length of output: 180

🌐 Web query:

Python logging Formatter exc_info stack_info behavior format method

💡 Result:

Short answer — how Formatter.format() handles exc_info and stack_info:

  • When you call a logging method with exc_info truthy or stack_info=True, those values are stored on the LogRecord (record.exc_info and record.stack_info). The Formatter.format(record) method:
    1. computes record.message from record.msg and record.args,
    2. uses your format string to produce a first-line result,
    3. if record.exc_info is truthy it calls Formatter.formatException(record.exc_info) and appends the resulting traceback text to the formatted string,
    4. if record.stack_info is truthy it calls Formatter.formatStack(record.stack_info) and appends that text as well.
  • The default formatException uses the traceback-printing machinery (traceback.print_exception) to produce a standard exception traceback. The default formatStack returns the supplied stack-info string (traceback.print_stack output). You can override formatException/formatStack to change how those blocks are rendered. [1][2]

References:

  • Python logging docs: Formatter.formatException / formatStack and description of exc_info and stack_info behavior. [1][2]

Stack traces are being preserved in formatted output, contradicting project requirements.

The current implementation calls self.format(record) without clearing exc_info or stack_info from the LogRecord. Python's Formatter.format() method automatically appends exception tracebacks and stack information to the formatted string if these fields are set. This directly violates the project learning that "exc_info and stack_info should not be preserved" when rerouting through InterceptHandler.

To comply with project requirements, clear these fields before formatting:

record.exc_info = None
record.stack_info = None
msg = self.format(record)

The recursion guard and error handling structure are sound, but the stack trace preservation must be addressed.

🤖 Prompt for AI Agents 
In backend/app/logging/setup_logging.py around lines 238 to 252, the handler
calls self.format(record) while record.exc_info and record.stack_info may still
be set, causing formatted output to include stack traces which violates the
project rule to not preserve these fields; before calling self.format(record)
clear them by setting record.exc_info = None and record.stack_info = None, then
format and print as before, leaving the existing recursion guard, exception
handling, and final deletion of record._in_intercept_handler unchanged.

Copy link
Contributor Author

@VasuS609 VasuS609 Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the automated analysis!
This issue is unrelated to the changes in this PR (which only modifies metadata parsing behavior).
The logging-related finding is valid, but I believe it should be handled in a separate PR to keep changes atomic.

Let me know if you'd like me to open a dedicated issue/PR for that fix.

Copy link
Contributor

@coderabbitai coderabbitai bot Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@VasuS609, you're absolutely right that keeping changes atomic is the best practice here! I appreciate you acknowledging the logging concern.

Since you mentioned the logging fix should be handled separately, would you like me to create a GitHub issue for it? I can open one with the details about the stack trace preservation issue in the InterceptHandler class, including:

  • The specific problem (exc_info/stack_info being preserved in formatted output)
  • Reference to the project requirement from the learnings
  • Link back to this PR and comment thread

Just let me know, and I'll create the issue and assign it to you (or you can create it manually if you prefer). Either way, keeping this PR focused on the metadata parsing fix makes sense! 👍

🧠 Learnings used 
Learnt from: Hemil36
Repo: AOSSIE-Org/PictoPy PR: 548
File: backend/app/logging/setup_logging.py:224-243
Timestamp: 2025-10-13T16:40:22.622Z
Learning: In the PictoPy project's centralized logging system (backend/app/logging/setup_logging.py and sync-microservice/app/logging/setup_logging.py), stack traces should not be shown in logs. When rerouting logs through InterceptHandler, exc_info and stack_info should not be preserved.

Learnt from: Hemil36
Repo: AOSSIE-Org/PictoPy PR: 548
File: backend/app/database/images.py:115-115
Timestamp: 2025-10-15T07:13:34.502Z
Learning: In the PictoPy backend and sync-microservice, exception logging using `logger.error(f"...")` without stack traces (i.e., without `exc_info=True` or `logger.exception()`) is acceptable for console logging, as this is a deliberate design decision made with the maintainer for console-only logs.



def configure_uvicorn_logging(component_name: str) -> None:
Expand Down
1 change: 0 additions & 1 deletion docs/backend/backend_python/openapi.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2003,7 +2003,6 @@
"metadata": {
"anyOf": [
{
"additionalProperties": true,
"type": "object"
},
{
Expand Down