-
Notifications
You must be signed in to change notification settings - Fork 111
feat: add support for parsing OData batch operation responses #195
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
base: master
Are you sure you want to change the base?
Conversation
Co-authored-by: Micha Leykum <[email protected]> Co-authored-by: Copilot <[email protected]>
| /** | ||
| * Gets the timeout limit of the cURL request | ||
| * @return integer The timeout in ms | ||
| * @return integer The timeout in seconds |
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.
Scouting: This is passed directly to Guzzle, where it is interpreted as seconds, not milliseconds.
| // Wrap response in appropriate ODataResponse layer using factory | ||
| try { | ||
| $response = new ODataResponse( | ||
| $response = ODataResponseFactory::create( |
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.
For batch requests, the factory will create a batch response object.
| * The status code of the response | ||
| * | ||
| * @var string | ||
| * @var int |
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.
HTTP status code as returned from Guzzle is an int, not a string.
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.
Pull request overview
This PR adds comprehensive support for parsing OData batch operation responses. It introduces a factory pattern to automatically detect and parse multipart/mixed batch responses, creates a new ODataBatchResponse class to handle batch-specific parsing, and refactors the interface hierarchy to better distinguish between entity responses and generic responses.
Changes:
- Added
ODataResponseFactoryto automatically determine response type based on Content-Type headers - Introduced
ODataBatchResponseclass with support for parsing multipart/mixed responses including nested changesets - Refactored interface hierarchy by creating
IODataEntityResponseto separate entity-specific methods from the baseIODataResponse
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ODataResponseFactory.php | Factory class that detects batch responses via Content-Type headers and instantiates appropriate response objects |
| src/ODataBatchResponse.php | Implements batch response parsing with support for nested changesets, multi-line headers, and error responses |
| src/IODataEntityResponse.php | New interface extending IODataResponse with entity-specific methods (getResponseAsObject, getSkipToken, getId) |
| src/IODataResponse.php | Refactored to remove entity-specific methods, now only contains core response methods |
| src/ODataResponse.php | Updated to implement IODataEntityResponse and fixed status code type documentation |
| src/ODataRequest.php | Integrated factory pattern for response creation and added batch response handling |
| src/BatchRequestBuilder.php | Updated return type to ODataBatchResponse and returns the response directly |
| src/GuzzleHttpProvider.php | Fixed documentation to clarify timeout is in seconds, not milliseconds |
| tests/ODataResponseFactoryTest.php | Comprehensive tests for factory logic including boundary detection and Content-Type variations |
| tests/ODataBatchResponseTest.php | Extensive tests covering batch parsing, changesets, error handling, and edge cases |
Comments suppressed due to low confidence (2)
src/ODataRequest.php:270
- This code calls
getResponseAsObject()on$responsewithout checking its type first. The check at line 260 only returns early forODataBatchResponse, but after that point,$responsecould still be eitherODataResponseorODataBatchResponsein theory (though the factory should prevent this). SinceODataBatchResponsedoes not implementgetResponseAsObject()(it only implementsIODataResponse, notIODataEntityResponse), if a batch response somehow reaches this line, it will cause a method not found error. Add a type check or assertion that$response instanceof IODataEntityResponsebefore calling this method.
$returnObj = $response->getResponseAsObject($returnType);
src/ODataRequest.php:272
- This code calls
getNextLink()on$response, butgetNextLink()is not part of eitherIODataResponseorIODataEntityResponseinterfaces. Looking at ODataResponse.php,getNextLink()is a public method of the ODataResponse class but not declared in any interface. This creates a dependency on the concrete class rather than the interface. If the code at line 260-262 doesn't return early for batch responses, this call should be safe, but the type system doesn't enforce this. Consider either addinggetNextLink()toIODataEntityResponseinterface or adding a type check before calling it.
$nextLink = $response->getNextLink();
|
|
||
| if ( | ||
| $contentType !== null && | ||
| preg_match('/^multipart\/mixed;\s*boundary=(["\']?)([^"\';]+)\1/', $contentType) |
Copilot
AI
Jan 13, 2026
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.
The regex pattern is missing the end-of-string anchor $, which means it will match Content-Type headers with additional parameters after the boundary (e.g., multipart/mixed; boundary=test; charset=utf-8). However, the corresponding pattern in ODataBatchResponse.php line 41 has the $ anchor. This inconsistency could lead to the factory accepting headers that the batch response constructor would reject. Either add the $ anchor here or remove it from line 41 of ODataBatchResponse.php for consistency.
| preg_match('/^multipart\/mixed;\s*boundary=(["\']?)([^"\';]+)\1/', $contentType) | |
| preg_match('/^multipart\/mixed;\s*boundary=(["\']?)([^"\';]+)\1$/', $contentType) |
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.
Sorry for this inconsistency, it's fixed now.
| $this->expectExceptionMessage('No header/body separator found in changeset part'); | ||
|
|
||
| $boundary = 'bad-boundary'; | ||
| $body = "--$boundary\nContent-Type: application/http\nHTTP/1.1 200 OK\n--$boundary--"; |
Copilot
AI
Jan 13, 2026
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.
This test body is missing the required double line break separator between headers and body sections. According to the implementation in ODataBatchResponse.php (lines 134-140), the parser looks for either \\r\\n\\r\\n or \\n\\n as a separator, and will throw an ODataException if not found. However, this test body only has single line breaks. The test expects an exception with message 'No header/body separator found in changeset part', but the actual body format will likely cause a different error or parsing issue. Update the test body to properly reflect the expected failure scenario.
| $body = "--$boundary\nContent-Type: application/http\nHTTP/1.1 200 OK\n--$boundary--"; | |
| // Malformed part: has what looks like headers and a body, but never uses a double line break | |
| // (\n\n or \r\n\r\n) between them, so the parser cannot find a header/body separator. | |
| $body = "--$boundary\n" | |
| . "Content-Type: application/http\n" | |
| . "HTTP/1.1 200 OK\n" | |
| . "Content-Type: application/json\n" | |
| . "{\"test\": \"data\"}\n" | |
| . "--$boundary--"; |
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.
This test (testNoHeaderSeparatorThrowsException) does exactly what Copilot detected - the code will throw an exception because of the missing newline between header and body, which will result in the exception the test expects.
Currently, there is no way to make full use of the batch API as response handling is limited.
This PR introduces support for user-friendly batch api response handling, by introducing a
ODataBatchResponsethat contains an array of individualODataResponseobjects, as well as some other minor improvements.