fix: Add proper HTTP error handling to BaseAssignmentViewSet.perform_create #888

hsong-rh · 2025-11-25T15:31:58Z

https://issues.redhat.com/browse/AAP-58366

Description

Add try-catch block around remote_sync_assignment call
Convert HTTP 400 errors to ValidationError
Convert HTTP 401/403 errors to PermissionDenied
Re-raise other HTTP errors unchanged
Add comprehensive pytest tests for all error scenarios
Tests cover both user and team assignment error handling

🤖 Generated with Claude Code

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation update
Test update
Refactoring (no functional changes)
Development environment change
Configuration change

Self-Review Checklist

I have performed a self-review of my code
I have added relevant comments to complex code sections
I have updated documentation where needed
I have considered the security impact of these changes
I have considered performance implications
I have thought about error handling and edge cases
I have tested the changes in my local environment

Screenshots/Logs

2025-11-25 15:16:16,977 INFO     ansible_base.resources_api.rest_client Making post request to https://host.containers.internal:8443/api/gateway/v1/service-index/role-user-assignments/assign/.
2025-11-25 15:16:17,024 DEBUG    ansible_base.resources_api.rest_client Response status code from https://host.containers.internal:8443/api/gateway/v1/service-index/role-user-assignments/assign/: 400
2025-11-25 15:16:17,024 ERROR    [rid:af0db68c-2c11-4b55-b967-c1d7d5895245] aap_eda.api.exceptions ValidationError: {'detail': ErrorDetail(string='400 Client Error: Bad Request for url: https://host.containers.internal:8443/api/gateway/v1/service-index/role-user-assignments/assign/\nResponse content: {"role_definition":["Object with name=Organization Event Stream Admin does not exist."]}', code='invalid')}
2025-11-25 15:16:17,024 ERROR    [rid:af0db68c-2c11-4b55-b967-c1d7d5895245] aap_eda.api.exceptions /app/venv/lib64/python3.11/site-packages/ansible_base/rbac/api/views.py:239 perform_create
2025-11-25 15:16:17,024 ERROR    [rid:af0db68c-2c11-4b55-b967-c1d7d5895245] aap_eda.api.exceptions /app/venv/lib64/python3.11/site-packages/rest_framework/mixins.py:19 create
2025-11-25 15:16:17,025 ERROR    [rid:af0db68c-2c11-4b55-b967-c1d7d5895245] aap_eda.api.exceptions /app/venv/lib64/python3.11/site-packages/drf_spectacular/drainage.py:193 wrapped_method
2025-11-25 15:16:17,025 ERROR    [rid:af0db68c-2c11-4b55-b967-c1d7d5895245] aap_eda.api.exceptions /app/venv/lib64/python3.11/site-packages/rest_framework/views.py:506 dispatch
2025-11-25 15:16:17,026 WARNING  django.channels.server HTTP POST /api/eda/v1/role_user_assignments/ 400 [0.08, 10.89.2.14:44844]

…form_create - Add try-catch block around remote_sync_assignment call - Convert HTTP 400 errors to ValidationError - Convert HTTP 401/403 errors to PermissionDenied - Re-raise other HTTP errors unchanged - Add comprehensive pytest tests for all error scenarios - Tests cover both user and team assignment error handling 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>

hsong-rh · 2025-12-01T13:56:59Z

@AlanCoding Please review

john-westcott-iv

📋 Code Review Summary

This PR adds important error handling for HTTP failures during remote role assignment synchronization. The core approach of wrapping the operation in a transaction is sound and addresses the race condition issue. However, there are several areas that need attention before approval.

Files Reviewed: 2 files
Comments Posted: 6 review comments

🔍 Issues Found

1 correctness/logic issue (missing return value)
2 code quality suggestions (error message preservation, test coverage)
2 minor improvements (error handling consistency, exception type choice)

Overall Assessment

Needs work - The transaction handling is correct, but there are important improvements needed around error message preservation and test coverage.

General Feedback

✅ Good use of transaction.atomic() to ensure rollback on sync failures
✅ Comprehensive test coverage for various HTTP error codes
✅ Proper conversion of HTTP errors to DRF exceptions
⚠️ Consider preserving structured error messages from the gateway response
⚠️ Add test coverage for team assignments (not just users)
ℹ️ Document the intentional lack of ConnectionError handling

john-westcott-iv

File: ansible_base/rbac/api/views.py, Line 195-202

The original code returned the result from super().perform_create(serializer). While perform_create typically doesn't use return values in DRF, removing the return is a behavioral change. Consider:

with transaction.atomic():
    ret = super().perform_create(serializer)
    self.remote_sync_assignment(serializer.instance)
return ret

This preserves the original behavior while still providing transaction safety.

john-westcott-iv

File: ansible_base/rbac/api/views.py, Line 207

The error data loses the structured error information from the gateway response. The HTTPError already includes response content (line 129 in rest_client.py), but it's being converted to a simple string.

Consider parsing the response JSON to preserve the structured error:

error_data = {"detail": str(exc)}
try:
    if hasattr(exc.response, 'json'):
        error_data = exc.response.json()
except Exception:
    pass  # Fall back to string representation

This would preserve error messages like {"role_definition": ["Object with name=... does not exist."]} which are more useful for API consumers.

john-westcott-iv

File: ansible_base/rbac/api/views.py, Line 214-216

