Fix for Edit and Delete request issue 53681

[mandeepzemo]

closes: #53681

---

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in airflow-core/newsfragments.

[boring-cyborg]

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
Here are some useful points:

• Pay attention to the quality of your code (ruff, mypy and type annotations). Our pre-commits will help you with that.
• In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
• Consider using Breeze environment for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
• Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
• Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
• Be sure to read the Airflow Coding style.
• Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
  Apache Airflow is a community-driven project and together we are making it better 🚀.
  In case of doubts contact the developers at:
  Mailing List: dev@airflow.apache.org
  Slack: https://s.apache.org/airflow-slack

[jscheffl]

Did we also consider to restrict pool names to use a "/" character? Like we did with run_ids? I assume there might be a bit more places like variables where the same problem applies?

closes: #53681

[uranusjr]

We could consider banning slashes altogether, but it’d cause too much breakage we need a long deprecation period, and a fix like this would be required in the meantime anyway.

As a side, this will not only affect slashes, but also some percent usages that may be unintentionally identified as escape sequences. This should have a significant newsfragment calling out potential incompatibilities.

[akshayvijayjain]

@uranusjr ;
I am not directly working on task, just overviewing with @mandeepzemo , please excuse if I do oversight.

could we take both the approaches

we could restrict create request from using \ or other chars; using a valid regular expression for identifier https://stackoverflow.com/questions/14953861/representing-identifiers-using-regular-expression

and for those entities already created we can avoid the error using encodeUriComponent?

should we check how to put deprecation notice on this.

[pierrejeambrun]

We don't need to do that to handle / in pool name. Pool is an easy resource to fix because there is no 'action' define under pools/{pool_name}/some_action.

So we can just use the starlette path convertor:

@pools_router.patch(
    "/{pool_name:path}",
    responses=create_openapi_http_exception_doc(
        [
            status.HTTP_400_BAD_REQUEST,
            status.HTTP_404_NOT_FOUND,
        ]
    ),
    dependencies=[Depends(requires_access_pool(method="PUT")), Depends(action_logging())],
)
def patch_pool(
    pool_name: str,
    patch_body: PoolPatchBody,
    session: SessionDep,
    update_mask: list[str] | None = Query(None),
) -> PoolResponse:

[mandeepzemo]

@jscheffl As taken refer from variables and updated the pool routes code for PATCH, DELETE and GET Methods
#53815

[pierrejeambrun]

Can you add a small test that edit or retrieve a pool that has a / in its name please?

[jason810496]

Thanks for the PR!

Can you add a small test that edit or retrieve a pool that has a / in its name please?

FYI:

airflow/airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_pools.py

Lines 34 to 35 in 7e5785d

	POOL2_NAME = "pool2"
	POOL2_SLOT = 10

Maybe replacing the original value of POOL2_NAME with pool name with / in it will be enough.

[mandeepzemo]

@pierrejeambrun @jason810496 Thanks for the update,
Updated the test cases for testing Pool Name with '/'.
And Also sharing the local test result below.
Please also let me know, if any other changes need to update.

[pierrejeambrun]

Nice thanks.

[pierrejeambrun]

Static check need fixing, we can also take the opportunity to rename POOL3_NAME = "test/3" to POOL3_NAME = "pool/3". (pool1 and pool2 are named pool1 and pool2)

[mandeepzemo]

@pierrejeambrun
The pool list ordering test was passing locally but failing in CI/CD due to locale-dependent sorting of POOL3_NAME = "pool/3". To make the result consistent across environments, I updated the assertion to compare sorted lists in Python:

assert sorted([pool["name"] for pool in body["pools"]]) == sorted(expected_ids)

This avoids collation issues while still verifying the correct pool names are returned.

[pierrejeambrun]

We need to investigate, that's not normal and not the correct approach. Returned items of the API should be ordred in a deterministic way. This is what is happening for all endpoints and this test shouldn't differ.

Something might be wrong.

[pierrejeambrun]

failing in CI/CD due to locale-dependent sorting of POOL3_NAME = "pool/3"

Which test ordering was failing specifically?

[mandeepzemo]

@pierrejeambrun

In test_pool.py (lines 186–195), the test checks if the /pools endpoint returns the pool list in the expected order when using the order_by=name query param.

Example

assert [pool["name"] for pool in body["pools"]] == expected_ids

Expected Behavior (Local):

Given: POOL1_NAME = "pool1", POOL2_NAME = "pool2", POOL3_NAME = "pool/3"

Database sorting by name should return:

["pool/3", "pool2", "pool1"]

(/ sorts before alphanumeric characters in ASCII order).

Issue (CI/CD):
In CI runs, the same query returns:

["pool1", "pool2", "pool/3"]

—indicating sorting behavior differs between the local environment and the CI environment (likely due to DB collation or locale settings). Logs

Resolution Founded

Initial Fix:

To make the test consistent across environments, I changed the assertion to compare sorted lists:

assert sorted([pool["name"] for pool in body["pools"]]) == sorted(expected_ids)

Permanent Fix (Preferred):

On reviewing other test cases such as test_variable.py, where keys include slashes:

TEST_VARIABLE_KEY = "test_variable_key" TEST_VARIABLE_KEY4 = "test_variable_key/with_slashes"

I updated POOL3_NAME to "pool3/with_slashes".
This makes the sorting order deterministic across environments and removes the need for the sorted() workaround entirely.

[boring-cyborg]

Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions.

[github-actions]

Backport successfully created: v3-0-test

Status	Branch	Result
✅	v3-0-test