Conversation
📝 WalkthroughWalkthroughThe PR extends Chat REST client with retention policy management endpoints and chat preferences configuration, adds activity metrics configuration to app settings, introduces a feeds collections query method, enhances video SIP authentication and trunk management, and updates data models to support these new features. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
getstream/models/__init__.py (1)
8107-8109: Keep the retention-policy selector required if these endpoints are still policy-scoped.Line 8107 and Line 19363 now accept
None, which lets the SDK build empty delete/set payloads and defer the failure to the server. Ifpolicyis still the selector for these operations, keeping it required preserves a fail-fast client contract.💡 Suggested tightening
class DeleteRetentionPolicyRequest(DataClassJsonMixin): - policy: Optional[str] = dc_field( - default=None, metadata=dc_config(field_name="policy") - ) + policy: str = dc_field(metadata=dc_config(field_name="policy")) class SetRetentionPolicyRequest(DataClassJsonMixin): - policy: Optional[str] = dc_field( - default=None, metadata=dc_config(field_name="policy") - ) + policy: str = dc_field(metadata=dc_config(field_name="policy"))Also applies to: 19363-19365
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@getstream/models/__init__.py` around lines 8107 - 8109, The policy dataclass field is currently Optional with default=None (policy: Optional[str] = dc_field(default=None, metadata=dc_config(field_name="policy"))) which allows building empty payloads; change it to a required str without a None default to enforce fail-fast behavior: replace Optional[str] with str and remove default=None from the dc_field call (i.e., declare policy: str = dc_field(metadata=dc_config(field_name="policy"))). Apply the same change to the other occurrence of the policy field (the second declaration using dc_field/dc_config) so both are required selectors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@getstream/models/__init__.py`:
- Around line 1822-1825: The change altered the shared export-error model's
serialized default 'type' field (defined via dc_field/dc_config) from
"export.channels.error" to "export.bulk_image_moderation.error", breaking
consumers and routing via AsyncExportErrorEvent; revert the default back to
"export.channels.error" on that shared model (the 'type' dc_field) so
AsyncExportErrorEvent and existing tests remain unchanged, and if you need a
distinct bulk image moderation type instead, create a new dedicated
subclass/model with its own 'type' default "export.bulk_image_moderation.error"
rather than retargeting the shared model.
- Around line 7565-7567: The dataclass password fields are currently included in
auto-generated __repr__ outputs; update each password field declaration (the
lines using "password: Optional[str] = dc_field(...)") to pass repr=False to
dc_field so the password is excluded from repr() output—apply this change to all
four instances referenced (the password field declarations at the spots
mentioned) so dataclass reprs no longer leak plaintext passwords.
- Line 18809: SIPTrunkResponse's allowed_ips field is currently required which
can cause deserialization failures; make it optional with a default (e.g.,
Optional[List[str]] = None or default_factory=list) to match
CreateSIPTrunkRequest and UpdateSIPTrunkRequest handling. Locate the allowed_ips
declaration in the SIPTrunkResponse dataclass and change its type and default to
be optional (or provide a safe default), and update the dc_field metadata usage
accordingly so responses missing allowed_ips deserialize safely.
---
Nitpick comments:
In `@getstream/models/__init__.py`:
- Around line 8107-8109: The policy dataclass field is currently Optional with
default=None (policy: Optional[str] = dc_field(default=None,
metadata=dc_config(field_name="policy"))) which allows building empty payloads;
change it to a required str without a None default to enforce fail-fast
behavior: replace Optional[str] with str and remove default=None from the
dc_field call (i.e., declare policy: str =
dc_field(metadata=dc_config(field_name="policy"))). Apply the same change to the
other occurrence of the policy field (the second declaration using
dc_field/dc_config) so both are required selectors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 92eccdb6-8609-4893-9845-6b6b7ac32021
📒 Files selected for processing (8)
getstream/chat/async_rest_client.pygetstream/chat/rest_client.pygetstream/common/async_rest_client.pygetstream/common/rest_client.pygetstream/feeds/rest_client.pygetstream/models/__init__.pygetstream/video/async_rest_client.pygetstream/video/rest_client.py
| type: str = dc_field( | ||
| default="export.channels.error", metadata=dc_config(field_name="type") | ||
| default="export.bulk_image_moderation.error", | ||
| metadata=dc_config(field_name="type"), | ||
| ) |
There was a problem hiding this comment.
Don't retarget the default type on a shared export-error model.
Line 1823 changes the serialized default from export.channels.error to export.bulk_image_moderation.error, but getstream/webhook.py:475-495 still routes both event types through AsyncExportErrorEvent, and getstream/tests/test_webhook.py:535-545 still treats channel-export errors as this class. That makes direct construction/serialization of AsyncExportErrorEvent a silent breaking change for existing channel-export consumers.
💡 Minimal backward-compatible fix
type: str = dc_field(
- default="export.bulk_image_moderation.error",
+ default="export.channels.error",
metadata=dc_config(field_name="type"),
)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@getstream/models/__init__.py` around lines 1822 - 1825, The change altered
the shared export-error model's serialized default 'type' field (defined via
dc_field/dc_config) from "export.channels.error" to
"export.bulk_image_moderation.error", breaking consumers and routing via
AsyncExportErrorEvent; revert the default back to "export.channels.error" on
that shared model (the 'type' dc_field) so AsyncExportErrorEvent and existing
tests remain unchanged, and if you need a distinct bulk image moderation type
instead, create a new dedicated subclass/model with its own 'type' default
"export.bulk_image_moderation.error" rather than retargeting the shared model.
| password: Optional[str] = dc_field( | ||
| default=None, metadata=dc_config(field_name="password") | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the file size and locate the specific lines
wc -l getstream/models/__init__.pyRepository: GetStream/stream-py
Length of output: 97
🏁 Script executed:
# Read the three locations mentioned in the review comment
echo "=== Lines 7565-7567 ===" && sed -n '7565,7567p' getstream/models/__init__.py
echo -e "\n=== Lines 17975-17977 ===" && sed -n '17975,17977p' getstream/models/__init__.py
echo -e "\n=== Lines 22120-22122 ===" && sed -n '22120,22122p' getstream/models/__init__.pyRepository: GetStream/stream-py
Length of output: 470
🏁 Script executed:
# Search for all password field definitions that might be missing repr=False
rg -n "password.*dc_field|password.*field\(" getstream/models/__init__.py -A 1 -B 1Repository: GetStream/stream-py
Length of output: 849
🏁 Script executed:
# Search for all password field definitions more broadly
rg -n "password\s*:" getstream/models/__init__.py | grep -E "dc_field|field\(" | head -20Repository: GetStream/stream-py
Length of output: 277
🏁 Script executed:
# Check the context around line 18798 to see what class it belongs to
sed -n '18790,18810p' getstream/models/__init__.pyRepository: GetStream/stream-py
Length of output: 987
🏁 Script executed:
# Get context showing which classes contain these password fields
for line in 7565 17975 18798 22120; do
echo "=== Context for line $line ==="
sed -n "$((line - 30)),$((line))p" getstream/models/__init__.py | grep -E "^class|password" | tail -5
doneRepository: GetStream/stream-py
Length of output: 886
Hide SIP passwords from dataclass reprs.
Lines 7565, 17975, 18798, and 22120 add plaintext password fields to ordinary dataclasses. Python dataclasses include field values in __repr__ by default, so these secrets will leak into logs, traces, and debugging output as soon as one of these objects is printed. Please apply repr=False in all four spots.
🔒 Representative fix
password: Optional[str] = dc_field(
- default=None, metadata=dc_config(field_name="password")
+ default=None, repr=False, metadata=dc_config(field_name="password")
)For line 18798:
- password: str = dc_field(metadata=dc_config(field_name="password"))
+ password: str = dc_field(repr=False, metadata=dc_config(field_name="password"))Also applies to: 17975-17977, 18798, 22120-22122
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@getstream/models/__init__.py` around lines 7565 - 7567, The dataclass
password fields are currently included in auto-generated __repr__ outputs;
update each password field declaration (the lines using "password: Optional[str]
= dc_field(...)") to pass repr=False to dc_field so the password is excluded
from repr() output—apply this change to all four instances referenced (the
password field declarations at the spots mentioned) so dataclass reprs no longer
leak plaintext passwords.
| ) | ||
| uri: str = dc_field(metadata=dc_config(field_name="uri")) | ||
| username: str = dc_field(metadata=dc_config(field_name="username")) | ||
| allowed_ips: List[str] = dc_field(metadata=dc_config(field_name="allowed_ips")) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the file and line numbers mentioned
wc -l getstream/models/__init__.pyRepository: GetStream/stream-py
Length of output: 97
🏁 Script executed:
# Now let's look at the code around line 18809
sed -n '18800,18820p' getstream/models/__init__.pyRepository: GetStream/stream-py
Length of output: 797
🏁 Script executed:
# Check lines 7568 and 22123 mentioned in the review
echo "=== Line 7568 area ===" && sed -n '7560,7575p' getstream/models/__init__.py
echo ""
echo "=== Line 22123 area ===" && sed -n '22115,22130p' getstream/models/__init__.pyRepository: GetStream/stream-py
Length of output: 1246
🏁 Script executed:
# Search for allowed_ips usage across the codebase
rg "allowed_ips" --context 3Repository: GetStream/stream-py
Length of output: 4881
🏁 Script executed:
# Get more context to see which response class this belongs to
sed -n '18750,18820p' getstream/models/__init__.py | head -80Repository: GetStream/stream-py
Length of output: 2568
allowed_ips in SIPTrunkResponse should be optional to match request-side handling and prevent deserialization failures.
Both CreateSIPTrunkRequest and UpdateSIPTrunkRequest already make allowed_ips optional with a default value. The response class at line 18809 requires it unconditionally, which will break when deserializing responses from old or unrestricted trunks that omit this field. Make it consistent:
Suggested fix
- allowed_ips: List[str] = dc_field(metadata=dc_config(field_name="allowed_ips"))
+ allowed_ips: Optional[List[str]] = dc_field(
+ default=None, metadata=dc_config(field_name="allowed_ips")
+ )📝 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.
| allowed_ips: List[str] = dc_field(metadata=dc_config(field_name="allowed_ips")) | |
| allowed_ips: Optional[List[str]] = dc_field( | |
| default=None, metadata=dc_config(field_name="allowed_ips") | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@getstream/models/__init__.py` at line 18809, SIPTrunkResponse's allowed_ips
field is currently required which can cause deserialization failures; make it
optional with a default (e.g., Optional[List[str]] = None or
default_factory=list) to match CreateSIPTrunkRequest and UpdateSIPTrunkRequest
handling. Locate the allowed_ips declaration in the SIPTrunkResponse dataclass
and change its type and default to be optional (or provide a safe default), and
update the dc_field metadata usage accordingly so responses missing allowed_ips
deserialize safely.
1 participant
Summary by CodeRabbit
Release Notes