feat: add field groups#168
Merged
Merged
Conversation
Contributor
|
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:
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 (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughAdds FieldGroup/FieldSpec and FieldSet to support grouped field selectors (ALL/SCALARS/RELATIONSHIPS), refactors DTOConfig inclusion/exclusion and DTOFactory checks to use selector-aware logic, updates related schema/registry/mapper/validation typings and logic, exports FieldGroup, and extends unit tests for group-based selection behavior. ChangesFieldGroup-based field selection for DTOs
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #168 +/- ##
==========================================
+ Coverage 92.18% 93.06% +0.87%
==========================================
Files 70 69 -1
Lines 6170 6200 +30
Branches 816 817 +1
==========================================
+ Hits 5688 5770 +82
+ Misses 333 288 -45
+ Partials 149 142 -7 ☔ View full report in Codecov by Harness. |
![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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/strawchemy/dto/types.py (2)
248-251:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize direct group strings before set operations.
Both
_merge_field_iterables()andwith_base_annotations()treat"scalars"/"relationships"as generic iterables, soset()/union()will explode them into characters. That corrupts the selector set for the documented direct-string API.Also applies to: 363-364
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/strawchemy/dto/types.py` around lines 248 - 251, Normalize string-valued selectors before performing set operations: in _merge_field_iterables, detect when an iterable is a string (e.g., "scalars" or "relationships") and convert it to a single-item container (or otherwise treat it as an atomic selector) instead of letting set()/union() iterate its characters; keep the existing "all" short-circuit. Apply the same normalization in with_base_annotations so direct group strings are preserved as single selectors rather than being exploded into characters.
427-448:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep validation helpers consistent with the documented string-literal API.
is_fields_iterable("scalars")returnsFalse, andhas_field_group(["scalars"])/has_field_group({"relationships"})returnFalsebecause only enum instances are recognized inside iterables. That means plain string literals are accepted by the docs but rejected by validation and__post_init__checks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/strawchemy/dto/types.py` around lines 427 - 448, The helpers reject documented string-literal group names (e.g., "scalars"/"relationships") inside iterables; update is_fields_iterable, include_field, and has_field_group to accept FieldGroup string names as well as enum members: in is_fields_iterable treat a string (other than "all") as valid if it matches any FieldGroup name/value; in has_field_group consider an iterable item a group if item is a FieldGroup or if isinstance(item, str) and it matches a FieldGroup name/value; and in include_field when checking `group in fields` also accept string group names by comparing group.name or group.value to string items so both enum and string-literal APIs work consistently (refer to is_fields_iterable, include_field, has_field_group).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/strawchemy/dto/types.py`:
- Around line 417-422: The current cast_include_fields function incorrectly maps
"scalars" and "relationships" to the "all" group; update the match so only "all"
returns "all" and the "scalars" and "relationships" branches return their
original group identifier (e.g., return the incoming value) so callers get the
correct FieldGroupStr instead of everything; specifically edit
cast_include_fields to match "all" -> "all" and for "scalars" | "relationships"
-> return value (or the corresponding FieldGroupStr) rather than "all".
- Around line 393-400: The current early returns in the visibility check drop
one exclude scope when include or global_include equals "all"; update the
branches for include == "all" and global_include == "all" to apply both local
and global excludes (self.exclude and self.global_exclude) before including a
field. Specifically, in the method using include, global_include and
include_field, replace the two early-return lines so they return not
(include_field(name, is_relation, self.exclude) or include_field(name,
is_relation, self.global_exclude)) when include == "all" or when global_include
== "all", ensuring both exclude sets are enforced for DTOConfig field selection.
---
Outside diff comments:
In `@src/strawchemy/dto/types.py`:
- Around line 248-251: Normalize string-valued selectors before performing set
operations: in _merge_field_iterables, detect when an iterable is a string
(e.g., "scalars" or "relationships") and convert it to a single-item container
(or otherwise treat it as an atomic selector) instead of letting set()/union()
iterate its characters; keep the existing "all" short-circuit. Apply the same
normalization in with_base_annotations so direct group strings are preserved as
single selectors rather than being exploded into characters.
- Around line 427-448: The helpers reject documented string-literal group names
(e.g., "scalars"/"relationships") inside iterables; update is_fields_iterable,
include_field, and has_field_group to accept FieldGroup string names as well as
enum members: in is_fields_iterable treat a string (other than "all") as valid
if it matches any FieldGroup name/value; in has_field_group consider an iterable
item a group if item is a FieldGroup or if isinstance(item, str) and it matches
a FieldGroup name/value; and in include_field when checking `group in fields`
also accept string group names by comparing group.name or group.value to string
items so both enum and string-literal APIs work consistently (refer to
is_fields_iterable, include_field, has_field_group).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 23f8aff3-faaa-45b7-a663-534c4932d18f
📒 Files selected for processing (8)
src/strawchemy/__init__.pysrc/strawchemy/dto/base.pysrc/strawchemy/dto/types.pysrc/strawchemy/schema/factories/types.pysrc/strawchemy/utils/registry.pytests/unit/dto/test_dto.pytests/unit/mapping/test_schemas.pytests/utils.py
Owner
Author
|
@adiberk what do you think about this implementation? |
|
Will take a look in a few! |
|
@gazorby I think the comment about behaving consistently with enum constants might be valid though? But this should cover our current overrides nicely So the way to exclude relationships by default is just to do |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/strawchemy/dto/types.py`:
- Around line 347-354: The _include_field method in types.py duplicates
FieldSet.__contains__ logic; replace the manual checks by building a FieldSet
from the incoming fields (FieldSet(fields)) and delegating membership to it,
adapting the check to supply either the field name or the appropriate FieldGroup
when needed (e.g., pass field_name for string selectors or
FieldGroup.RELATIONSHIPS/SCALARS when only is_relation is known), or construct a
minimal DTOFieldDefinition object and use that if FieldSet expects a
DTOFieldDefinition; update the _include_field implementation to call
FieldSet(fields).__contains__(...) instead of reimplementing the membership
logic.
- Around line 490-503: The root DTO field exclusion logic ignores global
include/exclude rules; update should_exclude_field so when checking node.is_root
it calls DTOConfig.is_field_included with scope="global" (or otherwise evaluates
DTOConfig.global_include/global_exclude) instead of only using the local scope;
specifically, modify the branch that uses included_by_config/scope="local" to
incorporate a check via is_field_included(..., scope="global") (or combine both
local and global results) so DTOConfig.global_include/global_exclude are applied
to root fields as intended (look for functions/methods: should_exclude_field,
is_field_included, DTOConfig.global_include/global_exclude,
FieldSelector/DTOFieldDefinition).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6e1c35e9-0194-43d9-bf3c-ea0fbdc4b453
📒 Files selected for processing (12)
src/strawchemy/config/base.pysrc/strawchemy/dto/base.pysrc/strawchemy/dto/types.pysrc/strawchemy/dto/utils.pysrc/strawchemy/mapper.pysrc/strawchemy/schema/factories/_kwargs.pysrc/strawchemy/schema/factories/base.pysrc/strawchemy/schema/factories/types.pysrc/strawchemy/schema/field.pysrc/strawchemy/utils/registry.pysrc/strawchemy/validation/pydantic.pytests/unit/dto/test_dto.py
gazorby
force-pushed
the
feat/164-scalars-relationships
branch
3 times, most recently
from
ea5ccc2 to
b0738a3
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/strawchemy/dto/base.py (1)
489-498:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInclude global selectors in the root DTO cache key.
_root_cache_key()hashesincluded_fields/excluded_fields, but those derived sets drop one scope whenever the other is present. For example,include={"author"}, global_include={"id"}andinclude={"author"}, global_include={"name"}currently share the same root cache key even though theauthorrelation should point at different nested DTO types.Suggested fix
def _root_cache_key(self, dto_config: DTOConfig) -> Hashable: root_key = [ - frozenset(dto_config.included_fields) - {FieldGroup.ALL}, - frozenset(dto_config.excluded_fields), + frozenset(FieldSet(dto_config.include)) - {FieldGroup.ALL}, + frozenset(FieldSet(dto_config.exclude)), + frozenset(FieldSet(dto_config.global_include)), + frozenset(FieldSet(dto_config.global_exclude)), frozenset(dto_config.aliases.items()), frozenset(dto_config.annotation_overrides.items()), ] return frozenset(key for key in root_key if key)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/strawchemy/dto/base.py` around lines 489 - 498, _root_cache_key currently builds the cache key from dto_config.included_fields/excluded_fields but omits any global selectors, causing different global_include/global_exclude configurations to produce the same root key; update _root_cache_key to append frozenset(dto_config.global_included_fields) (minus FieldGroup.ALL like the local includes if needed) and frozenset(dto_config.global_excluded_fields) into the root_key list (alongside the existing aliases and annotation_overrides), then return the filtered frozenset as before so global selectors are distinguished in the cache key.src/strawchemy/dto/types.py (1)
366-395:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
copy_with(exclude=...)drops the “everything except” semantics.The
elsebranches coerce an omitted paired selector toset(), socopy_with(exclude={RELATIONSHIPS})producesinclude=set()instead ofinclude=None. That prevents__post_init__()from promoting a bare exclude toALL, and the copied config excludes everything instead of “all except relationships”. The same bug exists forglobal_exclude.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/strawchemy/dto/types.py` around lines 366 - 395, The copy_with method is incorrectly coercing an omitted paired selector to set() (losing the None that __post_init__ relies on), so change the else branches in copy_with to stop using "include = include or set()" and "exclude = exclude or set()" (and the global_* equivalents); instead preserve None when a selector was omitted (i.e., only replace with set() if the caller explicitly passed an empty iterable), so that DTOConfig.__post_init__ can correctly promote a bare exclude to ALL; update the logic around include/exclude and global_include/global_exclude in copy_with accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/strawchemy/dto/types.py`:
- Around line 473-479: The is_fields_iterable type guard currently rejects the
string selectors "scalars" and "relationships" even though FieldSpec and
FieldSet.normalize accept them; update is_fields_iterable (the function named
is_fields_iterable) to treat "scalars" and "relationships" as valid by returning
True for those string values as well while preserving the existing behavior for
"all" and FieldGroup instances and still returning False for other plain
strings; ensure the change aligns with FieldSpec and FieldSet.normalize
expectations so callers that validate before normalizing won't reject supported
inputs.
- Around line 302-315: In DTOConfig.__post_init__, normalize the no-arg case so
that when both self.include and self.global_include are None they are set to
SCALARS (instead of leaving them None), while keeping the existing bare-exclude
⇒ "all" promotions; do this before constructing included_fields/excluded_fields
so FieldSet receives SCALARS and DTOFactory.should_exclude_field will treat the
default path as scalar-only. Reference symbols: DTOConfig.__post_init__,
self.include, self.global_include, SCALARS, FieldSet, and
DTOFactory.should_exclude_field.
---
Outside diff comments:
In `@src/strawchemy/dto/base.py`:
- Around line 489-498: _root_cache_key currently builds the cache key from
dto_config.included_fields/excluded_fields but omits any global selectors,
causing different global_include/global_exclude configurations to produce the
same root key; update _root_cache_key to append
frozenset(dto_config.global_included_fields) (minus FieldGroup.ALL like the
local includes if needed) and frozenset(dto_config.global_excluded_fields) into
the root_key list (alongside the existing aliases and annotation_overrides),
then return the filtered frozenset as before so global selectors are
distinguished in the cache key.
In `@src/strawchemy/dto/types.py`:
- Around line 366-395: The copy_with method is incorrectly coercing an omitted
paired selector to set() (losing the None that __post_init__ relies on), so
change the else branches in copy_with to stop using "include = include or set()"
and "exclude = exclude or set()" (and the global_* equivalents); instead
preserve None when a selector was omitted (i.e., only replace with set() if the
caller explicitly passed an empty iterable), so that DTOConfig.__post_init__ can
correctly promote a bare exclude to ALL; update the logic around include/exclude
and global_include/global_exclude in copy_with accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ea0f62aa-60d4-43df-ad86-d4f109844c62
📒 Files selected for processing (17)
README.mdpyproject.tomlsrc/strawchemy/__init__.pysrc/strawchemy/config/base.pysrc/strawchemy/dto/base.pysrc/strawchemy/dto/types.pysrc/strawchemy/dto/utils.pysrc/strawchemy/mapper.pysrc/strawchemy/schema/factories/_kwargs.pysrc/strawchemy/schema/factories/base.pysrc/strawchemy/schema/factories/types.pysrc/strawchemy/schema/field.pysrc/strawchemy/utils/registry.pysrc/strawchemy/validation/pydantic.pytests/unit/dto/test_dto.pytests/unit/mapping/test_schemas.pytests/utils.py
# Conflicts: # tests/unit/mapping/test_schemas.py
# Conflicts: # src/strawchemy/config/base.py
gazorby
force-pushed
the
feat/164-scalars-relationships
branch
from
288e54e to
b867f7e
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/strawchemy/dto/types.py (1)
473-479:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winType guard rejects valid
FieldSpecinputs"scalars"and"relationships".
FieldSpecincludesFieldGroupStr(Literal["all", "scalars", "relationships"]), butis_fields_iterable()only accepts"all"as a valid string selector. Inputs like"scalars"or"relationships"incorrectly returnFalse, which may cause callers validating with this helper to reject supported public inputs.🐛 Proposed fix
def is_fields_iterable(value: Any) -> TypeIs[FieldSpec]: """Test the given value is suitable to be used as either `include` or `exclude` in a DTOConfig.""" - if value == "all" or isinstance(value, FieldGroup): + if isinstance(value, FieldGroup): + return True + if isinstance(value, str): + return FieldGroup.is_group(value) - if isinstance(value, str): - return False return isinstance(value, (frozenset, set, list, tuple))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/strawchemy/dto/types.py` around lines 473 - 479, The type guard is_fields_iterable currently only treats "all" and FieldGroup instances as valid string selectors, causing FieldSpec values "scalars" and "relationships" to be rejected; update is_fields_iterable (which returns TypeIs[FieldSpec]) to accept the full FieldGroupStr set (e.g. "all", "scalars", "relationships") instead of only "all" — for example by checking if value is a str and value in {"all","scalars","relationships"} or by comparing against FieldGroupStr members, while preserving the existing set/tuple checking for iterable types and the isinstance(value, FieldGroup) branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/strawchemy/dto/types.py`:
- Around line 91-92: The __next__ method on FieldSet is misleading because
FieldSet already implements __iter__ (returning iter(self.field_set)) and does
not maintain iteration state; remove the __next__ method definition from the
FieldSet class (the def __next__(self) -> FieldSelector: return
next(iter(self.field_set)) block) to avoid implying FieldSet is an iterator,
then search the codebase for any direct uses of next(field_set) and replace
those with iter(field_set) or explicit iterator management (e.g.,
iter(field_set).__next__() or next(iter(field_set))) as appropriate, and run
tests to verify no behavior regression.
In `@src/strawchemy/schema/factories/base.py`:
- Around line 499-503: The membership check is comparing a DTOFieldDefinition
object against dto_config.included_fields (which may contain names or
selectors), so it always fails; update the condition inside the loop in base.py
to use dto_config.is_field_included(field) (or compare field.model_field_name to
the included names) instead of `field in dto_config.included_fields`, keeping
the rest of the logic (inspector.has_default check and setting
annotations_overrides[name] = Optional[field.type_hint]) intact so
default-generated PKs become optional when the DTO includes that field.
---
Duplicate comments:
In `@src/strawchemy/dto/types.py`:
- Around line 473-479: The type guard is_fields_iterable currently only treats
"all" and FieldGroup instances as valid string selectors, causing FieldSpec
values "scalars" and "relationships" to be rejected; update is_fields_iterable
(which returns TypeIs[FieldSpec]) to accept the full FieldGroupStr set (e.g.
"all", "scalars", "relationships") instead of only "all" — for example by
checking if value is a str and value in {"all","scalars","relationships"} or by
comparing against FieldGroupStr members, while preserving the existing set/tuple
checking for iterable types and the isinstance(value, FieldGroup) branch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4278eabb-cc23-46bc-8766-c9149fea4224
📒 Files selected for processing (17)
README.mdpyproject.tomlsrc/strawchemy/__init__.pysrc/strawchemy/config/base.pysrc/strawchemy/dto/base.pysrc/strawchemy/dto/types.pysrc/strawchemy/dto/utils.pysrc/strawchemy/mapper.pysrc/strawchemy/schema/factories/_kwargs.pysrc/strawchemy/schema/factories/base.pysrc/strawchemy/schema/factories/types.pysrc/strawchemy/schema/field.pysrc/strawchemy/utils/registry.pysrc/strawchemy/validation/pydantic.pytests/unit/dto/test_dto.pytests/unit/mapping/test_schemas.pytests/utils.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
Types of Changes
Issues Fixed or Closed by This PR
Checklist
Summary by CodeRabbit
New Features
Behavior Changes / Bug Fixes
Documentation
Tests