mikebridge commented on code in PR #44452: URL: https://github.com/apache/superset/pull/44452#discussion_r4233943999
########## superset-frontend/packages/superset-ui-chart-controls/src/utils/isServerPaginationUnsupported.ts: ########## @@ -0,0 +1,62 @@ +/** + * 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. + */ +import { DatasourceType } from '@superset-ui/core'; +import type { ControlPanelState } from '../types'; + +/** SemanticViewFeature value for providers that support row offsets. */ +export const ROW_OFFSET = 'ROW_OFFSET'; + +/** Only selected-datasource metadata can describe its capabilities. */ +export function hasMatchingDatasourceMetadata({ + datasource, + form_data, +}: Pick<ControlPanelState, 'datasource' | 'form_data'>): boolean { + return Boolean( + datasource && + 'uid' in datasource && + datasource.uid === form_data.datasource, Review Comment: You're right, and thank you — this was a real regression, not just a theoretical one. I confirmed your reading end to end: `form_data.datasource` is built as `f"{self._datasource_id}__{self._datasource_type}"` in `ExploreGetCommand` (`superset/commands/explore/get.py:137`), while `SemanticView.uid` returns `self.implementation.uid()` — the provider's opaque id — and the payload carries `{"id": self.id, "uid": self.uid, "type": "semantic_view"}` (`superset/semantic_layers/models.py:524-525`, `:611-613`). So the equality was false for the *correct* datasource, features dropped to `undefined`, and a view advertising `ROW_OFFSET` had Server pagination disabled with no way to enable it. Datasets were unaffected only because `BaseDatasource.uid` genuinely is `f"{self.id}__{self.type}"`. To answer your question directly: nothing rewrites `form_data.datasource` to the provider uid before the control panel runs, so comparing against `datasource.id`/`type` is the right fix. `13bf5c46cd` parses `<id>__<type>` from the form value and compares it to the metadata's `id` and `type`, keeping uid equality only as the fallback for a selection that isn't of that canonical shape — so opaque provider uids still work for stale detection without being required to match the host's selection key. Worth flagging why this slipped through review: the existing fixtures used `uid: '1__semantic_view'`, a shape that satisfied the predicate under test, so the suite couldn't contradict it. The fixtures now use the real Explore shape, and with that correction the *pre-existing* `keeps semantic_view pagination editable with ['ROW_OFFSET']` test fails against the old helper — 16 tests fail in total, across both plugins, so the old predicate was wrong in several directions and not only the one you reported. All 69 pass with the fix. The new test also pins the stale case with `uid` deliberately set to `'42__semantic_view'`, so reinstating uid equality would break it. -- 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]
