From 06969ecda17a8f9a0c96067abb5c2243cec3ecc9 Mon Sep 17 00:00:00 2001 From: Manaa Date: Fri, 26 Jun 2026 16:03:09 +0530 Subject: [PATCH 1/2] fix: add metadata JSON parsing with error handling for invalid JSON --- backend/app/database/face_clusters.py | 8 +- backend/app/routes/face_clusters.py | 15 ++- backend/test_db.sqlite3 | Bin 0 -> 102400 bytes backend/tests/test_face_clusters.py | 166 ++++++++++++++++++++++++-- 4 files changed, 179 insertions(+), 10 deletions(-) create mode 100644 backend/test_db.sqlite3 diff --git a/backend/app/database/face_clusters.py b/backend/app/database/face_clusters.py index ba9f4ba4a..b9dfbbfe6 100644 --- a/backend/app/database/face_clusters.py +++ b/backend/app/database/face_clusters.py @@ -328,7 +328,13 @@ def db_get_images_by_cluster_id( import json - metadata_dict = json.loads(metadata) if metadata else None + if metadata: + try: + metadata_dict = json.loads(metadata) + except json.JSONDecodeError: + metadata_dict = None + else: + metadata_dict = None # Parse bbox JSON if it exists bbox = None if bbox_json: diff --git a/backend/app/routes/face_clusters.py b/backend/app/routes/face_clusters.py index 4951ac960..9ff2372eb 100644 --- a/backend/app/routes/face_clusters.py +++ b/backend/app/routes/face_clusters.py @@ -174,12 +174,25 @@ def get_cluster_images(cluster_id: str): images_data = db_get_images_by_cluster_id(cluster_id) # Step 3: Convert to response models + + # Step 3: Convert to response models + import json + + def parse_metadata(metadata): + """Parse metadata if stored as JSON string.""" + if isinstance(metadata, str): + try: + return json.loads(metadata) + except json.JSONDecodeError: + return None + return metadata + images = [ ImageInCluster( id=img["image_id"], path=img["image_path"], thumbnailPath=img["thumbnail_path"], - metadata=img["metadata"], + metadata=parse_metadata(img["metadata"]), face_id=img["face_id"], confidence=img["confidence"], bbox=img["bbox"], diff --git a/backend/test_db.sqlite3 b/backend/test_db.sqlite3 new file mode 100644 index 0000000000000000000000000000000000000000..7fff565c5dc6ae1de1b88d899e7710bc2827bc3b GIT binary patch literal 102400 zcmeI(PmCK^9S897dgJk*`LmmU&Jxm2+NN2vNy#RmA>mK!Y@B49U2n3xp$#aw+59W=Xq~_^WNwEv7cwNa&5u%m{zr&mhNe%gb5)Q6JFFbL7=1{Owmv8AyZHr3)RlGB!o+J(YGfw1QCD|7h^g)L%**Xxhf(%Ws*s=3qaxf^1J zP0gLnCbEl1V}fZ_*d6y)lV(Us?|8QVx)i}H%|>1xcrdn4%wCy_Jviou@{t>*Q4fM-J8gsnTowWP=&rC zL4r{e)=0`xoLrwQ!7OprZn$7B20DZDBA5|c-!PAk!nW?! zwJ+tD=Pu`$wd=+CYu5*cY1M94m@{<#c=b-JZ0Tn6Di6eG-=~5iZ*+mv{o%dql7(16KycBZ>(q+Xpvi;UmU=Q%>Pg( zvH%1}-RZvEEhg>EWFq^*)RvRhEu9LwcK6K04ho+P3Qx|cJ188i1Mb8_LlUz`j>I0P z{6VHS%biwt4d_qHkpb7AGcfGk*BME1&&VV&KrGJi94OSAp2l!vjD zzXm+Ix!yf)(37(vz(n)C)u@6T-W z&pNzpe!j$)Tl!eh>y(Wb_JRU)cB|cF-ulYCv1DY<4K^z>OS*D9TNZgmbH|g3Z2sV| z+0u*Zj=69ufh&i zh+g4v72b&8S0%tK>n?l#v>!Iwl(X@zwgi#Jlx8C@`Ib4!cd`$5$AazPny=B@lJuU( zA_l#qn>L@;_aWnZW8?gG_db;tmj=2i$2av?r4>Q_Nc~X#qxyUGSL#pIchtAlx72T` zuc@|rQ(aMCQqQaB)D!Bn>J#dunpFOy{8jltd0+Xh@=N7i<;ThomG3HVDEF1GDK9IR zm2=9pa!65>g#2&$Z}NxoALRGs-^f3c-;sYL|3Ln>{8+v(J91tAs=O!{hQxU;3T&3+X4)+tORo_oX+buS>V2GF1={2tWV=5P$##AOHafK;YvjATPy* z!=7z>^|I4(>vEA^)^*d-Dvrq zHtCj%-QM-ZxG?G6PJMxn&@CYP8b|#+RfTr={F@(tE0_3OT-Zaqe)Sw>v~7=n(bI0~ zmX|!s+hKQ%dh)Y8#TmpiR7FE?u;lY}sA5|kFZmftwat6?^z>cqAMD@ zXTibD6fnk@Akl_={L{SlA*erwI- zG0Jt@wrwS6DAlmcDic3NnRM&7k;qX4j80pgCMUY7!?YT|hnPCb<4}8=Y~tCtaMWX# zWxAT98x7`3M@ZQ1I8|R!eumC;8@k8*my=J&g&f~Zmvus~HK|L~rwAuJRMtK5FkQVt zcf=GeE<8T_bUjYVVmW z{Rxgo8fCqfnW6%8KgFR=*E0t=AJw`|Kj|!$s+o>nt+M2PqPWM(jxO%wPJBS_rLwHa z+7!r3?BOonXt25S4PEx8)W0fT)D37`+q4F}+9EC?Pok~)sB4zr1r#>MO8hw}7 z;3I|Z+b~DU@TSqI(pIoXnwqx8ink+HqSI!{X6|1$CU(if&c^{009U<00Izz00bZa0SG|geV{{P5!EgA*^2tWV=5P$##AOHafKmY;|2nDeJ4=Ip<00bZa0SG_< z0uX=z1Rwwb2#l-%_WvW>wP+XwAOHafKmY;|fB*y_009UV{{P5!EgA*^2tWV=5P$##AOHafKmY;|2nBHeKcqkc0uX=z z1Rwwb2tWV=5P$##ATY85*#D1g*P>w%fB*y_009U<00Izz00bZaflvVZ|BwO+2tWV= z5P$##AOHafKmY;|fWXKKVE;d|U5kc600Izz00bZa0SG_<0uX=z1VRDq|3eBSAOHaf zKmY;|fB*y_009U<00JW`fc^i-b}bqP0SG_<0uX=z1Rwwb2tWV=5C{dZ{|_mUfB*y_ z009U<00Izz00bZa0SJt&fXrJqF|y4=!yo_w2tWV=5P$##AOHafKmY;|AOYO}$1s2Z z1Rwwb2tWV=5P$##AOHafjJ^QB{~uHTCD30yAOHafKmY;|fB*y_009U<00IygIf3!m J*yR1!{tKRtV|f4o literal 0 HcmV?d00001 diff --git a/backend/tests/test_face_clusters.py b/backend/tests/test_face_clusters.py index 2c09795c9..c340f6ebe 100644 --- a/backend/tests/test_face_clusters.py +++ b/backend/tests/test_face_clusters.py @@ -370,18 +370,168 @@ def test_get_cluster_images_empty(self, mock_get_cluster, mock_get_images): assert data["data"]["total_images"] == 0 assert data["data"]["images"] == [] + # ============================================================================ + # 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_get_cluster_images_database_error(self, mock_get_cluster): - """Test handling database errors during image retrieval.""" - cluster_id = "cluster_123" - mock_get_cluster.side_effect = Exception("Database connection failed") + def test_metadata_string_is_parsed(self, mock_get_cluster, mock_get_images): + """Metadata stored as JSON string should be parsed into a dict.""" + mock_get_cluster.return_value = {"cluster_id": "cluster_123", "cluster_name": "John"} + mock_get_images.return_value = [{ + "image_id": "img_1", + "image_path": "/path/to/image1.jpg", + "thumbnail_path": "/path/to/thumb1.jpg", + "metadata": '{"camera": "Canon EOS", "location": "NYC"}', + "face_id": 101, + "confidence": 0.95, + "bbox": {"x": 100, "y": 200, "width": 150, "height": 200}, + }] - response = client.get(f"/face_clusters/{cluster_id}/images") + response = client.get("/face_clusters/cluster_123/images") + data = response.json() + image = data["data"]["images"][0] - assert response.status_code == 500 + assert isinstance(image["metadata"], dict) + assert image["metadata"]["camera"] == "Canon EOS" + + @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): + """Metadata already as dict should stay unchanged.""" + mock_get_cluster.return_value = {"cluster_id": "cluster_123", "cluster_name": "John"} + mock_get_images.return_value = [{ + "image_id": "img_1", + "image_path": "/path/to/image1.jpg", + "thumbnail_path": "/path/to/thumb1.jpg", + "metadata": {"camera": "Canon EOS"}, + "face_id": 101, + "confidence": 0.95, + "bbox": {"x": 100, "y": 200, "width": 150, "height": 200}, + }] + + response = client.get("/face_clusters/cluster_123/images") data = response.json() - assert data["detail"]["success"] is False - assert data["detail"]["error"] == "Internal server error" + image = data["data"]["images"][0] + + assert isinstance(image["metadata"], dict) + assert image["metadata"]["camera"] == "Canon EOS" + + @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): + """Broken JSON metadata should return None without crashing.""" + mock_get_cluster.return_value = {"cluster_id": "cluster_123", "cluster_name": "John"} + mock_get_images.return_value = [{ + "image_id": "img_1", + "image_path": "/path/to/image1.jpg", + "thumbnail_path": "/path/to/thumb1.jpg", + "metadata": "{bad json!!!}", + "face_id": 101, + "confidence": 0.95, + "bbox": {"x": 100, "y": 200, "width": 150, "height": 200}, + }] + + response = client.get("/face_clusters/cluster_123/images") + + assert response.status_code == 200 + data = response.json() + assert data["data"]["images"][0]["metadata"] is None + + # ============================================================================ + # 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): + """Metadata stored as JSON string should be parsed into a dict.""" + mock_get_cluster.return_value = {"cluster_id": "cluster_123", "cluster_name": "John"} + mock_get_images.return_value = [{ + "image_id": "img_1", + "image_path": "/path/to/image1.jpg", + "thumbnail_path": "/path/to/thumb1.jpg", + "metadata": '{"camera": "Canon EOS", "location": "NYC"}', + "face_id": 101, + "confidence": 0.95, + "bbox": {"x": 100, "y": 200, "width": 150, "height": 200}, + }] + + response = client.get("/face_clusters/cluster_123/images") + data = response.json() + image = data["data"]["images"][0] + + assert isinstance(image["metadata"], dict) + assert image["metadata"]["camera"] == "Canon EOS" + + @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): + """Metadata already as dict should stay unchanged.""" + mock_get_cluster.return_value = {"cluster_id": "cluster_123", "cluster_name": "John"} + mock_get_images.return_value = [{ + "image_id": "img_1", + "image_path": "/path/to/image1.jpg", + "thumbnail_path": "/path/to/thumb1.jpg", + "metadata": {"camera": "Canon EOS"}, + "face_id": 101, + "confidence": 0.95, + "bbox": {"x": 100, "y": 200, "width": 150, "height": 200}, + }] + + response = client.get("/face_clusters/cluster_123/images") + data = response.json() + image = data["data"]["images"][0] + + assert isinstance(image["metadata"], dict) + assert image["metadata"]["camera"] == "Canon EOS" + + @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): + """Broken JSON metadata should return None without crashing.""" + mock_get_cluster.return_value = {"cluster_id": "cluster_123", "cluster_name": "John"} + mock_get_images.return_value = [{ + "image_id": "img_1", + "image_path": "/path/to/image1.jpg", + "thumbnail_path": "/path/to/thumb1.jpg", + "metadata": "{bad json!!!}", + "face_id": 101, + "confidence": 0.95, + "bbox": {"x": 100, "y": 200, "width": 150, "height": 200}, + }] + + response = client.get("/face_clusters/cluster_123/images") + + assert response.status_code == 200 + data = response.json() + assert data["data"]["images"][0]["metadata"] is None + + @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.""" + mock_get_cluster.return_value = {"cluster_id": "cluster_123", "cluster_name": "John"} + mock_get_images.return_value = [{ + "image_id": "img_1", + "image_path": "/path/to/image1.jpg", + "thumbnail_path": "/path/to/thumb1.jpg", + "metadata": None, + "face_id": 101, + "confidence": 0.95, + "bbox": {"x": 100, "y": 200, "width": 150, "height": 200}, + }] + + 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 # ============================================================================ # Additional Edge Case Tests From 6ca982e732513b0c28ec7408354224f688594452 Mon Sep 17 00:00:00 2001 From: Manaa Date: Fri, 26 Jun 2026 16:18:13 +0530 Subject: [PATCH 2/2] fix: address CodeRabbit review - improve parse_metadata and remove duplicate tests --- backend/app/routes/face_clusters.py | 4 +- backend/test_db.sqlite3 | Bin 102400 -> 102400 bytes backend/tests/test_face_clusters.py | 93 ---------------------------- 3 files changed, 2 insertions(+), 95 deletions(-) diff --git a/backend/app/routes/face_clusters.py b/backend/app/routes/face_clusters.py index 9ff2372eb..95470bfc1 100644 --- a/backend/app/routes/face_clusters.py +++ b/backend/app/routes/face_clusters.py @@ -182,10 +182,10 @@ def parse_metadata(metadata): """Parse metadata if stored as JSON string.""" if isinstance(metadata, str): try: - return json.loads(metadata) + metadata = json.loads(metadata) except json.JSONDecodeError: return None - return metadata + return metadata if isinstance(metadata, dict) else None images = [ ImageInCluster( diff --git a/backend/test_db.sqlite3 b/backend/test_db.sqlite3 index 7fff565c5dc6ae1de1b88d899e7710bc2827bc3b..fd5f2041fee9dcff242e4309cf39856b44b3a2e2 100644 GIT binary patch delta 22 dcmZozz}B#UZGtqT;6xc`M#08}tqF_^`T