amoghrajesh commented on code in PR #70889:
URL: https://github.com/apache/airflow/pull/70889#discussion_r3701789464
##########
airflow-core/tests/unit/api_fastapi/core_api/test_security.py:
##########
@@ -455,6 +458,48 @@ async def
test_requires_access_backfill_backfill_not_found_falls_back_to_body(
user=user,
)
+ @pytest.mark.db_test
+ @pytest.mark.asyncio
+ @pytest.mark.parametrize("backfill_id", ["42", "42.0", "42.00"])
+ @patch.object(DagModel, "get_team_name")
+ @patch("airflow.api_fastapi.core_api.security.get_auth_manager")
+ async def
test_requires_access_backfill_authorizes_the_backfill_the_handler_will_act_on(
+ self, mock_get_auth_manager, mock_get_team_name, backfill_id
+ ):
+ """The dependency must resolve the same backfill the handler does, for
every spelling.
+
+ The endpoints declare ``backfill_id: NonNegativeInt``, and pydantic's
lax mode coerces
+ ``"42.0"`` and ``"42.00"`` to ``42`` -- both are spellings the handler
accepts and serves
+ against backfill 42. Parsing with ``int()`` here rejected them and
left ``dag_id``
+ unresolved, so the two disagreed about which Dag the request concerned.
+ """
+ auth_manager = Mock()
+ auth_manager.is_authorized_dag.return_value = True
+ mock_get_auth_manager.return_value = auth_manager
+ mock_get_team_name.return_value = "team1"
+
+ backfill = Mock()
+ backfill.dag_id = "backfill_dag"
+ session = Mock()
+ session.scalars.return_value.one_or_none.return_value = backfill
+
+ request = Mock()
+ request.path_params = {"backfill_id": backfill_id}
+ request.query_params = {"dag_id": "some_other_dag"}
+ request.json = AsyncMock(return_value={"dag_id": "some_other_dag"})
+
+ user = Mock()
Review Comment:
Can we please use spec or autospec on the mocks here?
##########
airflow-core/src/airflow/api_fastapi/core_api/security.py:
##########
Review Comment:
Why dont we let fastapi validate it once natively.
Instead of building a second `TypeAdapter(NonNegativeInt)` and calling it
manually inside the dependency, the dependency function could declare
`backfill_id: NonNegativeInt` as an actual typed parameter? That would mean
literally one validation pass shared by construction, instead of two
independently constructed validators that happen to use the same type today.
This is architecturally tighter also and it removes even the possibility of the
two drifting apart in the future
--
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]
