sfirke commented on code in PR #45790: URL: https://github.com/apache/superset/pull/45790#discussion_r4404863591
########## superset/migrations/versions/2026-10-27_00-00_9670ab7f6bc6_normalize_metadata_cache_timeouts.py: ########## @@ -0,0 +1,96 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Normalize legacy database metadata cache timeouts. + +Older database forms stored metadata cache timeouts as strings. Convert only +recognized non-negative integer strings, preserving malformed configurations +and unrelated fields instead of guessing at their intended values. + +Revision ID: 9670ab7f6bc6 +Revises: f72b6e9c104a +Create Date: 2026-10-27 00:00:00.000000 +""" + +from decimal import InvalidOperation + +# The shared JSON wrapper cannot decode decimals without losing stored precision. +import simplejson # noqa: TID251 +from alembic import op +from sqlalchemy import column, Integer, select, table, Text + +from superset.utils import json + +revision = "9670ab7f6bc6" +down_revision = "f72b6e9c104a" + +_DATABASES = table("dbs", column("id", Integer), column("extra", Text)) +_TIMEOUT_KEYS = ( + "catalog_cache_timeout", + "schema_cache_timeout", + "table_cache_timeout", +) + + +def upgrade() -> None: + """Convert legacy non-negative integer strings to integer timeouts.""" + connection = op.get_bind() + for database_id, raw_extra in connection.execute( + select(_DATABASES.c.id, _DATABASES.c.extra).with_for_update() + ): + try: + extra = simplejson.loads(raw_extra, use_decimal=True) + except (TypeError, ValueError, InvalidOperation): + continue + if not isinstance(extra, dict) or not isinstance( + extra.get("metadata_cache_timeout"), dict + ): + continue + timeouts = extra["metadata_cache_timeout"] + + changed = False + for key in _TIMEOUT_KEYS: + value = timeouts.get(key) + if not isinstance(value, str): + continue + try: + timeout = int(value) + except ValueError: + continue + if timeout < 0: + continue + timeouts[key] = timeout + changed = True + + if changed: + result = connection.execute( + _DATABASES.update() + .where(_DATABASES.c.id == database_id, _DATABASES.c.extra == raw_extra) + .values(extra=json.dumps(extra)) + ) + if result.rowcount != 1: Review Comment: The `rowcount != 1` guard can abort the whole upgrade on backends where the DBAPI does not report a reliable rowcount for UPDATE (e.g. MSSQL with `SET NOCOUNT ON` or a trigger on `dbs`, or any dialect with `supports_sane_rowcount=False`, which returns -1). The first converted row would raise `RuntimeError("Database N configuration changed during migration")`, roll back the migration, and tell the operator to pause edits that aren't happening. The race this is guarding against can't occur here: the `SELECT ... FOR UPDATE` above already holds the row locks for the duration of the transaction, and SQLite serializes writers. I'd either drop the compare-and-set (`.where(_DATABASES.c.extra == raw_extra)` plus this check) and rely on the locks, or only treat `rowcount == 0` as a conflict and ignore -1. ########## tests/unit_tests/migrations/test_normalize_metadata_cache_timeouts.py: ########## @@ -0,0 +1,424 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Exercise metadata timeout migration against stored SQLite rows and cachelib.""" + +from collections.abc import Callable, Iterator +from decimal import Decimal +from importlib import import_module +from typing import Any + +import pytest + +# The project wrapper cannot decode exact decimals for this preservation assertion. +import simplejson # noqa: TID251 +from alembic.migration import MigrationContext +from alembic.operations import Operations +from flask import current_app +from flask_caching.backends import SimpleCache +from pytest_mock import MockerFixture +from sqlalchemy import ( + Column, + create_engine, + event, + Integer, + MetaData, + select, + Table, + Text, +) +from sqlalchemy.engine import Connection, Engine + +from superset.extensions import cache_manager +from superset.models.core import Database +from superset.utils import json + +migration = import_module( + "superset.migrations.versions." + "2026-10-27_00-00_9670ab7f6bc6_normalize_metadata_cache_timeouts" +) +dbs = Table( + "dbs", MetaData(), Column("id", Integer, primary_key=True), Column("extra", Text) +) + + [email protected] +def engine() -> Iterator[Engine]: + """Create the migration's database columns without unrelated schema setup.""" + engine = create_engine("sqlite:///:memory:") + dbs.metadata.create_all(engine) + yield engine + engine.dispose() + + +def _run( + migrate: Callable[[], None], connection: Connection, mocker: MockerFixture +) -> None: + """Use real Alembic operations bound to the test database transaction.""" + mocker.patch.object( + migration, "op", Operations(MigrationContext.configure(connection)) + ) + migrate() + + +def _seed(engine: Engine, extra: str | None) -> None: + """Persist a historical database configuration.""" + with engine.begin() as connection: + connection.execute(dbs.insert().values(id=1, extra=extra)) + + +def _read_extra(engine: Engine) -> str | None: + """Read the persisted configuration through a new connection.""" + with engine.connect() as connection: + return connection.execute(select(dbs.c.extra).where(dbs.c.id == 1)).scalar_one() + + [email protected]("cache_type", ["catalog", "schema", "table"]) [email protected]("timeout", ["600", "0"]) +def test_upgrade_restores_metadata_cache( + engine: Engine, mocker: MockerFixture, cache_type: str, timeout: str +) -> None: + """The same real cached model method fails before migration and works after it.""" + key = f"{cache_type}_cache_timeout" + _seed(engine, json.dumps({"metadata_cache_timeout": {key: timeout}})) + database = Database( + id=1, database_name="db", sqlalchemy_uri="sqlite://", extra=_read_extra(engine) + ) + backend = SimpleCache() + mocker.patch.dict(current_app.extensions["cache"], {cache_manager.cache: backend}) + mocker.patch.object(database, "get_inspector") + methods: dict[str, tuple[str, str, dict[str, Any], str, Any]] = { + "catalog": ( + "get_all_catalog_names", + "get_catalog_names", + {}, + "db:1:catalog_list", + {"public"}, + ), + "schema": ( + "get_all_schema_names", + "get_schema_names", + {"catalog": None}, + "db:1:catalog:None:schema_list", + {"public"}, + ), + "table": ( + "get_all_table_names_in_schema", + "get_table_names", + {"catalog": None, "schema": "public"}, + "db:1:catalog:None:schema:public:table_list", + {("public", "public", None)}, + ), + } + method_name, spec_method, options, cache_key, expected = methods[cache_type] + mocker.patch.object(database.db_engine_spec, spec_method, return_value={"public"}) + get_metadata = getattr(database, method_name) + assert getattr(database, f"{cache_type}_cache_enabled") + + with pytest.raises(TypeError, match="timeout must be an int"): + get_metadata(**options, cache_timeout=getattr(database, key)) + + with engine.begin() as connection: + _run(migration.upgrade, connection, mocker) + database.extra = _read_extra(engine) + + assert type(getattr(database, key)) is int + assert getattr(database, key) == int(timeout) + assert get_metadata(**options, cache_timeout=getattr(database, key)) == expected + assert backend.get(cache_key) == expected Review Comment: This test hard-codes two implementation details that have nothing to do with the migration: `memoized_func`'s internal cache-key template (`db:1:catalog:None:schema_list` etc.) and cachelib's exact exception text (`match="timeout must be an int"` above). A refactor of the key template in `superset/utils/cache.py`, or a cachelib bump that rewords the error, would fail a migration test even though the migration is correct. The migration's behaviour is already fully covered by the pure-row tests below. If the goal is to prove the end-to-end round trip, asserting that the cached accessor returns `expected` (which you already do on the line above) and that `cache.set` was called with an `int` timeout would be enough, without reaching into the backend by key. ########## superset/migrations/versions/2026-10-27_00-00_9670ab7f6bc6_normalize_metadata_cache_timeouts.py: ########## @@ -0,0 +1,96 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Normalize legacy database metadata cache timeouts. + +Older database forms stored metadata cache timeouts as strings. Convert only +recognized non-negative integer strings, preserving malformed configurations +and unrelated fields instead of guessing at their intended values. + +Revision ID: 9670ab7f6bc6 +Revises: f72b6e9c104a +Create Date: 2026-10-27 00:00:00.000000 +""" + +from decimal import InvalidOperation + +# The shared JSON wrapper cannot decode decimals without losing stored precision. +import simplejson # noqa: TID251 +from alembic import op +from sqlalchemy import column, Integer, select, table, Text + +from superset.utils import json + +revision = "9670ab7f6bc6" +down_revision = "f72b6e9c104a" + +_DATABASES = table("dbs", column("id", Integer), column("extra", Text)) +_TIMEOUT_KEYS = ( + "catalog_cache_timeout", + "schema_cache_timeout", + "table_cache_timeout", +) + + +def upgrade() -> None: + """Convert legacy non-negative integer strings to integer timeouts.""" + connection = op.get_bind() + for database_id, raw_extra in connection.execute( + select(_DATABASES.c.id, _DATABASES.c.extra).with_for_update() + ): + try: + extra = simplejson.loads(raw_extra, use_decimal=True) + except (TypeError, ValueError, InvalidOperation): + continue + if not isinstance(extra, dict) or not isinstance( + extra.get("metadata_cache_timeout"), dict + ): + continue + timeouts = extra["metadata_cache_timeout"] + + changed = False + for key in _TIMEOUT_KEYS: + value = timeouts.get(key) + if not isinstance(value, str): + continue + try: + timeout = int(value) + except ValueError: + continue Review Comment: Rows that can't be converted here (negative strings, `"600.0"`, `"1e3"`, `""`) and rows skipped above for unparseable JSON are dropped with no log output. For those databases the upgrade completes "successfully" and the first schema/table listing after deploy still raises cachelib's `TypeError: timeout must be an int or datetime.timedelta, got 'str'`, with nothing pointing the operator at the row. Preserving the value is the right call, but a `logger.warning` naming the database id and key left unconverted (as the other recent data migrations do for skipped rows) would give operators a way to find and fix them. ########## superset/migrations/versions/2026-10-27_00-00_9670ab7f6bc6_normalize_metadata_cache_timeouts.py: ########## @@ -0,0 +1,96 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Normalize legacy database metadata cache timeouts. + +Older database forms stored metadata cache timeouts as strings. Convert only +recognized non-negative integer strings, preserving malformed configurations +and unrelated fields instead of guessing at their intended values. + +Revision ID: 9670ab7f6bc6 +Revises: f72b6e9c104a +Create Date: 2026-10-27 00:00:00.000000 +""" + +from decimal import InvalidOperation + +# The shared JSON wrapper cannot decode decimals without losing stored precision. +import simplejson # noqa: TID251 +from alembic import op +from sqlalchemy import column, Integer, select, table, Text + +from superset.utils import json + +revision = "9670ab7f6bc6" +down_revision = "f72b6e9c104a" + +_DATABASES = table("dbs", column("id", Integer), column("extra", Text)) +_TIMEOUT_KEYS = ( + "catalog_cache_timeout", + "schema_cache_timeout", + "table_cache_timeout", +) + + +def upgrade() -> None: + """Convert legacy non-negative integer strings to integer timeouts.""" + connection = op.get_bind() + for database_id, raw_extra in connection.execute( Review Comment: This iterates the SELECT result lazily while issuing UPDATEs on the same connection inside the loop. That works because the default DBAPI cursors are client-buffered, but under server-side/streaming cursors (`stream_results=True`, psycopg2 named cursors, MySQL `SSCursor`) the interleaved UPDATE on the same connection can invalidate or interleave with the open result cursor and raise mid-iteration or skip rows. Materializing first is cheap for a table this size and removes the dependency on cursor mode: ```python rows = connection.execute( select(_DATABASES.c.id, _DATABASES.c.extra).with_for_update() ).all() for database_id, raw_extra in rows: ``` ########## superset/migrations/versions/2026-10-27_00-00_9670ab7f6bc6_normalize_metadata_cache_timeouts.py: ########## @@ -0,0 +1,96 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Normalize legacy database metadata cache timeouts. + +Older database forms stored metadata cache timeouts as strings. Convert only +recognized non-negative integer strings, preserving malformed configurations +and unrelated fields instead of guessing at their intended values. + +Revision ID: 9670ab7f6bc6 +Revises: f72b6e9c104a +Create Date: 2026-10-27 00:00:00.000000 +""" + +from decimal import InvalidOperation + +# The shared JSON wrapper cannot decode decimals without losing stored precision. +import simplejson # noqa: TID251 Review Comment: Minor: this PR adds two direct `import simplejson # noqa: TID251` suppressions (here and in the test at line 27) to get `use_decimal=True`. The banned-API rule exists to funnel JSON handling through `superset.utils.json`, so rather than bypassing it, a `use_decimal` (or `parse_float`) passthrough kwarg on `superset.utils.json.loads` would be a two-line change that removes both suppressions and keeps the project's `allow_nan=False` default. It also avoids the next migration that needs lossless numeric round-tripping copying the same `noqa`. ########## superset/migrations/versions/2026-10-27_00-00_9670ab7f6bc6_normalize_metadata_cache_timeouts.py: ########## @@ -0,0 +1,96 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Normalize legacy database metadata cache timeouts. + +Older database forms stored metadata cache timeouts as strings. Convert only +recognized non-negative integer strings, preserving malformed configurations +and unrelated fields instead of guessing at their intended values. + +Revision ID: 9670ab7f6bc6 +Revises: f72b6e9c104a +Create Date: 2026-10-27 00:00:00.000000 +""" + +from decimal import InvalidOperation + +# The shared JSON wrapper cannot decode decimals without losing stored precision. +import simplejson # noqa: TID251 +from alembic import op +from sqlalchemy import column, Integer, select, table, Text + +from superset.utils import json + +revision = "9670ab7f6bc6" +down_revision = "f72b6e9c104a" + +_DATABASES = table("dbs", column("id", Integer), column("extra", Text)) +_TIMEOUT_KEYS = ( + "catalog_cache_timeout", + "schema_cache_timeout", + "table_cache_timeout", +) + + +def upgrade() -> None: + """Convert legacy non-negative integer strings to integer timeouts.""" + connection = op.get_bind() + for database_id, raw_extra in connection.execute( + select(_DATABASES.c.id, _DATABASES.c.extra).with_for_update() + ): + try: + extra = simplejson.loads(raw_extra, use_decimal=True) + except (TypeError, ValueError, InvalidOperation): + continue + if not isinstance(extra, dict) or not isinstance( + extra.get("metadata_cache_timeout"), dict + ): + continue + timeouts = extra["metadata_cache_timeout"] + + changed = False + for key in _TIMEOUT_KEYS: + value = timeouts.get(key) + if not isinstance(value, str): + continue + try: + timeout = int(value) Review Comment: `int(value)` accepts quite a bit more than the "recognized non-negative integer strings" the docstring promises. Python's `int()` parses underscores, surrounding whitespace/newlines, a leading `+`, and non-ASCII Unicode digits: ```python >>> int("1_000"), int(" 600 "), int("6\n"), int("+5"), int("\u0666\u0660\u0660") (1000, 600, 6, 5, 600) ``` So a stored `"schema_cache_timeout": "1_000"` gets silently rewritten to `1000`, which cuts against the stated policy of not guessing at malformed values. Gating on `value.isascii() and value.isdigit()` (or a `^\d+$` regex) before calling `int()` would make the behaviour match the docstring; it would also make the `timeout < 0` check below redundant. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
