sushu0 commented on code in PR #45790: URL: https://github.com/apache/superset/pull/45790#discussion_r4408091067
########## 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: Addressed in e4bf004e212108b1858093a21b6378ca633ab367. `superset.utils.json.loads` now appends an optional `use_decimal: bool = False` parameter and passes it through. The migration and preservation test use that wrapper with `use_decimal=True`; both direct imports and their TID251 suppressions are removed. Wrapper tests cover exact long decimals and `1e500`, default float decoding, the original four positional arguments/object hook, and NaN rejection/explicit opt-in in both Decimal modes. All 32 JSON tests pass as part of the final 99-test matching suite. The defaults, including `allow_nan=False`, are unchanged. Ruff, the real Pylint hook and Linux-target mypy also passed. ########## 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: Addressed in e4bf004e212108b1858093a21b6378ca633ab367. The cache regression no longer contains memoized cache-key templates and expects only the exception type before migration, without matching cachelib's error text. After migration it calls the actual cached accessor twice against a real SimpleCache, verifies the expected results, and uses a wrapping spy to confirm a single real `set` call with an integer timeout. All six catalog/schema/table and zero/nonzero cases pass in the final 99-test matching suite. -- 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]
