sadpandajoe commented on code in PR #45790: URL: https://github.com/apache/superset/pull/45790#discussion_r4408215459
########## superset/migrations/versions/2026-10-27_00-00_9670ab7f6bc6_normalize_metadata_cache_timeouts.py: ########## @@ -0,0 +1,115 @@ +# 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 +ASCII digit 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 +""" + +import logging +from decimal import InvalidOperation + +from alembic import op +from sqlalchemy import column, Integer, select, table, Text + +from superset.utils import json + +revision = "9670ab7f6bc6" +down_revision = "f72b6e9c104a" + +logger = logging.getLogger(__name__) + +_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 ASCII digit strings to integer timeouts.""" + connection = op.get_bind() + rows = connection.execute( + select(_DATABASES.c.id, _DATABASES.c.extra).with_for_update() + ).all() + for database_id, raw_extra in rows: + try: + extra = json.loads(raw_extra, use_decimal=True) Review Comment: A `NULL` or empty-string `extra` is a normal, healthy state here (the API accepts `extra=""`, and `get_extra_params` treats both as `{}`), but `json.loads(None)` raises `TypeError` and `json.loads("")` raises a decode error, so each such database logs "invalid or unrepresentable JSON" on upgrade. Operators will see a warning per blank row that sounds like corrupt data and can't be told apart from a truly broken row. Should blank `extra` be skipped silently before the `try`? ########## superset/migrations/versions/2026-10-27_00-00_9670ab7f6bc6_normalize_metadata_cache_timeouts.py: ########## @@ -0,0 +1,115 @@ +# 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 +ASCII digit 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 +""" + +import logging +from decimal import InvalidOperation + +from alembic import op +from sqlalchemy import column, Integer, select, table, Text + +from superset.utils import json + +revision = "9670ab7f6bc6" +down_revision = "f72b6e9c104a" + +logger = logging.getLogger(__name__) + +_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 ASCII digit strings to integer timeouts.""" + connection = op.get_bind() + rows = connection.execute( + select(_DATABASES.c.id, _DATABASES.c.extra).with_for_update() + ).all() + for database_id, raw_extra in rows: + try: + extra = json.loads(raw_extra, use_decimal=True) + except (TypeError, ValueError, InvalidOperation): + logger.warning( + "Database %s: skipping metadata cache timeouts; " + "invalid or unrepresentable JSON", + database_id, + ) + 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 + if not (value.isascii() and value.isdigit()): + logger.warning( + "Database %s: leaving metadata_cache_timeout.%s unchanged; " + "expected an ASCII digit string", + database_id, + key, + ) + continue + try: + timeout = int(value) + except ValueError: + logger.warning( + "Database %s: leaving metadata_cache_timeout.%s unchanged; " + "cannot parse integer string", + database_id, + key, + ) + 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)) Review Comment: Re-serializing the whole `extra` document can make it larger than what was stored: `json.dumps` adds a space after every `,` and `:` and escapes non-ASCII to `\uXXXX`. `dbs.extra` is `Text`, which is 65,535 bytes on MySQL. For example, a compact document with `{"metadata_cache_timeout": {"table_cache_timeout": "60"}}` plus a 7,500-entry `default_schemas` list is about 60 KB stored and about 67.5 KB after this UPDATE (non-ASCII schema names grow much faster). Under strict mode the UPDATE fails and blocks the upgrade for a config that was valid; without strict mode it can store truncated JSON and `rowcount` is still 1. Would it make sense to compare the new payload's length with the original (or fall back to a compact `separators`/`ensure_ascii=False` form) and skip with a warning rather than expand past the column limit? -- 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]
