Feat: Implement GET /setup/{id} endpoint#280
Open
igennova wants to merge 2 commits intoopenml:mainfrom
Open
Conversation
Contributor
WalkthroughAdds database function 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 found 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/database/setups.py" line_range="38-42" />
<code_context>
+ t_input.dataType AS data_type,
+ t_input.defaultValue AS default_value,
+ t_setting.value AS value
+ FROM input_setting t_setting
+ JOIN input t_input ON t_setting.input_id = t_input.id
+ JOIN implementation t_impl ON t_input.implementation_id = t_impl.id
</code_context>
<issue_to_address>
**suggestion:** Consider adding an explicit ORDER BY clause to make parameter ordering deterministic.
Without an `ORDER BY`, the database may return parameters in any order, which can break callers that assume stable ordering (e.g., for diffs, cache keys, or UI). Please add an explicit sort, e.g. `ORDER BY t_impl.id, t_input.id`, to make the result deterministic.
```suggestion
FROM input_setting t_setting
JOIN input t_input ON t_setting.input_id = t_input.id
JOIN implementation t_impl ON t_input.implementation_id = t_impl.id
WHERE t_setting.setup = :setup_id
ORDER BY t_impl.id, t_input.id
""",
```
</issue_to_address>
### Comment 2
<location path="src/schemas/setups.py" line_range="17" />
<code_context>
+ name: str
+ data_type: str | None = None
+ default_value: str | None = None
+ value: str
+
+ model_config = ConfigDict(from_attributes=True)
</code_context>
<issue_to_address>
**issue:** Check whether `value` should be nullable to match the database and avoid validation issues.
Since `t_setting.value` is selected directly and may be NULL at the DB level, `value: str` will cause Pydantic validation failures for such rows. If NULL is valid, use `value: str | None = None` or normalize NULL to a non-null value (e.g., empty string) before model validation to avoid unexpected 500/422 responses.
</issue_to_address>
### Comment 3
<location path="src/routers/openml/setups.py" line_range="36" />
<code_context>
+
+ setup_parameters = await database.setups.get_parameters(setup_id, expdb_db)
+
+ params_model = SetupParameters(
+ setup_id=str(setup_id),
+ flow_id=str(setup.implementation_id),
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the new endpoint by always returning a list for `parameter` and letting Pydantic handle object conversion instead of manually building dicts.
You can simplify the new endpoint a bit without changing behavior.
1. **Always return a list for `parameter`**
Avoid the `None` vs `[]` duality and the extra conditional. This keeps the schema simpler for clients and reduces branching:
```python
params_model = SetupParameters(
setup_id=str(setup_id),
flow_id=str(setup.implementation_id),
parameter=[dict(param) for param in setup_parameters],
)
```
If `setup_parameters` is empty, `parameter` will just be `[]`.
2. **Avoid the dict conversion if possible**
Since you already have Pydantic models and likely `from_attributes=True` on `SetupParameter`, you can skip the `dict` round-trip and let Pydantic do the adaptation. For example, if `get_parameters` returns SQLAlchemy rows, you can validate them directly:
```python
from schemas.setups import SetupParameter
params_model = SetupParameters(
setup_id=str(setup_id),
flow_id=str(setup.implementation_id),
parameter=[
SetupParameter.model_validate(param, from_attributes=True)
for param in setup_parameters
],
)
```
If this doesn’t work with the current return type of `get_parameters`, consider updating `get_parameters` to return objects with attribute access (`.name`, `.value`, etc.) instead of dict-like mappings, so Pydantic can map them directly.
</issue_to_address>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 #280 +/- ##
=======================================
Coverage ? 54.00%
=======================================
Files ? 35
Lines ? 1509
Branches ? 125
=======================================
Hits ? 815
Misses ? 692
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.
🧹 Nitpick comments (2)
src/database/setups.py (1)
28-42: Stabilize parameter ordering in the SQL result.At Line 41, the query filters but does not sort. Without
ORDER BY, parameter list order is not guaranteed and can become flaky for response comparisons.Suggested diff
FROM input_setting t_setting JOIN input t_input ON t_setting.input_id = t_input.id JOIN implementation t_impl ON t_input.implementation_id = t_impl.id WHERE t_setting.setup = :setup_id + ORDER BY t_input.id """,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/database/setups.py` around lines 28 - 42, The SQL query selecting inputs/settings (the SELECT joining input_setting, input, implementation and filtering by :setup_id) lacks a deterministic ORDER BY causing flaky parameter ordering; update the query to append an ORDER BY clause (for example ORDER BY t_input.implementation_id, t_input.name or ORDER BY t_impl.name, t_input.name) so results are consistently sorted by flow and parameter name (refer to the SELECT block, tables input_setting/input/implementation, and the :setup_id filter).tests/routers/openml/setups_test.py (1)
141-146: Strengthen the success-path assertions to lock the response contract.Right now the test only checks
setup_idand key presence. Adding a few schema-level assertions would catch more regressions.Suggested diff
async def test_get_setup_success(py_api: httpx.AsyncClient) -> None: response = await py_api.get("/setup/1") assert response.status_code == HTTPStatus.OK data = response.json()["setup_parameters"] assert data["setup_id"] == "1" - assert "parameter" in data + assert "flow_id" in data + assert isinstance(data.get("parameter"), list) + assert data["parameter"], "Expected setup 1 to contain at least one parameter" + first = data["parameter"][0] + assert {"id", "flow_id", "flow_name", "full_name", "parameter_name", "name", "value"} <= set(first)🤖 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 141 - 146, The test_get_setup_success should lock the response contract: assert that the top-level response is a dict and contains only the expected keys for the setup payload (verify set(data.keys()) == {'setup_id', 'parameter'}), assert type(data['setup_id']) is str and equals "1", assert that data['parameter'] is the expected container type (dict or list) and is non-empty, and if it's a list assert each item is a dict with at least 'name' and 'value' keys (if it's a dict assert it contains 'name' and 'value'); update the test_get_setup_success assertions accordingly to enforce these schema-level checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/database/setups.py`:
- Around line 28-42: The SQL query selecting inputs/settings (the SELECT joining
input_setting, input, implementation and filtering by :setup_id) lacks a
deterministic ORDER BY causing flaky parameter ordering; update the query to
append an ORDER BY clause (for example ORDER BY t_input.implementation_id,
t_input.name or ORDER BY t_impl.name, t_input.name) so results are consistently
sorted by flow and parameter name (refer to the SELECT block, tables
input_setting/input/implementation, and the :setup_id filter).
In `@tests/routers/openml/setups_test.py`:
- Around line 141-146: The test_get_setup_success should lock the response
contract: assert that the top-level response is a dict and contains only the
expected keys for the setup payload (verify set(data.keys()) == {'setup_id',
'parameter'}), assert type(data['setup_id']) is str and equals "1", assert that
data['parameter'] is the expected container type (dict or list) and is
non-empty, and if it's a list assert each item is a dict with at least 'name'
and 'value' keys (if it's a dict assert it contains 'name' and 'value'); update
the test_get_setup_success assertions accordingly to enforce these schema-level
checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 33a4cdd7-cfcd-4b24-9a38-8284b29f797c
📒 Files selected for processing (5)
src/database/setups.pysrc/routers/openml/setups.pysrc/schemas/setups.pytests/routers/openml/migration/setups_migration_test.pytests/routers/openml/setups_test.py
Contributor
Author
|
@PGijsbers I intentionally built explicit BaseModel schemas under The PR is ready for review. Thank you |
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.
FIxes: #61
Description
(SetupResponseand SetupParameter)undersrc/schemas/setups.pystrictly representing the returned topology.Checklist
Always:
Required for code changes:
If applicable:
/docs)Extra context:
Screenshot:
