-
Notifications
You must be signed in to change notification settings - Fork 20
feat: add DestinationHttpClient to simplify calls to target systems #119
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
jplbrun
wants to merge
3
commits into
main
Choose a base branch
from
feat/special-header-providers
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
127 changes: 127 additions & 0 deletions
127
src/sap_cloud_sdk/destination/_destination_http_client.py
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,127 @@ | ||
| """HTTP client for calling the target system described by a Destination.""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| from typing import Any, Dict, Optional | ||
|
|
||
| import requests | ||
| from requests import Response | ||
|
|
||
| from sap_cloud_sdk.destination._models import Destination, DestinationType | ||
|
|
||
|
|
||
| class DestinationHttpClient: | ||
| """Wraps requests.Session to call the target system described by a Destination. | ||
|
|
||
| Pre-bakes SAP ERP headers (sap-client, sap-language) and auth headers from | ||
| the destination so callers never have to set them manually. | ||
|
|
||
| Usage:: | ||
|
|
||
| dest = client.get_destination("my-erp") | ||
| http = DestinationHttpClient(dest) | ||
| response = http.get("/sap/opu/odata/sap/API_BUSINESS_PARTNER") | ||
| """ | ||
|
|
||
| def __init__(self, destination: Destination) -> None: | ||
| if destination.type not in (DestinationType.HTTP, "HTTP"): | ||
| raise ValueError( | ||
| f"DestinationHttpClient only supports HTTP destinations, got: {destination.type}" | ||
| ) | ||
|
|
||
| self._destination = destination | ||
| self._session = requests.Session() | ||
|
|
||
| # Pre-bake sap-client / sap-language — relevant mainly for OnPremise destinations | ||
| self._session.headers.update(destination.get_erp_headers()) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All headers from destination should be used, not only token and erp. Is this the case? |
||
|
|
||
| # Pre-bake auth headers — BTP may return multiple tokens, skip empty ones | ||
| for token in destination.auth_tokens: | ||
| key = token.http_header.get("key") | ||
| value = token.http_header.get("value") | ||
| if key and value: | ||
| self._session.headers[key] = value | ||
|
Comment on lines
+38
to
+43
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about token refreshing? |
||
|
|
||
| self._base_url = destination.url.rstrip("/") if destination.url else "" | ||
|
|
||
| def request( | ||
| self, | ||
| method: str, | ||
| path: str, | ||
| *, | ||
| params: Optional[Dict[str, Any]] = None, | ||
| json: Optional[Any] = None, | ||
| headers: Optional[Dict[str, str]] = None, | ||
| **kwargs: Any, | ||
| ) -> Response: | ||
| """Send an HTTP request to the target system. | ||
|
|
||
| Args: | ||
| method: HTTP verb (GET, POST, PUT, PATCH, DELETE). | ||
| path: Path relative to the destination URL. | ||
| params: Optional query parameters. | ||
| json: Optional JSON body. | ||
| headers: Optional additional headers merged on top of pre-baked ones. | ||
| **kwargs: Passed through to requests.Session.request. | ||
|
|
||
| Returns: | ||
| requests.Response from the target system. | ||
| """ | ||
| url = f"{self._base_url}/{path.lstrip('/')}" if path else self._base_url | ||
| return self._session.request( | ||
| method=method.upper(), | ||
| url=url, | ||
| params=params, | ||
| json=json, | ||
| headers=headers, | ||
| **kwargs, | ||
| ) | ||
|
|
||
| def get( | ||
| self, | ||
| path: str, | ||
| *, | ||
| params: Optional[Dict[str, Any]] = None, | ||
| headers: Optional[Dict[str, str]] = None, | ||
| **kwargs: Any, | ||
| ) -> Response: | ||
| return self.request("GET", path, params=params, headers=headers, **kwargs) | ||
|
|
||
| def post( | ||
| self, | ||
| path: str, | ||
| *, | ||
| json: Optional[Any] = None, | ||
| headers: Optional[Dict[str, str]] = None, | ||
| **kwargs: Any, | ||
| ) -> Response: | ||
| return self.request("POST", path, json=json, headers=headers, **kwargs) | ||
|
|
||
| def put( | ||
| self, | ||
| path: str, | ||
| *, | ||
| json: Optional[Any] = None, | ||
| headers: Optional[Dict[str, str]] = None, | ||
| **kwargs: Any, | ||
| ) -> Response: | ||
| return self.request("PUT", path, json=json, headers=headers, **kwargs) | ||
|
|
||
| def patch( | ||
| self, | ||
| path: str, | ||
| *, | ||
| json: Optional[Any] = None, | ||
| headers: Optional[Dict[str, str]] = None, | ||
| **kwargs: Any, | ||
| ) -> Response: | ||
| return self.request("PATCH", path, json=json, headers=headers, **kwargs) | ||
|
|
||
| def delete( | ||
| self, | ||
| path: str, | ||
| *, | ||
| headers: Optional[Dict[str, str]] = None, | ||
| **kwargs: Any, | ||
| ) -> Response: | ||
| return self.request("DELETE", path, headers=headers, **kwargs) | ||
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,134 @@ | ||
| """Unit tests for DestinationHttpClient.""" | ||
|
|
||
| from unittest.mock import MagicMock, patch | ||
|
|
||
| import pytest | ||
|
|
||
| from sap_cloud_sdk.destination._destination_http_client import DestinationHttpClient | ||
| from sap_cloud_sdk.destination._models import AuthToken, Destination | ||
|
|
||
|
|
||
| def _dest(**kwargs) -> Destination: | ||
| base = {"Name": "test", "Type": "HTTP", "URL": "https://example.com"} | ||
| base.update(kwargs) | ||
| return Destination.from_dict(base) | ||
|
|
||
|
|
||
| def _auth_token(key: str, value: str) -> AuthToken: | ||
| return AuthToken(type="Bearer", value="raw", http_header={"key": key, "value": value}) | ||
|
|
||
|
|
||
| class TestDestinationHttpClientInit: | ||
| def test_raises_for_non_http_destination(self): | ||
| dest = Destination.from_dict({"Name": "test", "Type": "RFC"}) | ||
| with pytest.raises(ValueError, match="only supports HTTP destinations"): | ||
| DestinationHttpClient(dest) | ||
|
|
||
| def test_erp_headers_pre_baked(self): | ||
| dest = _dest(**{"sap-client": "100", "sap-language": "en"}) | ||
| client = DestinationHttpClient(dest) | ||
| assert client._session.headers["sap-client"] == "100" | ||
| assert client._session.headers["sap-language"] == "en" | ||
|
|
||
| def test_no_erp_headers_when_properties_empty(self): | ||
| dest = _dest() | ||
| client = DestinationHttpClient(dest) | ||
| assert "sap-client" not in client._session.headers | ||
| assert "sap-language" not in client._session.headers | ||
|
|
||
| def test_auth_header_pre_baked_from_auth_tokens(self): | ||
| dest = _dest() | ||
| dest.auth_tokens = [_auth_token("Authorization", "Bearer eyJ123")] | ||
| client = DestinationHttpClient(dest) | ||
| assert client._session.headers["Authorization"] == "Bearer eyJ123" | ||
|
|
||
| def test_multiple_auth_tokens_all_injected(self): | ||
| dest = _dest() | ||
| dest.auth_tokens = [ | ||
| _auth_token("Authorization", "Bearer eyJ123"), | ||
| _auth_token("x-sap-security-session", "mysession"), | ||
| ] | ||
| client = DestinationHttpClient(dest) | ||
| assert client._session.headers["Authorization"] == "Bearer eyJ123" | ||
| assert client._session.headers["x-sap-security-session"] == "mysession" | ||
|
|
||
| def test_error_token_with_empty_values_is_skipped(self): | ||
| dest = _dest() | ||
| dest.auth_tokens = [_auth_token("", "")] | ||
| client = DestinationHttpClient(dest) | ||
| assert "Authorization" not in client._session.headers | ||
|
|
||
| def test_no_auth_header_when_auth_tokens_empty(self): | ||
| dest = _dest() | ||
| client = DestinationHttpClient(dest) | ||
| assert "Authorization" not in client._session.headers | ||
|
|
||
|
|
||
| class TestDestinationHttpClientRequest: | ||
| def setup_method(self): | ||
| self.dest = _dest() | ||
| self.client = DestinationHttpClient(self.dest) | ||
| self.mock_response = MagicMock() | ||
|
|
||
| def test_constructs_full_url(self): | ||
| with patch.object(self.client._session, "request", return_value=self.mock_response) as mock_req: | ||
| self.client.request("GET", "/api/v1/users") | ||
| assert mock_req.call_args[1]["url"] == "https://example.com/api/v1/users" | ||
|
|
||
| def test_uppercases_method(self): | ||
| with patch.object(self.client._session, "request", return_value=self.mock_response) as mock_req: | ||
| self.client.request("get", "/resource") | ||
| assert mock_req.call_args[1]["method"] == "GET" | ||
|
|
||
| def test_passes_params(self): | ||
| with patch.object(self.client._session, "request", return_value=self.mock_response) as mock_req: | ||
| self.client.request("GET", "/resource", params={"$top": "10"}) | ||
| assert mock_req.call_args[1]["params"] == {"$top": "10"} | ||
|
|
||
| def test_passes_json_body(self): | ||
| with patch.object(self.client._session, "request", return_value=self.mock_response) as mock_req: | ||
| self.client.request("POST", "/resource", json={"key": "value"}) | ||
| assert mock_req.call_args[1]["json"] == {"key": "value"} | ||
|
|
||
| def test_passes_extra_headers(self): | ||
| with patch.object(self.client._session, "request", return_value=self.mock_response) as mock_req: | ||
| self.client.request("GET", "/resource", headers={"X-Custom": "yes"}) | ||
| assert mock_req.call_args[1]["headers"] == {"X-Custom": "yes"} | ||
|
|
||
| def test_returns_response(self): | ||
| with patch.object(self.client._session, "request", return_value=self.mock_response): | ||
| assert self.client.request("GET", "/resource") is self.mock_response | ||
|
|
||
|
|
||
| class TestDestinationHttpClientVerbHelpers: | ||
| def setup_method(self): | ||
| self.client = DestinationHttpClient(_dest()) | ||
| self.mock_response = MagicMock() | ||
|
|
||
| def _patch(self): | ||
| return patch.object(self.client._session, "request", return_value=self.mock_response) | ||
|
|
||
| def test_get_uses_get_method(self): | ||
| with self._patch() as mock_req: | ||
| self.client.get("/r") | ||
| assert mock_req.call_args[1]["method"] == "GET" | ||
|
|
||
| def test_post_uses_post_method(self): | ||
| with self._patch() as mock_req: | ||
| self.client.post("/r", json={"a": 1}) | ||
| assert mock_req.call_args[1]["method"] == "POST" | ||
|
|
||
| def test_put_uses_put_method(self): | ||
| with self._patch() as mock_req: | ||
| self.client.put("/r", json={}) | ||
| assert mock_req.call_args[1]["method"] == "PUT" | ||
|
|
||
| def test_patch_uses_patch_method(self): | ||
| with self._patch() as mock_req: | ||
| self.client.patch("/r", json={}) | ||
| assert mock_req.call_args[1]["method"] == "PATCH" | ||
|
|
||
| def test_delete_uses_delete_method(self): | ||
| with self._patch() as mock_req: | ||
| self.client.delete("/r") | ||
| assert mock_req.call_args[1]["method"] == "DELETE" |
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it really necessary for us to wrap the
requests.Sessionobject? I mean we are wrapping all the methods fromrequests.Session(get,put,post,deleteandrequest) without doing anything extra.Maybe we could just offer a factory for developers to create a
Sessionobject populated with the necessary headers from the destination.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we don't need to return Session, but we could expose a single method of request allowing selection of path and HttpMethod