feat: add hstore column filter support for PostgreSQL#152
feat: add hstore column filter support for PostgreSQL#152Ckk3 wants to merge 3 commits intogazorby:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds PostgreSQL HSTORE support across the library: new HStore scalar, parsing/serialization, filter types and comparison inputs, SQLAlchemy inspector handling for HSTORE fields, tests (unit and integration), test models/fixtures, and README documentation (HStore Filters section, duplicated). Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 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)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
|
@gazorby I saw that we have some problems with the lint CI, I will create a new PR to fix it 😉 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #152 +/- ##
==========================================
+ Coverage 94.28% 94.32% +0.03%
==========================================
Files 69 69
Lines 5814 5854 +40
Branches 770 777 +7
==========================================
+ Hits 5482 5522 +40
Misses 190 190
Partials 142 142 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Line 810: The wording for the README bullet describing hasKeyAll is awkward;
update the text for the **hasKeyAll** bullet to read "have all the given keys"
instead of "have all of the given keys" so it matches nearby bullets and reads
more clearly.
In `@src/strawchemy/repository/sqlalchemy/_sync.py`:
- Line 11: Regenerate src/strawchemy/repository/sqlalchemy/_sync.py from its
source _async.py so the import order matches the original: ensure the "from
sqlalchemy import ..." line appears before the "from sqlalchemy.orm import ..."
line and that all imported symbols (e.g., ColumnElement, Row, and_, delete,
inspect, select, update and any ORM imports) and formatting exactly mirror the
_async.py source; replace the current reversed imports with the corrected
ordering from _async.py and run the generator/formatter to keep the file
consistent.
In `@src/strawchemy/schema/scalars/base.py`:
- Around line 67-68: The _parse_hstore function currently returns the original
non-dict input which violates its contract; update _parse_hstore to validate the
input and raise a clear error when value is not a dict (e.g., raise TypeError or
ValueError), and otherwise perform the existing dict[str,str] conversion
(function _parse_hstore should only accept dict inputs and reject others rather
than returning them unchanged).
In `@tests/integration/data_types/test_hstore.py`:
- Around line 95-96: The test currently asserts row-by-row equality using an
index-dependent loop (expected_ids, result.data["hstore"][i]["id"]) which makes
it order-dependent and flaky; change it to assert unordered equivalence by
collecting the returned ids (from result.data["hstore"]) and comparing as a set
(or multisets) against the expected ids derived from raw_hstore
(raw_hstore[...]["id"]) so the test validates membership regardless of row
order. Target the variables result.data["hstore"], expected_ids, and raw_hstore
when updating the assertion logic.
In `@tests/integration/fixtures.py`:
- Line 105: The current code mutates the module-global mapping scalar_overrides
in place by applying HSTORE_SCALAR_OVERRIDES, which can leak Postgres-specific
overrides; instead create a new mapping when combining them (e.g., copy
scalar_overrides or build a new dict merging scalar_overrides and
HSTORE_SCALAR_OVERRIDES) and assign that new mapping to the local variable used
for schema creation so the global scalar_overrides remains unchanged; update the
spot where scalar_overrides is combined (reference symbol: scalar_overrides and
HSTORE_SCALAR_OVERRIDES) to use a non-mutating merge.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/integration/data_types/__snapshots__/test_hstore.ambris excluded by!**/__snapshots__/**
📒 Files selected for processing (13)
README.mdsrc/strawchemy/dto/inspectors/sqlalchemy.pysrc/strawchemy/repository/sqlalchemy/_sync.pysrc/strawchemy/schema/filters/__init__.pysrc/strawchemy/schema/filters/base.pysrc/strawchemy/schema/filters/inputs.pysrc/strawchemy/schema/scalars/__init__.pysrc/strawchemy/schema/scalars/base.pytests/integration/conftest.pytests/integration/data_types/test_hstore.pytests/integration/fixtures.pytests/integration/models.pytests/integration/types/postgres.py
| ) | ||
|
|
||
|
|
||
| def _serialize_hstore(value: dict[str, str]) -> dict[str, str]: |
There was a problem hiding this comment.
This is unneeded, you can just use a bare lambda x: x in the scalar definition
|
|
||
|
|
||
| def _parse_hstore(value: object) -> dict[str, str]: | ||
| if not isinstance(value, dict): |
There was a problem hiding this comment.
You can rely on msgspec parsing here instead: msgspec.json.decode(object, type=dict[str, str])
| @pytest.mark.snapshot | ||
| async def test_hstore_filters( | ||
| filter_name: str, | ||
| value: Any, |
There was a problem hiding this comment.
You can type hint this: list[str] | str
| datetime: DateTime, | ||
| } | ||
| engine_plugins: list[str] = [] | ||
| scalar_overrides |= HSTORE_SCALAR_OVERRIDES |
There was a problem hiding this comment.
I agree with the rabbit here, we could just add it to the dict directly ;)
|
I don't get what's wrong with the CI errors, they seems legit to me, and ruff can fix them for you. Have you tried? |
2 participants
Description
Add support for filtering on PostgreSQL
hstorecolumns. This introduces a newHStoreFilter,_HStoreComparisoninput, andHStoreGraphQL scalar, following the same patterns used by the existing JSON and Array filter implementations.Supported filter operations:
contains,containedIn,hasKey,hasKeyAll,hasKeyAny.Types of Changes
Issues Fixed or Closed by This PR
Closes #65
Checklist
Summary by CodeRabbit
New Features
Documentation
Tests