pierrejeambrun commented on code in PR #73322:
URL: https://github.com/apache/airflow/pull/73322#discussion_r4104927210
##########
airflow-core/src/airflow/ui/src/components/VersionCompareSelect.tsx:
##########
@@ -49,9 +51,13 @@ export const VersionCompareSelect = ({
const selectedVersion = data?.dag_versions.find((dv) => dv.version_number
=== selectedVersionNumber);
- const versionOptions = createListCollection({
- items: (data?.dag_versions ?? []).map((dv) => ({ value: dv.version_number,
version: dv })),
- });
+ const versionOptions = useMemo(
Review Comment:
I don't think we need that useMemo. React compiler will do that for us.
`DagVersionSelect` call it directly too.
##########
airflow-ctl/src/airflowctl/api/datamodels/generated.py:
##########
@@ -2155,6 +2222,93 @@ class DagStatsResponse(BaseModel):
stats: Annotated[list[DagStatsStateResponse], Field(title="Stats")]
+class DagVersionDiffChangeResponse(BaseModel):
+ """
+ One structural difference between two stored Dag versions.
+ """
+
+ path: Annotated[str, Field(title="Path")]
+ operation: DagVersionDiffOperation
+ category: DagVersionDiffCategory
+ impact: DagVersionDiffImpact
+ occurrence_count: Annotated[
+ int,
+ Field(
+ description="How many underlying changes this record stands for.
Always 1 when values are disclosed, since each change is then its own record; a
redacted record merges every change sharing its path and operation.",
+ title="Occurrence Count",
+ ),
+ ]
+ before_digest: Annotated[
+ str | None,
+ Field(
+ description="SHA-256 over the canonical JSON of `before_value`.
Present only when values are disclosed, and null when the change has no before
side.",
+ title="Before Digest",
+ ),
+ ] = None
+ after_digest: Annotated[
+ str | None,
+ Field(
+ description="SHA-256 over the canonical JSON of `after_value`.
Present only when values are disclosed, and null when the change has no after
side.",
+ title="After Digest",
+ ),
+ ] = None
+ before_value: Annotated[
+ Any | None,
+ Field(
+ description="The value this path held in the base version. Present
only when values are disclosed, and omitted entirely when the change has no
before side — which is how an absent side is told apart from a stored null.",
+ title="Before Value",
+ ),
+ ] = None
+ after_value: Annotated[
+ Any | None,
+ Field(
+ description="The value this path holds in the target version.
Present only when values are disclosed, and omitted entirely when the change
has no after side — which is how an absent side is told apart from a stored
null.",
+ title="After Value",
+ ),
+ ] = None
+
+
+class DagVersionDiffResponse(BaseModel):
+ """
+ Observed-state difference between two stored Dag versions.
+ """
+
+ diff_schema_version: Annotated[
+ int,
+ Field(
+ description="Wire format of this payload. Incremented when its
shape changes.",
+ title="Diff Schema Version",
+ ),
+ ]
+ base_version_number: Annotated[int, Field(title="Base Version Number")]
+ target_version_number: Annotated[int, Field(title="Target Version Number")]
+ serialized_dag_schema_versions: DagVersionDiffSchemaVersions
Review Comment:
`DagVersionSerializerVersions` ? Cause it's confusing with
`diff_schema_version` which is the diff algorithm version
##########
airflow-core/src/airflow/ui/src/pages/Dag/Versions/VersionDiff.tsx:
##########
@@ -0,0 +1,168 @@
+/*!
+ * 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.
+ */
+import { Alert, Badge, Box, Code, Flex, Heading, Table, Text } from
"@chakra-ui/react";
+import { useTranslation } from "react-i18next";
+
+import type { DagVersionDiffResponse } from "openapi/requests/types.gen";
+
+type VersionDiffProps = {
+ readonly baseVersionNumber: number;
+ readonly diff: DagVersionDiffResponse;
+ readonly targetVersionNumber: number;
+};
+
+const IMPACT_COLORS: Record<string, string> = {
+ authorization: "orange",
+ execution: "red",
+ metadata: "blue",
+ provenance: "gray",
+ unknown: "gray",
+};
+
+// A whole task added or removed carries its entire serialized payload, which
would otherwise
+// unfold into one unreadable table cell.
+const MAX_VALUE_LENGTH = 120;
+
+const renderValue = (value: unknown) => {
+ if (value === undefined) {
+ return "—";
+ }
+
+ // A stored null is a value; only an absent side gets the dash above. An
empty string is quoted
+ // so it reads as a value rather than as a blank cell.
+ const text = typeof value === "string" ? value || '""' :
JSON.stringify(value);
+
+ return text.length > MAX_VALUE_LENGTH ? `${text.slice(0,
MAX_VALUE_LENGTH)}…` : text;
+};
+
+export const VersionDiff = ({ baseVersionNumber, diff, targetVersionNumber }:
VersionDiffProps) => {
+ const { t: translate } = useTranslation("dag");
+ const valuesShown = diff.values_status === "available";
+
+ if (diff.mode === "unavailable") {
+ return (
+ <Alert.Root status="info"
title={translate("versions.unavailable.title")}>
+ <Alert.Description>
+ {diff.unavailable_reason === null || diff.unavailable_reason ===
undefined ? (
+ translate("versions.unavailable.withoutReason")
+ ) : (
+ <>
+ {translate("versions.unavailable.description")}{" "}
+ {/* A machine token, so it is shown as code rather than folded
into a translated sentence. */}
+ <Code fontSize="sm">{diff.unavailable_reason}</Code>
+ </>
+ )}
+ </Alert.Description>
+ </Alert.Root>
+ );
+ }
+
+ return (
+ <Box>
+ <Flex alignItems="center" gap={3} justifyContent="space-between" mb={2}>
+ <Box>
+ <Heading size="md">
+ {translate("versions.heading", { base: baseVersionNumber, target:
targetVersionNumber })}
+ </Heading>
+ <Text color="fg.muted" fontSize="sm">
+ {translate("versions.summary", {
+ baseSchema: diff.serialized_dag_schema_versions.base ?? "?",
+ count: diff.total_changes,
+ targetSchema: diff.serialized_dag_schema_versions.target ?? "?",
+ })}
+ </Text>
+ </Box>
+ <Flex alignItems="center" gap={2}>
+ <Badge
colorPalette="teal">{translate("versions.observedState")}</Badge>
+ <Badge colorPalette={valuesShown ? "green" : "gray"}>
+ {translate(valuesShown ? "versions.valuesShown" :
"versions.valuesHidden")}
+ </Badge>
+ {/* Says what grants values, since the viewer cannot grant it here:
the server decides
+ from the caller's access to Dag code. */}
+ <Text color="fg.muted" fontSize="sm">
+ {translate("versions.codeAccess")}
+ </Text>
+ </Flex>
+ </Flex>
+
+ {diff.truncated ? (
+ <Alert.Root mb={2} status="warning"
title={translate("versions.truncated.title")}>
+
<Alert.Description>{translate("versions.truncated.description")}</Alert.Description>
+ </Alert.Root>
+ ) : undefined}
+
+ {diff.changes.length === 0 ? (
+ <Text color="fg.muted">{translate("versions.noChanges")}</Text>
+ ) : (
+ <Table.Root striped>
Review Comment:
Probably better to use the shared `DataTable` component at our disposal
instead of doing all that manually.
(for consistent styling and functionalities)
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/dag_versions.py:
##########
@@ -132,3 +138,76 @@ def get_dag_versions(
dag_versions=dag_versions,
total_entries=total_entries,
)
+
+
+@dag_versions_router.get(
+ "/{base_version_number}/diff/{target_version_number}",
+ responses=create_openapi_http_exception_doc(
+ [
+ status.HTTP_400_BAD_REQUEST,
+ status.HTTP_404_NOT_FOUND,
+ ]
+ ),
+ dependencies=[Depends(requires_access_dag(method="GET",
access_entity=DagAccessEntity.VERSION))],
+ # Serializing unset fields as null would erase the absent-vs-null
distinction the value
+ # fields document.
+ response_model_exclude_unset=True,
+)
+def get_dag_version_diff(
Review Comment:
Should this be public API or UI only at this point ?
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/dag_versions.py:
##########
@@ -132,3 +138,76 @@ def get_dag_versions(
dag_versions=dag_versions,
total_entries=total_entries,
)
+
+
+@dag_versions_router.get(
+ "/{base_version_number}/diff/{target_version_number}",
Review Comment:
No strong opinion but this tends to read as 'nested' resources in a REST
convention.
Can we consider `dags/{dag_id}/dagVersions/diff?base={}&target={}` ?
Depends if we consider a 'diff' a subresource of a DagVersion, or `diffing`
as an action between two versions.
WDYT?
##########
airflow-core/src/airflow/ui/src/pages/Dag/Versions/VersionDiff.tsx:
##########
@@ -0,0 +1,168 @@
+/*!
+ * 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.
+ */
+import { Alert, Badge, Box, Code, Flex, Heading, Table, Text } from
"@chakra-ui/react";
+import { useTranslation } from "react-i18next";
+
+import type { DagVersionDiffResponse } from "openapi/requests/types.gen";
+
+type VersionDiffProps = {
+ readonly baseVersionNumber: number;
+ readonly diff: DagVersionDiffResponse;
+ readonly targetVersionNumber: number;
+};
+
+const IMPACT_COLORS: Record<string, string> = {
+ authorization: "orange",
+ execution: "red",
+ metadata: "blue",
+ provenance: "gray",
+ unknown: "gray",
+};
+
+// A whole task added or removed carries its entire serialized payload, which
would otherwise
+// unfold into one unreadable table cell.
+const MAX_VALUE_LENGTH = 120;
+
+const renderValue = (value: unknown) => {
+ if (value === undefined) {
+ return "—";
+ }
+
+ // A stored null is a value; only an absent side gets the dash above. An
empty string is quoted
+ // so it reads as a value rather than as a blank cell.
+ const text = typeof value === "string" ? value || '""' :
JSON.stringify(value);
+
+ return text.length > MAX_VALUE_LENGTH ? `${text.slice(0,
MAX_VALUE_LENGTH)}…` : text;
+};
+
+export const VersionDiff = ({ baseVersionNumber, diff, targetVersionNumber }:
VersionDiffProps) => {
+ const { t: translate } = useTranslation("dag");
+ const valuesShown = diff.values_status === "available";
+
+ if (diff.mode === "unavailable") {
+ return (
+ <Alert.Root status="info"
title={translate("versions.unavailable.title")}>
+ <Alert.Description>
+ {diff.unavailable_reason === null || diff.unavailable_reason ===
undefined ? (
+ translate("versions.unavailable.withoutReason")
+ ) : (
+ <>
+ {translate("versions.unavailable.description")}{" "}
+ {/* A machine token, so it is shown as code rather than folded
into a translated sentence. */}
+ <Code fontSize="sm">{diff.unavailable_reason}</Code>
+ </>
+ )}
+ </Alert.Description>
+ </Alert.Root>
+ );
+ }
+
+ return (
+ <Box>
+ <Flex alignItems="center" gap={3} justifyContent="space-between" mb={2}>
+ <Box>
+ <Heading size="md">
+ {translate("versions.heading", { base: baseVersionNumber, target:
targetVersionNumber })}
+ </Heading>
+ <Text color="fg.muted" fontSize="sm">
+ {translate("versions.summary", {
+ baseSchema: diff.serialized_dag_schema_versions.base ?? "?",
+ count: diff.total_changes,
+ targetSchema: diff.serialized_dag_schema_versions.target ?? "?",
+ })}
+ </Text>
+ </Box>
+ <Flex alignItems="center" gap={2}>
+ <Badge
colorPalette="teal">{translate("versions.observedState")}</Badge>
+ <Badge colorPalette={valuesShown ? "green" : "gray"}>
+ {translate(valuesShown ? "versions.valuesShown" :
"versions.valuesHidden")}
+ </Badge>
+ {/* Says what grants values, since the viewer cannot grant it here:
the server decides
+ from the caller's access to Dag code. */}
+ <Text color="fg.muted" fontSize="sm">
+ {translate("versions.codeAccess")}
+ </Text>
+ </Flex>
+ </Flex>
+
+ {diff.truncated ? (
+ <Alert.Root mb={2} status="warning"
title={translate("versions.truncated.title")}>
+
<Alert.Description>{translate("versions.truncated.description")}</Alert.Description>
+ </Alert.Root>
+ ) : undefined}
+
+ {diff.changes.length === 0 ? (
+ <Text color="fg.muted">{translate("versions.noChanges")}</Text>
+ ) : (
+ <Table.Root striped>
+ <Table.Header>
+ <Table.Row>
+
<Table.ColumnHeader>{translate("versions.columns.path")}</Table.ColumnHeader>
+
<Table.ColumnHeader>{translate("versions.columns.operation")}</Table.ColumnHeader>
+
<Table.ColumnHeader>{translate("versions.columns.category")}</Table.ColumnHeader>
+
<Table.ColumnHeader>{translate("versions.columns.impact")}</Table.ColumnHeader>
+
<Table.ColumnHeader>{translate("versions.columns.occurrences")}</Table.ColumnHeader>
+ {valuesShown ? (
+ <>
+
<Table.ColumnHeader>{translate("versions.columns.before")}</Table.ColumnHeader>
+
<Table.ColumnHeader>{translate("versions.columns.after")}</Table.ColumnHeader>
+ </>
+ ) : undefined}
+ </Table.Row>
+ </Table.Header>
+ <Table.Body>
+ {diff.changes.map((change) => (
+ <Table.Row key={`${change.path}-${change.operation}`}>
+ <Table.Cell>
+ <Code fontSize="sm">{change.path}</Code>
+ </Table.Cell>
+ <Table.Cell>
+ {translate(`versions.operations.${change.operation}`, {
defaultValue: change.operation })}
+ </Table.Cell>
+ <Table.Cell>
+ {translate(`versions.categories.${change.category}`, {
defaultValue: change.category })}
+ </Table.Cell>
+ <Table.Cell>
+ <Badge colorPalette={IMPACT_COLORS[change.impact] ?? "gray"}>
+ {translate(`versions.impacts.${change.impact}`, {
defaultValue: change.impact })}
+ </Badge>
+ </Table.Cell>
+ <Table.Cell>{change.occurrence_count}</Table.Cell>
+ {valuesShown ? (
+ <>
+ <Table.Cell>
+ <Code
fontSize="sm">{renderValue(change.before_value)}</Code>
+ </Table.Cell>
+ <Table.Cell>
+ <Code
fontSize="sm">{renderValue(change.after_value)}</Code>
+ </Table.Cell>
+ </>
+ ) : undefined}
+ </Table.Row>
+ ))}
+ </Table.Body>
+ </Table.Root>
+ )}
+
+ <Text color="fg.muted" fontSize="sm" mt={3}>
+ {translate("versions.observedStateBoundary")}
Review Comment:
This probably should go above the table, with an 'info' icon, something like
<img width="420" height="290" alt="Image"
src="https://github.com/user-attachments/assets/c6dc09de-2ed9-4365-9103-5544c41b23c5";
/>
The reason is because if the table grows in size, the 'information' piece
will be pushed down and missed for your users.
--
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]
