feat(HttpMocker): adding support for PUT requests and bytes responses #342
Conversation
|
/autofix
|
📝 WalkthroughWalkthroughThis pull request adds support for the PUT HTTP method in the HTTP mocking framework. The Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HttpMocker
participant _mock_request_method
participant ResponseHandler
Client->>HttpMocker: PUT(request, responses)
HttpMocker->>_mock_request_method: Call with SupportedHttpMethods.PUT
_mock_request_method->>ResponseHandler: Process response.body
alt response.body is a string
ResponseHandler-->>_mock_request_method: Return {"text": <data>}
else response.body is bytes
ResponseHandler-->>_mock_request_method: Return {"content": <data>}
end
_mock_request_method-->>HttpMocker: Return response dictionary
HttpMocker-->>Client: Return mocked response
Suggested reviewers
Would you like any further details or modifications to this overview, wdyt? ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
airbyte_cdk/test/mock_http/mocker.py (1)
81-81: Consider adding a docstring for the response body key selection logic.The conditional logic for selecting between "text" and "content" keys is crucial for handling different response types. Would you consider adding a brief docstring explaining this behavior, wdyt?
def _mock_request_method( self, method: SupportedHttpMethods, request: HttpRequest, responses: Union[HttpResponse, List[HttpResponse]], ) -> None: + """Mock HTTP requests with support for both string and bytes responses. + + The response body is set using: + - "text" key for string responses + - "content" key for bytes responses + """unit_tests/test/mock_http/test_mocker.py (1)
93-105: LGTM! Great test for bytes response handling.The test effectively validates gzip compressed response handling. Consider adding a test for non-compressed bytes responses as well, wdyt?
@HttpMocker() def test_given_raw_bytes_response_when_decorate_then_match(self, http_mocker): response_content = bytes("Raw bytes response", "utf-8") http_mocker.put( HttpRequest(_A_URL, _SOME_QUERY_PARAMS, _SOME_HEADERS, _SOME_REQUEST_BODY_STR), HttpResponse(response_content, 474), ) response = requests.put( _A_URL, params=_SOME_QUERY_PARAMS, headers=_SOME_HEADERS, data=_SOME_REQUEST_BODY_STR ) assert response.content == response_content
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
airbyte_cdk/test/mock_http/mocker.py(3 hunks)airbyte_cdk/test/mock_http/response.py(1 hunks)unit_tests/test/mock_http/test_mocker.py(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: Linters
airbyte_cdk/test/mock_http/mocker.py
[warning] 1-1: File would be reformatted. Please ensure the code adheres to the formatting standards.
airbyte_cdk/test/mock_http/response.py
[warning] 1-1: File would be reformatted. Please ensure the code adheres to the formatting standards.
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Check: 'source-pokeapi' (skip=false)
- GitHub Check: Check: 'source-amplitude' (skip=false)
- GitHub Check: Check: 'source-shopify' (skip=false)
- GitHub Check: Check: 'source-hardcoded-records' (skip=false)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (Fast)
- GitHub Check: Analyze (python)
🔇 Additional comments (4)
airbyte_cdk/test/mock_http/response.py (1)
4-4: LGTM! Clean type hint updates.The changes elegantly extend the HttpResponse class to support both string and bytes responses without requiring implementation changes.
Also applies to: 9-9, 16-16
airbyte_cdk/test/mock_http/mocker.py (2)
20-20: LGTM! Clean enum extension.The PUT method is added consistently with other HTTP methods.
102-105: LGTM! Clean PUT method implementation.The put method follows the established pattern consistently.
unit_tests/test/mock_http/test_mocker.py (1)
79-91: LGTM! Comprehensive PUT request test.The test follows the established pattern and provides good coverage.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
airbyte_cdk/test/mock_http/mocker.py (1)
81-81: Consider making the response type handling more explicit?The type check works, but what do you think about making it more explicit and future-proof? Here's a suggestion, wdyt?
- "text" if isinstance(response.body, str) else "content": response.body, + # requests library uses 'text' for str and 'content' for bytes + "text" if isinstance(response.body, str) else "content" if isinstance(response.body, bytes) else None: response.body,This would:
- Document the requests library convention
- Explicitly handle only str/bytes types
- Fail fast if an unsupported type is provided
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
airbyte_cdk/test/mock_http/mocker.py(3 hunks)airbyte_cdk/test/mock_http/response.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- airbyte_cdk/test/mock_http/response.py
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Check: 'source-pokeapi' (skip=false)
- GitHub Check: Check: 'source-amplitude' (skip=false)
- GitHub Check: Check: 'source-shopify' (skip=false)
- GitHub Check: Check: 'source-hardcoded-records' (skip=false)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (Fast)
- GitHub Check: Analyze (python)
🔇 Additional comments (2)
airbyte_cdk/test/mock_http/mocker.py (2)
20-20: LGTM! Clean addition of PUT method.The PUT method is added cleanly to the enum, maintaining alphabetical order with existing methods.
102-104: LGTM! Clean implementation of PUT method.The implementation follows the established pattern consistently with other HTTP methods.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
airbyte_cdk/test/mock_http/mocker.py (2)
89-92: LGTM! Clean implementation of response body field handling.Nice encapsulation of the response field logic! Would you consider adding a docstring to explain the purpose and return values? Something like:
@staticmethod def _get_body_field(response: HttpResponse) -> str: """Determine the appropriate response field based on body type. Returns: str: 'text' for string responses, 'content' for bytes responses """ return "text" if isinstance(response.body, str) else "content"wdyt? 🤔
106-108: LGTM! Consistent implementation of PUT method.The implementation follows the established pattern perfectly. Since we're adding a new method, would you be interested in adding docstrings to all HTTP methods to improve documentation? We could start with:
def put(self, request: HttpRequest, responses: Union[HttpResponse, List[HttpResponse]]) -> None: """Mock PUT requests matching the given request with the specified response(s). Args: request: The HTTP request to match responses: Single response or list of responses to return in sequence """ self._mock_request_method(SupportedHttpMethods.PUT, request, responses)wdyt about adding similar docs to all methods? 📚
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
airbyte_cdk/test/mock_http/mocker.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Check: 'source-pokeapi' (skip=false)
- GitHub Check: Check: 'source-amplitude' (skip=false)
- GitHub Check: Check: 'source-shopify' (skip=false)
- GitHub Check: Check: 'source-hardcoded-records' (skip=false)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (Fast)
- GitHub Check: Analyze (python)
🔇 Additional comments (1)
airbyte_cdk/test/mock_http/mocker.py (1)
20-20: LGTM! Nice addition of PUT method.The PUT method is added to the enum in alphabetical order, maintaining good code organization.
maxi297
deleted the
maxi297/httpmocker-support-put-and-binary-responses
branch
* main: fix: update cryptography package to latest version to address CVE (airbytehq#377) fix: (CDK) (HttpRequester) - Make the `HttpRequester.path` optional (airbytehq#370) feat: improved custom components handling (airbytehq#350) feat: add microseconds timestamp format (airbytehq#373) fix: Replace Unidecode with anyascii for permissive license (airbytehq#367) feat: add IncrementingCountCursor (airbytehq#346) feat: (low-code cdk) datetime format with milliseconds (airbytehq#369) fix: (CDK) (AsyncRetriever) - Improve UX on variable naming and interpolation (airbytehq#368) fix: (CDK) (AsyncRetriever) - Add the `request` and `response` to each `async` operations (airbytehq#356) fix: (CDK) (ConnectorBuilder) - Add `auxiliary requests` to slice; support `TestRead` for AsyncRetriever (part 1/2) (airbytehq#355) feat(concurrent perpartition cursor): Add parent state updates (airbytehq#343) fix: update csv parser for builder compatibility (airbytehq#364) feat(low-code cdk): add interpolation for limit field in Rate (airbytehq#353) feat(low-code cdk): add AbstractStreamFacade processing as concurrent streams in declarative source (airbytehq#347) fix: (CDK) (CsvParser) - Fix the `\\` escaping when passing the `delimiter` from Builder's UI (airbytehq#358) feat: expose `str_to_datetime` jinja macro (airbytehq#351) fix: update CDK migration for 6.34.0 (airbytehq#348) feat: Removes `stream_state` interpolation from CDK (airbytehq#320) fix(declarative): Pass `extra_fields` in `global_substream_cursor` (airbytehq#195) feat(concurrent perpartition cursor): Refactor ConcurrentPerPartitionCursor (airbytehq#331) feat(HttpMocker): adding support for PUT requests and bytes responses (airbytehq#342) chore: use certified source for manifest-only test (airbytehq#338) feat: check for request_option mapping conflicts in individual components (airbytehq#328) feat(file-based): sync file acl permissions and identities (airbytehq#260) fix: (CDK) (Connector Builder) - refactor the `MessageGrouper` > `TestRead` (airbytehq#332) fix(low code): Fix missing cursor for ClientSideIncrementalRecordFilterDecorator (airbytehq#334) feat(low-code): Add API Budget (airbytehq#314) chore(decoder): clean decoders and make csvdecoder available (airbytehq#326)
This is needed for the new destination we are currently implementing (the exact PR is not yet up but see https://github.com/airbytehq/airbyte-enterprise/pull/89 for more context)
Summary by CodeRabbit
New Features
Tests