fix: add metadata JSON parsing with error handling in get_cluster_images

[Manasvibansal1]

Fixes #705

Problem

The GET /face-clusters/{id}/images endpoint was crashing with 500 error
when metadata was stored as a JSON string instead of a dict. Invalid JSON
metadata also caused unhandled crashes.

Changes Made

• Added parse_metadata() helper in app/routes/face_clusters.py to
  safely parse metadata strings into dicts
• Handles invalid JSON gracefully by returning None instead of crashing
• Added error handling in app/database/face_clusters.py

Tests Added

4 new tests in tests/test_face_clusters.py:

• test_metadata_string_is_parsed : string metadata parsed to dict
• test_metadata_dict_passes_through : dict metadata stays unchanged
• test_invalid_metadata_returns_none : broken JSON returns None safely
• test_null_metadata_handled — None metadata handled safely

Test Results

All 30 tests passing

Summary by CodeRabbit

• Bug Fixes

  • Improved handling of cluster image metadata so invalid JSON or unexpected values no longer cause errors.
  • Cluster image results now return null for metadata when it can’t be decoded or isn’t a valid object.

• Tests

  • Added/expanded coverage for metadata parsing: JSON strings, already-parsed objects, invalid JSON, and missing metadata.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 5f8395ac-027f-4001-a7ad-1fe5dc23eaa8

📥 Commits

Reviewing files that changed from the base of the PR and between 06969ec and 6ca982e.

📒 Files selected for processing (3)

• backend/app/routes/face_clusters.py
• backend/test_db.sqlite3
• backend/tests/test_face_clusters.py

💤 Files with no reviewable changes (1)

• backend/tests/test_face_clusters.py

🚧 Files skipped from review as they are similar to previous changes (1)

• backend/app/routes/face_clusters.py

---

Walkthrough

Face cluster image metadata is now parsed from JSON before being returned by the API, with invalid or missing metadata mapped to None. Tests cover JSON strings, dict values, and malformed JSON.

Changes

Face cluster metadata handling

Layer / File(s)	Summary
Database metadata parsing backend/app/database/face_clusters.py	db_get_images_by_cluster_id now catches JSONDecodeError when parsing row metadata and returns None for invalid JSON.
Route response metadata parsing backend/app/routes/face_clusters.py	get_cluster_images parses string metadata before building ImageInCluster responses and keeps only dict values.
API metadata tests backend/tests/test_face_clusters.py	Tests cover parsed JSON metadata, unchanged dict metadata, invalid JSON returning None, and null metadata handling.

Sequence Diagram(s)

Estimated Code Review Effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

• AOSSIE-Org/PictoPy#552: Also updates /face_clusters/{cluster_id}/images metadata parsing and the matching tests.
• AOSSIE-Org/PictoPy#598: Also changes db_get_images_by_cluster_id to deserialize metadata before returning it.

Suggested labels

Python

Suggested reviewers

• rahulharpal1603

Poem

🐇 I hopped through JSON, neat and spry,
Strings turned to objects before they fly.
Bad bits now rest in a gentle None,
And tests twinkle bright when the parsing is done.
Carrot-approved! 🥕

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the main change: safe metadata JSON parsing in get_cluster_images.
Linked Issues check	✅ Passed	The PR fixes #705 by parsing string metadata into objects, preserving dicts, and handling invalid JSON without crashing.
Out of Scope Changes check	✅ Passed	The test updates and parser cleanup stay focused on the metadata parsing fix and do not introduce unrelated changes.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

backend/tests/test_face_clusters.py (1)

442-534: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Remove the duplicated metadata tests.

These methods duplicate the ones above with the same names, so Python keeps only the later definitions. The extra block is dead code, and the second GET in test_null_metadata_handled does not add new coverage.

Suggested cleanup

-    # ============================================================================
-    # Metadata JSON Parsing Tests (Fix for Issue `#705`)
-    # ============================================================================
-
-    `@patch`("app.routes.face_clusters.db_get_images_by_cluster_id")
-    `@patch`("app.routes.face_clusters.db_get_cluster_by_id")
-    def test_metadata_string_is_parsed(self, mock_get_cluster, mock_get_images):
-        ...
-
-    `@patch`("app.routes.face_clusters.db_get_images_by_cluster_id")
-    `@patch`("app.routes.face_clusters.db_get_cluster_by_id")
-    def test_metadata_dict_passes_through(self, mock_get_cluster, mock_get_images):
-        ...
-
-    `@patch`("app.routes.face_clusters.db_get_images_by_cluster_id")
-    `@patch`("app.routes.face_clusters.db_get_cluster_by_id")
-    def test_invalid_metadata_returns_none(self, mock_get_cluster, mock_get_images):
-        ...
-
     `@patch`("app.routes.face_clusters.db_get_images_by_cluster_id")
     `@patch`("app.routes.face_clusters.db_get_cluster_by_id")
     def test_null_metadata_handled(self, mock_get_cluster, mock_get_images):
         """None metadata from database should be handled safely."""
@@
         response = client.get("/face_clusters/cluster_123/images")

         assert response.status_code == 200
         assert response.json()["data"]["images"][0]["metadata"] is None
-
-        response = client.get("/face_clusters/cluster_123/images")
-
-        assert response.status_code == 200
-        assert response.json()["data"]["images"][0]["metadata"] is None

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@backend/tests/test_face_clusters.py` around lines 442 - 534, The metadata
parsing tests in test_face_clusters are duplicated, and the later definitions
override the earlier ones, leaving dead code. Remove the repeated test methods
in the face cluster metadata section, especially the duplicate
test_metadata_string_is_parsed, test_metadata_dict_passes_through,
test_invalid_metadata_returns_none, and test_null_metadata_handled blocks, and
keep only one version of each test in the test class. Also drop the extra
repeated client.get call inside test_null_metadata_handled since it adds no
coverage.

Source: Path instructions

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@backend/app/routes/face_clusters.py`:
- Around line 181-195: The parse_metadata helper in the image cluster response
should normalize only object-shaped JSON for ImageInCluster.metadata. Update
parse_metadata so that when metadata is a string it returns the parsed value
only if it is a dict/object, and otherwise returns None for arrays, scalars, or
invalid JSON. Keep the ImageInCluster construction using parse_metadata so
metadata stays within dict | None and does not fall through to the 500 handler.

---

Nitpick comments:
In `@backend/tests/test_face_clusters.py`:
- Around line 442-534: The metadata parsing tests in test_face_clusters are
duplicated, and the later definitions override the earlier ones, leaving dead
code. Remove the repeated test methods in the face cluster metadata section,
especially the duplicate test_metadata_string_is_parsed,
test_metadata_dict_passes_through, test_invalid_metadata_returns_none, and
test_null_metadata_handled blocks, and keep only one version of each test in the
test class. Also drop the extra repeated client.get call inside
test_null_metadata_handled since it adds no coverage.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 6de7e23a-c9c7-4c7e-9aa2-0cb227350630

📥 Commits

Reviewing files that changed from the base of the PR and between b3eee2c and 06969ec.

📒 Files selected for processing (4)

• backend/app/database/face_clusters.py
• backend/app/routes/face_clusters.py
• backend/test_db.sqlite3
• backend/tests/test_face_clusters.py