This is an automated email from the ASF dual-hosted git repository.
bbovenzi pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/airflow.git
The following commit(s) were added to refs/heads/main by this push:
new 7c014e1b61a Fix plugin applies_to scoping defects found in review
(#74425)
7c014e1b61a is described below
commit 7c014e1b61a6fe9a77f7ecb0a6600f5c1e9b26e7
Author: Brent Bovenzi <[email protected]>
AuthorDate: Thu Oct 8 13:27:19 2026 -0400
Fix plugin applies_to scoping defects found in review (#74425)
Several things shipped with the field-path scoping in #74278 that review
caught after it merged, plus two fixes that depend on work landing since.
The troubleshooting steps name `airflow plugins list`, which does not exist
--
`plugins` takes no subcommand -- so anyone following them to find out why a
view is not scoped as expected gets an argparse error instead.
The operator section recommends leaving `operator` and
`class_ref.class_name`
unqualified so each page reads its own record, which is right, but the
load-time check then reports the one the page lacks as a path that will make
the view "appear in more places than intended". That is untrue for this
block:
the other path is what scopes this destination. Following the documentation
should not produce a warning saying the documentation is wrong. The check
now
tells a path that names no field anywhere -- a misspelling -- apart from one
that names a real field on a record this destination does not have, and says
only what is true of each. The section leads with one view per destination,
which needs no such reasoning, and keeps the single block as the shorter
form.
A path that matches no field at all now withholds the view instead of
loading
it. The browser cannot tell a misspelling from a field this page's record
simply does not have -- a block shared across destinations relies on the
latter being skipped -- so it skips both, and skipping widens the scope. A
typo therefore showed the view everywhere, the opposite of what its author
asked for. The response models are in hand when plugins load, so the two are
distinguishable there: a field no record has cannot be anything but a
mistake,
while one a sibling record has is the shared-block pattern and still loads.
A
view that disappears gets noticed; one on every page looks deliberate.
Validation also edited the plugin author's own `applies_to` dictionary to
drop
null values. Harmless in isolation, but a plugin may pass one dictionary to
several views, and rewriting it is not this code's to do.
Which record an unqualified path is rooted at is also decided twice: once
when
plugins load, to choose what to validate, and again in the browser, to
choose
what to match. Nothing at runtime notices when the two tables disagree -- a
destination only one of them knows about makes every path on it unevaluable,
which shows the view everywhere instead of scoping it, and would surface
later
as someone else's bug. A prek check compares them, so that drift fails
before
it merges.
Scoping a task-level view on `task.*` judged historical task instances by
the
Dag's definition today, so a view scoped to an operator stopped appearing on
the runs that actually used it as soon as the task changed. #74365 added a
version to the task endpoint, so the task is now read at the version the
instance ran. That costs a round trip on task instance routes, since the
version is not known until the instance arrives, and asking earlier would
fetch a task that has to be discarded.
The newsfragment was a documentation section rather than a release note, so
it
is cut to what the release notes need and points at the docs for the rest.
Editing it trips `check-newsfragment-pr-number`, which expects any
newsfragment
a PR touches to be named for that PR; renaming this one would credit the
wrong
PR for the feature, so the check is skipped by label instead.
---
.pre-commit-config.yaml | 11 ++
.../docs/administration-and-deployment/plugins.rst | 105 ++++++++------
airflow-core/newsfragments/74278.feature.rst | 2 +-
airflow-core/src/airflow/plugins_manager.py | 93 +++++++++---
.../ui/src/hooks/usePluginAppliesToContext.test.ts | 32 ++++-
.../ui/src/hooks/usePluginAppliesToContext.ts | 49 ++++---
.../airflow/ui/src/utils/pluginAppliesTo.test.ts | 7 +-
.../tests/unit/plugins/test_plugins_manager.py | 158 +++++++++++++++++++--
scripts/ci/prek/check_applies_to_roots_in_sync.py | 117 +++++++++++++++
.../ci/prek/test_check_applies_to_roots_in_sync.py | 84 +++++++++++
10 files changed, 562 insertions(+), 96 deletions(-)
diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
index bfb428ed116..8392915d926 100644
--- a/.pre-commit-config.yaml
+++ b/.pre-commit-config.yaml
@@ -248,6 +248,17 @@ repos:
^pyproject\.toml$
pass_filenames: false
require_serial: true
+ - id: check-applies-to-roots-in-sync
+ name: Check plugin applies_to destination tables stay in sync
+ description: "The UI mirrors plugins_manager's destination -> record
map; drift silently widens plugin scope"
+ entry: ./scripts/ci/prek/check_applies_to_roots_in_sync.py
+ language: python
+ files: >
+ (?x)
+ ^airflow-core/src/airflow/plugins_manager\.py$|
+ ^airflow-core/src/airflow/ui/src/utils/pluginAppliesTo\.ts$
+ pass_filenames: false
+ require_serial: true
- id: check-ci-workflows-in-sync
name: Check ci-arm.yml and ci-amd.yml stay in sync
entry: ./scripts/ci/prek/check_ci_workflows_in_sync.py
diff --git a/airflow-core/docs/administration-and-deployment/plugins.rst
b/airflow-core/docs/administration-and-deployment/plugins.rst
index 57a0722c78e..d5861d0bb17 100644
--- a/airflow-core/docs/administration-and-deployment/plugins.rst
+++ b/airflow-core/docs/administration-and-deployment/plugins.rst
@@ -411,10 +411,11 @@ relevant instead of appearing on every Dag:
"dag.dag_id": ["train_pipeline"],
}
-Keys are **dotted field paths** into the records the page has, not a fixed set
of criteria, so
-anything the REST API returns for an entity is addressable — ``state``,
``operator``, ``pool``,
-``queue``, ``try_number``, and so on. Values are matched for equality, and are
compared as
-strings, so numeric and boolean fields work without quoting rules of their own
+Keys are **dotted field paths** into the records the page has, so any field
the REST API
+returns for an entity is addressable. Use the names the API returns, which are
not always the
+Python attribute names: a task instance's run is ``dag_run_id``, not
``run_id``, and computed
+fields such as a Dag's ``is_backfillable`` work like any other. Values are
matched for equality
+and compared as strings, so numbers and booleans need no quoting rules of
their own
(``"try_number": ["2"]``, ``"is_paused": ["false"]``).
An **unqualified path is rooted at the entity the destination is about** — on
``dag_run``,
@@ -423,14 +424,11 @@ name a related record as its first segment: ``dag``,
``dag_run``, ``task`` or
``task_instance``. Traversing a list fans out across it, so ``dag.tags.name``
collects every
tag name and matches if any of them is listed.
-Paths combine like Kubernetes label selectors — **OR within a path, AND across
paths**. A Dag
-matching any listed tag satisfies ``dag.tags.name``, and a view configured
with both
-``dag.tags.name`` and ``state`` requires both to match.
-
-Crucially, the AND applies **only across paths the current page can
evaluate**. A
-``task_instance.*`` path cannot be judged on a Dag-level page, so it is
skipped there rather
-than failing the match. This lets one ``applies_to`` block be shared by a
plugin's Dag- and
-task-level destinations. Which records each destination resolves:
+Paths combine like Kubernetes label selectors — **OR within a path, AND across
paths** — but
+the AND applies only across paths the page can evaluate. A ``task_instance.*``
path cannot be
+judged on a Dag-level page, so it is skipped there rather than failing the
match, which is what
+lets one block be shared across a plugin's destinations. Which records each
destination
+resolves:
.. list-table::
:header-rows: 1
@@ -457,19 +455,29 @@ task-level destinations. Which records each destination
resolves:
If none of the configured paths can be evaluated on a given page, the view is
shown. On task
group pages the task-level records are absent, since a group is not a task.
-A path is also skipped when the record exists but has no such field. That
means a **bad path
-widens the scope rather than narrowing it**, so paths are checked against the
API response
-models when plugins load and a path naming no field is logged with a suggested
correction.
-The check stops at a field whose contents the models do not describe — the
dict behind
-``class_ref``, or a Dag Run's ``conf`` — so a path below one of those is still
accepted and
-still widens silently. If a view appears in more places than you expect, run
-``airflow plugins list --verbose`` first, then check the path against the REST
API response for
-that entity.
+A path is also skipped when the record exists but has no such field, which on
its own would mean
+a **typo widens the scope rather than narrowing it**. So paths are checked
against the API
+response models when plugins load, and one that names a field *no* record has
is treated as a
+misconfiguration: the view is **not loaded at all**, and the reason is logged
as an error with a
+suggested correction.
+
+.. code-block:: text
+
+ Plugin 'acme' has an external view 'Incidents' with an 'applies_to' path
that matches no
+ field: 'dag.tags.nme' names no field 'nme' on DagTagResponse (did you mean
'name'?). It
+ could never scope the way it asks to, so the view will not be loaded.
-Segments are the field names **the REST API returns**, which are not always
the Python
-attribute names: a task instance's run is ``dag_run_id``, not ``run_id``, and
computed fields
-such as a Dag's ``is_backfillable`` are addressable like any other. If a field
appears in the
-API response for an entity, it can be named here.
+Withholding it is deliberate: the author asked to narrow by something that
cannot exist, so the
+view can never appear for the reason they intended, and a view that disappears
gets noticed
+while one on every page looks deliberate.
+
+A path naming a field some *other* record has is a different matter — that is
the shared-block
+pattern, so it is reported as skipped here and the view loads. Neither check
can see past a
+field the models do not describe — the dict behind ``class_ref``, or a Dag
Run's ``conf`` — so a
+typo below one of those still widens silently.
+
+If a view appears in more places than you expect, run ``airflow plugins
--verbose``, then check
+the path against the REST API response for that entity.
An empty list is different from a missing field: a Dag with no tags has
definitively answered
``dag.tags.name``, so the view is not shown. A path that stops short of a leaf
is likewise a
@@ -487,14 +495,30 @@ unqualified path targets an operator on either page**:
"applies_to": {"operator_name": ["KubernetesPodOperator"]}
-``operator_name`` is the display name shown in the UI (an operator's
-``custom_operator_name``). For a plain operator it is the class name, but the
two differ for
-decorator-based tasks: a ``@task.bash`` task has the display name
``@task.bash`` and the
-private class name ``_BashDecoratedOperator``, so ``operator_name`` is the one
to match on.
+``operator_name`` is the display name the UI shows (an operator's
``custom_operator_name``).
+It equals the class name for a plain operator, but not for a decorated one: a
``@task.bash``
+task is named ``@task.bash`` and classed ``_BashDecoratedOperator``.
+
+It is also the only operator field both records carry. The *class* name is
spelled differently
+on each — a task instance has ``operator``, a task has
``class_ref.class_name`` — so targeting
+it takes one view per destination, each scoped on its own record:
-If you specifically need the operator *class* name, the two records spell it
differently — a
-task instance has ``operator``, while a task carries it through
``class_ref.class_name``.
-Leave both paths **unqualified** so that each page reads its own record:
+.. code-block:: python
+
+ external_views = [
+ {
+ # ... name, href, and a url_route of its own
+ "destination": "task_instance",
+ "applies_to": {"operator": ["KubernetesPodOperator"]},
+ },
+ {
+ # ...
+ "destination": "task",
+ "applies_to": {"class_ref.class_name": ["KubernetesPodOperator"]},
+ },
+ ]
+
+One block naming both also works, since whichever field this page's record
lacks is skipped:
.. code-block:: python
@@ -505,17 +529,14 @@ Leave both paths **unqualified** so that each page reads
its own record:
"class_ref.class_name": ["KubernetesPodOperator"],
}
-.. warning::
- Do not write these as ``task_instance.operator`` and
``task.class_ref.class_name``. A task
- instance page resolves **both** records, so both paths would be evaluated
and AND-ed — and
- the task record always carries the Dag's *current* definition. An instance
that ran under
- ``KubernetesPodOperator`` would stop matching the moment the task was
changed to a
- different operator, hiding the view on historical runs.
+Qualifying them — ``task_instance.operator`` and ``task.class_ref.class_name``
— works too, but
+says the same thing twice: a task instance page resolves both records, so both
are evaluated and
+AND-ed. The task record is read at the version that instance ran, so the two
agree; naming one
+of them is enough.
-This is the general rule where records overlap: **prefer an unqualified
path**, which always
-reads the entity the page is about. Qualify a path only when you genuinely
mean the related
-record — ``dag.tags.name`` from a task instance, say — and not as a way of
naming the same
-concept twice.
+The general rule where records overlap: **prefer an unqualified path**, which
always reads the
+entity the page is about, and qualify one only when you genuinely mean a
different record —
+``dag.tags.name`` from a task instance, say.
A malformed ``applies_to`` — one that is not a dictionary, has a non-string or
empty path, or
gives a path something other than a list of strings — is reported as a warning
when plugins
@@ -530,7 +551,7 @@ server log, run:
.. code-block:: bash
- airflow plugins list --verbose
+ airflow plugins --verbose
Each invocation is a fresh process, so every warning for every plugin is
reported. The
``--verbose`` flag is required: without it the command suppresses log output.
diff --git a/airflow-core/newsfragments/74278.feature.rst
b/airflow-core/newsfragments/74278.feature.rst
index 69f483b4d0f..b6fba929773 100644
--- a/airflow-core/newsfragments/74278.feature.rst
+++ b/airflow-core/newsfragments/74278.feature.rst
@@ -1 +1 @@
-Plugin external views and React apps can be scoped with an optional
``applies_to`` block: a map of dotted field path to the values that path may
take, such as ``{"state": ["failed"], "dag.tags.name": ["ml"]}``. Any field the
REST API returns for the entity is addressable, so a tab or panel appears only
where it is relevant instead of on every Dag. An unqualified path is rooted at
the entity the ``destination`` is about; a path may instead name a related
record (``dag``, ``dag_run``, ``ta [...]
+Plugin external views and React apps can be scoped with an optional
``applies_to`` block, a map of dotted field path to the values that path may
take, such as ``{"state": ["failed"], "dag.tags.name": ["ml"]}``. This replaces
the five fixed criteria added earlier in this development cycle and never
released (``dag_tags``, ``dag_ids``, ``task_ids``, ``operators``,
``operator_names``). A path naming a field no record has is treated as a
misconfiguration rather than a filter that never match [...]
diff --git a/airflow-core/src/airflow/plugins_manager.py
b/airflow-core/src/airflow/plugins_manager.py
index 599b22fc201..67021ef58ae 100644
--- a/airflow-core/src/airflow/plugins_manager.py
+++ b/airflow-core/src/airflow/plugins_manager.py
@@ -334,21 +334,46 @@ def _describe_applies_to_error(applies_to: Any) -> str |
None:
return None
-def _validate_applies_to(plugin_name: str | None, view: ExternalViewDict |
ReactAppDict, kind: str) -> None:
+def _is_applies_to_path_known_elsewhere(path: str) -> bool:
"""
- Warn about scoping a UI plugin cannot honour, and strip it if it is
malformed.
+ Whether an unqualified ``path`` names a real field on some record other
than this one.
- A malformed block is removed so the view still loads unscoped, matching
the default for
- a view that omits ``applies_to`` entirely. Criteria the destination cannot
evaluate are
- only warned about — they are skipped at match time by design, so that one
block can be
- shared across a plugin's Dag- and task-level destinations.
+ ``class_ref.class_name`` means nothing on a task instance but everything
on a task, and an
+ author targeting both pages from one block writes exactly that. Such a
path is skipped at
+ match time by design, so it must not be reported as a misspelling.
+
+ Only asked about unqualified paths: one that names its record explicitly
has no other
+ reading, so a bad field there is simply wrong.
+ """
+ return any(_describe_applies_to_path_error(path, root) is None for root in
_applies_to_root_models())
+
+
+def _validate_applies_to(plugin_name: str | None, view: ExternalViewDict |
ReactAppDict, kind: str) -> bool:
+ """
+ Check a UI plugin's scoping, returning whether the view should be loaded
at all.
+
+ Three failures, wanting three different things:
+
+ * **Malformed** block -- not a dictionary, a non-string path, a value that
is not a list of
+ strings. Dropped, and the view loads unscoped, as it would without
``applies_to`` at all.
+ Nothing of the author's intent survives to act on.
+ * **Unevaluable** path -- its root record is one this destination does not
have, or it names
+ a field only a sibling record has. Warned about and kept: both are
skipped at match time
+ by design, which is what lets one block be shared across a plugin's
destinations.
+ * **A path naming a field no record has.** The author asked to narrow by
something that
+ cannot exist, so the view can never appear for the reason they intended.
Returning
+ ``False`` withholds it rather than letting the browser skip the path and
show it
+ everywhere. A view that disappears gets noticed; one on every page looks
deliberate.
+
+ The view gets a new dict rather than an edited one, because a plugin may
pass the same
+ ``applies_to`` object to several views and it is not this function's to
rewrite.
"""
if "applies_to" not in view:
- return
+ return True
applies_to = view["applies_to"]
if applies_to is None:
- return
+ return True
if error := _describe_applies_to_error(applies_to):
log.warning(
@@ -359,20 +384,23 @@ def _validate_applies_to(plugin_name: str | None, view:
ExternalViewDict | React
error,
)
del view["applies_to"]
- return
+ return True
# A null value means "no values configured", which the matcher already
ignores the same way
- # it ignores an empty list. Leaving it in place would fail
`PluginAppliesToResponse`
+ # it ignores an empty list. Carrying it through would fail
`PluginAppliesToResponse`
# serialization and drop the whole plugin -- including its other, valid
views -- from the
- # plugins API. Dropping the path keeps the rest of the block working.
- for path in [path for path, values in applies_to.items() if values is
None]:
- del applies_to[path]
+ # plugins API.
+ #
+ # The view gets a new dict rather than an edited one, because a plugin may
pass the same
+ # `applies_to` object to several views and it is not this function's to
rewrite.
+ applies_to = {path: list(values) for path, values in applies_to.items() if
values is not None}
+ view["applies_to"] = applies_to
destination = view.get("destination", "nav")
if destination not in _APPLIES_TO_ROOTS:
# An unrecognised destination already fails serialization; warning
here too would
# only add noise pointing at the wrong problem.
- return
+ return True
available = _APPLIES_TO_ROOTS[destination]
unevaluable = sorted(
@@ -398,16 +426,35 @@ def _validate_applies_to(plugin_name: str | None, view:
ExternalViewDict | React
root = _applies_to_path_root(path, destination)
if not values or root is None or root not in available:
continue
- if error := _describe_applies_to_path_error(path, root):
+ error = _describe_applies_to_path_error(path, root)
+ if error is None:
+ continue
+ head, _, rest = path.partition(".")
+ if not (head in _APPLIES_TO_ROOT_NAMES and rest) and
_is_applies_to_path_known_elsewhere(path):
+ # A real field, just not on this destination's record. Saying the
scope widened
+ # would be untrue: the block's other paths are what scope this
destination.
log.warning(
- "Plugin '%s' has %s '%s' with an 'applies_to' path that
matches no field: %s. "
- "That path will be ignored, so the %s will appear in more
places than intended.",
+ "Plugin '%s' has %s '%s' with an 'applies_to' path '%s' that
no %s has. It is "
+ "skipped here; it only scopes the destinations whose record
does have it.",
plugin_name,
kind,
view.get("name"),
- error,
- kind.split()[-1],
+ path,
+ root,
)
+ continue
+ log.error(
+ "Plugin '%s' has %s '%s' with an 'applies_to' path that matches no
field: %s. "
+ "It could never scope the way it asks to, so the %s will not be
loaded.",
+ plugin_name,
+ kind,
+ view.get("name"),
+ error,
+ kind.split()[-1],
+ )
+ return False
+
+ return True
@cache
@@ -430,7 +477,9 @@ def _get_ui_plugins() -> tuple[list[ExternalViewDict],
list[ReactAppDict]]:
)
external_views_to_remove.append(external_view)
continue
- _validate_applies_to(plugin.name, external_view, "an external
view")
+ if not _validate_applies_to(plugin.name, external_view, "an
external view"):
+ external_views_to_remove.append(external_view)
+ continue
url_route = external_view.get("url_route")
if url_route is None:
continue
@@ -455,7 +504,9 @@ def _get_ui_plugins() -> tuple[list[ExternalViewDict],
list[ReactAppDict]]:
)
react_apps_to_remove.append(react_app)
continue
- _validate_applies_to(plugin.name, react_app, "a React App")
+ if not _validate_applies_to(plugin.name, react_app, "a React App"):
+ react_apps_to_remove.append(react_app)
+ continue
url_route = react_app.get("url_route")
if url_route is None:
continue
diff --git
a/airflow-core/src/airflow/ui/src/hooks/usePluginAppliesToContext.test.ts
b/airflow-core/src/airflow/ui/src/hooks/usePluginAppliesToContext.test.ts
index aaf515ea077..8fb52712973 100644
--- a/airflow-core/src/airflow/ui/src/hooks/usePluginAppliesToContext.test.ts
+++ b/airflow-core/src/airflow/ui/src/hooks/usePluginAppliesToContext.test.ts
@@ -41,15 +41,17 @@ vi.mock("react-router-dom", async (importOriginal) => ({
useParams: () => mockParams,
}));
-const { calls, record } = vi.hoisted(() => {
+const { calls, data, record } = vi.hoisted(() => {
const captured: Record<string, { key: unknown; options?: { enabled?: boolean
}; params: unknown }> = {};
+ const resolved: Record<string, unknown> = { taskInstance: undefined };
return {
calls: captured,
+ data: resolved,
record: (name: string) => (params: unknown, key: unknown, options?: {
enabled?: boolean }) => {
captured[name] = { key, options, params };
- return { data: undefined, isLoading: false };
+ return { data: resolved[name], isLoading: false };
},
};
});
@@ -65,6 +67,7 @@ vi.mock("openapi/queries", async (importOriginal) => ({
describe("usePluginAppliesToContext", () => {
beforeEach(() => {
mockParams = { dagId, mapIndex: "-1", runId, taskId };
+ data.taskInstance = undefined;
});
it("issues no query when no view needs scoping", () => {
@@ -76,7 +79,32 @@ describe("usePluginAppliesToContext", () => {
expect(calls.taskInstance?.options?.enabled).toBe(false);
});
+ it("holds the task query until the instance names the version it ran", () =>
{
+ renderHook(() => usePluginAppliesToContext(true));
+
+ // Asking now would fetch the Dag's current task and have to discard it.
+ expect(calls.taskInstance?.options?.enabled).toBe(true);
+ expect(calls.task?.options?.enabled).toBe(false);
+
+ data.taskInstance = { dag_version: { version_number: 3 } };
+ renderHook(() => usePluginAppliesToContext(true));
+
+ expect(calls.task?.options?.enabled).toBe(true);
+ expect(calls.task?.params).toStrictEqual({ dagId, taskId, versionNumber: 3
});
+ });
+
+ it("asks for the latest task where there is no instance to pin it to", () =>
{
+ mockParams = { dagId, taskId };
+
+ renderHook(() => usePluginAppliesToContext(true));
+
+ expect(calls.task?.options?.enabled).toBe(true);
+ expect(calls.task?.params).toStrictEqual({ dagId, taskId, versionNumber:
undefined });
+ });
+
it("enables every query a full task instance route can resolve", () => {
+ data.taskInstance = { dag_version: { version_number: 3 } };
+
renderHook(() => usePluginAppliesToContext(true));
expect(calls.dag?.options?.enabled).toBe(true);
diff --git a/airflow-core/src/airflow/ui/src/hooks/usePluginAppliesToContext.ts
b/airflow-core/src/airflow/ui/src/hooks/usePluginAppliesToContext.ts
index 829247235f6..a67aa187be3 100644
--- a/airflow-core/src/airflow/ui/src/hooks/usePluginAppliesToContext.ts
+++ b/airflow-core/src/airflow/ui/src/hooks/usePluginAppliesToContext.ts
@@ -32,11 +32,14 @@ import type { AppliesToContext } from
"src/utils/pluginAppliesTo";
*
* Pass `enabled: false` where no view configures scoping, so none of these
run at all.
* Otherwise each query is gated on the route params it needs, so it stays
disabled where
- * those params are absent. Passing no explicit `queryKey` is what keeps these
on the same
- * cache entry the surrounding page already populated — `DetailsLayout` for
the Dag, the Task
- * and TaskInstance pages for their own records — since the generated hook
builds the key from
- * the params via the same `Use*KeyFn` the pages go through.
`usePluginAppliesToContext.test.ts`
- * asserts that equality so a param change on either side cannot silently
split the cache.
+ * those params are absent. Passing no explicit `queryKey` builds the key from
the params via
+ * the same `Use*KeyFn` the pages go through, so these share the surrounding
page's cache entry
+ * wherever that page passes the same params — `DetailsLayout` for the Dag,
the Task and
+ * TaskInstance pages for their own records. The task query adds the
instance's version number,
+ * so it shares with a page that asks for the same version and pays for its
own fetch otherwise,
+ * rather than going wrong.
+ * `usePluginAppliesToContext.test.ts` asserts the equality that holds today,
so a param change
+ * on either side surfaces here rather than silently splitting the cache.
* Task groups are skipped for the task query, since `groupId` is not a
task_id and would 404.
*/
export const usePluginAppliesToContext = (enabled: boolean): AppliesToContext
=> {
@@ -53,25 +56,37 @@ export const usePluginAppliesToContext = (enabled:
boolean): AppliesToContext =>
{ enabled: enabled && Boolean(dagId) && Boolean(runId) },
);
- const { data: task, isLoading: isTaskLoading } = useTaskServiceGetTask({
dagId, taskId }, undefined, {
- enabled: enabled && Boolean(dagId) && Boolean(taskId) && groupId ===
undefined,
- });
+ const hasTaskInstance =
+ enabled &&
+ Boolean(dagId) &&
+ Boolean(runId) &&
+ Boolean(taskId) &&
+ groupId === undefined &&
+ !isNaN(parsedMapIndex);
const { data: taskInstance, isLoading: isTaskInstanceLoading } =
useTaskInstanceServiceGetMappedTaskInstance(
{ dagId, dagRunId: runId, mapIndex: parsedMapIndex, taskId },
undefined,
- {
- enabled:
- enabled &&
- Boolean(dagId) &&
- Boolean(runId) &&
- Boolean(taskId) &&
- groupId === undefined &&
- !isNaN(parsedMapIndex),
- },
+ { enabled: hasTaskInstance },
);
+ // Pinned to the version this instance ran, so `task.*` paths judge it by
the definition it
+ // used rather than the Dag's current one. Waiting for the instance costs a
round trip on task
+ // instance routes, but asking before then would fetch the latest task and
have to discard it.
+ const { data: task, isLoading: isTaskLoading } = useTaskServiceGetTask(
+ { dagId, taskId, versionNumber: taskInstance?.dag_version?.version_number
},
+ undefined,
+ {
+ enabled:
+ enabled &&
+ Boolean(dagId) &&
+ Boolean(taskId) &&
+ groupId === undefined &&
+ (!hasTaskInstance || taskInstance !== undefined),
+ },
+ );
+
return {
dag,
dagRun,
diff --git a/airflow-core/src/airflow/ui/src/utils/pluginAppliesTo.test.ts
b/airflow-core/src/airflow/ui/src/utils/pluginAppliesTo.test.ts
index 3db8e67ef77..136c84d1166 100644
--- a/airflow-core/src/airflow/ui/src/utils/pluginAppliesTo.test.ts
+++ b/airflow-core/src/airflow/ui/src/utils/pluginAppliesTo.test.ts
@@ -242,9 +242,10 @@ describe("matchesAppliesTo — combining paths", () => {
expect(matchesAppliesTo({ ...view, destination: "task_instance" },
taskInstanceContext)).toBe(true);
});
- // A task instance page resolves the task record too, and that record always
carries the Dag's
- // current definition -- which may differ from what this instance actually
ran.
- it("reads the page's own operator when the task has since been changed", ()
=> {
+ // A task instance page resolves the task record too.
`usePluginAppliesToContext` fetches it at
+ // the version the instance ran, so the two agree; this pins the matcher's
half of that, that
+ // each unqualified path reads its own record.
+ it("reads the page's own operator when the records disagree", () => {
const staleContext: AppliesToContext = {
dag,
dagRun: makeDagRun("failed"),
diff --git a/airflow-core/tests/unit/plugins/test_plugins_manager.py
b/airflow-core/tests/unit/plugins/test_plugins_manager.py
index 56dc6095e97..686caacd55d 100644
--- a/airflow-core/tests/unit/plugins/test_plugins_manager.py
+++ b/airflow-core/tests/unit/plugins/test_plugins_manager.py
@@ -446,26 +446,37 @@ class TestPluginsManager:
]
@pytest.mark.parametrize(
- ("path", "error"),
+ ("destination", "path", "error"),
[
pytest.param(
+ "dag_run",
"dag.tags.nme",
"'dag.tags.nme' names no field 'nme' on DagTagResponse (did
you mean 'name'?)",
id="misspelled-nested-leaf",
),
pytest.param(
+ "dag_run",
"stat",
"'stat' names no field 'stat' on DAGRunResponse (did you mean
'state'?)",
id="misspelled-own-field",
),
pytest.param(
+ "dag_run",
"state.length",
"'state.length' reads 'length' from DagRunState, which has no
fields",
id="path-past-a-scalar",
),
+ # Naming the record explicitly leaves no second reading, so a bad
field there is
+ # simply wrong -- even though `class_ref` is real on the record it
names.
+ pytest.param(
+ "task_instance",
+ "task.class_rf",
+ "'task.class_rf' names no field 'class_rf' on TaskResponse
(did you mean 'class_ref'?)",
+ id="misspelled-behind-an-explicit-record",
+ ),
],
)
- def test_warns_about_a_path_matching_no_field(self, path, error, caplog):
+ def test_withholds_a_view_whose_path_matches_no_field(self, destination,
path, error, caplog):
class TestPlugin(AirflowPlugin):
name = "test_plugin"
@@ -474,9 +485,16 @@ class TestPluginsManager:
"name": "Scoped",
"href": "/scoped",
"url_route": "/scoped",
- "destination": "dag_run",
+ "destination": destination,
"applies_to": {path: ["x"]},
- }
+ },
+ {
+ "name": "Fine",
+ "href": "/fine",
+ "url_route": "/fine",
+ "destination": destination,
+ "applies_to": {"state": ["failed"]},
+ },
]
with (
@@ -487,17 +505,16 @@ class TestPluginsManager:
external_views, _ = plugins_manager._get_ui_plugins()
- # Warned about, not stripped: the path is inert either way, and
dropping it would
- # change the block the UI receives.
- assert external_views[0]["applies_to"] == {path: ["x"]}
+ # Withheld rather than shown everywhere; its sibling is untouched.
+ assert [view["name"] for view in external_views] == ["Fine"]
assert caplog.record_tuples == [
(
"airflow.plugins_manager",
- logging.WARNING,
+ logging.ERROR,
f"Plugin 'test_plugin' has an external view 'Scoped' with an
'applies_to' path that "
- f"matches no field: {error}. That path will be ignored, so the
view will appear in "
- f"more places than intended.",
+ f"matches no field: {error}. It could never scope the way it
asks to, so the view "
+ f"will not be loaded.",
),
]
@@ -621,6 +638,127 @@ class TestPluginsManager:
assert caplog.record_tuples == []
+ def test_documented_operator_recipes_load_without_warnings(self, caplog):
+ """The per-destination recipe in plugins.rst must not trip the path
validator."""
+
+ class TestPlugin(AirflowPlugin):
+ name = "test_plugin"
+
+ external_views = [
+ {
+ "name": "Pod logs",
+ "href": "https://example.com/pods";,
+ "url_route": "pod-logs",
+ "destination": "task_instance",
+ "applies_to": {"operator": ["KubernetesPodOperator"]},
+ },
+ {
+ "name": "Pod logs",
+ "href": "https://example.com/pods";,
+ "url_route": "pod-logs-task",
+ "destination": "task",
+ "applies_to": {"class_ref.class_name":
["KubernetesPodOperator"]},
+ },
+ # The portable single-path form the docs lead with, on both
destinations.
+ {
+ "name": "By display name",
+ "href": "https://example.com/op";,
+ "url_route": "op",
+ "destination": "task_instance",
+ "applies_to": {"operator_name": ["KubernetesPodOperator"]},
+ },
+ {
+ "name": "By display name",
+ "href": "https://example.com/op";,
+ "url_route": "op-task",
+ "destination": "task",
+ "applies_to": {"operator_name": ["KubernetesPodOperator"]},
+ },
+ ]
+
+ with (
+ mock_plugin_manager(plugins=[TestPlugin()]),
+ caplog.at_level(logging.WARNING, logger="airflow.plugins_manager"),
+ ):
+ from airflow import plugins_manager
+
+ plugins_manager._get_ui_plugins()
+
+ assert caplog.record_tuples == []
+
+ def test_reports_a_shared_block_path_as_skipped_not_as_widening(self,
caplog):
+ """`class_ref.class_name` is meaningless on a task instance but real
on a task.
+
+ An author targeting both pages from one block writes exactly that, so
saying the view
+ will appear in more places than intended would be untrue -- `operator`
is scoping it.
+ """
+
+ class TestPlugin(AirflowPlugin):
+ name = "test_plugin"
+
+ external_views = [
+ {
+ "name": "Pod logs",
+ "href": "/pods",
+ "url_route": "/pods",
+ "destination": "task_instance",
+ "applies_to": {
+ "operator": ["KubernetesPodOperator"],
+ "class_ref.class_name": ["KubernetesPodOperator"],
+ },
+ }
+ ]
+
+ with (
+ mock_plugin_manager(plugins=[TestPlugin()]),
+ caplog.at_level(logging.WARNING, logger="airflow.plugins_manager"),
+ ):
+ from airflow import plugins_manager
+
+ external_views, _ = plugins_manager._get_ui_plugins()
+
+ assert [view["name"] for view in external_views] == ["Pod logs"]
+
+ assert len(caplog.record_tuples) == 1
+ assert "'class_ref.class_name' that no task_instance has" in
caplog.record_tuples[0][2]
+ assert "more places than intended" not in caplog.record_tuples[0][2]
+
+ def test_does_not_mutate_the_plugin_author_s_applies_to(self):
+ """A plugin may hand the same dict to several views; validation must
not rewrite it."""
+ shared = {"operator_name": ["@task.bash"], "task_id": None}
+
+ class TestPlugin(AirflowPlugin):
+ name = "test_plugin"
+
+ external_views = [
+ {
+ "name": "On task",
+ "href": "/a",
+ "url_route": "/a",
+ "destination": "task",
+ "applies_to": shared,
+ },
+ {
+ "name": "On task instance",
+ "href": "/b",
+ "url_route": "/b",
+ "destination": "task_instance",
+ "applies_to": shared,
+ },
+ ]
+
+ with mock_plugin_manager(plugins=[TestPlugin()]):
+ from airflow import plugins_manager
+
+ external_views, _ = plugins_manager._get_ui_plugins()
+
+ # The null path is dropped from what each view serializes...
+ for view in external_views:
+ assert view["applies_to"] == {"operator_name": ["@task.bash"]}
+
+ # ...but the author's own object still has it.
+ assert shared == {"operator_name": ["@task.bash"], "task_id": None}
+
def test_does_not_warn_about_valid_applies_to(self, caplog):
class TestPlugin(AirflowPlugin):
name = "test_plugin"
diff --git a/scripts/ci/prek/check_applies_to_roots_in_sync.py
b/scripts/ci/prek/check_applies_to_roots_in_sync.py
new file mode 100755
index 00000000000..45fbb085258
--- /dev/null
+++ b/scripts/ci/prek/check_applies_to_roots_in_sync.py
@@ -0,0 +1,117 @@
+#!/usr/bin/env python3
+# 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.
+"""
+Verify the plugin ``applies_to`` destination tables stay in sync.
+
+Which record an unqualified ``applies_to`` path is rooted at is decided twice:
by
+``_APPLIES_TO_ENTITY_ROOT`` when plugins load, to decide what to validate and
warn about, and
+again by ``ENTITY_ROOT_BY_DESTINATION`` in the browser, to decide what to
match. The two have to
+agree, and nothing at runtime notices when they do not -- a destination added
to one side only
+makes every path on it unevaluable, which shows the view everywhere instead of
narrowing it.
+
+The tables are not textually identical: Python keys records by their wire name
(``dag_run``)
+while the UI keys them by context field (``dagRun``), so this compares them
through
+``RECORD_FIELD_BY_WIRE_NAME`` below.
+"""
+
+from __future__ import annotations
+
+import ast
+import re
+import sys
+from pathlib import Path
+
+sys.path.insert(0, str(Path(__file__).parent.resolve()))
+
+from common_prek_utils import AIRFLOW_ROOT_PATH
+
+PYTHON_SOURCE = Path("airflow-core/src/airflow/plugins_manager.py")
+PYTHON_TABLE = "_APPLIES_TO_ENTITY_ROOT"
+
+UI_SOURCE = Path("airflow-core/src/airflow/ui/src/utils/pluginAppliesTo.ts")
+UI_TABLE = "ENTITY_ROOT_BY_DESTINATION"
+
+# The UI holds each record on a camelCase field of `AppliesToContext`; the
wire names the Python
+# side uses are the `applies_to` path prefixes a plugin author writes.
+RECORD_FIELD_BY_WIRE_NAME = {
+ "dag": "dag",
+ "dag_run": "dagRun",
+ "task": "task",
+ "task_instance": "taskInstance",
+}
+
+
+def extract_python_table(source: str) -> dict[str, str]:
+ """Read the destination -> record mapping out of the module without
importing airflow."""
+ for node in ast.parse(source).body:
+ if isinstance(node, ast.AnnAssign):
+ targets: list[ast.expr] = [node.target]
+ elif isinstance(node, ast.Assign):
+ targets = list(node.targets)
+ else:
+ continue
+ if node.value is None:
+ continue
+ if any(isinstance(target, ast.Name) and target.id == PYTHON_TABLE for
target in targets):
+ return ast.literal_eval(node.value)
+ raise SystemExit(f"Could not find `{PYTHON_TABLE}` in {PYTHON_SOURCE}")
+
+
+def extract_ui_table(source: str) -> dict[str, str]:
+ """Read the destination -> record mapping out of the TypeScript object
literal."""
+ match = re.search(
+ rf"const {UI_TABLE}: Record<string, RootName> = \{{(.*?)\n\}};",
+ source,
+ re.DOTALL,
+ )
+ if match is None:
+ raise SystemExit(f"Could not find `{UI_TABLE}` in {UI_SOURCE}")
+ return dict(re.findall(r'^\s*([A-Za-z_]+):\s*"([A-Za-z]+)",',
match.group(1), re.MULTILINE))
+
+
+def main() -> int:
+ python_table = extract_python_table((AIRFLOW_ROOT_PATH /
PYTHON_SOURCE).read_text())
+ ui_table = extract_ui_table((AIRFLOW_ROOT_PATH / UI_SOURCE).read_text())
+
+ expected = {
+ destination: RECORD_FIELD_BY_WIRE_NAME.get(record, record)
+ for destination, record in python_table.items()
+ }
+ if expected == ui_table:
+ return 0
+
+ print(f"`{PYTHON_TABLE}` in {PYTHON_SOURCE} and `{UI_TABLE}` in
{UI_SOURCE} disagree.\n")
+ for destination in sorted(set(expected) | set(ui_table)):
+ in_python, in_ui = expected.get(destination), ui_table.get(destination)
+ if in_python == in_ui:
+ continue
+ print(
+ f" {destination}: {PYTHON_TABLE} says {in_python or '(missing)'},
"
+ f"{UI_TABLE} says {in_ui or '(missing)'}"
+ )
+ print(
+ "\nA destination only one side knows about makes every unqualified
path on it "
+ "unevaluable, which shows the view everywhere rather than scoping it.
Update both "
+ "tables, and the destination table in "
+ "docs/administration-and-deployment/plugins.rst."
+ )
+ return 1
+
+
+if __name__ == "__main__":
+ sys.exit(main())
diff --git a/scripts/tests/ci/prek/test_check_applies_to_roots_in_sync.py
b/scripts/tests/ci/prek/test_check_applies_to_roots_in_sync.py
new file mode 100644
index 00000000000..a630b4de39c
--- /dev/null
+++ b/scripts/tests/ci/prek/test_check_applies_to_roots_in_sync.py
@@ -0,0 +1,84 @@
+# 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.
+from __future__ import annotations
+
+import textwrap
+
+import pytest
+from check_applies_to_roots_in_sync import extract_python_table,
extract_ui_table
+
+
+class TestExtractPythonTable:
+ def test_reads_the_mapping_without_importing_airflow(self):
+ source = textwrap.dedent(
+ '''
+ """Module docstring."""
+ _APPLIES_TO_ROOTS: dict[str, frozenset[str]] = {"dag":
frozenset({"dag"})}
+
+ _APPLIES_TO_ENTITY_ROOT: dict[str, str] = {
+ "dag": "dag",
+ "dag_run": "dag_run",
+ "task_instance": "task_instance",
+ }
+ '''
+ )
+
+ assert extract_python_table(source) == {
+ "dag": "dag",
+ "dag_run": "dag_run",
+ "task_instance": "task_instance",
+ }
+
+ def test_fails_loudly_when_the_table_is_renamed(self):
+ with pytest.raises(SystemExit, match="_APPLIES_TO_ENTITY_ROOT"):
+ extract_python_table("SOMETHING_ELSE = {}\n")
+
+
+class TestExtractUiTable:
+ def test_reads_the_object_literal(self):
+ source = textwrap.dedent(
+ """
+ type RootName = "dag" | "dagRun";
+
+ const ROOT_BY_PREFIX: Record<string, RootName> = {
+ dag: "dag",
+ };
+
+ const ENTITY_ROOT_BY_DESTINATION: Record<string, RootName> = {
+ dag: "dag",
+ dag_run: "dagRun",
+ task_instance: "taskInstance",
+ };
+ """
+ )
+
+ assert extract_ui_table(source) == {
+ "dag": "dag",
+ "dag_run": "dagRun",
+ "task_instance": "taskInstance",
+ }
+
+ def test_fails_loudly_when_the_table_is_renamed(self):
+ with pytest.raises(SystemExit, match="ENTITY_ROOT_BY_DESTINATION"):
+ extract_ui_table("const SOMETHING_ELSE: Record<string, RootName> =
{\n};\n")
+
+
+def test_the_checked_in_tables_agree():
+ """The check this hook exists for, run against the real files."""
+ from check_applies_to_roots_in_sync import main
+
+ assert main() == 0
