Added Abstract Endpoint#8
Closed
isivaselvan wants to merge 6 commits into
Closed
Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
mukulmohan177
suggested changes
Sep 1, 2025
| T = TypeVar("T", bound="Response") | ||
|
|
||
|
|
||
| class Response(ABC): |
There was a problem hiding this comment.
Are we going to fully implement this? Currently there is no implementation or any sort of relation with endpoint class.
| def __init__(self, client: Session) -> None: | ||
| self._http_client = client | ||
|
|
||
| def _make_request( |
There was a problem hiding this comment.
Why aren't we checking for response codes / raising any errors for those HTTP errors?
| ), # Test case insensitive | ||
| ], | ||
| ) | ||
| def test_make_request_all_methods( |
There was a problem hiding this comment.
Again no test cases for error codes
taru-garg-2000
suggested changes
Sep 2, 2025
Comment on lines
+93
to
+142
| def test_get_method(self, endpoint, mock_response): | ||
| """Test the _get convenience method.""" | ||
| endpoint._http_client.get.return_value = mock_response | ||
|
|
||
| response = endpoint._get("/test/path") | ||
|
|
||
| endpoint._http_client.get.assert_called_once_with("/test/path") | ||
| assert response == mock_response | ||
|
|
||
| def test_post_method(self, endpoint, mock_response): | ||
| """Test the _post convenience method.""" | ||
| endpoint._http_client.post.return_value = mock_response | ||
| test_data = {"key": "value"} | ||
|
|
||
| response = endpoint._post("/test/path", test_data) | ||
|
|
||
| endpoint._http_client.post.assert_called_once_with("/test/path", json=test_data) | ||
| assert response == mock_response | ||
|
|
||
| def test_put_method(self, endpoint, mock_response): | ||
| """Test the _put convenience method.""" | ||
| endpoint._http_client.put.return_value = mock_response | ||
| test_data = {"key": "value"} | ||
|
|
||
| response = endpoint._put("/test/path", test_data) | ||
|
|
||
| endpoint._http_client.put.assert_called_once_with("/test/path", json=test_data) | ||
| assert response == mock_response | ||
|
|
||
| def test_patch_method(self, endpoint, mock_response): | ||
| """Test the _patch convenience method.""" | ||
| endpoint._http_client.patch.return_value = mock_response | ||
| test_data = {"key": "value"} | ||
|
|
||
| response = endpoint._patch("/test/path", test_data) | ||
|
|
||
| endpoint._http_client.patch.assert_called_once_with( | ||
| "/test/path", json=test_data | ||
| ) | ||
| assert response == mock_response | ||
|
|
||
| def test_delete_method(self, endpoint, mock_response): | ||
| """Test the _delete convenience method.""" | ||
| endpoint._http_client.delete.return_value = mock_response | ||
|
|
||
| response = endpoint._delete("/test/path") | ||
|
|
||
| endpoint._http_client.delete.assert_called_once_with("/test/path") | ||
| assert response == mock_response | ||
|
|
Contributor
There was a problem hiding this comment.
these tests look redundant as we are testing this above as well ?
Comment on lines
+36
to
+143
| def _handle_connection_error( | ||
| self, method: str, path: str, error: Exception | ||
| ) -> NoReturn: | ||
| """Handle connection-related errors.""" | ||
| logger.error( | ||
| "Connection error while making %s request to %s: %s", method, path, error | ||
| ) | ||
| raise TFEConnectionException( | ||
| message="Failed to connect to TFE API", | ||
| method=method, | ||
| path=path, | ||
| cause=error, | ||
| ) | ||
|
|
||
| def _handle_timeout_error( | ||
| self, method: str, path: str, error: Exception | ||
| ) -> NoReturn: | ||
| """Handle timeout errors.""" | ||
| logger.error( | ||
| "Timeout error while making %s request to %s: %s", method, path, error | ||
| ) | ||
| raise TFETimeoutException( | ||
| message="Request timed out", method=method, path=path, cause=error | ||
| ) | ||
|
|
||
| def _handle_http_error( | ||
| self, method: str, path: str, error: exceptions.HTTPError | ||
| ) -> NoReturn: | ||
| """Handle HTTP errors with specific status codes.""" | ||
| status_code = error.response.status_code if error.response else None | ||
| error_data = self._extract_error_data(error.response) | ||
|
|
||
| logger.error( | ||
| "HTTP error while making %s request to %s: %s (Status: %s)", | ||
| method, | ||
| path, | ||
| error, | ||
| status_code, | ||
| ) | ||
|
|
||
| # Map status code to specific exception | ||
| STATUS_CODE_MAPPING: dict[int, tuple[type[TFEEndpointException], str]] = { | ||
| 401: ( | ||
| TFEUnauthorizedException, | ||
| "Authentication failed - invalid or missing token", | ||
| ), | ||
| 403: (TFEForbiddenException, "Access forbidden - insufficient permissions"), | ||
| 404: (TFENotFoundException, "Resource not found"), | ||
| 422: (TFEValidationException, "Validation failed"), | ||
| } | ||
|
|
||
| # Handle 5xx server errors | ||
| if status_code and 500 <= status_code < 600: | ||
| exception_class: type[TFEEndpointException] = TFEServerException | ||
| message = "TFE server error" | ||
| else: | ||
| # Get exception class and message from mapping, or use default | ||
| if status_code is not None: | ||
| exception_class, message = STATUS_CODE_MAPPING.get( | ||
| status_code, (TFEEndpointException, "HTTP error occurred") | ||
| ) | ||
| else: | ||
| exception_class, message = TFEEndpointException, "HTTP error occurred" | ||
|
|
||
| # Special handling for validation errors (422) | ||
| if status_code == 422: | ||
| parsed_errors = ( | ||
| self.parse_tfe_error_response(error_data) if error_data else {} | ||
| ) | ||
| message = parsed_errors.get("message", message) | ||
|
|
||
| raise exception_class( | ||
| message=message, | ||
| status_code=status_code, | ||
| error_data=error_data, | ||
| method=method, | ||
| path=path, | ||
| cause=error, | ||
| ) | ||
|
|
||
| def _handle_request_error( | ||
| self, method: str, path: str, error: Exception | ||
| ) -> NoReturn: | ||
| """Handle general request errors.""" | ||
| logger.error( | ||
| "Request error while making %s request to %s: %s", method, path, error | ||
| ) | ||
| raise TFEEndpointException( | ||
| message=f"Request failed: {str(error)}", | ||
| method=method, | ||
| path=path, | ||
| cause=error, | ||
| ) | ||
|
|
||
| def _handle_unexpected_error( | ||
| self, method: str, path: str, error: Exception | ||
| ) -> NoReturn: | ||
| """Handle unexpected errors.""" | ||
| logger.error( | ||
| "Unexpected error while making %s request to %s: %s", method, path, error | ||
| ) | ||
| raise TFEEndpointException( | ||
| message=f"Unexpected error occurred during {method} request", | ||
| method=method, | ||
| path=path, | ||
| cause=error, | ||
| ) | ||
|
|
Contributor
There was a problem hiding this comment.
I think if we have dedicated classes for each error type then this should be implemented by that class itself and not in the endpoint class
There was a problem hiding this comment.
Pull Request Overview
This PR introduces abstract endpoint functionality to the Terraform Enterprise Python client by adding a new exception hierarchy, error handling utilities, and a base endpoint class.
- Added comprehensive exception classes for different HTTP error types
- Implemented error parsing utilities for TFE API responses
- Created base endpoint class with common HTTP request functionality
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tfe/exception.py | Defines exception hierarchy with TFEEndpointException base class and specific error types |
| tfe/error_utils.py | Provides utilities for parsing TFE error responses and handling HTTP errors |
| tfe/endpoint.py | Implements base Endpoint class with HTTP request handling and error management |
| tfe/init.py | Removes existing client imports (entire file deleted) |
| tests/units/test_exception.py | Unit tests for exception classes and error message formatting |
| tests/units/test_error_utils.py | Unit tests for error parsing and HTTP error handling functions |
| tests/units/test_endpoint.py | Unit tests for endpoint HTTP methods and error scenarios |
Comments suppressed due to low confidence (1)
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Member
@taru-garg-hashicorp Retries / rate limiting / backoff / timeouts will be part of same transport wrapper right ? |
Contributor
Retries, Rate limiting, will be a part of the request session, which is held by the client. While it will be passed to the Endpoint abstract, endpoint does not own those parameters |
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
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.
Description
Added Abstract Endpoint
Testing plan
Unit tests for endpoint added
External links
Output from tests
Including output from tests may require access to a TFE instance. Ignore this section if you have no environment to test against.
Rollback Plan
Changes to Security Controls
PCI review checklist
I have documented a clear reason for, and description of, the change I am making.
If applicable, I've documented a plan to revert these changes if they require more than reverting the pull request.
If applicable, I've documented the impact of any changes to security controls.
Examples of changes to security controls include using new access control methods, adding or removing logging pipelines, etc.
If you have any questions, please contact your direct supervisor, GRC (#team-grc), or the PCI working group (#proj-pci-reboot). You can also find more information at PCI Compliance.