Add account to serialized group when missing #3286
Open
Conversation
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
Contributor
Reviewer's GuideEnsures the serialized group dictionary always includes an Sequence diagram for serialize_group_without_host_count account resolutionsequenceDiagram
participant Caller
participant Serialization as serialize_group_without_host_count
participant Group
participant Auth as get_current_identity
participant Identity
Caller->>Serialization: serialize_group_without_host_count(group)
Serialization->>Group: getattr(group, account)
alt group.account is not None and not empty
Serialization-->>Caller: dict with account = group.account
else group.account is None or empty
Serialization->>Auth: get_current_identity()
alt request context and identity available
Auth-->>Serialization: Identity
Serialization->>Identity: getattr(identity, account_number)
alt identity.account_number is not None and not empty
Serialization-->>Caller: dict with account = identity.account_number
else identity.account_number is None or empty
Serialization-->>Caller: dict with account = None
end
else RuntimeError or AttributeError
Auth-->>Serialization: raises RuntimeError/AttributeError
Serialization-->>Caller: dict with account = None
end
end
Flow diagram for account selection in serialize_group_without_host_countflowchart TD
A[Start serialize_group_without_host_count] --> B[Set account = None]
B --> C[Get group_account = getattr group, account]
C --> D{group_account is not None
and group_account != ""}
D -->|Yes| E[Set account = group_account]
D -->|No| F[Import get_current_identity]
F --> G[Call get_current_identity]
G --> H{RuntimeError or
AttributeError?}
H -->|Yes| I[Keep account = None]
H -->|No| J[identity = get_current_identity result]
J --> K[identity_account = getattr identity, account_number]
K --> L{identity_account is not None
and identity_account != ""}
L -->|Yes| M[Set account = identity_account]
L -->|No| N[Keep account = None]
E --> O[Build result dict with account]
I --> O
M --> O
N --> O
O --> P[Return result dict]
P --> Q[End]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The account resolution logic in
serialize_group_without_host_countis quite verbose; consider simplifying it (e.g.,group.account or identity.account_number or Nonewith a small helper for non-empty strings) to improve readability and reduce repeatedNone/empty checks. - Catching
(RuntimeError, AttributeError)aroundget_current_identity()makes it easy to hide unexpected errors; it would be safer to narrow the exception handling to the specific failure modes you expect or to log when identity resolution fails.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The account resolution logic in `serialize_group_without_host_count` is quite verbose; consider simplifying it (e.g., `group.account or identity.account_number or None` with a small helper for non-empty strings) to improve readability and reduce repeated `None`/empty checks.
- Catching `(RuntimeError, AttributeError)` around `get_current_identity()` makes it easy to hide unexpected errors; it would be safer to narrow the exception handling to the specific failure modes you expect or to log when identity resolution fails.
## Individual Comments
### Comment 1
<location> `app/serialization.py:264-268` </location>
<code_context>
+ # Account is missing from group or has no value, try to get from identity
+ from app.auth import get_current_identity
+
+ try:
+ identity = get_current_identity()
+ identity_account = getattr(identity, "account_number", None)
+ account = identity_account if identity_account is not None and identity_account != "" else None
+ except (RuntimeError, AttributeError):
+ # No request context available or identity missing account_number
+ account = None
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Narrowing the AttributeError catch or validating identity before attribute access could avoid masking unrelated bugs
Catching `AttributeError` over both `get_current_identity()` and `getattr` can hide real bugs inside `get_current_identity`. Consider calling `identity = get_current_identity()` outside the `try`, and only wrapping the attribute access (or relying on `getattr` and just catching `RuntimeError`). This keeps unexpected `AttributeError`s from being silently turned into `account = None`.
</issue_to_address>
### Comment 2
<location> `app/serialization.py:252` </location>
<code_context>
def serialize_group_without_host_count(group: Group) -> dict:
+ # Handle account field: if missing or has no value, try to get from identity
+ account = None
+
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the account resolution logic into a dedicated helper to keep the serializer focused on shaping the response data.
You can reduce the added complexity by extracting the account resolution into a small helper and leveraging truthiness instead of repeated `is not None and != ""` checks.
For example:
```python
def _resolve_group_account(group: Group) -> str | None:
group_account = getattr(group, "account", None)
if group_account: # non-empty string
return group_account
from app.auth import get_current_identity
try:
identity = get_current_identity()
identity_account = getattr(identity, "account_number", None)
return identity_account or None # normalize empty string / falsy to None
except (RuntimeError, AttributeError):
return None
```
Then your serializer becomes simpler and focused only on shaping the response:
```python
def serialize_group_without_host_count(group: Group) -> dict:
return {
"id": _serialize_uuid(group.id),
"org_id": group.org_id,
"account": _resolve_group_account(group),
"name": group.name,
"ungrouped": group.ungrouped,
"created": _serialize_datetime(group.created_on),
"updated": _serialize_datetime(group.modified_on),
}
```
This preserves the current behavior (group account → identity account → `None`) while moving the inline control flow, dynamic import, and exception handling into a dedicated helper.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
app/serialization.py
Outdated
Comment on lines
264
to
268
| try: | ||
| identity = get_current_identity() | ||
| identity_account = getattr(identity, "account_number", None) | ||
| account = identity_account if identity_account is not None and identity_account != "" else None | ||
| except (RuntimeError, AttributeError): |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Narrowing the AttributeError catch or validating identity before attribute access could avoid masking unrelated bugs
Catching AttributeError over both get_current_identity() and getattr can hide real bugs inside get_current_identity. Consider calling identity = get_current_identity() outside the try, and only wrapping the attribute access (or relying on getattr and just catching RuntimeError). This keeps unexpected AttributeErrors from being silently turned into account = None.
app/serialization.py
Outdated
|
|
||
|
|
||
| def serialize_group_without_host_count(group: Group) -> dict: | ||
| # Handle account field: if missing or has no value, try to get from identity |
Contributor
There was a problem hiding this comment.
issue (complexity): Consider extracting the account resolution logic into a dedicated helper to keep the serializer focused on shaping the response data.
You can reduce the added complexity by extracting the account resolution into a small helper and leveraging truthiness instead of repeated is not None and != "" checks.
For example:
def _resolve_group_account(group: Group) -> str | None:
group_account = getattr(group, "account", None)
if group_account: # non-empty string
return group_account
from app.auth import get_current_identity
try:
identity = get_current_identity()
identity_account = getattr(identity, "account_number", None)
return identity_account or None # normalize empty string / falsy to None
except (RuntimeError, AttributeError):
return NoneThen your serializer becomes simpler and focused only on shaping the response:
def serialize_group_without_host_count(group: Group) -> dict:
return {
"id": _serialize_uuid(group.id),
"org_id": group.org_id,
"account": _resolve_group_account(group),
"name": group.name,
"ungrouped": group.ungrouped,
"created": _serialize_datetime(group.created_on),
"updated": _serialize_datetime(group.modified_on),
}This preserves the current behavior (group account → identity account → None) while moving the inline control flow, dynamic import, and exception handling into a dedicated helper.
kruai
requested changes
Dec 15, 2025
Collaborator
kruai
left a comment
kruai
left a comment
There was a problem hiding this comment.
This PR fetches the account number during serialization if it doesn't exist. This both doesn't set the account on the Group record, and adds inefficiency when serializing groups that just don't have account data.
I recommend checking the event we consume from the workspaces outbox topic to see if the account number is populated. If not, we can double-check that we're sending the account number in the RBAC POST API request, and go from there.
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.
Overview
This PR is being created to address RHINENG-21925. Just account key:value to the serialized group to keep the returned goup same as before.
PR Checklist
Secure Coding Practices Documentation Reference
You can find documentation on this checklist here.
Secure Coding Checklist
Summary by Sourcery
Bug Fixes: