Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds a POST /setup/tag endpoint, a new async database function tag() to insert into setup_tag, and updates router logic to validate existence, enforce case-insensitive duplicate detection (raising TagAlreadyExistsError), and return tags as a list (casefolded). Adjusts untag comparisons to be case-insensitive. Adds documentation and tests plus test fixtures (temporary_records/temporary_tags) to cover tag/untag, auth, nonexistent setups, and duplicate-tag cases. Possibly related PRs
🚥 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
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}
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
tag_setupflow does a read (get_tags) followed by a write (tag) to detect duplicates, which can still allow a concurrent duplicate insert; if this matters, consider enforcing uniqueness at the DB level (e.g. unique(id, LOWER(tag))index) and mapping constraint violations toTagAlreadyExistsErrorinstead of relying purely on the pre-check. - In
tag_setup, tags are case-folded before being returned (casefold()), which may change the casing of existing tags; please confirm this matches the legacy API behavior and, if not, consider preserving original stored casing while still doing case-insensitive comparison for duplicate detection.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `tag_setup` flow does a read (`get_tags`) followed by a write (`tag`) to detect duplicates, which can still allow a concurrent duplicate insert; if this matters, consider enforcing uniqueness at the DB level (e.g. unique `(id, LOWER(tag))` index) and mapping constraint violations to `TagAlreadyExistsError` instead of relying purely on the pre-check.
- In `tag_setup`, tags are case-folded before being returned (`casefold()`), which may change the casing of existing tags; please confirm this matches the legacy API behavior and, if not, consider preserving original stored casing while still doing case-insensitive comparison for duplicate detection.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #271 +/- ##
=======================================
Coverage ? 54.07%
=======================================
Files ? 34
Lines ? 1474
Branches ? 122
=======================================
Hits ? 797
Misses ? 675
Partials ? 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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: 1
🧹 Nitpick comments (3)
tests/routers/openml/setups_test.py (1)
105-117: Add a case-insensitive duplicate test variant.The endpoint blocks duplicates via
casefold(), but this test only validates exact-case duplicates. Please add a mixed-case scenario (e.g., storedExisting_Tag, requestexisting_tag) to lock in the intended behavior.Coverage extension example
`@pytest.mark.mut` +@pytest.mark.parametrize( + ("stored_tag", "requested_tag"), + [("existing_tag_123", "existing_tag_123"), ("Existing_Tag_123", "existing_tag_123")], + ids=["exact-match", "case-insensitive-match"], +) async def test_setup_tag_already_exists( - py_api: httpx.AsyncClient, expdb_test: AsyncConnection + py_api: httpx.AsyncClient, expdb_test: AsyncConnection, stored_tag: str, requested_tag: str ) -> None: await expdb_test.execute( - text("INSERT INTO setup_tag (id, tag, uploader) VALUES (1, 'existing_tag_123', 2);") + text("INSERT INTO setup_tag (id, tag, uploader) VALUES (1, :tag, 2);"), + parameters={"tag": stored_tag}, ) response = await py_api.post( f"/setup/tag?api_key={ApiKey.SOME_USER}", - json={"setup_id": 1, "tag": "existing_tag_123"}, + json={"setup_id": 1, "tag": requested_tag}, )🤖 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 105 - 117, Add a new async test alongside test_setup_tag_already_exists that verifies case-insensitive duplicate detection: insert a setup_tag row with a mixed-case tag (e.g., 'Existing_Tag') for setup id 1 using expdb_test.execute on the setup_tag table, then POST to /setup/tag with json {"setup_id": 1, "tag": "existing_tag"} and api_key=ApiKey.SOME_USER, and assert HTTPStatus.CONFLICT and that response.json()["detail"] matches the existing-tag message for setup 1; mirror the original test's assertions and use the same helpers (py_api, expdb_test) and identifiers so the test ensures casefold-based duplicate blocking in test_setup_tag_already_exists_variant (or similar name).tests/routers/openml/migration/setups_migration_test.py (2)
224-228: Use multiset comparison instead ofset(...)for tag parity.
set(tags)can hide duplicate-tag regressions. PreferCounter(...)so parity checks keep multiplicity.Small assertion hardening
+from collections import Counter @@ - assert set(tags) == set(new_tag["tag"]) + assert Counter(tags) == Counter(new_tag["tag"])🤖 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 224 - 228, Replace the unordered set comparison with a multiset comparison so duplicate tags are checked: when comparing original_tag (variable original_tag) to new_tag["tag"] in the test, keep the existing branch for string tags but for the sequence branch replace assert set(tags) == set(new_tag["tag"]) with a Counter-based equality (use collections.Counter) to compare multiplicities; add the necessary import (from collections import Counter) at the top of the test file.
172-194: Extracttemporary_tagsto a shared helper in this module.This helper duplicates logic already present earlier in the file. Consolidating it once will reduce drift across migration tests.
🤖 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 172 - 194, The temporary_tags async context manager is duplicated; extract it once as a shared helper function named temporary_tags in this module (keeping the signature temporary_tags(tags: Iterable[str], setup_id: int, *, persist: bool = False) -> AsyncGenerator[None]) and replace the other duplicate definitions/copies with calls to that single helper; ensure the implementation preserves its INSERT/DELETE SQL, parameters using OWNER_USER.user_id, commit behavior when persist is True, and that all tests import/use the consolidated temporary_tags symbol instead of defining their own local versions.
🤖 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 34-42: The current check-then-insert for tags can race; wrap the
await database.setups.tag(setup_id, tag, user.user_id, expdb_db) call in a
try/except that catches IntegrityError (import from sqlalchemy.exc) and raises
TagAlreadyExistsError with the same message used for the pre-check (e.g.,
f"Setup {setup_id} already has tag {tag!r}."); keep the existing pre-check for
fast-fail but treat the DB uniqueness violation as the source of truth, and only
append tag.casefold() to all_tags after a successful insert (or re-fetch tags if
you prefer) to avoid stale state.
---
Nitpick comments:
In `@tests/routers/openml/migration/setups_migration_test.py`:
- Around line 224-228: Replace the unordered set comparison with a multiset
comparison so duplicate tags are checked: when comparing original_tag (variable
original_tag) to new_tag["tag"] in the test, keep the existing branch for string
tags but for the sequence branch replace assert set(tags) == set(new_tag["tag"])
with a Counter-based equality (use collections.Counter) to compare
multiplicities; add the necessary import (from collections import Counter) at
the top of the test file.
- Around line 172-194: The temporary_tags async context manager is duplicated;
extract it once as a shared helper function named temporary_tags in this module
(keeping the signature temporary_tags(tags: Iterable[str], setup_id: int, *,
persist: bool = False) -> AsyncGenerator[None]) and replace the other duplicate
definitions/copies with calls to that single helper; ensure the implementation
preserves its INSERT/DELETE SQL, parameters using OWNER_USER.user_id, commit
behavior when persist is True, and that all tests import/use the consolidated
temporary_tags symbol instead of defining their own local versions.
In `@tests/routers/openml/setups_test.py`:
- Around line 105-117: Add a new async test alongside
test_setup_tag_already_exists that verifies case-insensitive duplicate
detection: insert a setup_tag row with a mixed-case tag (e.g., 'Existing_Tag')
for setup id 1 using expdb_test.execute on the setup_tag table, then POST to
/setup/tag with json {"setup_id": 1, "tag": "existing_tag"} and
api_key=ApiKey.SOME_USER, and assert HTTPStatus.CONFLICT and that
response.json()["detail"] matches the existing-tag message for setup 1; mirror
the original test's assertions and use the same helpers (py_api, expdb_test) and
identifiers so the test ensures casefold-based duplicate blocking in
test_setup_tag_already_exists_variant (or similar name).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a2a06757-f7cc-482c-a567-80f29d6e3d3f
📒 Files selected for processing (5)
docs/migration.mdsrc/database/setups.pysrc/routers/openml/setups.pytests/routers/openml/migration/setups_migration_test.pytests/routers/openml/setups_test.py
Contributor
Author
|
@PGijsbers The PR is ready for review. Thank you |
PGijsbers
requested changes
Mar 13, 2026
Contributor
There was a problem hiding this comment.
Some minor changes requests with regards to code structure, but the functional bits look good to me! Also, this PR accidentally got to the top of my pile, but in principle I am trying to work through the PRs in chronological order, so it may take a little bit of time for me to catch back up.
Comment on lines
+172
to
+193
| @contextlib.asynccontextmanager | ||
| async def temporary_tags( | ||
| tags: Iterable[str], setup_id: int, *, persist: bool = False | ||
| ) -> AsyncGenerator[None]: | ||
| for tag in tags: | ||
| await expdb_test.execute( | ||
| text( | ||
| "INSERT INTO setup_tag(`id`,`tag`,`uploader`) " | ||
| "VALUES (:setup_id, :tag, :user_id);" | ||
| ), | ||
| parameters={"setup_id": setup_id, "tag": tag, "user_id": OWNER_USER.user_id}, | ||
| ) | ||
| if persist: | ||
| await expdb_test.commit() | ||
| yield | ||
| for tag in tags: | ||
| await expdb_test.execute( | ||
| text("DELETE FROM setup_tag WHERE `id`=:setup_id AND `tag`=:tag"), | ||
| parameters={"setup_id": setup_id, "tag": tag}, | ||
| ) | ||
| if persist: | ||
| await expdb_test.commit() |
Contributor
There was a problem hiding this comment.
We should extract this as a module level fixture to avoid repeating it in multiple tests. Then you can also use it for the other test where you currently have a try/finally construction.
Contributor
There was a problem hiding this comment.
We probably want to have something even above module level that takes the queries as parameters, and then use that to define module level reusable context managers.
Contributor
Author
There was a problem hiding this comment.
Great idea, I've extracted it to a generic context manager in conftest.py and refactored the tests to use a single fixture here, including replacing that try/finally
PGijsbers
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/routers/openml/migration/setups_migration_test.py (1)
22-37: Materializetagsonce before building both query lists.
tagsis typed asIterable[str]; a single-pass iterator would be exhausted by the first comprehension, leaving the second empty. All current call sites pass lists, but the type signature allows any iterable.💡 Suggested fix
async def _temporary_tags( tags: Iterable[str], setup_id: int, *, persist: bool = False ) -> AsyncGenerator[None]: + tags = tuple(tags) insert_queries = [ ( "INSERT INTO setup_tag(`id`,`tag`,`uploader`) VALUES (:setup_id, :tag, :user_id);", {"setup_id": setup_id, "tag": tag, "user_id": OWNER_USER.user_id}, ) for tag in tags ] delete_queries = [ ( "DELETE FROM setup_tag WHERE `id`=:setup_id AND `tag`=:tag", {"setup_id": setup_id, "tag": tag}, ) for tag in tags ]🤖 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 22 - 37, Materialize the iterable `tags` into a concrete sequence before building the two comprehensions so a single-pass iterator isn't consumed by `insert_queries` and left empty for `delete_queries`; e.g. assign `tags_list = list(tags)` and use `tags_list` when constructing both `insert_queries` and `delete_queries` (references: `tags`, `insert_queries`, `delete_queries`, `setup_id`, `OWNER_USER`).
🤖 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/conftest.py`:
- Around line 31-50: The async context manager temporary_records should protect
the cleanup steps with a try/finally: execute insert_queries and commit (if
persist) before yielding, then use try: yield finally: run the delete_queries
and commit (if persist) so cleanup always runs even if the body raises; update
the temporary_records function to wrap the yield in a try/finally and move the
delete_queries/commit into the finally block, referencing the existing
parameters connection, insert_queries, delete_queries, and persist.
---
Nitpick comments:
In `@tests/routers/openml/migration/setups_migration_test.py`:
- Around line 22-37: Materialize the iterable `tags` into a concrete sequence
before building the two comprehensions so a single-pass iterator isn't consumed
by `insert_queries` and left empty for `delete_queries`; e.g. assign `tags_list
= list(tags)` and use `tags_list` when constructing both `insert_queries` and
`delete_queries` (references: `tags`, `insert_queries`, `delete_queries`,
`setup_id`, `OWNER_USER`).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6e17f23a-3ba3-4802-ac15-6639ca3e9e2b
📒 Files selected for processing (3)
src/routers/openml/setups.pytests/conftest.pytests/routers/openml/migration/setups_migration_test.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/routers/openml/setups.py
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
2 participants
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.
Description
Fixes: #64
py_apivsphp_api migrationparity tests.docs/migration.mdto document the new tag array response behavior.Checklist
Always:
Required for code changes:
If applicable:
/docs)Extra context:
Screenshot