Pushkal-Gupta commented on PR #74896:
URL: https://github.com/apache/airflow/pull/74896#issuecomment-6238001069
Nice catch, thanks. The bulk endpoints were affected and untested, and
chasing it turned up a third route I had missed.
Added 14 tests:
`TestBulkPools.test_bulk_rejects_blank_name` and
`TestBulkVariables.test_bulk_rejects_blank_key`, each over `""`, `" "` and
`"\t"` and over both the create and update actions. Validation happens on the
whole `BulkBody`, so one bad entity rejects the request with a 422 and none of
the actions run. Without the constraint these return 200 and the blank entity
is created.
`TestPatchVariable.test_patch_variable_rejects_blank_key`. I had not noticed
that `patch_variable` takes the full `VariableBody` rather than the partial, so
it was affected too, and it was the worst of the three: the test seeds a
variable with a blank key, and before the change `PATCH /variables/` with
`{"key": "", ...}` matched the equally blank `:path` parameter and updated the
row. It now returns 422 and the value is untouched. The parameters stop at `""`
and `" "` there because the body key has to match the uri parameter to get past
the mismatch check, and httpx will not put a raw tab in a url.
On `PATCH /pools/{pool_name}`: that one goes through `PoolPatchBody`, where
`name` is optional, and I left it unconstrained on purpose. Constraining an
optional field makes `datamodel-codegen` lift it into a `RootModel`, which
breaks `airflow-ctl`, where `operations.py` builds its url from
`f"pools/{pool_body.pool}"` and would request `pools/root='mypool'`. Two
airflow-ctl tests fail that way. So the blank name is rejected in the route
instead, by comparing against `None` rather than testing for truthiness, and
`test_patch_pool_rejects_blank_name_instead_of_renaming` covers it. Happy to
revisit if you would rather see the constraint on the model and the codegen
handled some other way.
`test_pools.py` and `test_variables.py` are at 202 passed, up from 188. All
14 new tests fail without the change, the bulk ones as `assert 200 == 422`.
--
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]
