mikebridge commented on code in PR #45100:
URL: https://github.com/apache/superset/pull/45100#discussion_r4238428765
##########
superset/semantic_layers/models.py:
##########
@@ -169,6 +171,55 @@ class ColumnMetadata:
extra: str | None = None
+_METADATA_GENERATIONS_KEY: str = "semantic_layer_metadata_generations"
+
+
+@dataclass
+class _MetadataGenerations:
+ """Session-local generations, captured before discovery and forgotten on
Save."""
+
+ generations: dict[uuid.UUID, str] = field(default_factory=dict)
+
+ @classmethod
+ def for_session(cls, session: Session) -> _MetadataGenerations:
+ """Keep the memo's storage convention in one place."""
+ if _METADATA_GENERATIONS_KEY not in session.info:
+ session.info[_METADATA_GENERATIONS_KEY] = cls()
+ return cast(_MetadataGenerations,
session.info[_METADATA_GENERATIONS_KEY])
+
+ def capture(self, layer: SemanticLayer, session: Session) -> str:
+ """Web, task and MCP operations share a credential-independent
identity."""
+ if layer.uuid not in self.generations:
+ namespace: str | Callable[[], str] = current_app.config.get(
+ "SEMANTIC_LAYER_CACHE_NAMESPACE", ""
+ )
+ if callable(namespace):
+ namespace = namespace()
+ if not isinstance(namespace, str):
+ raise TypeError(
+ "SEMANTIC_LAYER_CACHE_NAMESPACE must resolve to a string"
+ )
+ url: sa.engine.URL =
session.get_bind(mapper=SemanticLayer).engine.url
Review Comment:
Confirmed: the default scope differs when web and workers use different
metadata-DB URL representations, so their semantic cache keys diverge.
Different hosts are not required; an omitted versus explicit default port also
reproduces it. The cached-read/resubmit path can then keep returning 202. I
verified the key mismatch and traced that path, but did not reproduce a live
Celery loop.
I'm keeping this open for a follow-up. Removing the database identity also
changes isolation for deployments sharing caches with copied layer UUIDs, so it
needs a separate design decision. The existing mitigation is to set the same
SEMANTIC_LAYER_CACHE_NAMESPACE across web, Celery and MCP for a workspace, and
distinct namespaces for isolated workspaces. I don't have live deployment
evidence to quantify how common differing connection addresses are.
_Codex analysis; follow-up disposition independently reviewed by Claude._
##########
superset-frontend/src/features/semanticLayers/SemanticLayerModal.test.tsx:
##########
@@ -225,3 +231,109 @@ test('cancels pending schema refresh when dependencies
become unsatisfied', asyn
// No additional POST should have fired; the cancelled timer must not land.
expect(mockedPost).toHaveBeenCalledTimes(1);
});
+
+test('Reload metadata confirms and closes without navigating away from the
toast', async () => {
+ mockJsonFormsChangeTriggered = true;
+ props.onHide.mockClear();
+ render(<SemanticLayerModal {...props} />);
+ const button = await screen.findByRole('button', { name: 'Reload metadata'
});
+ await waitFor(() => expect(button).toBeEnabled());
+ fireEvent.click(button);
+ await waitFor(() => expect(props.onHide).toHaveBeenCalledTimes(1));
+ expect(mockedPost).toHaveBeenCalledWith({
+ endpoint: `/api/v1/semantic_layer/${props.semanticLayerUuid}/clear_cache`,
+ jsonPayload: {},
+ });
+ expect(props.addSuccessToast).toHaveBeenCalledWith(
+ 'Cache cleared; reload to fetch metadata',
+ );
+});
+
+test('Reload metadata stays enabled after schema values are backfilled but
disables on an edit', async () => {
+ mockJsonFormsChangeTriggered = true;
+ mockedPost.mockResolvedValue({
+ json: {
+ result: {
+ type: 'object',
+ properties: {
+ warehouse: { type: 'string' },
+ region: { type: 'string', readOnly: true, default: 'us-east-1' },
+ provider: { type: 'string', const: 'snowflake' },
+ },
+ },
+ },
+ });
+
+ render(<SemanticLayerModal {...props} />);
+ const button = await screen.findByRole('button', { name: 'Reload metadata'
});
+ await waitFor(() => expect(capturedOnChange).not.toBeNull());
+
+ act(() =>
+ capturedOnChange?.({
+ data: { warehouse: 'wh0', region: 'us-east-1', provider: 'snowflake' },
+ errors: [],
+ }),
+ );
+ expect(button).toBeEnabled();
+
+ act(() =>
+ capturedOnChange?.({
+ data: { warehouse: 'wh1', region: 'us-east-1', provider: 'snowflake' },
+ errors: [],
+ }),
+ );
+ expect(button).toBeDisabled();
+});
+
+test('Reload metadata is disabled for unsaved configuration and name edits',
async () => {
+ render(<SemanticLayerModal {...props} />);
+ const button = await screen.findByRole('button', { name: 'Reload metadata'
});
+ expect(button).toBeDisabled();
+ act(() => capturedOnChange?.({ data: { warehouse: 'wh0' }, errors: [] }));
+ await waitFor(() => expect(button).toBeEnabled());
+ fireEvent.change(screen.getByPlaceholderText('Name of the semantic layer'), {
+ target: { value: 'Unsaved name' },
+ });
+ expect(button).toBeDisabled();
+});
+
+test('Reload metadata reports failure without reloading or closing the
editor', async () => {
+ mockJsonFormsChangeTriggered = true;
+ props.addDangerToast.mockClear();
+ props.onHide.mockClear();
+ render(<SemanticLayerModal {...props} />);
+ const button = await screen.findByRole('button', { name: 'Reload metadata'
});
+ await waitFor(() => expect(button).toBeEnabled());
+ mockedPost.mockRejectedValueOnce(new Error('clear failed'));
+ fireEvent.click(button);
+ await waitFor(() =>
+ expect(props.addDangerToast).toHaveBeenCalledWith(
+ 'Unable to clear metadata cache',
+ ),
+ );
+ expect(props.onHide).not.toHaveBeenCalled();
+ expect(button).toBeEnabled();
+});
+
+test('A pending clear prevents edits and a closed editor ignores its late
response', async () => {
Review Comment:
Fixed in 5c317a744428b8341189d4147eb92cdc5f80dc2e. The JsonForms mock
renders a configuration input that honors readonly. The test checks that it is
editable before Reload metadata and read-only while the clear request is
pending. Removing readonly={clearing} now fails the test; restoring it passes
all 8 modal tests. Production code is unchanged.
_Codex-authored; independently reviewed by Claude._
##########
superset/semantic_layers/models.py:
##########
@@ -166,6 +168,55 @@ class ColumnMetadata:
extra: str | None = None
+_METADATA_IDENTITY_KEY: str = "semantic_layer_cache_identity"
+
+
+@dataclass
+class _MetadataCacheIdentity:
+ """Session-local identities, captured before discovery and forgotten on
Save."""
+
+ tokens: dict[uuid.UUID, str] = field(default_factory=dict)
+
+ @classmethod
+ def for_session(cls, session: Session) -> _MetadataCacheIdentity:
+ """Keep the memo's storage convention in one place."""
+ if _METADATA_IDENTITY_KEY not in session.info:
+ session.info[_METADATA_IDENTITY_KEY] = cls()
+ return cast(_MetadataCacheIdentity,
session.info[_METADATA_IDENTITY_KEY])
+
+ def capture(self, layer: SemanticLayer, session: Session) -> str:
+ """Web, task and MCP operations share a credential-independent
identity."""
+ if layer.uuid not in self.tokens:
+ namespace: str | Callable[[], str] = current_app.config.get(
+ "SEMANTIC_LAYER_CACHE_NAMESPACE", ""
+ )
+ if callable(namespace):
+ namespace = namespace()
+ if not isinstance(namespace, str):
+ raise TypeError(
+ "SEMANTIC_LAYER_CACHE_NAMESPACE must resolve to a string"
+ )
+ url: sa.engine.URL =
session.get_bind(mapper=SemanticLayer).engine.url
Review Comment:
Agreed, thanks for clarifying. I verified that Engine.engine returns itself
and Connection.engine returns its owning Engine with an unmocked SQLite
engine/connection. No code change is needed for this observation.
_Codex verification; disposition independently reviewed by Claude._
--
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]
