diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a7bed60..e2ec9f4 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -29,7 +29,7 @@ jobs: with: python-version: "3.12" cache: pip - cache-dependency-path: document-parser/requirements.txt + cache-dependency-path: document-parser/requirements-test.txt - name: Install system dependencies run: sudo apt-get update && sudo apt-get install -y --no-install-recommends poppler-utils @@ -37,8 +37,8 @@ jobs: - name: Install Python dependencies run: | pip install --upgrade pip - pip install -r requirements.txt - pip install pytest pytest-asyncio httpx ruff + pip install -r requirements-test.txt + pip install httpx ruff - name: Lint run: ruff check . diff --git a/document-parser/domain/value_objects.py b/document-parser/domain/value_objects.py index f633117..dd558fc 100644 --- a/document-parser/domain/value_objects.py +++ b/document-parser/domain/value_objects.py @@ -8,6 +8,10 @@ from __future__ import annotations from dataclasses import dataclass, field +# US Letter page dimensions (points) — fallback when page size is unknown +DEFAULT_PAGE_WIDTH: float = 612.0 +DEFAULT_PAGE_HEIGHT: float = 792.0 + @dataclass(frozen=True) class PageElement: diff --git a/document-parser/infra/local_converter.py b/document-parser/infra/local_converter.py index d7865b9..6be939e 100644 --- a/document-parser/infra/local_converter.py +++ b/document-parser/infra/local_converter.py @@ -36,6 +36,8 @@ from docling_core.types.doc import ( ) from domain.value_objects import ( + DEFAULT_PAGE_HEIGHT, + DEFAULT_PAGE_WIDTH, ConversionOptions, ConversionResult, PageDetail, @@ -50,10 +52,6 @@ logger = logging.getLogger(__name__) # Uses a timeout to prevent a frozen conversion from blocking all others. _converter_lock = threading.Lock() -# US Letter page dimensions (points) — fallback when page size is unknown -_DEFAULT_PAGE_WIDTH = 612.0 -_DEFAULT_PAGE_HEIGHT = 792.0 - # Default converter (lazy-init on first request) _default_converter: DoclingConverter | None = None @@ -175,11 +173,11 @@ def _process_content_item( logger.warning( "Page %d not found in document metadata — using US Letter fallback (%sx%s pt)", page_no, - _DEFAULT_PAGE_WIDTH, - _DEFAULT_PAGE_HEIGHT, + DEFAULT_PAGE_WIDTH, + DEFAULT_PAGE_HEIGHT, ) pages[page_no] = PageDetail( - page_number=page_no, width=_DEFAULT_PAGE_WIDTH, height=_DEFAULT_PAGE_HEIGHT + page_number=page_no, width=DEFAULT_PAGE_WIDTH, height=DEFAULT_PAGE_HEIGHT ) page_height = pages[page_no].height @@ -248,10 +246,10 @@ def _convert_sync( pages_detail = [ PageDetail( page_number=i + 1, - width=doc.pages[i + 1].size.width if (i + 1) in doc.pages else _DEFAULT_PAGE_WIDTH, + width=doc.pages[i + 1].size.width if (i + 1) in doc.pages else DEFAULT_PAGE_WIDTH, height=doc.pages[i + 1].size.height if (i + 1) in doc.pages - else _DEFAULT_PAGE_HEIGHT, + else DEFAULT_PAGE_HEIGHT, ) for i in range(page_count) ] diff --git a/document-parser/infra/opensearch_store.py b/document-parser/infra/opensearch_store.py index c4a97f9..48dd6da 100644 --- a/document-parser/infra/opensearch_store.py +++ b/document-parser/infra/opensearch_store.py @@ -69,13 +69,14 @@ class OpenSearchStore: verify_certs: Whether to verify TLS certificates. """ - def __init__(self, url: str, *, verify_certs: bool = False) -> None: + def __init__(self, url: str, *, verify_certs: bool = False, default_limit: int = 1000) -> None: self._client = AsyncOpenSearch( hosts=[url], use_ssl=url.startswith("https"), verify_certs=verify_certs, ssl_show_warn=False, ) + self._default_limit = default_limit # -- lifecycle ------------------------------------------------------------- @@ -147,9 +148,11 @@ class OpenSearchStore: index_name: str, doc_id: str, *, - limit: int = 1000, + limit: int | None = None, ) -> list[SearchResult]: """Retrieve all indexed chunks for a document, ordered by chunk_index.""" + if limit is None: + limit = self._default_limit resp = await self._client.search( index=index_name, body={ diff --git a/document-parser/infra/serve_converter.py b/document-parser/infra/serve_converter.py index 5013e1a..d591a14 100644 --- a/document-parser/infra/serve_converter.py +++ b/document-parser/infra/serve_converter.py @@ -21,6 +21,8 @@ import httpx from docling_core.types.doc.base import BoundingBox, CoordOrigin from domain.value_objects import ( + DEFAULT_PAGE_HEIGHT, + DEFAULT_PAGE_WIDTH, ConversionOptions, ConversionResult, PageDetail, @@ -31,7 +33,6 @@ from infra.bbox import to_topleft_list logger = logging.getLogger(__name__) _API_PREFIX = "/v1" -_DEFAULT_TIMEOUT = 600.0 # Docling Serve label → our element type _LABEL_MAP = { @@ -60,7 +61,7 @@ class ServeConverter: self, base_url: str, api_key: str | None = None, - timeout: float = _DEFAULT_TIMEOUT, + timeout: float = 600.0, ): self._base_url = base_url.rstrip("/") self._api_key = api_key @@ -192,8 +193,8 @@ def _extract_pages_from_docling_document(doc: dict) -> list[PageDetail]: size = page_data.get("size", {}) pages_dict[page_no] = PageDetail( page_number=page_no, - width=size.get("width", 612.0), - height=size.get("height", 792.0), + width=size.get("width", DEFAULT_PAGE_WIDTH), + height=size.get("height", DEFAULT_PAGE_HEIGHT), ) # Process all element arrays @@ -220,8 +221,8 @@ def _add_element(item: dict, pages: dict[int, PageDetail]) -> None: if page_no not in pages: pages[page_no] = PageDetail( page_number=page_no, - width=612.0, - height=792.0, + width=DEFAULT_PAGE_WIDTH, + height=DEFAULT_PAGE_HEIGHT, ) bbox_data = prov.get("bbox", {}) diff --git a/document-parser/infra/settings.py b/document-parser/infra/settings.py index 50e465d..cc322fe 100644 --- a/document-parser/infra/settings.py +++ b/document-parser/infra/settings.py @@ -25,6 +25,7 @@ class Settings: batch_page_size: int = 0 # 0 = disabled, > 0 = pages per batch opensearch_url: str = "" # empty = disabled embedding_url: str = "" # empty = disabled (e.g. http://localhost:8001) + opensearch_default_limit: int = 1000 # max chunks returned by get_chunks embedding_dimension: int = 384 # Granite Embedding 30M / all-MiniLM-L6-v2 upload_dir: str = "./uploads" db_path: str = "./data/docling_studio.db" @@ -54,6 +55,10 @@ class Settings: errors.append(f"rate_limit_rpm must be >= 0 (got {self.rate_limit_rpm})") if self.batch_page_size < 0: errors.append(f"batch_page_size must be >= 0 (got {self.batch_page_size})") + if self.opensearch_default_limit < 1: + errors.append( + f"opensearch_default_limit must be >= 1 (got {self.opensearch_default_limit})" + ) if self.embedding_dimension < 1: errors.append(f"embedding_dimension must be >= 1 (got {self.embedding_dimension})") if self.default_table_mode not in ("accurate", "fast"): @@ -97,6 +102,7 @@ class Settings: batch_page_size=int(os.environ.get("BATCH_PAGE_SIZE", "10")), opensearch_url=os.environ.get("OPENSEARCH_URL", ""), embedding_url=os.environ.get("EMBEDDING_URL", ""), + opensearch_default_limit=int(os.environ.get("OPENSEARCH_DEFAULT_LIMIT", "1000")), embedding_dimension=int(os.environ.get("EMBEDDING_DIMENSION", "384")), upload_dir=os.environ.get("UPLOAD_DIR", "./uploads"), db_path=os.environ.get("DB_PATH", "./data/docling_studio.db"), diff --git a/document-parser/main.py b/document-parser/main.py index 0f6a4a4..408a299 100644 --- a/document-parser/main.py +++ b/document-parser/main.py @@ -47,6 +47,7 @@ def _build_converter(): return ServeConverter( base_url=settings.docling_serve_url, api_key=settings.docling_serve_api_key, + timeout=settings.conversion_timeout, ) else: from infra.local_converter import LocalConverter @@ -99,7 +100,10 @@ def _build_ingestion_service() -> IngestionService | None: from infra.opensearch_store import OpenSearchStore embedding = EmbeddingClient(settings.embedding_url) - vector_store = OpenSearchStore(settings.opensearch_url) + vector_store = OpenSearchStore( + settings.opensearch_url, + default_limit=settings.opensearch_default_limit, + ) config = IngestionConfig( embedding_dimension=settings.embedding_dimension, ) diff --git a/document-parser/persistence/database.py b/document-parser/persistence/database.py index b32e694..c56620e 100644 --- a/document-parser/persistence/database.py +++ b/document-parser/persistence/database.py @@ -9,11 +9,9 @@ from contextlib import asynccontextmanager import aiosqlite -from infra.settings import settings - logger = logging.getLogger(__name__) -DB_PATH = settings.db_path +DB_PATH = os.environ.get("DB_PATH", "./data/docling_studio.db") _SCHEMA = """ CREATE TABLE IF NOT EXISTS documents ( diff --git a/document-parser/requirements-test.txt b/document-parser/requirements-test.txt new file mode 100644 index 0000000..255ac0b --- /dev/null +++ b/document-parser/requirements-test.txt @@ -0,0 +1,4 @@ +-r requirements.txt +pytest>=8.0.0,<9.0.0 +pytest-asyncio>=0.23.0,<1.0.0 +pytestarch>=2.0.0,<3.0.0 diff --git a/document-parser/tests/test_architecture.py b/document-parser/tests/test_architecture.py new file mode 100644 index 0000000..c74f68d --- /dev/null +++ b/document-parser/tests/test_architecture.py @@ -0,0 +1,208 @@ +"""Hexagonal architecture tests — enforce layer dependency rules. + +Uses pytestarch for inter-layer dependency rules and ast-based import +scanning for external (third-party) dependency constraints. + +Rules enforced: +- domain -> no import from api, services, infra, persistence +- services -> no import from api, infra, persistence +- api -> no import from infra, persistence +- infra -> no import from api, services +- persistence -> no import from api, services, infra +- domain -> no import of fastapi, sqlalchemy, httpx, opensearchpy +- services -> no import of fastapi +""" + +from __future__ import annotations + +import ast +from pathlib import Path + +import pytest +from pytestarch import Rule, get_evaluable_architecture + +# --------------------------------------------------------------------------- +# pytestarch evaluable (project root = document-parser/) +# --------------------------------------------------------------------------- + +_PROJECT_ROOT = Path(__file__).resolve().parent.parent + +# pytestarch uses the directory name as module prefix when given absolute paths. +# We use the directory name to build qualified module references. +_PREFIX = _PROJECT_ROOT.name # "document-parser" + +_evaluable = get_evaluable_architecture(str(_PROJECT_ROOT), str(_PROJECT_ROOT)) + + +def _mod(layer: str) -> str: + """Return the fully-qualified pytestarch module name for a layer.""" + return f"{_PREFIX}.{layer}" + + +# --------------------------------------------------------------------------- +# Helper: collect top-level imports from all .py files in a package +# --------------------------------------------------------------------------- + + +def _collect_imports(package: str) -> set[str]: + """Return the set of top-level module names imported by *package*.""" + pkg_path = Path(_PROJECT_ROOT) / package + imports: set[str] = set() + for py_file in pkg_path.rglob("*.py"): + tree = ast.parse(py_file.read_text(), filename=str(py_file)) + for node in ast.walk(tree): + if isinstance(node, ast.Import): + for alias in node.names: + imports.add(alias.name.split(".")[0]) + elif isinstance(node, ast.ImportFrom) and node.module: + imports.add(node.module.split(".")[0]) + return imports + + +# --------------------------------------------------------------------------- +# Inter-layer dependency rules (pytestarch) +# --------------------------------------------------------------------------- + + +class TestDomainLayerIsolation: + """domain must not depend on any other layer.""" + + @pytest.mark.parametrize("forbidden", ["api", "services", "infra", "persistence"]) + def test_domain_does_not_import(self, forbidden: str): + rule = ( + Rule() + .modules_that() + .are_sub_modules_of(_mod("domain")) + .should_not() + .import_modules_that() + .are_sub_modules_of(_mod(forbidden)) + ) + rule.assert_applies(_evaluable) + + +class TestServicesLayerIsolation: + """services may import domain only.""" + + @pytest.mark.parametrize("forbidden", ["api", "infra", "persistence"]) + def test_services_does_not_import(self, forbidden: str): + rule = ( + Rule() + .modules_that() + .are_sub_modules_of(_mod("services")) + .should_not() + .import_modules_that() + .are_sub_modules_of(_mod(forbidden)) + ) + rule.assert_applies(_evaluable) + + +class TestApiLayerIsolation: + """api may import services and domain, but not infra or persistence.""" + + @pytest.mark.parametrize("forbidden", ["infra", "persistence"]) + def test_api_does_not_import(self, forbidden: str): + rule = ( + Rule() + .modules_that() + .are_sub_modules_of(_mod("api")) + .should_not() + .import_modules_that() + .are_sub_modules_of(_mod(forbidden)) + ) + rule.assert_applies(_evaluable) + + +class TestInfraLayerIsolation: + """infra may import domain (ports), but not api or services.""" + + @pytest.mark.parametrize("forbidden", ["api", "services"]) + def test_infra_does_not_import(self, forbidden: str): + rule = ( + Rule() + .modules_that() + .are_sub_modules_of(_mod("infra")) + .should_not() + .import_modules_that() + .are_sub_modules_of(_mod(forbidden)) + ) + rule.assert_applies(_evaluable) + + +class TestPersistenceLayerIsolation: + """persistence may import domain, but not api, services, or infra.""" + + @pytest.mark.parametrize("forbidden", ["api", "services", "infra"]) + def test_persistence_does_not_import(self, forbidden: str): + rule = ( + Rule() + .modules_that() + .are_sub_modules_of(_mod("persistence")) + .should_not() + .import_modules_that() + .are_sub_modules_of(_mod(forbidden)) + ) + rule.assert_applies(_evaluable) + + +# --------------------------------------------------------------------------- +# External dependency rules (ast-based) +# --------------------------------------------------------------------------- + +_DOMAIN_FORBIDDEN_EXTERNALS = {"fastapi", "sqlalchemy", "httpx", "opensearchpy"} +_SERVICES_FORBIDDEN_EXTERNALS = {"fastapi"} + + +class TestDomainExternalDependencies: + """domain must not import infrastructure-specific third-party libraries.""" + + @pytest.mark.parametrize("lib", sorted(_DOMAIN_FORBIDDEN_EXTERNALS)) + def test_domain_does_not_import_external(self, lib: str): + imports = _collect_imports("domain") + assert lib not in imports, f"domain imports forbidden external library '{lib}'" + + +class TestServicesExternalDependencies: + """services must not import web-framework libraries.""" + + @pytest.mark.parametrize("lib", sorted(_SERVICES_FORBIDDEN_EXTERNALS)) + def test_services_does_not_import_external(self, lib: str): + imports = _collect_imports("services") + assert lib not in imports, f"services imports forbidden external library '{lib}'" + + +# --------------------------------------------------------------------------- +# Convention: ports live exclusively in domain.ports +# --------------------------------------------------------------------------- + + +class TestPortConvention: + """Protocol definitions (ports) must live in domain.ports only.""" + + def test_no_protocol_outside_domain_ports(self): + """No Protocol subclass should be defined outside domain/ports.py.""" + ports_file = Path(_PROJECT_ROOT) / "domain" / "ports.py" + for py_file in Path(_PROJECT_ROOT).rglob("*.py"): + if py_file == ports_file: + continue + # Skip test files and __pycache__ + if "tests" in py_file.parts or "__pycache__" in py_file.parts: + continue + tree = ast.parse(py_file.read_text(), filename=str(py_file)) + for node in ast.walk(tree): + if isinstance(node, ast.ClassDef): + for base in node.bases: + base_name = _get_name(base) + if base_name == "Protocol": + pytest.fail( + f"Protocol '{node.name}' defined in {py_file.relative_to(_PROJECT_ROOT)}" + f" — ports must live in domain/ports.py" + ) + + +def _get_name(node: ast.expr) -> str: + """Extract a simple name from an AST expression node.""" + if isinstance(node, ast.Name): + return node.id + if isinstance(node, ast.Attribute): + return node.attr + return "" diff --git a/document-parser/tests/test_settings.py b/document-parser/tests/test_settings.py index 4e8e2e6..4326790 100644 --- a/document-parser/tests/test_settings.py +++ b/document-parser/tests/test_settings.py @@ -19,6 +19,7 @@ class TestSettingsDefaults: assert s.max_page_count == 0 assert s.max_file_size_mb == 50 assert s.batch_page_size == 0 + assert s.opensearch_default_limit == 1000 assert s.upload_dir == "./uploads" assert s.db_path == "./data/docling_studio.db" assert "http://localhost:3000" in s.cors_origins @@ -103,6 +104,12 @@ class TestSettingsValidation: with pytest.raises(ValueError, match="lock_timeout must be > 0"): Settings(lock_timeout=0) + def test_zero_opensearch_default_limit_rejected(self): + import pytest + + with pytest.raises(ValueError, match="opensearch_default_limit must be >= 1"): + Settings(opensearch_default_limit=0) + def test_invalid_table_mode_rejected(self): import pytest @@ -146,6 +153,7 @@ class TestSettingsFromEnv: monkeypatch.setenv("MAX_PAGE_COUNT", "20") monkeypatch.setenv("MAX_FILE_SIZE_MB", "100") monkeypatch.setenv("BATCH_PAGE_SIZE", "15") + monkeypatch.setenv("OPENSEARCH_DEFAULT_LIMIT", "500") monkeypatch.setenv("UPLOAD_DIR", "/data/uploads") monkeypatch.setenv("DB_PATH", "/data/test.db") monkeypatch.setenv("CORS_ORIGINS", "http://a.com, http://b.com") @@ -163,6 +171,7 @@ class TestSettingsFromEnv: assert s.max_page_count == 20 assert s.max_file_size_mb == 100 assert s.batch_page_size == 15 + assert s.opensearch_default_limit == 500 assert s.upload_dir == "/data/uploads" assert s.db_path == "/data/test.db" assert s.cors_origins == ["http://a.com", "http://b.com"]