Open
Conversation
… 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 openml#166.
Closes openml#166
for more information, see https://pre-commit.ci
…cated 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
Contributor
WalkthroughThe flows existence endpoint was changed from GET /flows/exists/{name}/{external_version} to POST /flows/exists accepting a new Pydantic model FlowExistsBody(name, external_version). A deprecated GET endpoint at /flows/exists/{name}/{external_version} now delegates to the POST handler. Tests were updated to use the POST API (and include a legacy-GET test). The GitHub Actions workflow adds a job-level TEST_MARKER env var and splits tests into "Run internal tests" and "Run API tests"; coverage and Codecov upload steps remain. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Author
|
@PGijsbers |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.github/workflows/tests.yml (1)
37-44: The test split looks good and achieves the stated objective.Coverage is correctly handled with fresh
coverage runfor internal tests and append mode (-a) for API tests. Both steps share the same container, preserving combined coverage.One minor observation: the
markerexpression is duplicated verbatim in lines 39 and 43. Consider extracting to a job-level environment variable to reduce drift risk:♻️ Optional: Extract marker to job-level env
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:Then in each step:
- 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"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/tests.yml around lines 37 - 44, Extract the duplicated marker expression into a job-level environment variable (e.g., MARKER) and update the two steps "Run internal tests" and "Run API tests" to reference that env var instead of repeating the expression; specifically replace the inline marker expression used in both steps with the single job env variable (marker) so the value is defined once and reused, reducing duplication and drift risk.src/schemas/flows.py (1)
9-11: Mirror the existing name/version bounds on the request model.
FlowExistsBodycurrently accepts unbounded strings, whileFlow.nameandFlow.external_versionin this file already cap the same fields at 1024 and 128 chars. Keeping the lookup payload consistent would fail fast on impossible values instead of sending them to the database.Suggested diff
class FlowExistsBody(BaseModel): - name: str - external_version: str + name: str = Field(max_length=1024) + external_version: str = Field(max_length=128)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/schemas/flows.py` around lines 9 - 11, FlowExistsBody accepts unbounded strings whereas the Flow model caps these fields; update FlowExistsBody to enforce the same length bounds by changing its fields to constrained types (e.g., use pydantic.constr or Field with max_length) so name has max_length=1024 and external_version has max_length=128, keeping the field names exactly FlowExistsBody.name and FlowExistsBody.external_version to mirror Flow.name and Flow.external_version.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/routers/openml/flows.py`:
- Around line 34-41: Add a small smoke test that exercises the deprecated GET
shim function flow_exists_get by calling the route GET
/flows/exists/{name}/{external_version} via the test client and asserting it
returns the same structure/value as the POST /flows/exists helper (or directly
asserts {"flow_id": <expected>}), to prevent regressions; locate the test near
other flows tests, create or reuse a fixture to ensure the flow exists (or use
an existing FlowExistsBody/flow creation helper), call
client.get(f"/flows/exists/{name}/{external_version}"), and assert status code
200 and the JSON payload matches the expected flow_id.
In `@tests/routers/openml/flows_test.py`:
- Around line 65-75: Add a regression test for URI-unsafe names by creating (or
using a Flow fixture) whose name contains a reserved character like "/" and then
POSTing to "/flows/exists" with that name and its external_version; assert
HTTPStatus.OK and that the response body equals {"flow_id": flow.id}. Implement
this as a new test (e.g., test_flow_exists_uri_unsafe) alongside
test_flow_exists and test_flow_exists_not_exists, using the same py_api:
TestClient invocation pattern so the lookup works when names cannot be placed in
the URL path.
---
Nitpick comments:
In @.github/workflows/tests.yml:
- Around line 37-44: Extract the duplicated marker expression into a job-level
environment variable (e.g., MARKER) and update the two steps "Run internal
tests" and "Run API tests" to reference that env var instead of repeating the
expression; specifically replace the inline marker expression used in both steps
with the single job env variable (marker) so the value is defined once and
reused, reducing duplication and drift risk.
In `@src/schemas/flows.py`:
- Around line 9-11: FlowExistsBody accepts unbounded strings whereas the Flow
model caps these fields; update FlowExistsBody to enforce the same length bounds
by changing its fields to constrained types (e.g., use pydantic.constr or Field
with max_length) so name has max_length=1024 and external_version has
max_length=128, keeping the field names exactly FlowExistsBody.name and
FlowExistsBody.external_version to mirror Flow.name and Flow.external_version.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d101f279-5c5e-4b51-b629-22d329ba2d0c
📒 Files selected for processing (5)
.github/workflows/tests.ymlsrc/routers/openml/flows.pysrc/schemas/flows.pytests/routers/openml/flows_test.pytests/routers/openml/migration/flows_migration_test.py
Comment on lines
+34
to
+41
| @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) |
Contributor
There was a problem hiding this comment.
Please add coverage for the deprecated shim.
flow_exists_get is now part of the public compatibility surface, but none of the updated tests exercise GET /flows/exists/{name}/{external_version}. A small smoke test would keep that wrapper from silently drifting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/routers/openml/flows.py` around lines 34 - 41, Add a small smoke test
that exercises the deprecated GET shim function flow_exists_get by calling the
route GET /flows/exists/{name}/{external_version} via the test client and
asserting it returns the same structure/value as the POST /flows/exists helper
(or directly asserts {"flow_id": <expected>}), to prevent regressions; locate
the test near other flows tests, create or reuse a fixture to ensure the flow
exists (or use an existing FlowExistsBody/flow creation helper), call
client.get(f"/flows/exists/{name}/{external_version}"), and assert status code
200 and the JSON payload matches the expected flow_id.
Jayant-kernel
pushed a commit
to Jayant-kernel/server-api
that referenced
this pull request
Mar 8, 2026
- 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 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- 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
Jayant-kernel
force-pushed
the
feature/issue-177
branch
from
331ab1d to
45af02a
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/tests.yml:
- Around line 39-44: The two GitHub Actions steps "Run internal tests" and "Run
API tests" are in the same job so a failure in the first step short-circuits the
second; update .github/workflows/tests.yml by either moving the "Run API tests"
step into its own job so it runs independently, or add an explicit condition to
the later step (the "Run API tests" step) such as if: ${{ always() }} so it
executes and records its result even if the "Run internal tests" step fails;
reference the step names "Run internal tests" and "Run API tests" when making
the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f07db707-e2b6-4af9-a7bc-417f58966c78
📒 Files selected for processing (3)
.github/workflows/tests.ymlsrc/schemas/flows.pytests/routers/openml/flows_test.py
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.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/tests.yml (1)
42-51: Keep coverage publishing running after a suite fails.Line 43 fixes the split-test short-circuit, but Lines 46-51 still use the default
success()condition. If either suite fails,coverage xmland Codecov are skipped, so the combinedcoverage run -adata never gets surfaced on the runs where it is most useful.Suggested follow-up
- 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 + if: ${{ !cancelled() }} run: docker exec openml-python-rest-api coverage xml - name: Upload results to Codecov + if: ${{ !cancelled() }} uses: codecov/codecov-action@v4 with: token: ${{ secrets.CODECOV_TOKEN }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/tests.yml around lines 42 - 51, The "Produce coverage report" and "Upload results to Codecov" steps are currently running only on success and get skipped when a test suite fails; update those steps (the steps named "Produce coverage report" and "Upload results to Codecov") to use if: always() so coverage xml and the Codecov upload run even after failures, ensuring the combine-with-coverage run -a data is published for failed suites as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/tests.yml:
- Around line 42-51: The "Produce coverage report" and "Upload results to
Codecov" steps are currently running only on success and get skipped when a test
suite fails; update those steps (the steps named "Produce coverage report" and
"Upload results to Codecov") to use if: always() so coverage xml and the Codecov
upload run even after failures, ensuring the combine-with-coverage run -a data
is published for failed suites as well.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ea7754ee-952d-4c2d-a897-337dd8ba8c8c
📒 Files selected for processing (1)
.github/workflows/tests.yml
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
1 participant
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.
Summary
Closes #177
Server testing previously ran as a single undifferentiated block, making it hard to tell whether a failure was an internal logic regression or a public API contract breakage.
This PR splits the
Run testsstep in the CI workflow into two explicit steps:tests/database/and tests/config_test.py to validate database logic and configuration internals.tests/routers/to validate HTTP response contracts, usingcoverage run -a(append mode) so combined coverage is preserved correctly.Changes
Run testsstep with two dedicatedRun internal testsandRun API testssteps.Testing
No new test logic is added — only the CI execution is reorganised. Existing tests cover all assertions.
docker compose upsetup.