mikebridge commented on code in PR #45288:
URL: https://github.com/apache/superset/pull/45288#discussion_r4287679539
##########
tests/unit_tests/semantic_layers/values_endpoint_test.py:
##########
@@ -577,3 +577,139 @@ def
test_unversioned_suggestion_cache_material_is_unchanged(
}
keys.append(cache.get.call_args.args[0])
assert keys[0] == keys[1]
+
+
[email protected]("use_default", [False, True])
[email protected]("warm", [False, True])
[email protected]("force", [False, True])
[email protected]("search", [False, True])
+def test_disabled_values_cache_skips_reads_and_writes(
+ client: Any,
+ full_api_access: None,
+ semantic_view_datasource: SemanticView,
+ mocker: MockerFixture,
+ use_default: bool,
+ warm: bool,
+ force: bool,
+ search: bool,
+) -> None:
+ """A disabled effective timeout bypasses warm and cold suggestion
caches."""
+ from flask import current_app
+
+ from superset.constants import CACHE_DISABLED_TIMEOUT
+
+ semantic_view_datasource.cache_timeout = (
+ None if use_default else CACHE_DISABLED_TIMEOUT
+ )
+ mocker.patch.dict(
+ current_app.config,
+ {"CACHE_DEFAULT_TIMEOUT": CACHE_DISABLED_TIMEOUT if use_default else
300},
+ )
+ cache: MagicMock =
mocker.patch("superset.datasource.api.cache_manager").data_cache
+ cache.get.return_value = ["stale"] if warm else None
+ response: Any = _get(
+ client,
+ f"category/values/?force={str(force).lower()}" + ("&q=oo" if search
else ""),
+ )
+
+ assert response.status_code == 200
+ assert response.json["result"] == ["Books", "Clothing"]
+ assert response.headers["X-Cache-Status"] == "MISS"
+ cache.get.assert_not_called()
+ cache.set.assert_not_called()
+ implementation: MagicMock = cast(MagicMock,
semantic_view_datasource.implementation)
+ implementation.get_values.assert_called_once()
+
+
[email protected]("use_default", [False, True])
[email protected]("warm", [False, True])
+def test_disabled_compatible_cache_skips_reads_and_writes(
+ client: Any,
+ full_api_access: None,
+ semantic_view_datasource: SemanticView,
+ mocker: MockerFixture,
+ use_default: bool,
+ warm: bool,
+) -> None:
+ """A disabled compatibility cache must recompute even with a stored
result."""
+ from flask import current_app
+
+ from superset.constants import CACHE_DISABLED_TIMEOUT
+
+ semantic_view_datasource.cache_timeout = (
+ None if use_default else CACHE_DISABLED_TIMEOUT
+ )
+ mocker.patch.dict(
+ current_app.config,
+ {"CACHE_DEFAULT_TIMEOUT": CACHE_DISABLED_TIMEOUT if use_default else
300},
+ )
+ metrics: MagicMock = mocker.patch.object(
+ SemanticView, "get_compatible_metrics", return_value=["revenue"]
+ )
+ dimensions: MagicMock = mocker.patch.object(
+ SemanticView, "get_compatible_dimensions", return_value=["category"]
+ )
+ cache: MagicMock =
mocker.patch("superset.datasource.api.cache_manager").data_cache
+ cache.get.return_value = {"stale": True} if warm else None
+
+ response: Any = client.post(
+ "/api/v1/datasource/semantic_view/1/compatible", json={}
+ )
+
+ assert response.status_code == 200
+ assert response.json["result"] == {
+ "compatible_metrics": ["revenue"],
+ "compatible_dimensions": ["category"],
+ }
+ metrics.assert_called_once_with([], [])
+ dimensions.assert_called_once_with([], [])
+ cache.get.assert_not_called()
+ cache.set.assert_not_called()
+
+
[email protected]("compatible", [False, True])
[email protected](
+ "datasource_timeout, default_timeout, expected_timeout",
+ [(120, 300, 120), (None, 300, 300), (0, 300, 300), (120, -1, 120)],
Review Comment:
Added the `(None, 0, 0)` row locally to
`test_enabled_suggestion_caches_preserve_timeout_and_hits`, for both
column-values and compatibility. Replacing the sentinel guards with `timeout >
0` fails both new cases; the other eight enabled-cache cases pass. The final
endpoint module passes all 89 tests, and hooks/MyPy pass. This is staged with
the compatibility-key change below and is awaiting independent review before
push.
##########
superset/datasource/api.py:
##########
@@ -672,7 +678,14 @@ def compatible(self, datasource_type: str, datasource_id:
int) -> FlaskResponse:
).hexdigest()
)
- if (cached := cache_manager.data_cache.get(cache_key)) is not None:
+ timeout: int = datasource.cache_timeout or app.config.get(
Review Comment:
I prepared the small key change locally: `compatible:` includes the same
string form of `changed_on` used by column-values keys.
`test_compatible_cache_invalidates_after_datasource_edit` first proves an
unchanged datasource hits its warm entry, then changes its modification time
and verifies fresh compatibility results. It failed with the old key and passes
with the new component. The rollout makes previous compatibility entries
unreachable once, causing a one-time miss; it does not delete them or reclaim
their storage. Docs and a PR-body update are prepared. Since this changes cache
identity, I am holding the staged candidate for independent review before
committing or pushing.
--
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]
