[v3-3-test] Resolve backfill_id in the access dependency with the type the routes declare (#70889) - #71090
Merged
Merged
Conversation
…e the routes declare (#70889) * Resolve backfill_id in the access dependency with the type the routes declare The backfill routes declare `backfill_id: NonNegativeInt`, but `requires_access_backfill` parsed the raw path value with `int()` and swallowed the failure. The two parsers do not agree: pydantic's lax mode validates "1.0" and "1.00" to 1, while `int()` rejects both. Dependencies resolve before the endpoint's own parameter validation, so for those spellings the dependency left the Dag unresolved on a request the handler then served against backfill 1 -- the two disagreed about which Dag the request concerned. Parse with the same TypeAdapter the routes declare so they cannot diverge. * Use spec'd mocks in the backfill authorization dependency test An unspecced Mock accepts any attribute, so the test would keep passing if the dependency started reading something the real Request, Session or Backfill does not have. * Point at the tracking issue for the unknown-backfill fallback A backfill_id that parses but matches no row falls through to the body's dag_id, so an unknown backfill answers 404 where an unauthorized one answers 403 and a caller can tell which ids exist. That is a separate fix from the parser divergence this change closes, and it has to keep the three body-authorized routes working, so it is tracked rather than folded in here. The comment above the adapter also loses the history that led to it; what matters going forward is the rule it states. (cherry picked from commit a6265b7) Co-authored-by: Jarek Potiuk <jarek@potiuk.com>
6 tasks
vatsrahul1001
marked this pull request as ready for review
August 4, 2026 12:56
vatsrahul1001
requested review from
bugraoz93,
choo121600,
ephraimbuddy,
henry3260,
jason810496,
pierrejeambrun,
rawwar and
shubhamraj-git
as code owners
August 4, 2026 12:56
vatsrahul1001
approved these changes
Aug 4, 2026
vatsrahul1001
added a commit
that referenced
this pull request
Aug 5, 2026
…e the routes declare (#70889) (#71090) * Resolve backfill_id in the access dependency with the type the routes declare The backfill routes declare `backfill_id: NonNegativeInt`, but `requires_access_backfill` parsed the raw path value with `int()` and swallowed the failure. The two parsers do not agree: pydantic's lax mode validates "1.0" and "1.00" to 1, while `int()` rejects both. Dependencies resolve before the endpoint's own parameter validation, so for those spellings the dependency left the Dag unresolved on a request the handler then served against backfill 1 -- the two disagreed about which Dag the request concerned. Parse with the same TypeAdapter the routes declare so they cannot diverge. * Use spec'd mocks in the backfill authorization dependency test An unspecced Mock accepts any attribute, so the test would keep passing if the dependency started reading something the real Request, Session or Backfill does not have. * Point at the tracking issue for the unknown-backfill fallback A backfill_id that parses but matches no row falls through to the body's dag_id, so an unknown backfill answers 404 where an unauthorized one answers 403 and a caller can tell which ids exist. That is a separate fix from the parser divergence this change closes, and it has to keep the three body-authorized routes working, so it is tracked rather than folded in here. The comment above the adapter also loses the history that led to it; what matters going forward is the rule it states. (cherry picked from commit a6265b7) Co-authored-by: Jarek Potiuk <jarek@potiuk.com> Co-authored-by: Rahul Vats <43964496+vatsrahul1001@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The backfill routes declare
backfill_id: NonNegativeInt, butrequires_access_backfillparsed the raw path value withint()andswallowed the failure. The two parsers do not agree: pydantic's lax mode
validates "1.0" and "1.00" to 1, while
int()rejects both.Dependencies resolve before the endpoint's own parameter validation, so for
those spellings the dependency left the Dag unresolved on a request the
handler then served against backfill 1 -- the two disagreed about which Dag
the request concerned.
Parse with the same TypeAdapter the routes declare so they cannot diverge.
An unspecced Mock accepts any attribute, so the test would keep passing if the
dependency started reading something the real Request, Session or Backfill does
not have.
A backfill_id that parses but matches no row falls through to the body's dag_id,
so an unknown backfill answers 404 where an unauthorized one answers 403 and a
caller can tell which ids exist. That is a separate fix from the parser
divergence this change closes, and it has to keep the three body-authorized
routes working, so it is tracked rather than folded in here.
The comment above the adapter also loses the history that led to it; what
matters going forward is the rule it states.
(cherry picked from commit a6265b7)
Co-authored-by: Jarek Potiuk jarek@potiuk.com