-
Notifications
You must be signed in to change notification settings - Fork 0
Chore/backend dev #8
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| .venv/ | ||
| __pycache__/ | ||
| .pytest_cache/ | ||
| venv/ |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| # Company Data Service | ||
|
|
||
| This is the core backend service for the Data-Forge System. | ||
|
|
||
| ## Prerequisites | ||
|
|
||
| - Python 3.10+ | ||
| - `pip` and `venv` | ||
|
|
||
| ## Local Development Setup | ||
|
|
||
| 1. **Navigate to the service directory**: | ||
| ```bash | ||
| cd company-data/company-api | ||
|
|
||
| ## Create and activate virtual environment | ||
|
|
||
| ```bash | ||
| python -m venv venv | ||
| # On macOS/Linux: | ||
| source venv/bin/activate | ||
| # On Windows: | ||
| venv\Scripts\activate | ||
| ``` | ||
|
|
||
| ## Install Dependencies | ||
|
|
||
| ```bash | ||
| pip install -r requirements.txt | ||
| ``` | ||
| ## Run the development server | ||
|
|
||
| ```bash | ||
| uvicorn main:app --reload | ||
| ``` | ||
|
|
||
| ## Verify it's working: | ||
| - Open your browser to ```http://localhost:8000``` → Should return ```{"status": "ok"}``` | ||
| - Interactive API docs are available at http://localhost:8000/docs | ||
|
|
||
| ## Running the tests | ||
| ```bash | ||
| # From the company-api directory | ||
| python -m pytest | ||
| ``` |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| """ | ||
| Exception handler registration for company-api. | ||
|
|
||
| Kept separate from main.py so tests can mount these handlers on a | ||
| throwaway FastAPI app without needing the full production app (real DB | ||
| connection, all routes, etc) - see tests/test_error_handling.py. | ||
| """ | ||
|
|
||
| import logging | ||
|
|
||
| from fastapi import FastAPI, Request | ||
| from fastapi.responses import JSONResponse | ||
|
|
||
| from exceptions import AppError | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| def register_exception_handlers(app: FastAPI) -> None: | ||
| @app.exception_handler(AppError) | ||
| async def app_error_handler(request: Request, exc: AppError) -> JSONResponse: | ||
| logger.warning( | ||
| "app_error", | ||
| extra={"error_code": exc.error_code, "path": str(request.url), "details": exc.details}, | ||
| ) | ||
| return JSONResponse( | ||
| status_code=exc.status_code, | ||
| content={"error": exc.error_code, "message": exc.message, "details": exc.details}, | ||
| ) | ||
|
|
||
| @app.exception_handler(Exception) | ||
| async def unhandled_exception_handler(request: Request, exc: Exception) -> JSONResponse: | ||
| logger.exception("unhandled_exception", extra={"path": str(request.url)}) | ||
| return JSONResponse( | ||
| status_code=500, | ||
| content={"error": "internal_error", "message": "An unexpected error occurred", "details": {}}, | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| """ | ||
| company-api entrypoint. | ||
|
|
||
| Deliberately minimal right now - no DB dependency, no business routes yet. | ||
| This exists so error-handling conventions have somewhere to attach and | ||
| get tested before routes/DB integration land on top. | ||
| """ | ||
|
|
||
| import sys | ||
| from pathlib import Path | ||
|
|
||
| sys.path.insert(0, str(Path(__file__).resolve().parent.parent / "company-common")) | ||
|
|
||
| from fastapi import FastAPI | ||
|
|
||
| from error_handlers import register_exception_handlers | ||
|
|
||
| app = FastAPI(title="Company API", version="0.1.0") | ||
| register_exception_handlers(app) | ||
|
|
||
|
|
||
| @app.get("/") | ||
| async def health() -> dict: | ||
| return {"status": "ok"} |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| [pytest] | ||
| testpaths = tests | ||
| python_files = test_*.py | ||
| python_classes = Test* | ||
| python_functions = test_* | ||
| addopts = -v --strict-markers | ||
| markers = | ||
| unit: Unit tests | ||
| integration: Integration tests |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| fastapi>=0.110.0 | ||
| uvicorn[standard]>=0.30.0 | ||
| httpx>=0.27.0 | ||
| pytest>=9.0.0 | ||
| pydantic>=2.0.0 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| import sys | ||
| from pathlib import Path | ||
|
|
||
| sys.path.insert(0, str(Path(__file__).resolve().parent.parent.parent / "company-common")) | ||
|
|
||
| import pytest | ||
| from fastapi.testclient import TestClient | ||
|
|
||
| from main import app | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def client() -> TestClient: | ||
| return TestClient(app) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,61 @@ | ||
| """ | ||
| Tests the exception -> JSON response contract itself, independent of any | ||
| real route. A throwaway app with routes that deliberately raise is mounted | ||
| here so this contract stays tested even before company-api has real | ||
| business routes to exercise it through. | ||
| """ | ||
|
|
||
| import pytest | ||
| from fastapi import FastAPI | ||
| from fastapi.testclient import TestClient | ||
|
|
||
| from error_handlers import register_exception_handlers | ||
| from exceptions import AppError, NotFoundError, DuplicateRecordError | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def error_client() -> TestClient: | ||
| app = FastAPI() | ||
| register_exception_handlers(app) | ||
|
|
||
| @app.get("/boom/not-found") | ||
| async def boom_not_found(): | ||
| raise NotFoundError("company techcorp.com not found", details={"uid": "techcorp.com"}) | ||
|
|
||
| @app.get("/boom/duplicate") | ||
| async def boom_duplicate(): | ||
| raise DuplicateRecordError("company already exists") | ||
|
|
||
| @app.get("/boom/unhandled") | ||
| async def boom_unhandled(): | ||
| raise RuntimeError("something broke that we didn't anticipate") | ||
|
|
||
| return TestClient(app, raise_server_exceptions=False) | ||
|
|
||
|
|
||
| class TestAppErrorHandling: | ||
| def test_not_found_error_shape(self, error_client): | ||
| response = error_client.get("/boom/not-found") | ||
| assert response.status_code == 404 | ||
| body = response.json() | ||
| assert body["error"] == "not_found" | ||
| assert body["message"] == "company techcorp.com not found" | ||
| assert body["details"] == {"uid": "techcorp.com"} | ||
|
|
||
| def test_duplicate_error_shape(self, error_client): | ||
| response = error_client.get("/boom/duplicate") | ||
| assert response.status_code == 409 | ||
| assert response.json()["error"] == "duplicate_record" | ||
|
|
||
| def test_app_error_status_codes_are_respected(self): | ||
| assert NotFoundError("x").status_code == 404 | ||
| assert DuplicateRecordError("x").status_code == 409 | ||
| assert AppError("x").status_code == 500 # base class default | ||
|
|
||
| def test_unhandled_exception_returns_generic_500(self, error_client): | ||
| response = error_client.get("/boom/unhandled") | ||
| assert response.status_code == 500 | ||
| body = response.json() | ||
| assert body["error"] == "internal_error" | ||
| # message should NOT leak internal exception details to the client | ||
| assert "something broke" not in body["message"] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| class TestHealth: | ||
| def test_health_returns_ok(self, client): | ||
| response = client.get("/") | ||
| assert response.status_code == 200 | ||
| assert response.json() == {"status": "ok"} |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| """ | ||
| Shared exception hierarchy (company-common). | ||
|
|
||
| Repository, service, and crawler code raises these instead of generic | ||
| Exceptions or DB-specific errors. company-api catches AppError once, at | ||
| the top, and turns it into a consistent JSON error response - no route | ||
| handler needs its own try/except for expected failure cases. | ||
|
|
||
| Keep this file free of framework imports (no FastAPI, no duckdb) - it has | ||
| to be importable by every package, including ones that never touch HTTP. | ||
| """ | ||
|
|
||
| from typing import Any, Optional | ||
|
|
||
|
|
||
| class AppError(Exception): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should use SOLID principle. An abstract class can be defined and other exception class shall inherit that, if that's the intent already than we may need to define a base folder and from there we import it. Tightly coupled code will backfire very soon
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we are already using solid principle and abstract class here. i agree with we should create base folder for exception handling and import it from their |
||
| """Base class for all expected application errors. | ||
|
|
||
| status_code: the HTTP status the API layer should respond with. | ||
| error_code: a stable, machine-readable string for API consumers to | ||
| branch on - don't rely on the human-readable message for that. | ||
| """ | ||
|
|
||
| status_code: int = 500 | ||
| error_code: str = "internal_error" | ||
|
|
||
| def __init__(self, message: str, *, details: Optional[dict[str, Any]] = None): | ||
| super().__init__(message) | ||
| self.message = message | ||
| self.details = details or {} | ||
|
|
||
|
|
||
| class NotFoundError(AppError): | ||
| """Requested resource doesn't exist, e.g. get(uid) found nothing.""" | ||
|
|
||
| status_code = 404 | ||
| error_code = "not_found" | ||
|
|
||
|
|
||
| class DuplicateRecordError(AppError): | ||
| """Attempted to create a record that already exists under a unique key.""" | ||
|
|
||
| status_code = 409 | ||
| error_code = "duplicate_record" | ||
|
|
||
|
|
||
| class ValidationFailedError(AppError): | ||
| """Input failed domain-level validation beyond what Pydantic checks | ||
| (e.g. business rules), as opposed to malformed request payloads - | ||
| FastAPI/Pydantic already handle those with their own 422s.""" | ||
|
|
||
| status_code = 422 | ||
| error_code = "validation_failed" | ||
|
|
||
|
|
||
| class DatabaseError(AppError): | ||
| """Underlying storage layer failed (connection issue, constraint | ||
| violation that isn't a simple duplicate, etc).""" | ||
|
|
||
| status_code = 503 | ||
| error_code = "database_error" | ||
|
|
||
|
|
||
| class UpstreamSourceError(AppError): | ||
| """A data source (crawler target, third-party API) failed or returned | ||
| something unusable. Distinct from DatabaseError since the fix and the | ||
| retry strategy are different.""" | ||
|
|
||
| status_code = 502 | ||
| error_code = "upstream_source_error" | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're planning to use
commonas a dependency. Injecting the code may not be suitableThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are absolutely right. That sys.path insertion was a temporary hack to get the initial main.py running without requiring a pip install step. It is not Dependency Injection; it is global path mutation, which is fragile.
To properly treat company-common as a dependency, I will:
Add a pyproject.toml file to company-common to define it as a standard Python package.
Install it in editable mode (pip install -e company-common).
Remove the sys.path hack entirely from main.py and conftest.py.
This aligns with your request to use common as a dependency and ensures the code is portable and production-ready.
if you have any other suggestion , let me know?