sadpandajoe commented on code in PR #45522: URL: https://github.com/apache/superset/pull/45522#discussion_r4406096536
########## superset-frontend/src/core/http/index.ts: ########## @@ -0,0 +1,91 @@ +/** + * 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. + */ + +/** + * @fileoverview Host side of the `http` namespace. + * + * A thin adapter over `SupersetClient`, which is what actually holds the + * session, the CSRF token and the API host. The adapter exists so that + * contract is the only thing an extension sees: `@superset-ui/core` is a + * large package whose whole surface would otherwise become extension API by + * accident, and its internals move between releases while this does not. + */ + +import { SupersetClient } from '@superset-ui/core'; +import type { JsonResponse } from '@superset-ui/core'; +import type { http as httpApi } from '@apache-superset/core'; + +type Verb = 'get' | 'post' | 'put' | 'delete'; + +const toSearchParams = ( + searchParams: httpApi.HttpRequest['searchParams'], +): URLSearchParams | undefined => { + if (!searchParams) { + return undefined; + } + if (searchParams instanceof URLSearchParams) { + return searchParams; + } + return new URLSearchParams( + Object.entries(searchParams).map(([key, value]) => [key, String(value)]), + ); +}; + +const request = + (method: Verb) => + async <T = unknown>( + options: httpApi.HttpRequest, + ): Promise<httpApi.HttpResponse<T>> => { + const { endpoint, jsonPayload, searchParams, headers, signal, timeout } = + options; + // `//host/path` is protocol-relative and resolves to another origin + // while still beginning with a slash, so the second character has to be + // checked too. Rejected rather than passed through: this call carries the + // session and the CSRF token, and neither belongs to anyone else. + if (!endpoint.startsWith('/') || endpoint.startsWith('//')) { + throw new Error( + `http.${method}: endpoint must be a path beginning with "/", got ` + + `"${endpoint}". This is the client for this Superset's API; use ` + + `fetch to reach another origin.`, + ); + } + const { json, response } = (await SupersetClient[method]({ + endpoint, + ...(jsonPayload === undefined ? {} : { jsonPayload }), + ...(searchParams ? { searchParams: toSearchParams(searchParams) } : {}), Review Comment: The published contract says `searchParams` are appended to the endpoint, but the delegated `getFullUrl` assigns `url.search`, which replaces any query already on the endpoint. `http.get({ endpoint: '/api/v1/chart/?q=(page_size:1)', searchParams: { _: 123 } })` requests `.../chart/?_=123` and silently drops `q`, so the extension gets the unfiltered default result instead of one chart. Should the adapter merge the endpoint's existing query with `searchParams`, or should the type say the endpoint must carry no query? ########## superset-frontend/packages/superset-core/src/pages/index.ts: ########## @@ -0,0 +1,243 @@ +/** + * 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. + */ + +/** + * @fileoverview Pages API for Superset extensions. + * + * A page is a screen of the extension's own, at its own URL, as opposed to a + * view that occupies a slot inside a screen the host already ships. + * + * A page is declared in the extension's manifest and registered at runtime. + * Both halves are needed: the manifest declaration is what the server reads + * to register a URL rule, so a cold load of the page resolves; the runtime + * registration is what supplies the component to render there. + * + * The path is the extension author's to choose, but it is not theirs to take. + * The server checks every declared path against the routes the deployment + * actually serves, on every boot, and refuses a page whose path is already + * answered by Superset, by an operator's blueprint, or by another extension. + * + * @example + * ```typescript + * // extension.json + * // "contributions": { "pages": [{ "path": "/review-queue", "name": "Queue" }] } + * import { pages } from '@apache-superset/core'; + * + * pages.registerPage({ path: '/review-queue', name: 'Queue' }, ReviewQueue); + * ``` + */ + +import { ComponentType } from 'react'; +import { Disposable, Event } from '../common'; + +/** + * A page contributed by an extension. + */ +export interface Page { + /** + * The absolute path the page is served at, such as `/review-queue`. + * + * Lowercase segments, leading slash, no trailing slash, no parameters: a + * page owns a fixed prefix and routes within it on the client. The page + * also owns everything beneath that prefix, so pick a path specific enough + * that nothing else will want it. + * + * Must match a path declared in the extension's manifest. The host registers + * rules from the manifest, so a component registered against an undeclared + * path would mount on client navigation and 404 on a cold load; the host + * rejects that registration rather than let the two disagree. + */ + path: string; + /** The display name of the page. */ + name: string; + /** Optional description, for display in contribution manifests. */ + description?: string; + /** + * Opt the page into Superset's navigation bar. + * + * Without this a page is reachable only by its URL, which is rarely what + * an extension wants: there is otherwise nowhere in Superset a user could + * discover it. Omit it for a page reached from somewhere the extension + * renders itself, such as a button on one of its own screens. + */ + nav?: { + /** Defaults to the page's `name`. */ + label?: string; + /** + * A group of the Settings menu to sit in, such as `Manage`, rather than + * the top-level navigation bar. + * + * Use it for a screen an administrator goes looking for rather than one + * a user works in daily. The group has to be one Superset already + * renders: it builds Manage only when the reader may see something in + * it, and an entry cannot create a group of its own. + */ + category?: string; + }; + /** + * Permission a user must hold to open the page. + * + * Declared in the manifest; the host creates it against the extension when + * the extension loads, so an administrator can grant it in the Roles + * screen as, for example, `can_review on extension:acme.tools` -- the + * host names the view menu after the extension, with that prefix. The + * server refuses the page with 403 to anyone without it. + * + * Omit to require only that the user is signed in, which is how every page + * behaves without this field. + */ + permission?: string; +} + +/** + * A page together with the extension that contributed it. + */ +export interface ResolvedPage extends Page { + /** The extension that contributed the page, as `publisher.name`. */ + extensionId: string; + /** + * Whether the signed-in user may open this page. + * + * Always true for a page that declares no `permission`. The host hides a + * page the user may not open from the navigation bar and renders a refusal + * in its place, and the server refuses it independently -- this is what + * the host knows, not what enforces it. + */ + permitted: boolean; +} + +/** + * Registers a page, mounted by the host as a client-side route. + * + * The component renders inside the host's own chrome, under an error + * boundary. + * + * It cannot import the host's router or store directly: `react-router-dom` + * and `react-redux` are not in the Module Federation share scope, so an + * extension importing either gets a second module instance with its own + * React context, and `useLocation()` throws at runtime. Use `useSubPath` and + * `useNavigate` below, which read the host's router from inside the host + * bundle, and `@superset-ui/core`, which the host offers in the share scope, Review Comment: This tells extension authors that `@superset-ui/core` is offered in the share scope for `SupersetClient`, but `webpack.config.js` now deliberately does not share it (the comment there says the `http` namespace replaces it). An author who follows this bundles a second, unconfigured `SupersetClient`, so a page POST goes out without the host's CSRF token and fails validation. Should this point to `http` from `@apache-superset/core` instead, and drop the sentence about declaring the package in the extension's own `package.json`? ########## superset-frontend/src/core/http/index.ts: ########## @@ -0,0 +1,91 @@ +/** + * 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. + */ + +/** + * @fileoverview Host side of the `http` namespace. + * + * A thin adapter over `SupersetClient`, which is what actually holds the + * session, the CSRF token and the API host. The adapter exists so that + * contract is the only thing an extension sees: `@superset-ui/core` is a + * large package whose whole surface would otherwise become extension API by + * accident, and its internals move between releases while this does not. + */ + +import { SupersetClient } from '@superset-ui/core'; +import type { JsonResponse } from '@superset-ui/core'; +import type { http as httpApi } from '@apache-superset/core'; + +type Verb = 'get' | 'post' | 'put' | 'delete'; + +const toSearchParams = ( + searchParams: httpApi.HttpRequest['searchParams'], +): URLSearchParams | undefined => { + if (!searchParams) { + return undefined; + } + if (searchParams instanceof URLSearchParams) { + return searchParams; + } + return new URLSearchParams( + Object.entries(searchParams).map(([key, value]) => [key, String(value)]), + ); +}; + +const request = + (method: Verb) => + async <T = unknown>( + options: httpApi.HttpRequest, + ): Promise<httpApi.HttpResponse<T>> => { + const { endpoint, jsonPayload, searchParams, headers, signal, timeout } = + options; + // `//host/path` is protocol-relative and resolves to another origin + // while still beginning with a slash, so the second character has to be + // checked too. Rejected rather than passed through: this call carries the + // session and the CSRF token, and neither belongs to anyone else. + if (!endpoint.startsWith('/') || endpoint.startsWith('//')) { + throw new Error( + `http.${method}: endpoint must be a path beginning with "/", got ` + + `"${endpoint}". This is the client for this Superset's API; use ` + + `fetch to reach another origin.`, + ); + } + const { json, response } = (await SupersetClient[method]({ + endpoint, + ...(jsonPayload === undefined ? {} : { jsonPayload }), Review Comment: `HttpRequest.jsonPayload` is documented as ignored only by `get`, but `SupersetClient`'s `callApi` attaches a JSON body only for POST, PATCH and PUT. So `http.del({ endpoint: '/api/v1/acme/reviews/', jsonPayload: { ids: [42] } })` sends no body and no content type, and an endpoint that reads `request.get_json()` rejects the request instead of deleting. The verb test in `index.test.ts` checks only that the mocked client's `delete` was called, so it passes either way. Should `del` reject or document a body-less contract, with a test through the real client asserting what actually goes out? -- 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]
