From 1a5dea66eb1dd77b8c81ac8763e3e3032954b483 Mon Sep 17 00:00:00 2001 From: Jayant Kernel Date: Sat, 7 Mar 2026 22:57:47 +0530 Subject: [PATCH 1/7] fix(flows): replace GET /flows/exists with POST to support URI-unsafe flow names The GET /flows/exists/{name}/{external_version} endpoint broke for flows with URI-unsafe characters in their names (e.g. sklearn flows with parentheses). Replaced with POST /flows/exists accepting name and external_version in the request body, resolving issue #166. Closes #166 --- src/routers/openml/flows.py | 13 +++++++++---- tests/routers/openml/flows_test.py | 12 ++++++------ .../openml/migration/flows_migration_test.py | 13 +++++++------ 3 files changed, 22 insertions(+), 16 deletions(-) diff --git a/src/routers/openml/flows.py b/src/routers/openml/flows.py index cb6df5d9..2c941aa5 100644 --- a/src/routers/openml/flows.py +++ b/src/routers/openml/flows.py @@ -2,6 +2,7 @@ from typing import Annotated, Literal from fastapi import APIRouter, Depends, HTTPException +from pydantic import BaseModel from sqlalchemy import Connection import database.flows @@ -12,14 +13,18 @@ router = APIRouter(prefix="/flows", tags=["flows"]) -@router.get("/exists/{name}/{external_version}") +class FlowExistsBody(BaseModel): + name: str + external_version: str + + +@router.post("/exists") def flow_exists( - name: str, - external_version: str, + body: FlowExistsBody, expdb: Annotated[Connection, Depends(expdb_connection)], ) -> dict[Literal["flow_id"], int]: """Check if a Flow with the name and version exists, if so, return the flow id.""" - flow = database.flows.get_by_name(name=name, external_version=external_version, expdb=expdb) + flow = database.flows.get_by_name(name=body.name, external_version=body.external_version, expdb=expdb) if flow is None: raise HTTPException( status_code=HTTPStatus.NOT_FOUND, diff --git a/tests/routers/openml/flows_test.py b/tests/routers/openml/flows_test.py index d5188d0e..65ce18dd 100644 --- a/tests/routers/openml/flows_test.py +++ b/tests/routers/openml/flows_test.py @@ -7,7 +7,7 @@ from sqlalchemy import Connection from starlette.testclient import TestClient -from routers.openml.flows import flow_exists +from routers.openml.flows import FlowExistsBody, flow_exists from tests.conftest import Flow @@ -25,7 +25,7 @@ def test_flow_exists_calls_db_correctly( mocker: MockerFixture, ) -> None: mocked_db = mocker.patch("database.flows.get_by_name") - flow_exists(name, external_version, expdb_test) + flow_exists(FlowExistsBody(name=name, external_version=external_version), expdb_test) mocked_db.assert_called_once_with( name=name, external_version=external_version, @@ -47,26 +47,26 @@ def test_flow_exists_processes_found( "database.flows.get_by_name", return_value=fake_flow, ) - response = flow_exists("name", "external_version", expdb_test) + response = flow_exists(FlowExistsBody(name="name", external_version="external_version"), expdb_test) assert response == {"flow_id": fake_flow.id} def test_flow_exists_handles_flow_not_found(mocker: MockerFixture, expdb_test: Connection) -> None: mocker.patch("database.flows.get_by_name", return_value=None) with pytest.raises(HTTPException) as error: - flow_exists("foo", "bar", expdb_test) + flow_exists(FlowExistsBody(name="foo", external_version="bar"), expdb_test) assert error.value.status_code == HTTPStatus.NOT_FOUND assert error.value.detail == "Flow not found." def test_flow_exists(flow: Flow, py_api: TestClient) -> None: - response = py_api.get(f"/flows/exists/{flow.name}/{flow.external_version}") + response = py_api.post("/flows/exists", json={"name": flow.name, "external_version": flow.external_version}) assert response.status_code == HTTPStatus.OK assert response.json() == {"flow_id": flow.id} def test_flow_exists_not_exists(py_api: TestClient) -> None: - response = py_api.get("/flows/exists/foo/bar") + response = py_api.post("/flows/exists", json={"name": "foo", "external_version": "bar"}) assert response.status_code == HTTPStatus.NOT_FOUND assert response.json()["detail"] == "Flow not found." diff --git a/tests/routers/openml/migration/flows_migration_test.py b/tests/routers/openml/migration/flows_migration_test.py index 674bc439..128e0598 100644 --- a/tests/routers/openml/migration/flows_migration_test.py +++ b/tests/routers/openml/migration/flows_migration_test.py @@ -18,9 +18,8 @@ def test_flow_exists_not( py_api: TestClient, php_api: TestClient, ) -> None: - path = "exists/foo/bar" - py_response = py_api.get(f"/flows/{path}") - php_response = php_api.get(f"/flow/{path}") + py_response = py_api.post("/flows/exists", json={"name": "foo", "external_version": "bar"}) + php_response = php_api.get("/flow/exists/foo/bar") assert py_response.status_code == HTTPStatus.NOT_FOUND assert php_response.status_code == HTTPStatus.OK @@ -36,9 +35,11 @@ def test_flow_exists( py_api: TestClient, php_api: TestClient, ) -> None: - path = f"exists/{persisted_flow.name}/{persisted_flow.external_version}" - py_response = py_api.get(f"/flows/{path}") - php_response = php_api.get(f"/flow/{path}") + py_response = py_api.post( + "/flows/exists", + json={"name": persisted_flow.name, "external_version": persisted_flow.external_version}, + ) + php_response = php_api.get(f"/flow/exists/{persisted_flow.name}/{persisted_flow.external_version}") assert py_response.status_code == php_response.status_code, php_response.content From 2f60ac46855ca5d1260a176c75f32328497a5d08 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Sat, 7 Mar 2026 18:20:16 +0000 Subject: [PATCH 2/7] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- src/routers/openml/flows.py | 4 +++- tests/routers/openml/flows_test.py | 8 ++++++-- tests/routers/openml/migration/flows_migration_test.py | 4 +++- 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/src/routers/openml/flows.py b/src/routers/openml/flows.py index 2c941aa5..16913f86 100644 --- a/src/routers/openml/flows.py +++ b/src/routers/openml/flows.py @@ -24,7 +24,9 @@ def flow_exists( expdb: Annotated[Connection, Depends(expdb_connection)], ) -> dict[Literal["flow_id"], int]: """Check if a Flow with the name and version exists, if so, return the flow id.""" - flow = database.flows.get_by_name(name=body.name, external_version=body.external_version, expdb=expdb) + flow = database.flows.get_by_name( + name=body.name, external_version=body.external_version, expdb=expdb + ) if flow is None: raise HTTPException( status_code=HTTPStatus.NOT_FOUND, diff --git a/tests/routers/openml/flows_test.py b/tests/routers/openml/flows_test.py index 65ce18dd..f6a65351 100644 --- a/tests/routers/openml/flows_test.py +++ b/tests/routers/openml/flows_test.py @@ -47,7 +47,9 @@ def test_flow_exists_processes_found( "database.flows.get_by_name", return_value=fake_flow, ) - response = flow_exists(FlowExistsBody(name="name", external_version="external_version"), expdb_test) + response = flow_exists( + FlowExistsBody(name="name", external_version="external_version"), expdb_test + ) assert response == {"flow_id": fake_flow.id} @@ -60,7 +62,9 @@ def test_flow_exists_handles_flow_not_found(mocker: MockerFixture, expdb_test: C def test_flow_exists(flow: Flow, py_api: TestClient) -> None: - response = py_api.post("/flows/exists", json={"name": flow.name, "external_version": flow.external_version}) + response = py_api.post( + "/flows/exists", json={"name": flow.name, "external_version": flow.external_version} + ) assert response.status_code == HTTPStatus.OK assert response.json() == {"flow_id": flow.id} diff --git a/tests/routers/openml/migration/flows_migration_test.py b/tests/routers/openml/migration/flows_migration_test.py index 128e0598..5ee8e592 100644 --- a/tests/routers/openml/migration/flows_migration_test.py +++ b/tests/routers/openml/migration/flows_migration_test.py @@ -39,7 +39,9 @@ def test_flow_exists( "/flows/exists", json={"name": persisted_flow.name, "external_version": persisted_flow.external_version}, ) - php_response = php_api.get(f"/flow/exists/{persisted_flow.name}/{persisted_flow.external_version}") + php_response = php_api.get( + f"/flow/exists/{persisted_flow.name}/{persisted_flow.external_version}" + ) assert py_response.status_code == php_response.status_code, php_response.content From 3ff845fbfdc3ce56721c908503eb6eff6f17b137 Mon Sep 17 00:00:00 2001 From: Jayant Kernel Date: Sat, 7 Mar 2026 23:57:42 +0530 Subject: [PATCH 3/7] style: fix trailing comma in flow_exists db call --- src/routers/openml/flows.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/routers/openml/flows.py b/src/routers/openml/flows.py index 16913f86..b06f5369 100644 --- a/src/routers/openml/flows.py +++ b/src/routers/openml/flows.py @@ -25,7 +25,9 @@ def flow_exists( ) -> dict[Literal["flow_id"], int]: """Check if a Flow with the name and version exists, if so, return the flow id.""" flow = database.flows.get_by_name( - name=body.name, external_version=body.external_version, expdb=expdb + name=body.name, + external_version=body.external_version, + expdb=expdb, ) if flow is None: raise HTTPException( From a771b69a8f6d7ff6be4fd426bf01e7305b3c29e5 Mon Sep 17 00:00:00 2001 From: Jayant Kernel Date: Sun, 8 Mar 2026 00:06:40 +0530 Subject: [PATCH 4/7] refactor(flows): move FlowExistsBody to schemas and keep GET as deprecated wrapper - Move FlowExistsBody to schemas/flows.py for reusability - Keep GET /flows/exists/{name}/{version} as a deprecated thin wrapper around POST /flows/exists for backward compatibility - Update test imports accordingly --- src/routers/openml/flows.py | 18 +++++++++++------- src/schemas/flows.py | 5 +++++ tests/routers/openml/flows_test.py | 3 ++- 3 files changed, 18 insertions(+), 8 deletions(-) diff --git a/src/routers/openml/flows.py b/src/routers/openml/flows.py index b06f5369..31562a08 100644 --- a/src/routers/openml/flows.py +++ b/src/routers/openml/flows.py @@ -2,22 +2,16 @@ from typing import Annotated, Literal from fastapi import APIRouter, Depends, HTTPException -from pydantic import BaseModel from sqlalchemy import Connection import database.flows from core.conversions import _str_to_num from routers.dependencies import expdb_connection -from schemas.flows import Flow, Parameter, Subflow +from schemas.flows import Flow, FlowExistsBody, Parameter, Subflow router = APIRouter(prefix="/flows", tags=["flows"]) -class FlowExistsBody(BaseModel): - name: str - external_version: str - - @router.post("/exists") def flow_exists( body: FlowExistsBody, @@ -37,6 +31,16 @@ def flow_exists( return {"flow_id": flow.id} +@router.get("/exists/{name}/{external_version}", deprecated=True) +def flow_exists_get( + name: str, + external_version: str, + expdb: Annotated[Connection, Depends(expdb_connection)], +) -> dict[Literal["flow_id"], int]: + """Deprecated: use POST /flows/exists instead.""" + return flow_exists(FlowExistsBody(name=name, external_version=external_version), expdb) + + @router.get("/{flow_id}") def get_flow(flow_id: int, expdb: Annotated[Connection, Depends(expdb_connection)] = None) -> Flow: flow = database.flows.get(flow_id, expdb) diff --git a/src/schemas/flows.py b/src/schemas/flows.py index a6cd479c..50e2491c 100644 --- a/src/schemas/flows.py +++ b/src/schemas/flows.py @@ -6,6 +6,11 @@ from pydantic import BaseModel, ConfigDict, Field +class FlowExistsBody(BaseModel): + name: str + external_version: str + + class Parameter(BaseModel): name: str default_value: Any diff --git a/tests/routers/openml/flows_test.py b/tests/routers/openml/flows_test.py index f6a65351..658d6325 100644 --- a/tests/routers/openml/flows_test.py +++ b/tests/routers/openml/flows_test.py @@ -7,7 +7,8 @@ from sqlalchemy import Connection from starlette.testclient import TestClient -from routers.openml.flows import FlowExistsBody, flow_exists +from routers.openml.flows import flow_exists +from schemas.flows import FlowExistsBody from tests.conftest import Flow From a5cac494b086e529a02152b1a42303935796f35c Mon Sep 17 00:00:00 2001 From: Jayant-Kernel Date: Sun, 8 Mar 2026 03:27:16 +0530 Subject: [PATCH 5/7] ci: split tests into internal and API steps for clearer reporting Resolves #177 --- .github/workflows/tests.yml | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 18f61a49..a3d78236 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -34,10 +34,14 @@ jobs: fi docker compose $profiles up --detach --wait --remove-orphans || exit $(docker compose ps -q | xargs docker inspect -f '{{.State.ExitCode}}' | grep -v '^0' | wc -l) - - name: Run tests + - name: Run internal tests run: | marker="${{ matrix.php_api == true && 'php_api' || 'not php_api' }} and ${{ matrix.mutations == true && 'mut' || 'not mut' }}" - docker exec openml-python-rest-api coverage run -m pytest -n auto -v -m "$marker" + docker exec openml-python-rest-api coverage run -m pytest tests/database tests/config_test.py -n auto -v -m "$marker" + - name: Run API tests + run: | + marker="${{ matrix.php_api == true && 'php_api' || 'not php_api' }} and ${{ matrix.mutations == true && 'mut' || 'not mut' }}" + docker exec openml-python-rest-api coverage run -a -m pytest tests/routers -n auto -v -m "$marker" - name: Produce coverage report run: docker exec openml-python-rest-api coverage xml - name: Upload results to Codecov From 45af02a401f65fdb27623e1eabb3023d0ef6ded7 Mon Sep 17 00:00:00 2001 From: Jayant Kernel Date: Sun, 8 Mar 2026 12:06:35 +0530 Subject: [PATCH 6/7] fix(review): address CodeRabbit feedback on PR #266 - Add Field(max_length=) constraints to FlowExistsBody.name (1024) and .external_version (128) to mirror the bounds already on the Flow model - Add smoke test for the deprecated GET shim (flow_exists_get) to prevent silent regressions on the backward-compatibility surface - Add regression test for URI-unsafe flow names (containing '/') via POST /flows/exists, guarding the core motivation for the GET->POST migration - Extract duplicated marker expression into a job-level TEST_MARKER env var in tests.yml to eliminate drift risk --- .github/workflows/tests.yml | 8 ++++---- src/schemas/flows.py | 4 ++-- tests/routers/openml/flows_test.py | 25 ++++++++++++++++++++++++- 3 files changed, 30 insertions(+), 7 deletions(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index a3d78236..6bc2a890 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -14,6 +14,8 @@ permissions: jobs: tests: name: "${{ matrix.php_api == true && 'Migration' || 'Python-only' }} ${{ matrix.mutations == true && 'with mutations' || 'read-only' }}" + env: + TEST_MARKER: "${{ matrix.php_api == true && 'php_api' || 'not php_api' }} and ${{ matrix.mutations == true && 'mut' || 'not mut' }}" strategy: matrix: php_api: [true, false] @@ -36,12 +38,10 @@ jobs: - name: Run internal tests run: | - marker="${{ matrix.php_api == true && 'php_api' || 'not php_api' }} and ${{ matrix.mutations == true && 'mut' || 'not mut' }}" - docker exec openml-python-rest-api coverage run -m pytest tests/database tests/config_test.py -n auto -v -m "$marker" + docker exec openml-python-rest-api coverage run -m pytest tests/database tests/config_test.py -n auto -v -m "$TEST_MARKER" - name: Run API tests run: | - marker="${{ matrix.php_api == true && 'php_api' || 'not php_api' }} and ${{ matrix.mutations == true && 'mut' || 'not mut' }}" - docker exec openml-python-rest-api coverage run -a -m pytest tests/routers -n auto -v -m "$marker" + docker exec openml-python-rest-api coverage run -a -m pytest tests/routers -n auto -v -m "$TEST_MARKER" - name: Produce coverage report run: docker exec openml-python-rest-api coverage xml - name: Upload results to Codecov diff --git a/src/schemas/flows.py b/src/schemas/flows.py index 50e2491c..5d550d16 100644 --- a/src/schemas/flows.py +++ b/src/schemas/flows.py @@ -7,8 +7,8 @@ class FlowExistsBody(BaseModel): - name: str - external_version: str + name: str = Field(max_length=1024) + external_version: str = Field(max_length=128) class Parameter(BaseModel): diff --git a/tests/routers/openml/flows_test.py b/tests/routers/openml/flows_test.py index 658d6325..ae2ed66b 100644 --- a/tests/routers/openml/flows_test.py +++ b/tests/routers/openml/flows_test.py @@ -4,7 +4,7 @@ import pytest from fastapi import HTTPException from pytest_mock import MockerFixture -from sqlalchemy import Connection +from sqlalchemy import Connection, text from starlette.testclient import TestClient from routers.openml.flows import flow_exists @@ -76,6 +76,29 @@ def test_flow_exists_not_exists(py_api: TestClient) -> None: assert response.json()["detail"] == "Flow not found." +def test_flow_exists_get_deprecated(flow: Flow, py_api: TestClient) -> None: + response = py_api.get(f"/flows/exists/{flow.name}/{flow.external_version}") + assert response.status_code == HTTPStatus.OK + assert response.json() == {"flow_id": flow.id} + + +def test_flow_exists_uri_unsafe(expdb_test: Connection, py_api: TestClient) -> None: + expdb_test.execute( + text( + """ + INSERT INTO implementation(fullname,name,version,external_version,uploadDate) + VALUES ('weka/ZeroR','weka/ZeroR',2,'1.0/beta','2024-02-02 02:23:23'); + """, + ), + ) + (flow_id,) = expdb_test.execute(text("""SELECT LAST_INSERT_ID();""")).one() + response = py_api.post( + "/flows/exists", json={"name": "weka/ZeroR", "external_version": "1.0/beta"} + ) + assert response.status_code == HTTPStatus.OK + assert response.json() == {"flow_id": flow_id} + + def test_get_flow_no_subflow(py_api: TestClient) -> None: response = py_api.get("/flows/1") assert response.status_code == HTTPStatus.OK From 9b0c16ff6db08bbd65952c349ed2009b33880f5a Mon Sep 17 00:00:00 2001 From: Jayant Kernel Date: Sun, 8 Mar 2026 12:52:01 +0530 Subject: [PATCH 7/7] ci: always run API tests step even if internal tests fail Ensures both suites report independently so a failure in internal tests does not mask API test results, which is the whole point of the split. --- .github/workflows/tests.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 6bc2a890..d3118321 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -40,6 +40,7 @@ jobs: run: | docker exec openml-python-rest-api coverage run -m pytest tests/database tests/config_test.py -n auto -v -m "$TEST_MARKER" - name: Run API tests + if: always() run: | docker exec openml-python-rest-api coverage run -a -m pytest tests/routers -n auto -v -m "$TEST_MARKER" - name: Produce coverage report