From b3616f76a2af77c432654e613550b791e425336c Mon Sep 17 00:00:00 2001 From: mountain Date: Tue, 7 Jul 2026 12:06:58 +0800 Subject: [PATCH] fix: dedup race, cwd-relative image paths, unencoded legacy URLs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - SQLite dedup race: add UNIQUE(collection_name, file_hash) and switch save_document to a plain INSERT. add_document now catches the IntegrityError from a concurrent add of the same content, cleans up its managed files, and returns the winning doc_id — instead of two doc_ids for one file (each having paid for its own LLM indexing). - PDF image paths: store the absolute path to each extracted image instead of a path relative to the indexing process's cwd. The ![image](...) references broke as soon as a query ran from a different directory. - LegacyCloudAPI: URL-encode doc_id / retrieval_id path segments (added _enc()), matching CloudBackend. An id containing '/', '?', '#' or a space previously hit the wrong endpoint or produced a malformed URL. Adds regression tests: UNIQUE enforcement, the add-race resolving to the winner, absolute image paths, and encoded legacy URLs. Claude-Session: https://claude.ai/code/session_01Kx5DgKbhK1N8autqXH8SmS --- pageindex/backend/local.py | 12 ++++++++++++ pageindex/cloud_api.py | 16 +++++++++++----- pageindex/parser/pdf.py | 15 +++++++-------- pageindex/storage/sqlite.py | 8 ++++++-- tests/test_legacy_sdk_contract.py | 14 ++++++++++++++ tests/test_local_backend.py | 30 ++++++++++++++++++++++++++++++ tests/test_pdf_parser.py | 30 ++++++++++++++++++++++++++++++ tests/test_sqlite_storage.py | 13 +++++++++++++ 8 files changed, 123 insertions(+), 15 deletions(-) diff --git a/pageindex/backend/local.py b/pageindex/backend/local.py index a017730..8102deb 100644 --- a/pageindex/backend/local.py +++ b/pageindex/backend/local.py @@ -2,6 +2,7 @@ import hashlib import os import re +import sqlite3 import uuid import shutil from pathlib import Path @@ -137,6 +138,17 @@ class LocalBackend: "structure": clean_structure, "pages": pages, }) + except sqlite3.IntegrityError: + # Lost a concurrent add of the same content (UNIQUE collection+hash). + # Discard our managed files and return the winner's doc_id. + managed_path.unlink(missing_ok=True) + doc_dir = col_dir / doc_id + if doc_dir.exists(): + shutil.rmtree(doc_dir) + existing_id = self._storage.find_document_by_hash(collection, file_hash) + if existing_id: + return existing_id + raise except Exception as e: managed_path.unlink(missing_ok=True) doc_dir = col_dir / doc_id diff --git a/pageindex/cloud_api.py b/pageindex/cloud_api.py index f04182f..ae8dd67 100644 --- a/pageindex/cloud_api.py +++ b/pageindex/cloud_api.py @@ -1,6 +1,7 @@ from __future__ import annotations import json +import urllib.parse from typing import Any, Iterator import requests @@ -17,6 +18,11 @@ class LegacyCloudAPI: self.api_key = api_key self.base_url = base_url or self.BASE_URL + @staticmethod + def _enc(value: str) -> str: + """URL-encode a path segment (ids may contain / ? # or spaces).""" + return urllib.parse.quote(str(value), safe="") + def _headers(self) -> dict[str, str]: return {"api_key": self.api_key} @@ -71,7 +77,7 @@ class LegacyCloudAPI: response = self._request( "GET", - f"/doc/{doc_id}/?type=ocr&format={format}", + f"/doc/{self._enc(doc_id)}/?type=ocr&format={format}", "Failed to get OCR result", ) return response.json() @@ -79,7 +85,7 @@ class LegacyCloudAPI: def get_tree(self, doc_id: str, node_summary: bool = False) -> dict[str, Any]: response = self._request( "GET", - f"/doc/{doc_id}/?type=tree&summary={node_summary}", + f"/doc/{self._enc(doc_id)}/?type=tree&summary={node_summary}", "Failed to get tree result", ) return response.json() @@ -112,7 +118,7 @@ class LegacyCloudAPI: def get_retrieval(self, retrieval_id: str) -> dict[str, Any]: response = self._request( "GET", - f"/retrieval/{retrieval_id}/", + f"/retrieval/{self._enc(retrieval_id)}/", "Failed to get retrieval result", ) return response.json() @@ -203,7 +209,7 @@ class LegacyCloudAPI: def get_document(self, doc_id: str) -> dict[str, Any]: response = self._request( "GET", - f"/doc/{doc_id}/metadata/", + f"/doc/{self._enc(doc_id)}/metadata/", "Failed to get document metadata", ) return response.json() @@ -211,7 +217,7 @@ class LegacyCloudAPI: def delete_document(self, doc_id: str) -> dict[str, Any]: response = self._request( "DELETE", - f"/doc/{doc_id}/", + f"/doc/{self._enc(doc_id)}/", "Failed to delete document", ) return response.json() diff --git a/pageindex/parser/pdf.py b/pageindex/parser/pdf.py index f1b0f1f..83106da 100644 --- a/pageindex/parser/pdf.py +++ b/pageindex/parser/pdf.py @@ -48,11 +48,10 @@ class PdfParser: """ images_path = Path(images_dir) images_path.mkdir(parents=True, exist_ok=True) - # Use path relative to cwd so downstream consumers can access directly - try: - rel_images_path = images_path.relative_to(Path.cwd()) - except ValueError: - rel_images_path = images_path + # Store an absolute path so the ![image](...) reference resolves + # regardless of the process's cwd at query time. (cwd-relative paths + # break as soon as the query runs from a different directory.) + abs_images_path = images_path.resolve() parts: list[str] = [] images: list[dict] = [] @@ -87,13 +86,13 @@ class PdfParser: except Exception: continue - rel_path = str(rel_images_path / filename) + img_path = str(abs_images_path / filename) images.append({ - "path": rel_path, + "path": img_path, "width": width, "height": height, }) - parts.append(f"![image]({rel_path})") + parts.append(f"![image]({img_path})") img_idx += 1 content = "\n".join(parts) diff --git a/pageindex/storage/sqlite.py b/pageindex/storage/sqlite.py index 2ba4419..ff4ab8c 100644 --- a/pageindex/storage/sqlite.py +++ b/pageindex/storage/sqlite.py @@ -47,7 +47,8 @@ class SQLiteStorage: doc_type TEXT NOT NULL, structure JSON, pages JSON, - created_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP + created_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP, + UNIQUE(collection_name, file_hash) ); CREATE INDEX IF NOT EXISTS idx_docs_collection ON documents(collection_name); CREATE INDEX IF NOT EXISTS idx_docs_hash ON documents(collection_name, file_hash); @@ -76,8 +77,11 @@ class SQLiteStorage: def save_document(self, collection: str, doc_id: str, doc: dict) -> None: conn = self._get_conn() + # Plain INSERT (doc_id is a fresh uuid, never pre-existing). A duplicate + # (collection_name, file_hash) raises sqlite3.IntegrityError, which the + # caller uses to resolve a concurrent add-of-same-file race. conn.execute( - """INSERT OR REPLACE INTO documents + """INSERT INTO documents (doc_id, collection_name, doc_name, doc_description, file_path, file_hash, doc_type, structure, pages) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)""", (doc_id, collection, doc.get("doc_name"), doc.get("doc_description"), diff --git a/tests/test_legacy_sdk_contract.py b/tests/test_legacy_sdk_contract.py index 210769b..6b69e3a 100644 --- a/tests/test_legacy_sdk_contract.py +++ b/tests/test_legacy_sdk_contract.py @@ -335,3 +335,17 @@ def test_is_retrieval_ready_propagates_api_errors(monkeypatch): monkeypatch.setattr("pageindex.cloud_api.requests.request", fake_request) with pytest.raises(PageIndexAPIError): PageIndexClient("pi-test").is_retrieval_ready("doc-1") + + +def test_legacy_urls_encode_special_char_ids(monkeypatch): + """doc_id / retrieval_id must be URL-encoded into the path.""" + urls = [] + def fake_request(method, url, headers=None, **kwargs): + urls.append(url) + return FakeResponse(payload={"ok": True}) + monkeypatch.setattr("pageindex.cloud_api.requests.request", fake_request) + client = PageIndexClient("pi-test") + client.get_document("a/b?c") + client.get_retrieval("x y") + assert "a%2Fb%3Fc" in urls[0] and "/a/b?c/" not in urls[0] + assert "x%20y" in urls[1] diff --git a/tests/test_local_backend.py b/tests/test_local_backend.py index b78cf55..0b2f1f3 100644 --- a/tests/test_local_backend.py +++ b/tests/test_local_backend.py @@ -180,3 +180,33 @@ def test_add_document_unknown_collection_fails_fast(backend, tmp_path): # Collection never created -> must raise before any parse/LLM work. with pytest.raises(CollectionNotFoundError, match="does not exist"): backend.add_document("ghost-collection", str(pdf)) + + +def test_add_document_race_returns_existing_id(backend, tmp_path, monkeypatch): + """If the pre-check misses but the INSERT hits UNIQUE (concurrent add), + add_document must clean up and return the winner's doc_id, not duplicate.""" + import pageindex.backend.local as local_mod + pdf = tmp_path / "doc.pdf" + pdf.write_bytes(b"%PDF-1.4 body") + backend.get_or_create_collection("papers") + + # Pretend a winning add already stored this content under "winner-id". + file_hash = backend._file_hash(str(pdf)) + backend._storage.save_document("papers", "winner-id", { + "doc_name": "doc", "doc_type": "pdf", "file_hash": file_hash, "structure": [], + }) + # Pre-check misses (returns None) so we reach the INSERT; the post-conflict + # lookup then returns the winner's id. + calls = {"n": 0} + def fake_find(col, h): + calls["n"] += 1 + return None if calls["n"] == 1 else "winner-id" + monkeypatch.setattr(backend._storage, "find_document_by_hash", fake_find) + # avoid real parsing/LLM: stub parser + build_index + monkeypatch.setattr(backend, "_resolve_parser", lambda p: type("P", (), { + "parse": lambda self, fp, **k: type("PD", (), {"doc_name": "doc", "nodes": []})() + })()) + monkeypatch.setattr(local_mod, "build_index", lambda parsed, model=None, opt=None: {"structure": [], "doc_description": ""}) + + result = backend.add_document("papers", str(pdf)) + assert result == "winner-id" diff --git a/tests/test_pdf_parser.py b/tests/test_pdf_parser.py index c6a8cab..0d6e051 100644 --- a/tests/test_pdf_parser.py +++ b/tests/test_pdf_parser.py @@ -27,3 +27,33 @@ def test_parse_nodes_are_flat_without_level(): assert node.tokens >= 0 assert node.index is not None assert node.level is None + + +def test_image_paths_are_absolute(tmp_path): + """Image references must be absolute so they resolve regardless of cwd + (cwd-relative paths broke after the query ran from another directory).""" + import os + import pymupdf + from pageindex.parser.pdf import PdfParser + + # Build a 1-page PDF with an embedded image (>= _MIN_IMAGE_SIZE). + pix = pymupdf.Pixmap(pymupdf.csRGB, pymupdf.IRect(0, 0, 64, 64), False) + pix.clear_with(128) + png = tmp_path / "img.png" + pix.save(str(png)) + + doc = pymupdf.open() + page = doc.new_page() + page.insert_image(pymupdf.Rect(20, 20, 180, 180), filename=str(png)) + pdf_path = tmp_path / "withimg.pdf" + doc.save(str(pdf_path)) + doc.close() + + images_dir = tmp_path / "out" / "images" + result = PdfParser().parse(str(pdf_path), images_dir=str(images_dir)) + + img_paths = [im["path"] for n in result.nodes if n.images for im in n.images] + assert img_paths, "expected at least one extracted image" + for p in img_paths: + assert os.path.isabs(p), f"image path not absolute: {p}" + assert os.path.exists(p), f"image path does not resolve: {p}" diff --git a/tests/test_sqlite_storage.py b/tests/test_sqlite_storage.py index 9921f92..751fcf7 100644 --- a/tests/test_sqlite_storage.py +++ b/tests/test_sqlite_storage.py @@ -79,3 +79,16 @@ def test_close_closes_connections_created_in_other_threads(storage): storage.close() # main thread closes the worker's connection too with pytest.raises(sqlite3.ProgrammingError): conns["worker"].execute("SELECT 1") + + +def test_duplicate_file_hash_in_collection_raises(storage): + """UNIQUE(collection_name, file_hash) guards the add-same-file race.""" + import sqlite3 + storage.create_collection("papers") + doc = {"doc_name": "a", "doc_type": "pdf", "file_hash": "HASH1", "structure": []} + storage.save_document("papers", "doc-1", doc) + with pytest.raises(sqlite3.IntegrityError): + storage.save_document("papers", "doc-2", {**doc, "doc_name": "b"}) + # same hash in a DIFFERENT collection is fine + storage.create_collection("other") + storage.save_document("other", "doc-3", {**doc})