fix: Add proper HTTP error handling to BaseAssignmentViewSet.perform_create

[hsong-rh]

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]

[hsong-rh]

@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:

1. Re-raising the original HTTPError to preserve the full stack trace for debugging
2. Using a more specific exception type if there's a meaningful distinction
3. 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]

DVCS PR Check Results:

PR appears valid (JIRA key(s) found)

[hsong-rh]

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:

1. Re-raising the original HTTPError to preserve the full stack trace for debugging
2. Using a more specific exception type if there's a meaningful distinction
3. 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]

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]

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

[AlanCoding]

I wound up solving the same problem from the other side in aap-gateway. This is what it looks like.

class ProxyAPIException(APIException):
    "If a service gave an error response, we wrap it up and pass it onto the client as if it was ours"

    def __init__(self, detail, status_code):
        self.detail = detail
        self.status_code = status_code

The client here is mostly the same client as used in your diff, which is the resource registry client.

            client = self.get_direct_client(assignment.role_definition)
            try:
                client.sync_assignment(assignment)
            except HTTPError as e:
                try:
                    error_detail = e.response.json()
                except Exception:
                    error_detail = e.response.text
                raise ProxyAPIException(detail=error_detail, status_code=e.response.status_code)

You took some different turns, but I think ultimately to the same effect.

I agree with the largest-scale principle here that components, making an assignment, should re-raise the exception from Gateway if that happens. I think it might actually be better to indicate in the response somehow that it is "wrapped" by padding the text. Maybe that divulges too much information? But I don't think so.

Getting more technical, I'm iffy about applying the transaction, but I do think it's necessary. DAB is really herding cats here because some components set ATOMIC_REQUESTS to true and others don't. To handle that without additional additional transactions, I at a point created ensure_transaction. Because if we already have a per-request transaction, allowing that to do its work is probably going to result in better behavior that's ultimately easier to debug.

For next steps here - because I agree so strongly that both DAB and aap-gateway should be doing the same thing here (re-raising the remote error), I would like to have a standardized request wrapper, so that after this merges, we can import and use the same utility for aap-gateway, essentially replacing the code snippet I gave here.

[hsong-rh]

@AlanCoding Thanks for the detailed feedback! Since you've already solved this problem in aap-gateway and have a clear vision for the standardized approach, would you like to take over this PR and ticket: https://issues.redhat.com/browse/AAP-58366? I think you're better positioned to implement the right solution given your broader context.