pierrejeambrun commented on code in PR #70312:
URL: https://github.com/apache/airflow/pull/70312#discussion_r3683553900
##########
airflow-core/src/airflow/api_fastapi/execution_api/datamodels/taskinstance.py:
##########
@@ -390,6 +390,10 @@ def safe_extract_from_orm(cls, data: Any) -> Any:
else:
values["note"] = None
+ # A property rather than a column, so the loop above never picks it up.
+ if not insp.detached:
+ values["team_name"] = data.team_name
Review Comment:
Yep my agend flagged this too. There's no eager loading of team there, so
the team returned can be stale.
##########
airflow-core/src/airflow/models/dag.py:
##########
@@ -482,6 +482,15 @@ class DagModel(Base):
dag_versions = relationship(
"DagVersion", back_populates="dag_model", cascade="all, delete,
delete-orphan"
)
+ # The Dag's bundle, which carries the owning team. ``lazy="raise"``
forbids implicit
+ # loading so a caller that forgets ``eager_load_teams`` cannot emit a
silent N+1;
+ # ``TeamOwnedMixin.team_name`` only reads this once loader options have
populated it.
+ bundle = relationship(
+ "DagBundleModel",
+ primaryjoin="DagModel.bundle_name == DagBundleModel.name",
+ viewonly=True,
+ lazy="raise",
+ )
Review Comment:
Why do we need this ?
##########
airflow-core/src/airflow/ui/src/pages/Dag/Header.tsx:
##########
@@ -112,10 +115,19 @@ export const Header = ({
? undefined
: `${dag.active_runs_count ?? 0} of ${dag.max_active_runs}`,
},
- {
- label: translate("dagDetails.owner"),
- value: <DagOwners ownerLinks={dag?.owner_links ?? undefined}
owners={dag?.owners} />,
- },
+ showTeam
+ ? {
+ label: translate("dagDetails.team"),
+ value: (
+ <RouterLink to={`/dags?teams=${encodeURIComponent(dag.team_name as
string)}`}>
+ {dag.team_name}
+ </RouterLink>
+ ),
+ }
+ : {
+ label: translate("dagDetails.owner"),
+ value: <DagOwners ownerLinks={dag?.owner_links ?? undefined}
owners={dag?.owners} />,
Review Comment:
We shouldn't mix those two. (owner and teams are different).
##########
airflow-core/src/airflow/models/team.py:
##########
@@ -78,3 +79,48 @@ def get_all_team_names(cls, *, session: Session =
NEW_SESSION) -> set[str]:
:return: Set of all team names
"""
return set(session.scalars(select(Team.name)).all())
+
+
+class TeamOwnedMixin:
+ """
+ Exposes ``team_name`` on models that reach a team through a relationship
path.
+
+ Teams only exist in multi-team mode, so the config is checked before any
hop is
+ walked: single-team deployments answer ``None`` without loading a
relationship, and
+ endpoints there pay for no extra join. In multi-team mode the value comes
from the
+ already-loaded relationships when the query applied
+ :func:`~airflow.api_fastapi.common.db.dags.eager_load_teams`, and falls
back to the
+ per-Dag cached resolver otherwise — so the attribute stays correct on
paths that
+ cannot eager load (an in-memory Dag run, a callback re-fetching by primary
key)
+ instead of tripping the ``lazy="raise"`` guard that keeps N+1 loads out.
+ """
+
+ #: Attribute names to walk from ``self`` to the owning :class:`DagModel`.
+ _team_path: ClassVar[tuple[str, ...]] = ()
+
+ if TYPE_CHECKING:
+ # Every model mixing this in carries ``dag_id`` (it is the fallback
lookup key).
+ dag_id: str
+
+ @property
+ def team_name(self) -> str | None:
+ """Name of the team owning this entity, or ``None`` when it is not
team-owned."""
+ if not conf.getboolean("core", "multi_team"):
+ return None
+
+ from airflow.models.dag import DagModel
+
+ entity: Any = self
+ for attribute in (*self._team_path, "bundle", "teams"):
+ state = sa_inspect(entity)
+ if attribute in state.unloaded:
+ # Reuse this entity's own session. ``get_team_name`` is
``@provide_session``,
+ # and the session it would open is the *same* scoped session
the caller is
+ # using, so closing it on exit detaches every object the
caller still holds.
+ if state.session is not None:
+ return DagModel.get_team_name(self.dag_id,
session=state.session)
+ return DagModel.get_team_name(self.dag_id)
+ if (entity := getattr(entity, attribute)) is None:
+ return None
+ # A bundle maps to at most one team (unique index on
dag_bundle_team.dag_bundle_name).
+ return entity[0].name if entity else None
Review Comment:
No idea why we used an association table for
`dag_bundle_team_association_table` while we have a unique index directly on
it. That would just come down to a normal N-1 relathionship, and would avoid
this `[0]`.
Anyway out of this PR scope.
--
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]
