Visitar URL original
fix: Normalize SQL registry read_path to the psycopg3 driver like pat… · feast-dev/feast@996c6ea · GitHub
Skip to content

Commit 996c6ea

Browse files
larrysingleton007franciscojavierarceo
larrysingleton007
and
franciscojavierarceo
authored
fix: Normalize SQL registry read_path to the psycopg3 driver like path (#6644)
* fix: Normalize SQL registry read_path to the psycopg3 driver like path RegistryConfig.validate_path rewrites a bare postgresql:// path to postgresql+psycopg:// (psycopg3) and warns, but SqlRegistryConfig.read_path had no equivalent validator, so a bare postgresql:// read_path reached create_engine unchanged and silently used psycopg2 while path used psycopg3. Factor the rewrite+warning into a shared RegistryConfig._normalize_postgres_scheme static helper and add a field_validator on read_path that mirrors validate_path. The helper rewrites only the leading scheme (not later occurrences) and the path warning text is unchanged. Add unit tests: read_path normalized (explicit and defaulted registry_type), explicit +psycopg2/+psycopg and non-postgres schemes left untouched, None stays None, and the migration warning is emitted. Fixes #6643 Signed-off-by: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com> * test: Cover prefix-only PostgreSQL scheme normalization Signed-off-by: Francisco Javier Arceo <farceo@redhat.com> * fix: Address review feedback on read_path validator Drop the registry_type == "sql" condition from validate_read_path: SqlRegistryConfig is only ever used for SQL registries, so the check was redundant (and the now-unused ValidationInfo parameter and import go with it). Also fix the 'explicitely' typo in the normalization warning message. Signed-off-by: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com> --------- Signed-off-by: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com> Signed-off-by: Francisco Javier Arceo <farceo@redhat.com> Co-authored-by: Francisco Javier Arceo <farceo@redhat.com>
1 parent a9aaefc commit 996c6ea

3 files changed

Lines changed: 100 additions & 11 deletions

File tree

‎sdk/python/feast/infra/registry/sql.py‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
from pathlib import Path
77
from typing import Any, Callable, Dict, List, Optional, Union, cast
88

9-
from pydantic import StrictInt, StrictStr
9+
from pydantic import StrictInt, StrictStr, field_validator
1010
from sqlalchemy import ( # type: ignore
1111
BigInteger,
1212
Column,
@@ -306,6 +306,15 @@ class SqlRegistryConfig(RegistryConfig):
306306
thread_pool_executor_worker_count: StrictInt = 0
307307
""" int: Number of worker threads to use for asynchronous caching in SQL Registry. If set to 0, it doesn't use ThreadPoolExecutor. """
308308

309+
@field_validator("read_path")
310+
def validate_read_path(cls, read_path: Optional[str]) -> Optional[str]:
311+
# Mirror `RegistryConfig.validate_path`: a bare `postgresql://` read_path
312+
# must be rewritten to the psycopg3 driver too, otherwise it silently
313+
# falls back to psycopg2 while `path` uses psycopg3.
314+
if read_path is not None:
315+
return cls._normalize_postgres_scheme(read_path, "read_path")
316+
return read_path
317+
309318

310319
class SqlRegistry(CachingRegistry):
311320
def __init__(

‎sdk/python/feast/repo_config.py‎

Lines changed: 24 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -205,19 +205,33 @@ class RegistryConfig(FeastBaseModel):
205205
mcp: Optional[McpRegistryConfig] = None
206206
""" McpRegistryConfig: MCP (Model Context Protocol) configuration for the registry REST server. """
207207

208+
@staticmethod
209+
def _normalize_postgres_scheme(value: str, field_name: str) -> str:
210+
"""Rewrite a bare ``postgresql://`` URL to the psycopg3 driver, with a warning.
211+
212+
SQLAlchemy resolves a bare ``postgresql://`` to the psycopg2 driver, while
213+
feast standardizes on psycopg3 (``postgresql+psycopg``). Pass an explicit
214+
``postgresql+psycopg2`` to keep psycopg2. Shared by the ``path`` and
215+
``read_path`` validators so both endpoints normalize identically.
216+
"""
217+
if value.startswith("postgresql://"):
218+
_logger.warning(
219+
f"The `{field_name}` of the `RegistryConfig` starts with a plain "
220+
"`postgresql` string. We are updating this to `postgresql+psycopg` "
221+
"to ensure that the `psycopg3` driver is used by `sqlalchemy`. If "
222+
f"you want to use `psycopg2` pass `postgresql+psycopg2` explicitly "
223+
f"to `{field_name}`. To silence this warning, pass `postgresql+psycopg` "
224+
f"explicitly to `{field_name}`."
225+
)
226+
# Rewrite only the leading scheme, not any later occurrence (e.g.
227+
# inside credentials or a query string).
228+
return "postgresql+psycopg://" + value[len("postgresql://") :]
229+
return value
230+
208231
@field_validator("path")
209232
def validate_path(cls, path: str, values: ValidationInfo) -> str:
210233
if values.data.get("registry_type") == "sql":
211-
if path.startswith("postgresql://"):
212-
_logger.warning(
213-
"The `path` of the `RegistryConfig` starts with a plain "
214-
"`postgresql` string. We are updating this to `postgresql+psycopg` "
215-
"to ensure that the `psycopg3` driver is used by `sqlalchemy`. If "
216-
"you want to use `psycopg2` pass `postgresql+psycopg2` explicitely "
217-
"to `path`. To silence this warning, pass `postgresql+psycopg` "
218-
"explicitely to `path`."
219-
)
220-
return path.replace("postgresql://", "postgresql+psycopg://")
234+
return cls._normalize_postgres_scheme(path, "path")
221235
return path
222236

223237

‎sdk/python/tests/unit/infra/registry/test_sql_registry.py‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,72 @@ def shared_sqlite_db_path():
6565
yield path
6666

6767

68+
def test_read_path_normalizes_bare_postgresql_scheme(caplog):
69+
"""A bare `postgresql://` read_path is rewritten to the psycopg3 driver, the
70+
same way `path` is, and logs a migration warning. Without this, read_path
71+
silently falls back to psycopg2 while path uses psycopg3."""
72+
with caplog.at_level(logging.WARNING):
73+
config = SqlRegistryConfig(
74+
registry_type="sql",
75+
path="postgresql://localhost:5432/db",
76+
read_path="postgresql://localhost:5432/replica",
77+
)
78+
assert config.path == "postgresql+psycopg://localhost:5432/db"
79+
assert config.read_path == "postgresql+psycopg://localhost:5432/replica"
80+
# The migration warning names the read_path field specifically.
81+
assert "`read_path` of the `RegistryConfig`" in caplog.text
82+
83+
84+
def test_read_path_normalized_when_registry_type_defaulted():
85+
"""read_path normalization must still fire when registry_type is left to its
86+
default ('sql') rather than passed explicitly — the common real-world config."""
87+
config = SqlRegistryConfig(
88+
path="postgresql://localhost/db",
89+
read_path="postgresql://localhost/replica",
90+
)
91+
assert config.registry_type == "sql"
92+
assert config.read_path == "postgresql+psycopg://localhost/replica"
93+
94+
95+
@pytest.mark.parametrize(
96+
"read_path",
97+
[
98+
"postgresql+psycopg2://localhost/replica", # explicit psycopg2 preserved
99+
"postgresql+psycopg://localhost/replica", # already psycopg3
100+
"mysql://localhost/replica", # non-postgres left untouched
101+
],
102+
)
103+
def test_read_path_leaves_explicit_scheme_untouched(read_path):
104+
config = SqlRegistryConfig(
105+
registry_type="sql",
106+
path="sqlite:///unused.db",
107+
read_path=read_path,
108+
)
109+
assert config.read_path == read_path
110+
111+
112+
def test_read_path_none_stays_none():
113+
config = SqlRegistryConfig(registry_type="sql", path="sqlite:///unused.db")
114+
assert config.read_path is None
115+
116+
117+
@pytest.mark.parametrize("field_name", ["path", "read_path"])
118+
def test_postgres_normalization_only_rewrites_leading_scheme(field_name):
119+
raw_url = "postgresql://localhost/db?target=postgresql://replica"
120+
config_values = {
121+
"registry_type": "sql",
122+
"path": "sqlite:///unused.db",
123+
}
124+
config_values[field_name] = raw_url
125+
126+
config = SqlRegistryConfig(**config_values)
127+
128+
assert (
129+
getattr(config, field_name)
130+
== "postgresql+psycopg://localhost/db?target=postgresql://replica"
131+
)
132+
133+
68134
def test_proto_columns_use_longblob_on_mysql():
69135
"""On MySQL and MariaDB, serialized-proto columns must compile to LONGBLOB.
70136

0 commit comments

Comments
 (0)