Introduce reusable errors to use across APIs#2445
Open
snehagunta wants to merge 3 commits intoRedHatInsights:masterfrom
Open
Introduce reusable errors to use across APIs#2445snehagunta wants to merge 3 commits intoRedHatInsights:masterfrom
snehagunta wants to merge 3 commits intoRedHatInsights:masterfrom
Conversation
Contributor
Reviewer's GuideIntroduces a shared DomainError hierarchy for reusable, RFC7807-style errors and refactors role/permission services, serializers, and models to use these domain exceptions, while wiring them into the v2 API exception handler and fixing minor API spec formatting issues. Sequence diagram for Role create flow with DomainError handlingsequenceDiagram
actor Client
participant DRFView as DRF_View
participant RoleV2RequestSerializer as RoleRequestSerializer
participant RoleV2Service as RoleService
participant PermissionService as PermissionService
participant CustomRoleV2 as RoleModel
participant ExceptionHandler as custom_exception_handler_v2
Client->>DRFView: POST /v2/roles
DRFView->>RoleRequestSerializer: instantiate(data)
DRFView->>RoleRequestSerializer: is_valid()
DRFView->>RoleRequestSerializer: save()
RoleRequestSerializer->>RoleService: create(name, description, permission_data, tenant)
alt missing_required_fields
RoleService-->>RoleService: validate description, permissions
RoleService-->>RoleService: MissingRequiredFieldError raised
RoleService-->>DRFView: MissingRequiredFieldError propagates
DRFView->>ExceptionHandler: custom_exception_handler_v2(exc, context)
ExceptionHandler-->>ExceptionHandler: build Problem Details body
ExceptionHandler-->>Client: 400 response
else invalid_permissions
RoleService->>PermissionService: resolve(permission_data)
PermissionService-->>PermissionService: InvalidFieldError or MissingRequiredFieldError raised
PermissionService-->>RoleService: exception propagates
RoleService-->>RoleService: wrap as InvalidFieldError(field=permissions)
RoleService-->>DRFView: InvalidFieldError propagates
DRFView->>ExceptionHandler: custom_exception_handler_v2(exc, context)
ExceptionHandler-->>Client: 400 Problem Details with errors[field]
else name_conflict
RoleService->>RoleModel: save()
RoleModel-->>RoleService: ValidationError or IntegrityError
RoleService-->>RoleService: raise AlreadyExistsError(resource_type=role)
RoleService-->>DRFView: AlreadyExistsError propagates
DRFView->>ExceptionHandler: custom_exception_handler_v2(exc, context)
ExceptionHandler-->>Client: 409 Problem Details
else unexpected_db_error
RoleService->>RoleModel: save()
RoleModel-->>RoleService: IntegrityError
RoleService-->>RoleService: raise DatabaseError(operation_context=Create Role)
RoleService-->>DRFView: DatabaseError propagates
DRFView->>ExceptionHandler: custom_exception_handler_v2(exc, context)
ExceptionHandler-->>Client: 500 Problem Details
end
Class diagram for new DomainError hierarchy and usageclassDiagram
class DomainError {
<<exception>>
int status_code
str title
str operation_context
DomainError(message: str, operation_context: str)
}
class MissingRequiredFieldError {
<<exception>>
str field
MissingRequiredFieldError(field: str, operation_context: str)
}
class InvalidFieldError {
<<exception>>
str field
Any rejected_value
InvalidFieldError(field: str, message: str, operation_context: str, rejected_value: Any)
}
class NotFoundError {
<<exception>>
str resource_type
str resource_id
NotFoundError(resource_type: str, resource_id: str, operation_context: str)
}
class AlreadyExistsError {
<<exception>>
str resource_type
str identifier
AlreadyExistsError(resource_type: str, identifier: str, operation_context: str)
}
class InUseError {
<<exception>>
str resource_type
int reference_count
InUseError(resource_type: str, reference_count: int, operation_context: str)
}
class ImmutableError {
<<exception>>
str resource_type
str reason
ImmutableError(resource_type: str, reason: str, operation_context: str)
}
class DatabaseError {
<<exception>>
DatabaseError(operation_context: str)
}
DomainError <|-- MissingRequiredFieldError
DomainError <|-- InvalidFieldError
DomainError <|-- NotFoundError
DomainError <|-- AlreadyExistsError
DomainError <|-- InUseError
DomainError <|-- ImmutableError
DomainError <|-- DatabaseError
class PermissionValue {
+from_v2_dict(data: dict) PermissionValue
}
class RoleV2Service {
+create(name: str, description: str, permission_data: list, tenant: Tenant) CustomRoleV2
}
class CustomRoleV2 {
+clean() void
+save() void
}
class PermissionService {
+resolve(permission_data: list) list
}
class Tenant
RoleV2Service --> PermissionService : uses
RoleV2Service --> CustomRoleV2 : creates
RoleV2Service --> Tenant : scoped_by
PermissionValue ..> InvalidFieldError : raises
PermissionValue ..> MissingRequiredFieldError : raises
CustomRoleV2 ..> MissingRequiredFieldError : raises
RoleV2Service ..> MissingRequiredFieldError : raises
RoleV2Service ..> InvalidFieldError : raises
RoleV2Service ..> AlreadyExistsError : raises
RoleV2Service ..> DatabaseError : raises
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 - I've found 2 issues, and left some high level feedback:
- In
api/common/exception_handler.py,DomainErroris referenced incustom_exception_handler_v2but not imported in the file, which will cause a NameError at runtime—add the appropriate import frommanagement.exceptions. - The indentation changes in
docs/source/specs/typespec/main.tsp(e.g., aroundresource:andid:fields in examples) look inconsistent with the surrounding code and may reduce readability; consider re-running your formatter or aligning indentation with the existing style.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `api/common/exception_handler.py`, `DomainError` is referenced in `custom_exception_handler_v2` but not imported in the file, which will cause a NameError at runtime—add the appropriate import from `management.exceptions`.
- The indentation changes in `docs/source/specs/typespec/main.tsp` (e.g., around `resource:` and `id:` fields in examples) look inconsistent with the surrounding code and may reduce readability; consider re-running your formatter or aligning indentation with the existing style.
## Individual Comments
### Comment 1
<location> `rbac/management/role/v2_service.py:72-79` </location>
<code_context>
requested = {PermissionValue.from_v2_dict(p).v1_string() for p in permission_data}
- except InvalidPermissionDataError as e:
- raise InvalidRolePermissionsError(str(e))
+ except (InvalidFieldError, MissingRequiredFieldError) as e:
+ # Convert permission-level exceptions to role context with "permissions" as the field
+ field = getattr(e, "field", None)
+ message = str(e) if field == "permissions" else f"Permission field error: {e}"
+ raise InvalidFieldError("permissions", message, OPERATION_CREATE_ROLE)
found = {p.permission for p in permissions}
</code_context>
<issue_to_address>
**suggestion:** Preserve original exception context when remapping permission errors to a role-level InvalidFieldError.
Catching `InvalidFieldError`/`MissingRequiredFieldError` and re-raising a new `InvalidFieldError("permissions", ...)` discards the original exception context (stack trace, `rejected_value`, etc.), which makes root-cause debugging harder. Please chain the original exception (e.g. `raise InvalidFieldError("permissions", message, OPERATION_CREATE_ROLE) from e`) and/or copy over `rejected_value` from `e` while still normalizing the field to `"permissions"`.
```suggestion
try:
permissions = self.permission_service.resolve(permission_data)
requested = {PermissionValue.from_v2_dict(p).v1_string() for p in permission_data}
except (InvalidFieldError, MissingRequiredFieldError) as e:
# Convert permission-level exceptions to role context with "permissions" as the field,
# while preserving original context and rejected value for debugging.
field = getattr(e, "field", None)
rejected_value = getattr(e, "rejected_value", None)
message = str(e) if field == "permissions" else f"Permission field error: {e}"
raise InvalidFieldError(
"permissions",
message,
OPERATION_CREATE_ROLE,
rejected_value=rejected_value,
) from e
```
</issue_to_address>
### Comment 2
<location> `rbac/management/role/v2_service.py:75` </location>
<code_context>
requested = {PermissionValue.from_v2_dict(p).v1_string() for p in permission_data}
- except InvalidPermissionDataError as e:
- raise InvalidRolePermissionsError(str(e))
+ except (InvalidFieldError, MissingRequiredFieldError) as e:
+ # Convert permission-level exceptions to role context with "permissions" as the field
+ field = getattr(e, "field", None)
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the error handling in the permission resolution `except (InvalidFieldError, MissingRequiredFieldError) as e:` block by uniformly wrapping permission-layer errors into a single role-level `InvalidFieldError` without conditional introspection.
The `except (InvalidFieldError, MissingRequiredFieldError) as e:` block is doing conditional introspection and re-wrapping domain errors into the same type, which increases complexity without adding much value.
You can keep the behavior of “map permission errors into a role permissions context with operation info” while simplifying the flow and centralizing the responsibility in this layer, e.g.:
```python
try:
permissions = self.permission_service.resolve(permission_data)
requested = {PermissionValue.from_v2_dict(p).v1_string() for p in permission_data}
except (InvalidFieldError, MissingRequiredFieldError) as e:
# Wrap permission-level errors in a single, role-level context
raise InvalidFieldError(
"permissions",
f"Invalid permissions data: {e}",
OPERATION_CREATE_ROLE,
) from e
```
This removes:
- `getattr(e, "field", None)` probing
- Branching on `field == "permissions"`
- Reinterpretation of the underlying message format
while still:
- Converting permission-layer errors into a consistent `"permissions"` role-field error
- Preserving the operation context for the global exception handler.
If you later want even more cohesion, consider pushing the mapping into the global exception handler and letting the permission-layer `InvalidFieldError`/`MissingRequiredFieldError` bubble up unchanged from here.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| requested = {PermissionValue.from_v2_dict(p).v1_string() for p in permission_data} | ||
| except InvalidPermissionDataError as e: | ||
| raise InvalidRolePermissionsError(str(e)) | ||
| except (InvalidFieldError, MissingRequiredFieldError) as e: |
Contributor
There was a problem hiding this comment.
issue (complexity): Consider simplifying the error handling in the permission resolution except (InvalidFieldError, MissingRequiredFieldError) as e: block by uniformly wrapping permission-layer errors into a single role-level InvalidFieldError without conditional introspection.
The except (InvalidFieldError, MissingRequiredFieldError) as e: block is doing conditional introspection and re-wrapping domain errors into the same type, which increases complexity without adding much value.
You can keep the behavior of “map permission errors into a role permissions context with operation info” while simplifying the flow and centralizing the responsibility in this layer, e.g.:
try:
permissions = self.permission_service.resolve(permission_data)
requested = {PermissionValue.from_v2_dict(p).v1_string() for p in permission_data}
except (InvalidFieldError, MissingRequiredFieldError) as e:
# Wrap permission-level errors in a single, role-level context
raise InvalidFieldError(
"permissions",
f"Invalid permissions data: {e}",
OPERATION_CREATE_ROLE,
) from eThis removes:
getattr(e, "field", None)probing- Branching on
field == "permissions" - Reinterpretation of the underlying message format
while still:
- Converting permission-layer errors into a consistent
"permissions"role-field error - Preserving the operation context for the global exception handler.
If you later want even more cohesion, consider pushing the mapping into the global exception handler and letting the permission-layer InvalidFieldError/MissingRequiredFieldError bubble up unchanged from here.
22 tasks
Contributor
|
@snehagunta this needs to solve merge conflicts. |
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.
Link(s) to Jira
Description of Intent of Change(s)
Creating some generic errors that can be used for most cases instead of specific errors
Local Testing
How can the feature be exercised?
How can the bug be exploited and fix confirmed?
Is any special local setup required?
Checklist
Secure Coding Practices Checklist Link
Secure Coding Practices Checklist
Summary by Sourcery
Introduce a shared DomainError hierarchy for management-layer errors and wire it into role and permission APIs so they are surfaced as RFC 7807-style problem responses.
New Features:
Enhancements:
Documentation: