amaannawab923 commented on code in PR #45522: URL: https://github.com/apache/superset/pull/45522#discussion_r4400646026
########## superset-frontend/src/core/pages/index.ts: ########## @@ -0,0 +1,326 @@ +/** + * 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 registry for extension-contributed pages. + * + * Mirrors `src/core/views`: module-level state, a `useSyncExternalStore` + * subscription so a registration arriving after first render still lands, and + * an `ExtensionPlaceholder` for a page whose component has not arrived. + * + * The one thing this adds is holding an extension to its manifest. A page's + * path is declared there, which is what the server reads when it registers a + * URL rule and checks the path against every route the deployment serves. A + * component registered against a path the manifest does not declare has no + * rule behind it: it would mount on client navigation and 404 on a cold load. + * Rejecting it here keeps the two halves from disagreeing. + */ + +import React, { + ComponentType, + createContext, + useCallback, + useContext, + useSyncExternalStore, +} from 'react'; +import { useHistory, useLocation } from 'react-router-dom'; +import type { pages as pagesApi } from '@apache-superset/core'; +import { logging } from '@apache-superset/core/utils'; +import PageBoundary from './PageBoundary'; +import ExtensionPlaceholder from 'src/extensions/ExtensionPlaceholder'; +import { Disposable } from '../models'; +import { createEventEmitter } from '../utils'; + +type Page = pagesApi.Page; +type ResolvedPage = pagesApi.ResolvedPage; + +/** + * A page as the server reported it: what the manifest declares, plus whether + * this user may open it. Bound per extension by the loader. + */ +export interface DeclaredPage { + path: string; + /** False when the server refused the path and registered no rule for it. */ + registered?: boolean; + permitted?: boolean; + /** Everything else the manifest declared about the page. */ + [field: string]: unknown; +} + +/** A registration that did not take, so a caller can dispose it harmlessly. */ +const INERT = new Disposable(() => {}); + +/** + * Mirrors `PAGE_PATH_PATTERN` in `superset_core.extensions.constants`. + * + * Checked again here because this is the last point before a path is handed + * to the router, and because the message a developer sees for a malformed + * path should arrive in the console they are already watching. + */ +const PAGE_PATH_PATTERN = /^(\/[a-z]([a-z0-9]*(-[a-z0-9]+)*)?)+$/; + +const pageRegistry: Map< + string, + { page: ResolvedPage; component: ComponentType } +> = new Map(); + +const syncListeners = new Set<() => void>(); +const subscribe = (listener: () => void) => { + syncListeners.add(listener); + return () => syncListeners.delete(listener); +}; + +const registerEmitter = createEventEmitter<ResolvedPage>(); +const unregisterEmitter = createEventEmitter<ResolvedPage>(); + +// `useSyncExternalStore` requires a referentially stable snapshot between +// notifications, so the array is rebuilt only when the registry changes. +let snapshot: ResolvedPage[] = []; +const refresh = () => { + snapshot = Array.from(pageRegistry.values(), entry => entry.page); + syncListeners.forEach(listener => listener()); +}; + +/** + * Registers a page on behalf of a specific extension. + * + * Not part of the public API: the loader binds `extensionId` and the paths + * that extension's manifest declares before handing the namespace to + * extension code, so an extension can neither claim another extension's id + * nor register a path it did not declare. See `ExtensionsLoader.loadModule`. + */ +export const registerPageFor = ( + extensionId: string, + declared: readonly DeclaredPage[], + page: Page, + component: ComponentType, +): Disposable => { + // Nothing here throws. An extension calls `registerPage` at module scope, + // so a throw takes down the whole bundle -- its other pages, its views, its + // commands -- over one page. Worse, one of these conditions is not the + // author's doing at all: the server refuses a path it has already given to + // something else, and the author cannot see that from their own source. The + // page is dropped, loudly, and the extension carries on. + if (!page?.path || !PAGE_PATH_PATTERN.test(page.path)) { + logging.error( + `Extension "${extensionId}" registered a page at ` + + `"${page?.path ?? '<missing>'}", which is not an absolute path of ` + + `lowercase segments such as "/review-queue". The page was ignored.`, + ); + return INERT; + } + + const declaration = declared.find(entry => entry.path === page.path); + if (!declaration) { + logging.error( + `Extension "${extensionId}" registered a page at "${page.path}", which ` + + `its manifest does not declare. Add it to contributions.pages and ` + + `rebuild: the server registers URL rules from the manifest, so this ` + + `page would work on client navigation and 404 on a cold load. ` + + `Declared: ${declared.map(entry => entry.path).join(', ') || '(none)'}.`, + ); + return INERT; + } + + if (declaration.registered === false) { + // Two causes, and from here they look the same: the path is held by + // Superset, an operator's blueprint or another extension, or the page was + // added to the manifest after this server started, since rules are + // registered at boot while the extensions API re-reads the manifest every + // request. Naming only the conflict sent an author who had just added a + // page looking for one that does not exist, so the message names both and + // says where the answer is. + logging.error( + `Extension "${extensionId}" declares a page at "${page.path}", but the ` + + `server registered no route for it, so the page was ignored. Either ` + + `the path is already served -- the server log names what holds it, ` + + `and the manifest needs a different path -- or the page was added ` + + `after the server started, in which case restart it.`, + ); + return INERT; + } + + const id = `${extensionId}:${page.path}`; + // Registered whether or not the user may open it, so that the component is + // there to render a refusal rather than a blank screen, and so an + // extension's own bundle never has to branch on permissions. + // + // The manifest's own fields come first, so a page that declares `nav` or + // `name` there does not have to repeat them in code to be seen. They are + // only defaults: anything the registration passes wins, because that is the + // call the author wrote. + const { + path: _path, + registered: _registered, + permitted, + ...declaredFields + } = declaration; + const resolved: ResolvedPage = { + ...(declaredFields as Partial<Page>), + ...page, + extensionId, + permitted: permitted !== false, + }; + + const replaced = pageRegistry.get(id); + if (replaced) { + // Within one extension only -- ids carry the extension, and the server + // has already refused a path two extensions both declare. Silent + // replacement would make the first registration stop rendering with + // nothing said about it. + logging.warn( + `Page "${id}" is already registered; it has been replaced and the ` + + `earlier component no longer renders.`, + ); + } + + const entry = { page: resolved, component }; + pageRegistry.set(id, entry); + refresh(); + registerEmitter.fire(resolved); + + return new Disposable(() => { + // Only while this registration still holds the id: disposing one that has + // since been replaced would remove the replacement instead. + if (pageRegistry.get(id) !== entry) { + return; + } + pageRegistry.delete(id); + refresh(); + unregisterEmitter.fire(resolved); + }); +}; + +export const getPages = (): ResolvedPage[] => snapshot; + +export const usePages = (): ResolvedPage[] => + useSyncExternalStore(subscribe, getPages, getPages); + +/** The page registered for a path, or undefined when nothing claims it. */ +export const findPageByPath = (path: string): ResolvedPage | undefined => + // Longest match, not the first registered. `/reports` and + // `/reports/archive` can both claim `/reports/archive`, and taking the + // first leaves the deeper page unreachable whenever it happens to be + // registered second -- which is load order, not anything an author + // controls. The server resolves the same way, because Werkzeug prefers the + // rule with more static segments. + snapshot + .filter(page => path === page.path || path.startsWith(`${page.path}/`)) + .sort((a, b) => b.path.length - a.path.length)[0]; + +/** + * The root path of the page currently rendering. + * + * Supplied by the host when it mounts a page, so `useSubPath`/`useNavigate` + * can work in paths relative to that page without the page knowing its own + * URL. Defaults to empty, which makes both hooks inert rather than wrong if + * they are somehow called outside a page. + */ +export const PageRootContext = createContext<string>(''); + +/** + * The host's own router is read here, inside the host bundle. + * + * An extension cannot do this for itself: `react-router-dom` is not in the + * share scope, so an extension's import resolves to a second module instance + * whose context is never populated by the host's `<Router>`. Exposing these + * two hooks through `@apache-superset/core`, which *is* shared, is what lets + * a page navigate at all. + */ +export const useSubPath: typeof pagesApi.useSubPath = () => { + const root = useContext(PageRootContext); + const { pathname } = useLocation(); + if (!root) { + return '/'; + } + // `useLocation` reports a path the router has already stripped of the app + // root, so stripping again is wrong rather than merely redundant: it would + // eat a real first segment wherever a page path happens to begin with the + // same one, leaving the page unable to read its own sub-path. + const stripped = pathname; + const sub = stripped.startsWith(root) ? stripped.slice(root.length) : ''; + return sub.startsWith('/') ? sub : `/${sub}`; +}; + +export const useNavigate: typeof pagesApi.useNavigate = () => { + const root = useContext(PageRootContext); + const history = useHistory(); + return useCallback( + (subPath: string) => { + if (!root) { + return; + } + // Relative by contract: a page states where it is going within itself, + // and so cannot navigate out of its own namespace by accident. + const suffix = subPath.startsWith('/') ? subPath : `/${subPath}`; + // No `ensureAppRoot` here. Superset's router is mounted as + // `<BrowserRouter basename={applicationRoot()}>`, so it adds the app + // root on push and strips it from `location.pathname`. Adding it here + // too produced `/root/root/page` under a subdirectory deployment -- + // invisible wherever the app root is empty, which is most deployments. + history.push(`${root}${suffix === '/' ? '' : suffix}`); + }, + [history, root], + ); +}; + +export const resolvePage = (id: string): React.ReactElement => { + const entry = pageRegistry.get(id); + if (!entry) { + return React.createElement(ExtensionPlaceholder, { id }); + } + // `PageBoundary` keys itself by page id, so one page's crash cannot follow + // the user to another, and offers a retry and a reset on moving within the + // page -- neither of which `ErrorBoundary` can do on its own. + return React.createElement( + PageBoundary, + { key: id, id }, + React.createElement( + PageRootContext.Provider, + { value: entry.page.path }, + React.createElement(entry.component), + ), + ); +}; + +export const onDidRegisterPage = (listener: (e: ResolvedPage) => void) => Review Comment: Forwarded in f17429210a, matching `navigation.onDidChangePage` and the editors namespace, which both take `thisArgs` through to `subscribe`. A test in `src/core/pages/index.test.ts` subscribes a class method with its owner for both events; against the previous code it fails with `Cannot read properties of undefined`, which is the shape you described. ########## superset-frontend/packages/superset-core/src/pages/index.ts: ########## @@ -0,0 +1,200 @@ +/** + * 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 acme.tools`. The server refuses Review Comment: Changed in f17429210a. `view.py` already named it `extension:preset.asset-approval` in its own docstring, so the published type was the only place saying it without the prefix -- which is the one an author reads. ########## tests/unit_tests/extensions/test_pages_view.py: ########## @@ -0,0 +1,1009 @@ +# 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. + +"""Page-path registration and the collision check that guards it. + +Exercised against a bare Flask app rather than the Superset one: the thing +under test is how a declared path interacts with the URL map, and a bare app +lets each case state exactly which routes exist before the extension loads -- +which is the whole question. +""" + +import logging +from types import SimpleNamespace + +import pytest +from flask import Flask, g +from pydantic import ValidationError +from superset_core.extensions.types import Contributions, Manifest, Page +from werkzeug.exceptions import NotFound + +from superset.constants import EXTENSION_VIEW_MENU_PREFIX +from superset.extensions import view +from superset.extensions.view import ( + _first_claimant, + ensure_page_permissions, + permitted_page_paths, + register_extension_pages, + registered_page_paths, +) + +#: The view menu an extension's page permissions hang from. +VIEW_MENU = f"{EXTENSION_VIEW_MENU_PREFIX}acme.tools" + + +def manifest( + *paths: str, + extension_id: str = "acme.tools", + permission: str | None = None, +) -> Manifest: + publisher, name = extension_id.split(".", 1) + return Manifest( + publisher=publisher, + name=name, + displayName="Tools", + contributions=Contributions( + pages=[Page(path=path, name=path, permission=permission) for path in paths] + ), + ) + + +class FakeSecurityManager: + """Stands in for the real one, recording what was asked and created.""" + + def __init__(self, granted: set[tuple[str, str]] | None = None) -> None: + self.granted = granted or set() + self.existing: set[tuple[str, str]] = set() + self.created: list[tuple[str, str]] = [] + self.asked: list[tuple[str, str]] = [] + self.rollbacks = 0 + self.admin_role = SimpleNamespace(name="Admin", permissions=[]) + self.roles: dict[str, SimpleNamespace] = {"Admin": self.admin_role} + + def can_access(self, permission: str, view: str) -> bool: + self.asked.append((permission, view)) + return (permission, view) in self.granted + + def find_permission_view_menu( + self, permission: str, view: str + ) -> SimpleNamespace | None: + if (permission, view) not in self.existing: + return None + # FAB's PermissionView carries `role`, the roles holding it. The + # production code reads it, so the stand-in has to have it. + return SimpleNamespace( + permission=SimpleNamespace(name=permission), + view_menu=SimpleNamespace(name=view), + role=[ + role + for role in self.roles.values() + if (permission, view) in getattr(role, "permissions", []) + ], + ) + + def add_permission_view_menu(self, permission: str, view: str) -> None: + self.existing.add((permission, view)) + self.created.append((permission, view)) + + def find_role(self, name: str) -> SimpleNamespace | None: + return self.roles.get(name) + + def add_permission_role( + self, role: SimpleNamespace, perm_view: SimpleNamespace + ) -> None: + # Stored as the (permission, view_menu) pair the rest of this file + # asserts against, and the pair `find_permission_view_menu` derives + # `role` from -- so a grant made here is visible to the next lookup. + held = (perm_view.permission.name, perm_view.view_menu.name) + if held not in role.permissions: + role.permissions.append(held) + + # Named to match SupersetSecurityManager. A fake that invents its own + # attribute names lets production code call something that is not there. + @property + def session(self) -> SimpleNamespace: + self.rollbacks += 1 Review Comment: Changed in f17429210a so the fake counts calls to `rollback()` rather than reads of `.session`. Checked it the way you framed it: with production taking the session and not rolling back, the previous fixture passes 69/69 and the new one fails `assert 0 == 1`. ########## superset-frontend/src/extensions/ExtensionsLoader.ts: ########## @@ -211,19 +223,32 @@ class ExtensionsLoader { // receives the same pre-bound instance. Parallel loading is safe because each // container gets its own scope with its own resolved module instance. const context = createExtensionContext(extension); + const declaredPages = extension.contributions?.pages ?? []; const scopedCore = { ...window.superset, extensions: { ...window.superset.extensions, getContext: () => context, }, + // `registerPage` is bound to this extension's id and to the paths its + // manifest declares, for the same reason the backend derives an + // extension's API namespace from an ambient context rather than an + // argument: an extension states which page it is contributing, never + // whose. Because module federation resolves @apache-superset/core once + // per container, every import inside this extension receives this + // bound instance. + pages: { + ...window.superset.pages, + registerPage: (page: pagesApi.Page, component: ComponentType) => + registerPageFor(extension.id, declaredPages, page, component), Review Comment: Added in f17429210a. `ExtensionsLoader.test.ts` now loads two extensions declaring different paths, calls each scoped `pages.registerPage`, and asserts the second is rejected and the `permitted: false` declaration is preserved. Confirmed it catches the case you raised: with the closure forwarding `[{ path: page.path }]` instead of the extension's declarations, it is the only test that fails. -- 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]
