sadpandajoe commented on code in PR #45522:
URL: https://github.com/apache/superset/pull/45522#discussion_r4338926439
##########
superset-core/src/superset_core/extensions/types.py:
##########
@@ -29,12 +29,85 @@
from superset_core.extensions.constants import (
DISPLAY_NAME_PATTERN,
+ PAGE_PATH_PATTERN,
+ PERMISSION_NAME_PATTERN,
PUBLISHER_PATTERN,
TECHNICAL_NAME_PATTERN,
VERSION_PATTERN,
)
+class PageNav(BaseModel):
+ """Placement of a page in the navigation."""
+
+ label: str = Field(
+ ...,
+ description="Text of the navigation entry",
+ min_length=1,
+ )
Review Comment:
Agreed—`PageNav.label` is required here while the TypeScript `nav.label` is
optional and the docs say the page name is used when it's omitted, so a
manifest with `"nav": {}` fails validation at build or load and rejects the
whole extension. Should `label` be optional here, defaulting to the page name?
##########
tests/unit_tests/extensions/test_pages_view.py:
##########
@@ -0,0 +1,933 @@
+# 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 Forbidden, 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, object] = {"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) -> object
| 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) -> object | None:
+ return self.roles.get(name)
+
+ def add_permission_role(self, role: object, perm_view: object) -> 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)
Review Comment:
`perm_view` is annotated `object`, so reading `.permission` / `.view_menu`
here fails mypy (`attr-defined`), and lines 923-924 add an unused `type:
ignore[assignment]` and use `list.append()` as a value. Those are the four
errors failing `pre-commit (current)`. Can these be typed (e.g. `Any` or a
small protocol) and the lambda rewritten so the check passes?
--
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]
