Migrate POST /setup/untag endpoint (#65)#246
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds POST /setup/untag and supporting code: new async DB helpers in Possibly related PRs
Suggested labels
🚥 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)
📝 Coding Plan for PR comments
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 |
|
@PGijsbers Gentle reminder on this PR would appreciate a review when you have time. Thanks! |
PGijsbers
left a comment
PGijsbers
left a comment
There was a problem hiding this comment.
Hi! Thanks for taking the time to contribute! I am sorry but I am only able to give half a review right now. I have been out sick last week, and I noticed some issues testing this locally (which is unrelated to the PR itself). I will come back to this on Monday. In principle it looks like a good start though.
Please also add tests under tests/routers/openml/setup_test.py which tests the functionality of the API on its own. These tests should cover all scenarios so that after the migration is complete and we no longer run migration tests, we still have good coverage of the REST API. They are also useful to run a quicker set of tests (since testing with the PHP API is slow).
src/routers/openml/setups.py
Outdated
| def create_authentication_failed_error() -> HTTPException: | ||
| return HTTPException( | ||
| status_code=HTTPStatus.PRECONDITION_FAILED, | ||
| detail={"code": "103", "message": "Authentication failed"}, | ||
| ) |
There was a problem hiding this comment.
This function already exists in src/routers/openml/datasets.py. I would suggest making this a common functionality by creating an extra function in src/routers/dependencies.py called fetch_user_or_raise, which executes the current fetch_user code but raises this error if no user is found.
Remember to update the type hints accordingly and also use this new function in src/routers/openml/datasets.py.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/routers/openml/setups_test.py (1)
70-76: Optional: fold success-path tag setup into a fixture for consistency.This block duplicates the same arrange/commit pattern already used in
mock_setup_tag. Moving it to a dedicated fixture would reduce repetition and keep setup/teardown behavior uniform.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/routers/openml/setups_test.py` around lines 70 - 76, The test_setup_untag_success function duplicates setup DB operations that are already in the mock_setup_tag fixture; refactor by creating or reusing a fixture (e.g., mock_setup_tag or a new fixture named setup_success_tag) that performs the DELETE/INSERT/commit of the 'test_success_tag' record, then update test_setup_untag_success to accept that fixture instead of performing the DB arrange inline; ensure the fixture handles cleanup/commit so the test only exercises the untag logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/routers/openml/setups_test.py`:
- Around line 70-76: The test_setup_untag_success function duplicates setup DB
operations that are already in the mock_setup_tag fixture; refactor by creating
or reusing a fixture (e.g., mock_setup_tag or a new fixture named
setup_success_tag) that performs the DELETE/INSERT/commit of the
'test_success_tag' record, then update test_setup_untag_success to accept that
fixture instead of performing the DB arrange inline; ensure the fixture handles
cleanup/commit so the test only exercises the untag logic.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/routers/dependencies.pysrc/routers/openml/datasets.pysrc/routers/openml/setups.pytests/routers/openml/setups_test.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/routers/openml/setups.py
Hi @PGijsbers , |
|
Hi @PGijsbers, |
for more information, see https://pre-commit.ci
|
Thanks! Ended up having issues back-to-back, but mostly fine now. I am updating the PR to incorporate recent merges (async usage and RFC 9457), hope to wrap things up tomorrow! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #246 +/- ##
=======================================
Coverage ? 54.73%
=======================================
Files ? 34
Lines ? 1447
Branches ? 116
=======================================
Hits ? 792
Misses ? 653
Partials ? 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/routers/openml/setups_test.py (1)
66-94: Consider extracting setup/teardown to a fixture for robustness.The test correctly covers both owner and admin success paths. However, if the test fails between INSERT (line 79) and the successful API call, the tag persists until the next test run. While the leading DELETE mitigates this, consider using a fixture similar to
mock_setup_tagfor consistent cleanup.That said, since the DELETE-first pattern ensures test isolation across runs, this is a minor concern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/routers/openml/setups_test.py` around lines 66 - 94, The test_setup_untag_success leaves DB state fragile between the INSERT and assertions; extract the INSERT/DELETE into a reusable fixture (e.g., mock_setup_tag) that uses expdb_test.execute with the same text(...) SQL to insert the (id=1, tag='test_success_tag', uploader=2) before the test and always removes it after (teardown/fixture finalizer), then replace the inline INSERT/DELETE in test_setup_untag_success by accepting that fixture (add mock_setup_tag to the test signature) so setup and cleanup are guaranteed even if the test fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/routers/openml/setups_test.py`:
- Around line 66-94: The test_setup_untag_success leaves DB state fragile
between the INSERT and assertions; extract the INSERT/DELETE into a reusable
fixture (e.g., mock_setup_tag) that uses expdb_test.execute with the same
text(...) SQL to insert the (id=1, tag='test_success_tag', uploader=2) before
the test and always removes it after (teardown/fixture finalizer), then replace
the inline INSERT/DELETE in test_setup_untag_success by accepting that fixture
(add mock_setup_tag to the test signature) so setup and cleanup are guaranteed
even if the test fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 31e7d66f-de99-4c31-9b29-e046867efd25
📒 Files selected for processing (6)
src/database/setups.pysrc/routers/dependencies.pysrc/routers/openml/setups.pytests/routers/openml/dataset_tag_test.pytests/routers/openml/migration/setups_migration_test.pytests/routers/openml/setups_test.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/routers/openml/migration/setups_migration_test.py
- src/routers/openml/setups.py
- src/database/setups.py
Pulled the latest changes tests pass on my end! Let me know if there's anything you need from my side to wrap it up. 🙌 |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/setups.py`:
- Around line 29-42: The handler currently fetches tags with
database.setups.get_tags and then calls database.setups.untag, which permits a
race where the tag is deleted between those calls; update the fix inside
database.setups.untag to enforce rowcount == 1 and raise a specific error (e.g.,
TagNotFoundError) when 0 rows are deleted, or refactor to a single atomic
operation that both checks uploader ownership and deletes in one SQL statement;
adjust callers (the code using get_tags, matched_tag_row and awaiting
database.setups.untag) to rely on the updated untag behavior (remove the
separate existence-only check if you collapse into one DB call) so the handler
only proceeds when untag confirms exactly one row was removed.
In `@tests/routers/openml/migration/setups_migration_test.py`:
- Around line 67-71: The test reads the legacy PHP error payload using dict
order via original.json()["error"].values(); change it to explicitly extract the
keys so the assertion is deterministic: get code =
original.json()["error"]["code"] and message =
original.json()["error"]["message"] (then keep the existing assertions comparing
code to new.json()["code"] and message to the expected string). Update the
assertions in tests/routers/openml/migration/setups_migration_test.py that
reference original and new accordingly.
- Around line 28-42: temporary_tag currently calls await expdb_test.commit(),
which defeats the automatic_rollback() fixture; instead perform the staged
INSERT/DELETE on a separate connection that you explicitly commit so expdb_test
remains in its uncommitted transaction for the py_api phase. Concretely: inside
temporary_tag use a new/explicit connection object (e.g., acquire a fresh async
connection from the test DB engine or pool) to execute the INSERT and commit,
yield, then use that same separate connection to DELETE and commit; leave
expdb_test untouched so automatic_rollback() can roll back the in-process state.
Ensure the functions/variables referenced are temporary_tag and expdb_test and
that no commit() is called on expdb_test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 40ad6c3c-6c75-48a0-9d6b-428167ab0c2c
📒 Files selected for processing (3)
src/core/errors.pysrc/routers/openml/setups.pytests/routers/openml/migration/setups_migration_test.py
Thanks for the offer! I ended up finishing the PR because there were a few too many things I couldn't foresee since it had been a while since I worked on this code base (only now time is starting to free up again). A few notes on what I had to update (besides RFC9457/async which was added after you started):
If you're interested in continuing to help, please let me know! |
Hello @PGijsbers I understand now why the automatic rollback on the I’d love to keep contributing! Are there any issues or endpoints you’d recommend I work on next? |
2 participants
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