For 500 errors or HTTPError with no response, raising APIException results in another 500. This seems redundant. Consider:

Re-raising the original HTTPError to preserve the full stack trace for debugging
Using a more specific exception type if there's a meaningful distinction
Adding logging before raising to capture the error for diagnostics

Example:

else:
    logger.error(f"Unexpected HTTP error during remote sync: {exc}")
    raise  # Re-raise original exception

john-westcott-iv

File: test_app/tests/rbac/api/test_rbac_views.py, Missing Coverage

The tests only cover RoleUserAssignment (via roleuserassignment-list), but the fix is in BaseAssignmentViewSet which is also used by RoleTeamAssignmentViewSet.

Recommendation: Add parallel tests for team assignments to ensure the error handling works correctly for both assignment types:

@pytest.mark.parametrize("sync_error_status, expected_status", [...])
@pytest.mark.django_db
def test_team_assignment_remote_sync_error_handling(admin_api_client, inv_rd, team, inventory, sync_error_status, expected_status):
    """Test that remote sync HTTP errors are handled correctly for team assignments"""
    url = get_relative_url('roleteamassignment-list')
    data = dict(role_definition=inv_rd.id, team=team.id, object_id=inventory.id)
    # ... rest similar to user test

john-westcott-iv

File: ansible_base/rbac/api/views.py, Line 205-206

The comment mentions "eliminating the race condition where assignments briefly exist before manual deletion." This is excellent context!

However, the comment could be even clearer about why transaction.atomic() solves this:

# Wrap the entire operation in a single atomic transaction
# This ensures that if remote sync fails, the local assignment creation is rolled back
# automatically. Without this, there was a race condition where:
# 1. Local assignment is committed to the database
# 2. Remote sync fails
# 3. Assignment must be manually deleted, but may be visible to users briefly
# The transaction.atomic() ensures step 1 only commits if step 2 succeeds.

github-actions · 2025-12-01T16:08:28Z

DVCS PR Check Results:

PR appears valid (JIRA key(s) found)

hsong-rh · 2025-12-01T16:11:17Z

File: ansible_base/rbac/api/views.py, Line 214-216

For 500 errors or HTTPError with no response, raising APIException results in another 500. This seems redundant. Consider:

Re-raising the original HTTPError to preserve the full stack trace for debugging

Using a more specific exception type if there's a meaningful distinction

Adding logging before raising to capture the error for diagnostics

Example:
else:
    logger.error(f"Unexpected HTTP error during remote sync: {exc}")
    raise  # Re-raise original exception

Fixed

sonarqubecloud · 2025-12-01T16:22:44Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
83.3% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

hsong-rh · 2025-12-01T16:45:03Z

@john-westcott-iv thanks for the review, I addressed all the comments.

hsong-rh force-pushed the aap-58366 branch 4 times, most recently from 9671117 to 3ba9334 Compare November 25, 2025 21:38

hsong-rh force-pushed the aap-58366 branch 2 times, most recently from 5c3bc60 to 2494815 Compare November 25, 2025 22:06

john-westcott-iv reviewed Dec 1, 2025

View reviewed changes

Add changes according to review comments

e0c5cdd

hsong-rh force-pushed the aap-58366 branch from 2494815 to e0c5cdd Compare December 1, 2025 16:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: Add proper HTTP error handling to BaseAssignmentViewSet.perform_create #888

fix: Add proper HTTP error handling to BaseAssignmentViewSet.perform_create #888

hsong-rh commented Nov 25, 2025 •

edited

Loading

Uh oh!

hsong-rh commented Dec 1, 2025

Uh oh!

john-westcott-iv left a comment

Uh oh!

john-westcott-iv left a comment

Uh oh!

john-westcott-iv left a comment

Uh oh!

john-westcott-iv left a comment

Uh oh!

john-westcott-iv left a comment

Uh oh!

john-westcott-iv left a comment

Uh oh!

github-actions bot commented Dec 1, 2025

Uh oh!

hsong-rh commented Dec 1, 2025

Uh oh!

sonarqubecloud bot commented Dec 1, 2025

Uh oh!

hsong-rh commented Dec 1, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

fix: Add proper HTTP error handling to BaseAssignmentViewSet.perform_create #888

Are you sure you want to change the base?

fix: Add proper HTTP error handling to BaseAssignmentViewSet.perform_create #888

Conversation

hsong-rh commented Nov 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of Change

Self-Review Checklist

Screenshots/Logs

Uh oh!

hsong-rh commented Dec 1, 2025

Uh oh!

john-westcott-iv left a comment

Choose a reason for hiding this comment

📋 Code Review Summary

🔍 Issues Found

Overall Assessment

General Feedback

Uh oh!

john-westcott-iv left a comment

Choose a reason for hiding this comment

Uh oh!

john-westcott-iv left a comment

Choose a reason for hiding this comment

Uh oh!

john-westcott-iv left a comment

Choose a reason for hiding this comment

Uh oh!

john-westcott-iv left a comment

Choose a reason for hiding this comment

Uh oh!

john-westcott-iv left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Dec 1, 2025

Uh oh!

hsong-rh commented Dec 1, 2025

Uh oh!

sonarqubecloud bot commented Dec 1, 2025

Quality Gate passed

Uh oh!

hsong-rh commented Dec 1, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

hsong-rh commented Nov 25, 2025 •

edited

Loading