Migrate POST /setup/untag endpoint (#65)#246
Conversation
WalkthroughThis pull request implements the POST /setup/untag endpoint for the OpenML API. It adds three database utility functions in 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The error responses for codes 472/475/476 are being constructed inline in
untag_setup; consider extracting small helper functions similar tocreate_authentication_failed_error()so the legacy error shapes are centralized and easier to keep consistent across future migrated endpoints. - In
untag_setup, the dependency-injected parameters (user,expdb_db) are annotated with| Noneand given= Nonedefaults even thoughDepends(...)guarantees they are present; tightening these types (and dropping theNonedefaults) will make the function signature clearer and help static checkers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The error responses for codes 472/475/476 are being constructed inline in `untag_setup`; consider extracting small helper functions similar to `create_authentication_failed_error()` so the legacy error shapes are centralized and easier to keep consistent across future migrated endpoints.
- In `untag_setup`, the dependency-injected parameters (`user`, `expdb_db`) are annotated with `| None` and given `= None` defaults even though `Depends(...)` guarantees they are present; tightening these types (and dropping the `None` defaults) will make the function signature clearer and help static checkers.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/routers/openml/migration/setups_migration_test.py (1)
31-35: Silent pre-tag failure can mask test-ordering issues.The pre-tag result is discarded. If
setup_taghas aUNIQUE(id, tag)constraint and a previous test run left the tag in place (e.g., after a crash mid-test), the pre-tag silently fails but the leftover tag satisfies the next test — acceptable in practice, but noting it here for awareness.Consider asserting the pre-tag succeeds to make state prerequisites explicit:
- php_api.post( - "/setup/tag", - data={"api_key": ApiKey.SOME_USER, "tag": tag, "setup_id": setup_id}, - ) + pre_tag = php_api.post( + "/setup/tag", + data={"api_key": ApiKey.SOME_USER, "tag": tag, "setup_id": setup_id}, + ) + assert pre_tag.status_code in (HTTPStatus.OK, HTTPStatus.PRECONDITION_FAILED)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/routers/openml/migration/setups_migration_test.py` around lines 31 - 35, The pre-tag POST call to "/setup/tag" (invoked when setup_id == 1 via php_api.post with ApiKey.SOME_USER, tag, and setup_id) currently discards the response so a silent failure can hide leftover state; update the test to capture the response from php_api.post("/setup/tag", ...) and assert it indicates success (e.g., assert status code is 200 or response.json()["ok"] is True) so the precondition is explicitly verified before continuing.src/routers/openml/setups.py (1)
27-27:expdb_dbtype annotation isConnection, notConnection | None— minor annotation mismatch with= Nonedefault.FastAPI always injects the dependency so
Noneis never used at runtime, but static type checkers will flag this.🔧 Suggested fix
- expdb_db: Annotated[Connection, Depends(expdb_connection)] = None, + expdb_db: Annotated[Connection, Depends(expdb_connection)],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routers/openml/setups.py` at line 27, The parameter expdb_db in the function signature is annotated as Connection but given a default of None, which mismatches static typing; update the signature for the parameter expdb_db to use an optional type (e.g., Connection | None or Optional[Connection]) and add the corresponding import if needed, or remove the "= None" default and rely on FastAPI's dependency injection; refer to the expdb_db parameter and expdb_connection dependency in the setup function to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/routers/openml/migration/setups_migration_test.py`:
- Around line 15-19: The parameterized test for "api_key" omits invalid/missing
keys so the router branch that triggers create_authentication_failed_error
(error code 103) is never exercised; update the pytest.mark.parametrize list for
"api_key" to include ApiKey.INVALID (and/or None) and add corresponding id(s)
(e.g., "invalid key" or "no key") so the test runs the authentication-failure
path and asserts the 103 response in the test that uses ApiKey.ADMIN /
ApiKey.SOME_USER / ApiKey.OWNER_USER currently.
---
Nitpick comments:
In `@src/routers/openml/setups.py`:
- Line 27: The parameter expdb_db in the function signature is annotated as
Connection but given a default of None, which mismatches static typing; update
the signature for the parameter expdb_db to use an optional type (e.g.,
Connection | None or Optional[Connection]) and add the corresponding import if
needed, or remove the "= None" default and rely on FastAPI's dependency
injection; refer to the expdb_db parameter and expdb_connection dependency in
the setup function to locate the change.
In `@tests/routers/openml/migration/setups_migration_test.py`:
- Around line 31-35: The pre-tag POST call to "/setup/tag" (invoked when
setup_id == 1 via php_api.post with ApiKey.SOME_USER, tag, and setup_id)
currently discards the response so a silent failure can hide leftover state;
update the test to capture the response from php_api.post("/setup/tag", ...) and
assert it indicates success (e.g., assert status code is 200 or
response.json()["ok"] is True) so the precondition is explicitly verified before
continuing.
| @pytest.mark.parametrize( | ||
| "api_key", | ||
| [ApiKey.ADMIN, ApiKey.SOME_USER, ApiKey.OWNER_USER], | ||
| ids=["Administrator", "regular user", "possible owner"], | ||
| ) |
There was a problem hiding this comment.
Authentication failure (error code 103) is not covered by migration tests.
ApiKey.INVALID (or an absent key) is excluded from the api_key parametrize list, so the create_authentication_failed_error path in the router is never exercised here. Consider adding ApiKey.INVALID or None as a case to validate parity for the 103 response.
✅ Suggested addition
`@pytest.mark.parametrize`(
"api_key",
- [ApiKey.ADMIN, ApiKey.SOME_USER, ApiKey.OWNER_USER],
- ids=["Administrator", "regular user", "possible owner"],
+ [ApiKey.ADMIN, ApiKey.SOME_USER, ApiKey.OWNER_USER, ApiKey.INVALID],
+ ids=["Administrator", "regular user", "possible owner", "invalid key"],
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @pytest.mark.parametrize( | |
| "api_key", | |
| [ApiKey.ADMIN, ApiKey.SOME_USER, ApiKey.OWNER_USER], | |
| ids=["Administrator", "regular user", "possible owner"], | |
| ) | |
| `@pytest.mark.parametrize`( | |
| "api_key", | |
| [ApiKey.ADMIN, ApiKey.SOME_USER, ApiKey.OWNER_USER, ApiKey.INVALID], | |
| ids=["Administrator", "regular user", "possible owner", "invalid key"], | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/routers/openml/migration/setups_migration_test.py` around lines 15 -
19, The parameterized test for "api_key" omits invalid/missing keys so the
router branch that triggers create_authentication_failed_error (error code 103)
is never exercised; update the pytest.mark.parametrize list for "api_key" to
include ApiKey.INVALID (and/or None) and add corresponding id(s) (e.g., "invalid
key" or "no key") so the test runs the authentication-failure path and asserts
the 103 response in the test that uses ApiKey.ADMIN / ApiKey.SOME_USER /
ApiKey.OWNER_USER currently.
There was a problem hiding this comment.
I intentionally omitted ApiKey.INVALID from the migration test parameters because the legacy PHP API has a known bug where it returns raw XML instead of JSON when authentication fails for setup queries (Code 103). Because our migration test framework strictly relies on original.json() == new.json() to prove parity, testing the invalid key pathway directly crashes Pytest with a JSONDecodeError.
- We will be able to test the 103 Authentication failure fully and properly in standard Python unit tests once
POST /setup/tagis migrated natively in my follow up PR and we no longer have to rely on php_api to stage the data.
There was a problem hiding this comment.
@igennova, thank you for the detailed explanation! That makes perfect sense—if the legacy PHP API returns raw XML for Code 103 errors while the new API returns JSON, the migration test would indeed fail at the original.json() deserialization step before any comparison could happen.
Your plan to test authentication failure properly in standard unit tests once POST /setup/tag is migrated is the right approach. At that point, you won't need php_api to stage the data, and you'll be able to validate the 103 error path cleanly.
One optional suggestion: consider adding a brief comment in the test file or a TODO note explaining why ApiKey.INVALID is excluded from the parameterization, so future maintainers understand the constraint.
✏️ Learnings added
Learnt from: igennova
Repo: openml/server-api PR: 246
File: tests/routers/openml/migration/setups_migration_test.py:15-19
Timestamp: 2026-02-20T21:20:18.622Z
Learning: Migration tests for OpenML API endpoints that rely on php_api for data staging cannot test authentication failure paths (Code 103) when the legacy PHP API returns XML instead of JSON. These paths should be tested in standard Python unit tests after dependent endpoints are migrated.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
@PGijsbers The PR is ready for review. Thank you |
1 participant
Fixes #65
This PR migrates the
POST /setup/untagendpoint from the legacy PHP API to the new FastAPI backend. It is the first endpoint migrated under the Setup Epic (#60).Changes:
database.setupswith [untag()] helper.POST /untagrouting logic.API Response