sadpandajoe commented on code in PR #45550:
URL: https://github.com/apache/superset/pull/45550#discussion_r4342991725
##########
superset/semantic_layers/api.py:
##########
@@ -706,14 +710,40 @@ def types(self) -> FlaskResponse:
responses:
200:
description: A list of semantic layer types
+ content:
+ application/json:
+ schema:
+ type: object
+ properties:
+ result:
+ type: array
+ items:
+ type: object
+ properties:
+ id:
+ type: string
+ name:
+ type: string
+ description:
+ type: string
Review Comment:
`description` is documented as a non-null string, but `@semantic_layer`
defaults it to `None`, so `/types` returns `"description": null` for any type
registered without one. A client that validates responses against this schema
would reject an otherwise successful response. Should this be `nullable: true`,
with `docs/static/resources/openapi.json` regenerated to match?
##########
superset-frontend/src/extensions/ExtensionsList.tsx:
##########
@@ -40,15 +61,56 @@ const ExtensionsList:
FunctionComponent<ExtensionsListProps> = ({
addDangerToast,
addSuccessToast,
}) => {
- const {
- state: { loading, resourceCount, resourceCollection },
- fetchData,
- refreshData,
- } = useListViewResource<Extension>(
- 'extensions',
- t('Extensions'),
- addDangerToast,
- );
+ // The rows and the load status come from the same list response, so the
+ // restart notice always describes the list on screen, even when requests
+ // reach different servers during a rolling restart.
+ const [{ loading, collection, count, notLoaded }, setState] =
+ useState<ExtensionsListState>({
+ loading: true,
+ collection: [],
+ count: 0,
+ notLoaded: false,
+ });
+ const addDangerToastRef = useRef(addDangerToast);
+ addDangerToastRef.current = addDangerToast;
+ const latestRequestIdRef = useRef(0);
+
+ // The endpoint returns every servable extension in one page and ignores
+ // paging and sorting parameters.
+ const fetchData = useCallback(() => {
+ const requestId = latestRequestIdRef.current + 1;
+ latestRequestIdRef.current = requestId;
+ const isLatest = () => latestRequestIdRef.current === requestId;
Review Comment:
The stale-response guard here replaces the shared hook's race handling, but
none of the added tests exercise it: they all resolve the list request
immediately. If `isLatest()` were dropped, an older `not_loaded` response
arriving after a newer `loaded` one (for example during a rolling restart)
would put the restart notice back over the newer rows, and the suite would
still pass. Could you add a test with two deferred list requests, resolving the
newer `loaded` response first and the older `not_loaded` one second, asserting
the newer rows stay and no restart notice appears?
--
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]
