sadpandajoe commented on code in PR #45100:
URL: https://github.com/apache/superset/pull/45100#discussion_r4235833560
##########
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:
The default scope here is built from this process's metadata-DB URL
(`drivername`/`host`/`port`/`database`), and it becomes part of
`query_cache_key` for semantic-view charts. For async chart data, the web
process computes the submit-time `task_key` and the Celery worker writes the
DATA cache under its own `query_cache_key`; the follow-up `force_cached` read
on web only hits if those two keys are byte-identical. If web reaches the
metadata DB through a pooler or alias (say `pgbouncer:6432`) and workers
connect directly (`postgres`, or `host:5432` vs. bare `host`), the scopes
differ, so the worker's cached result is never read back: web raises the
cache-load miss, resubmits, and returns 202 again. Reports and alerts would
split their cache the same way. Before this change the semantic-view key had no
process-dependent component.
The docs do say to set `SEMANTIC_LAYER_CACHE_NAMESPACE` for different host
aliases, but with the default empty value nothing fails fast, it just loops.
Since the key already carries the datasource uid and the layer UUID, does it
need the DB address digest at all by default, or should this require an
explicit namespace when `GLOBAL_ASYNC_QUERIES` is enabled?
##########
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:
`readonly={clearing}` on the JSON form is what keeps users from editing
configuration while a clear is in flight, and a successful clear closes the
editor and discards those edits. In this file `JsonForms` is mocked to render
`null`, so the mock never reads `readonly` and the only lock asserted here is
the Name input and the Save button. Deleting `readonly={clearing}` from
`SemanticLayerModal.tsx` would leave this test green.
Could the mock expose a control that honors `readonly`, so this test can
assert the configuration field is not editable while the `mockedPost` promise
is pending?
--
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]
