diff --git a/.env.example b/.env.example index 3f6156aa..30dc2525 100644 --- a/.env.example +++ b/.env.example @@ -83,7 +83,8 @@ TYPESENSE_API_KEY=your-typesense-api-key-here # PostgreSQL — requires jvspatial >= 0.0.16 and the asyncpg driver # (pip install asyncpg). On 0.0.15 and earlier this fails at startup with # "ValueError: Unsupported database type: postgres". See docs/postgres.md. -# The DSN is read from the environment only — app.yaml database.uri is not wired. +# These also resolve from app.yaml config.database (uri / pooler_mode / +# min_pool_size / max_pool_size); env wins. See docs/postgres.md. # JVSPATIAL_DB_TYPE=postgres # JVSPATIAL_POSTGRES_DSN=postgresql://user:password@localhost:5432/jvagent_db # JVSPATIAL_POSTGRES_MIN_POOL_SIZE=2 diff --git a/.planning/reference/jvspatial-integration.md b/.planning/reference/jvspatial-integration.md index eed03247..8db76730 100644 --- a/.planning/reference/jvspatial-integration.md +++ b/.planning/reference/jvspatial-integration.md @@ -7,7 +7,7 @@ ## 1. Where jvspatial lives - **Source**: `/Users/eldonmarks/Briefcase/dev/jv/jvspatial` (sibling directory in this workspace). -- **Pip install**: declared in [`pyproject.toml`](../../pyproject.toml) as `jvspatial==0.0.16`. +- **Pip install**: declared in [`pyproject.toml`](../../pyproject.toml) as `jvspatial==0.0.17`. - **Own docs**: jvspatial has its own [`README.md`](../../../jvspatial/README.md) and [`SPEC.md`](../../../jvspatial/SPEC.md). Treat those as authoritative for anything below. --- @@ -171,7 +171,7 @@ Things jvagent **owns**: ## 5. Version policy -- Minimum required jvspatial: pinned in [`pyproject.toml`](../../pyproject.toml) as `jvspatial==X.Y.Z`. Current: `==0.0.16`. +- Minimum required jvspatial: pinned in [`pyproject.toml`](../../pyproject.toml) as `jvspatial==X.Y.Z`. Current: `==0.0.17`. - When jvspatial introduces breaking changes (e.g., walker API rename, persistence shape change), bump the pin and update this section. - When adding a new dependency on a jvspatial feature, document the symbol + version it was introduced in. Helps downstream consumers know the floor. - Rationale: [`adr/0006-jvspatial-dependency.md`](../adr/0006-jvspatial-dependency.md). diff --git a/CHANGELOG.md b/CHANGELOG.md index 4e883801..62a1a611 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,18 @@ and this project adheres to [PEP 440](https://peps.python.org/pep-0440/) / `tests/core/test_channel_override_coverage.py`, `tests/cli/test_validate_advisories.py`. +- **PostgreSQL settings resolve from `app.yaml`, not just env** (`jvagent/cli/server_config.py`). + `create_server_from_config` now threads `database.uri` / `pooler_mode` / + `min_pool_size` / `max_pool_size` into jvspatial's `DatabaseConfig` when + `database.type` is `postgres`/`postgresql`, alongside the mongodb and dynamodb + settings that were already wired. Previously the DSN reached the driver only + through jvspatial's own env read, so an `app.yaml` `database.uri` was silently + ignored for Postgres. Unset values stay `None` so `PostgresDB`'s defaults still + apply, and a non-integer pool size is logged and ignored rather than failing + startup. Requires `jvspatial >= 0.0.17` (pin bumped), which is where + `DatabaseConfig` began accepting values by field name. Coverage: + `tests/cli/test_server_config_postgres.py`. + ### Fixed - **`voice.closers` no longer strips trailing questions.** Scrub peel now skips diff --git a/docs/environment-keys-reference.md b/docs/environment-keys-reference.md index 2a47566c..56367132 100644 --- a/docs/environment-keys-reference.md +++ b/docs/environment-keys-reference.md @@ -95,7 +95,7 @@ These are commonly used by `jvagent` and should be configured in `jvagent` deplo - `JVSPATIAL_DYNAMODB_TABLE_NAME` - `JVSPATIAL_DYNAMODB_REGION` - `JVSPATIAL_DYNAMODB_ENDPOINT_URL` -- `JVSPATIAL_POSTGRES_DSN` - PostgreSQL DSN. Env-only (no `app.yaml` path). Requires `jvspatial >= 0.0.16` — see [postgres.md](postgres.md). +- `JVSPATIAL_POSTGRES_DSN` - PostgreSQL DSN. Also settable as `config.database.uri`. Requires `jvspatial >= 0.0.16` — see [postgres.md](postgres.md). - `JVSPATIAL_POSTGRES_MIN_POOL_SIZE` - `JVSPATIAL_POSTGRES_MAX_POOL_SIZE` - `JVSPATIAL_POSTGRES_POOLER_MODE` - `session` (default) or `transaction` for PgBouncer / RDS Proxy. diff --git a/docs/postgres.md b/docs/postgres.md index aa00907e..35872087 100644 --- a/docs/postgres.md +++ b/docs/postgres.md @@ -22,7 +22,19 @@ Notes: - **The driver is an extra.** `pip install asyncpg` (or `jvspatial[postgres]`); it is not pulled in by jvagent's base dependencies. - **`JVSPATIAL_DB_PATH` does not apply.** [`server_config.py:161`](../jvagent/cli/server_config.py) only exports a path for `json` / `sqlite`. -- **The DSN reaches the driver via env only.** [`server_config.py:126-164`](../jvagent/cli/server_config.py) threads a connection string into jvspatial's `DatabaseConfig` for `mongodb` and table/region for `dynamodb`; there is no Postgres field, so `JVSPATIAL_POSTGRES_DSN` must be set in the environment — an `app.yaml` `database.uri` will **not** be picked up. +- **`app.yaml` works too.** Every key above also resolves from the `config.database` stanza, with the usual precedence (env wins): + + ```yaml + config: + database: + type: postgres + uri: ${JVSPATIAL_POSTGRES_DSN} # or a literal DSN, but keep credentials in env + pooler_mode: transaction + min_pool_size: 2 + max_pool_size: 10 + ``` + + `database.uri` is shared with mongodb — `database.type` decides how it is read. A non-integer pool size is logged and ignored rather than taking the server down at startup. - **Logging DB has no Postgres branch.** jvspatial's `logging/config.py` falls through to a `json` file log for any unrecognized type — silently. Set `JVSPATIAL_LOG_DB_TYPE=json` (or `mongodb`) explicitly so the fallback is a decision rather than a surprise. See [logging.md](logging.md). - **PageIndex is a separate store** with its own `JVAGENT_PAGEINDEX_DB_TYPE` (`json` by default) and is unaffected by the main graph backend. diff --git a/jvagent/cli/server_config.py b/jvagent/cli/server_config.py index 27f4aaeb..2dce27f5 100644 --- a/jvagent/cli/server_config.py +++ b/jvagent/cli/server_config.py @@ -60,6 +60,21 @@ def _ensure_session_token_header(cors_headers: Optional[list]) -> list: return base +def _optional_int(value: Any, key: str) -> Optional[int]: + """Coerce a config/env value to ``int``, or ``None`` if unusable. + + A typo'd pool size should not take the server down before it starts — + warn and fall through to the driver's own default instead. + """ + if normalize_empty(value) is None: + return None + try: + return int(value) + except (TypeError, ValueError): + logger.warning("%s is not an integer (%r); ignoring it.", key, value) + return None + + def _set_db_env_from_config(app_root: str) -> None: """Set database environment variables from app config. @@ -133,6 +148,55 @@ def create_server_from_config(debug: bool = False, app_root: str = None) -> Serv if normalize_empty(mongodb_uri) is None: mongodb_uri = env("JVSPATIAL_MONGODB_URI", default="mongodb://localhost:27017") + # PostgreSQL configuration (jvspatial >= 0.0.16). ``database.uri`` is shared + # with mongodb — ``database.type`` decides how it is read. + is_postgres = db_type in ("postgres", "postgresql") + postgres_dsn = ( + normalize_empty( + get_config_value(app_config, "database.uri", "JVSPATIAL_POSTGRES_DSN", None) + ) + if is_postgres + else None + ) + postgres_pooler_mode = ( + normalize_empty( + get_config_value( + app_config, + "database.pooler_mode", + "JVSPATIAL_POSTGRES_POOLER_MODE", + None, + ) + ) + if is_postgres + else None + ) + postgres_min_pool_size = ( + _optional_int( + get_config_value( + app_config, + "database.min_pool_size", + "JVSPATIAL_POSTGRES_MIN_POOL_SIZE", + None, + ), + "JVSPATIAL_POSTGRES_MIN_POOL_SIZE", + ) + if is_postgres + else None + ) + postgres_max_pool_size = ( + _optional_int( + get_config_value( + app_config, + "database.max_pool_size", + "JVSPATIAL_POSTGRES_MAX_POOL_SIZE", + None, + ), + "JVSPATIAL_POSTGRES_MAX_POOL_SIZE", + ) + if is_postgres + else None + ) + # DynamoDB configuration dynamodb_table_name = get_config_value( app_config, "database.table_name", "JVSPATIAL_DYNAMODB_TABLE_NAME", None @@ -325,6 +389,11 @@ def create_server_from_config(debug: bool = False, app_root: str = None) -> Serv dynamodb_secret_access_key=( dynamodb_secret_access_key if db_type == "dynamodb" else None ), + # Unset values stay None so PostgresDB's own defaults still apply. + postgres_dsn=postgres_dsn, + postgres_pooler_mode=postgres_pooler_mode, + postgres_min_pool_size=postgres_min_pool_size, + postgres_max_pool_size=postgres_max_pool_size, ) # Auth configuration - merge default exempt paths with app-specific (auth.exempt_paths) diff --git a/pyproject.toml b/pyproject.toml index efbe7dbf..e383fbe4 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -31,7 +31,7 @@ classifiers = [ dependencies = [ "aiohttp>=3.9.0", # CI records the resolved jvspatial version after install (see .github/workflows/test-jvagent.yaml). - "jvspatial==0.0.16", + "jvspatial==0.0.17", "python-dotenv>=1.0.0", "pyyaml>=6.0.0", "httpx>=0.27.0", @@ -77,6 +77,10 @@ test = [ # web_fetch tests (same stack as jvagent/action/web_fetch/info.yaml) "beautifulsoup4", "markdownify", + # PostgreSQL config tests build a real Server with db_type=postgres, which + # instantiates PostgresDB and therefore needs the driver. Without it the + # suite passes only on machines that happen to have asyncpg installed. + "asyncpg>=0.29.0", ] # Optional: cluster-wide conversation locks (Redis SET NX or DynamoDB). diff --git a/requirements-all.txt b/requirements-all.txt index e6402c4e..b0c5d195 100644 --- a/requirements-all.txt +++ b/requirements-all.txt @@ -6,7 +6,7 @@ # Must stay in sync with [project] dependencies in pyproject.toml — # enforced by tests/test_requirements_sync.py. aiohttp>=3.9.0 -jvspatial==0.0.16 +jvspatial==0.0.17 python-dotenv>=1.0.0 pyyaml>=6.0.0 httpx>=0.27.0 diff --git a/requirements.txt b/requirements.txt index 670de4d8..6f6f2610 100644 --- a/requirements.txt +++ b/requirements.txt @@ -3,7 +3,7 @@ # Test-only deps (incl. Docling for PageIndex): pyproject.toml # [project.optional-dependencies] test — install with: pip install -e ".[test]" aiohttp>=3.9.0 -jvspatial==0.0.16 +jvspatial==0.0.17 python-dotenv>=1.0.0 pyyaml>=6.0.0 httpx>=0.27.0 diff --git a/tests/cli/test_server_config_postgres.py b/tests/cli/test_server_config_postgres.py new file mode 100644 index 00000000..c44af86c --- /dev/null +++ b/tests/cli/test_server_config_postgres.py @@ -0,0 +1,151 @@ +"""create_server_from_config: PostgreSQL settings reach jvspatial's DatabaseConfig. + +Every other backend has its connection settings threaded into the config +object (mongodb its uri/name, dynamodb its table/region). Postgres did not, +so an ``app.yaml`` ``database.uri`` was silently ignored and only the +driver's own env read made a connection possible. +""" + +import pytest + +# Building a Server with db_type=postgres instantiates PostgresDB, which imports +# asyncpg. It ships in the [test] extra; skip rather than error for anyone +# running the suite without it. +pytest.importorskip("asyncpg") + +POSTGRES_APP_YAML = """ +app: pg_config_test +context: + name: test + description: test +config: + database: + type: postgres + logging: + enabled: false + server: + host: 127.0.0.1 + port: 8765 +agents: [] +""" + +YAML_DSN_APP_YAML = """ +app: pg_config_test +context: + name: test + description: test +config: + database: + type: postgres + uri: postgresql://yaml:pw@yamlhost:5432/yamldb + pooler_mode: transaction + min_pool_size: 3 + max_pool_size: 9 + logging: + enabled: false + server: + host: 127.0.0.1 + port: 8765 +agents: [] +""" + +JSON_APP_YAML = """ +app: pg_config_test +context: + name: test + description: test +config: + database: + type: json + path: ./test_jvdb + logging: + enabled: false +agents: [] +""" + +_PG_ENV = ( + "JVSPATIAL_POSTGRES_DSN", + "JVSPATIAL_POSTGRES_POOLER_MODE", + "JVSPATIAL_POSTGRES_MIN_POOL_SIZE", + "JVSPATIAL_POSTGRES_MAX_POOL_SIZE", +) + + +@pytest.fixture +def build_server(tmp_path, monkeypatch): + """Build a Server from an app.yaml body, with a clean Postgres env.""" + from jvagent.cli.server_config import create_server_from_config + from jvagent.core.app_context import clear_app_root, set_app_root + + monkeypatch.setenv("JVSPATIAL_JWT_SECRET_KEY", "test-secret-for-pg-config-tests") + monkeypatch.setenv("JVAGENT_ADMIN_PASSWORD", "x") + for key in _PG_ENV: + monkeypatch.delenv(key, raising=False) + + def _build(app_yaml: str): + app_root = str(tmp_path) + (tmp_path / "app.yaml").write_text(app_yaml.strip(), encoding="utf-8") + set_app_root(app_root) + try: + return create_server_from_config(debug=False, app_root=app_root) + finally: + clear_app_root() + + yield _build + + +def test_dsn_from_env_reaches_database_config(build_server, monkeypatch): + monkeypatch.setenv("JVSPATIAL_POSTGRES_DSN", "postgresql://u:pw@envhost:5432/envdb") + db = build_server(POSTGRES_APP_YAML).config.database + assert db.db_type == "postgres" + assert db.postgres_dsn == "postgresql://u:pw@envhost:5432/envdb" + + +def test_settings_from_app_yaml_reach_database_config(build_server): + db = build_server(YAML_DSN_APP_YAML).config.database + assert db.postgres_dsn == "postgresql://yaml:pw@yamlhost:5432/yamldb" + assert db.postgres_pooler_mode == "transaction" + assert db.postgres_min_pool_size == 3 + assert db.postgres_max_pool_size == 9 + + +def test_env_overrides_app_yaml(build_server, monkeypatch): + """Documented precedence: env var > app.yaml > default.""" + monkeypatch.setenv("JVSPATIAL_POSTGRES_DSN", "postgresql://u:pw@envhost:5432/envdb") + monkeypatch.setenv("JVSPATIAL_POSTGRES_MAX_POOL_SIZE", "25") + db = build_server(YAML_DSN_APP_YAML).config.database + assert db.postgres_dsn == "postgresql://u:pw@envhost:5432/envdb" + assert db.postgres_max_pool_size == 25 + # Untouched by env, so the YAML value still applies. + assert db.postgres_pooler_mode == "transaction" + + +def test_unset_settings_stay_none_for_driver_defaults(build_server): + """Omitted settings must not be pinned here — PostgresDB has its own defaults.""" + db = build_server(POSTGRES_APP_YAML).config.database + assert db.postgres_dsn is None + assert db.postgres_pooler_mode is None + assert db.postgres_min_pool_size is None + assert db.postgres_max_pool_size is None + + +def test_postgres_settings_ignored_for_other_backends(build_server, monkeypatch): + """A stray Postgres DSN must not ride along on a json deployment.""" + monkeypatch.setenv("JVSPATIAL_POSTGRES_DSN", "postgresql://u:pw@envhost:5432/envdb") + db = build_server(JSON_APP_YAML).config.database + assert db.db_type == "json" + assert db.postgres_dsn is None + + +def test_postgresql_alias_is_honored(build_server, monkeypatch): + monkeypatch.setenv("JVSPATIAL_POSTGRES_DSN", "postgresql://u:pw@envhost:5432/envdb") + db = build_server(POSTGRES_APP_YAML.replace("type: postgres", "type: postgresql")) + assert db.config.database.postgres_dsn == "postgresql://u:pw@envhost:5432/envdb" + + +def test_non_integer_pool_size_is_ignored(build_server, monkeypatch): + """A typo'd pool size must not crash startup; the driver default applies.""" + monkeypatch.setenv("JVSPATIAL_POSTGRES_DSN", "postgresql://u:pw@envhost:5432/envdb") + monkeypatch.setenv("JVSPATIAL_POSTGRES_MAX_POOL_SIZE", "not-a-number") + db = build_server(POSTGRES_APP_YAML).config.database + assert db.postgres_max_pool_size is None