-
Notifications
You must be signed in to change notification settings - Fork 110
feat: add support for parsing OData batch operation responses #193
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| <?php | ||
|
|
||
| namespace SaintSystems\OData; | ||
|
|
||
| interface IODataEntityResponse extends IODataResponse | ||
| { | ||
| /** | ||
| * Converts the response JSON object to a OData SDK object | ||
| * | ||
| * @param mixed $returnType The type to convert the object(s) to | ||
| * | ||
| * @return mixed object or array of objects of type $returnType | ||
| */ | ||
| public function getResponseAsObject($returnType); | ||
|
|
||
| /** | ||
| * Gets the skip token of a response object from OData | ||
| * | ||
| * @return string skip token, if provided | ||
| */ | ||
| public function getSkipToken(); | ||
|
|
||
| /** | ||
| * Gets the Id of response object (if set) from OData | ||
| * | ||
| * @return mixed id if this was an insert, if provided | ||
| */ | ||
| public function getId(); | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,219 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| <?php | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| namespace SaintSystems\OData; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| class ODataBatchResponse implements IODataResponse | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+5
to
+6
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| class ODataBatchResponse implements IODataResponse | |
| { | |
| /** | |
| * Represents a response to an OData batch request. | |
| * | |
| * This class parses the multipart/mixed batch response from an OData service, | |
| * extracting individual responses and changesets. It provides access to the | |
| * original request, response body, headers, HTTP status code, and parsed batch responses. | |
| * | |
| * Usage example: | |
| * <code> | |
| * $batchResponse = new ODataBatchResponse($request, $body, $httpStatusCode, $headers); | |
| * $responses = $batchResponse->getResponses(); | |
| * </code> | |
| * | |
| * @param IODataRequest $request The original batch request object. | |
| * @param string|null $body The raw response body from the batch request. | |
| * @param string|null $httpStatusCode The HTTP status code of the batch response. | |
| * @param array $headers The response headers. | |
| */ | |
| class ODataBatchResponse implements IODataResponse |
Copilot
AI
Nov 20, 2025
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 boundary validation in extractBoundary() is inconsistent with the factory detection logic. The factory checks for 'boundary=batchresponse_' while this method checks if the boundary contains 'batchresponse' anywhere (case-insensitive). This could lead to false positives where boundaries containing 'batchresponse' in the middle or end would be accepted. Consider aligning the validation logic between the factory and this class.
| if (strpos(strtolower($boundary), 'batchresponse') !== false) { | |
| if (stripos($boundary, 'batchresponse_') === 0) { |
Copilot
AI
Nov 20, 2025
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 /boundary=(["\']?)([^"\';]+)\1/ doesn't handle boundaries that might contain special regex characters or have spaces. Additionally, the pattern [^"\';]+ will stop at semicolons, which means if the boundary value is followed by additional parameters (e.g., boundary=xyz; charset=utf-8), it will work, but boundary values containing semicolons would fail. While semicolons in boundaries are rare, consider using a more robust parsing approach or document this limitation.
| if ($contentType !== null && $contentType !== '' && preg_match('/boundary=(["\']?)([^"\';]+)\1/', $contentType, $matches)) { | |
| $boundary = $matches[2]; | |
| if (strpos(strtolower($boundary), 'batchresponse') !== false) { | |
| return $boundary; | |
| if ($contentType !== null && $contentType !== '') { | |
| // Improved regex: matches boundary=foo, boundary="foo;bar", boundary='foo bar', etc. | |
| if (preg_match('/boundary\s*=\s*(?:"([^"]+)"|\'([^\']+)\'|([^\s;]+))/i', $contentType, $matches)) { | |
| // $matches[1]: double-quoted, $matches[2]: single-quoted, $matches[3]: unquoted | |
| $boundary = $matches[1] ?? $matches[2] ?? $matches[3] ?? null; | |
| if ($boundary !== null && strpos(strtolower($boundary), 'batchresponse') !== false) { | |
| return $boundary; | |
| } |
Copilot
AI
Nov 20, 2025
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 extractChangesetContent() method splits the entire part into lines using explode("\n"), which creates an array of all lines in memory. For large batch responses with significant changeset content, this could be memory-intensive. Consider using a more memory-efficient approach if supporting large payloads is a requirement, such as using strpos() to find the first empty line and then using substr() to extract the content.
| $lines = explode("\n", $part); | |
| $contentStarted = false; | |
| $content = array(); | |
| foreach ($lines as $line) { | |
| $line = rtrim($line, "\r"); | |
| // Skip until we find an empty line (end of changeset headers) | |
| if (!$contentStarted) { | |
| if (trim($line) === '') { | |
| $contentStarted = true; | |
| } | |
| continue; | |
| } | |
| $content[] = $line; | |
| } | |
| return implode("\n", $content); | |
| // Find the position of the first empty line (end of headers) | |
| $pos = false; | |
| // Try CRLF first | |
| $pos = strpos($part, "\r\n\r\n"); | |
| $delimiterLength = 4; | |
| if ($pos === false) { | |
| // Try LF only | |
| $pos = strpos($part, "\n\n"); | |
| $delimiterLength = 2; | |
| } | |
| if ($pos === false) { | |
| // No header/content delimiter found, return empty string | |
| return ''; | |
| } | |
| // Extract everything after the header/content delimiter | |
| return substr($part, $pos + $delimiterLength); |
Copilot
AI
Nov 20, 2025
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.
Type mismatch in status code handling. HttpStatusCode::OK is an integer constant (200), but the function expects to return a string type. The cast should be (string) of the integer value 200, not the constant name. Consider: $statusCode = isset($statusParts[1]) ? $statusParts[1] : '200';
| $statusCode = (array_key_exists(1, $statusParts) && $statusParts[1] !== null) ? $statusParts[1] : (string)HttpStatusCode::OK; | |
| $statusCode = (array_key_exists(1, $statusParts) && $statusParts[1] !== null) ? $statusParts[1] : '200'; |
Copilot
AI
Nov 20, 2025
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 parseIndividualResponse() method splits the entire part into lines using explode("\n"). For batch responses with many operations or large response bodies, this creates unnecessary line arrays in memory. Consider using a string position-based approach or stream-based parsing for better memory efficiency with large responses.
Copilot
AI
Nov 20, 2025
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.
Concatenating lines with "\n" may not preserve the original line endings from the response. If the original response uses "\r\n" (CRLF), the reconstructed body will have inconsistent line endings. While line 149 strips "\r", the body reconstruction only uses "\n". This could cause issues with signature verification or content-length validation. Consider preserving the original line endings or consistently normalizing them.
| $lines = explode("\n", $part); | |
| $inHeaders = true; | |
| $responseHeaders = array(); | |
| $responseBody = ''; | |
| $statusCode = null; | |
| $foundHttpResponse = false; | |
| foreach ($lines as $line) { | |
| $line = rtrim($line, "\r"); | |
| if ($inHeaders) { | |
| if (trim($line) === '') { | |
| // Only switch to body parsing if we've found an HTTP response line | |
| if ($foundHttpResponse) { | |
| $inHeaders = false; | |
| } | |
| continue; | |
| } | |
| if (strpos($line, 'HTTP/') === 0) { | |
| $statusParts = explode(' ', $line, 3); | |
| $statusCode = (array_key_exists(1, $statusParts) && $statusParts[1] !== null) ? $statusParts[1] : (string)HttpStatusCode::OK; | |
| $foundHttpResponse = true; | |
| continue; | |
| } | |
| // Only parse headers after we've found the HTTP response line | |
| if ($foundHttpResponse && strpos($line, ':') !== false) { | |
| list($key, $value) = explode(':', $line, 2); | |
| $responseHeaders[trim($key)] = trim($value); | |
| } | |
| } else { | |
| $responseBody .= $line . "\n"; | |
| } | |
| } | |
| $responseBody = trim($responseBody); | |
| // Split into lines and capture line endings | |
| $parts = preg_split('/(\r\n|\n)/', $part, -1, PREG_SPLIT_DELIM_CAPTURE); | |
| $inHeaders = true; | |
| $responseHeaders = array(); | |
| $responseBody = ''; | |
| $statusCode = null; | |
| $foundHttpResponse = false; | |
| // $parts is an array alternating between line content and line ending | |
| $numParts = count($parts); | |
| for ($i = 0; $i < $numParts; $i += 2) { | |
| $line = isset($parts[$i]) ? $parts[$i] : ''; | |
| $lineEnding = isset($parts[$i + 1]) ? $parts[$i + 1] : ''; | |
| // Remove only trailing \r if present (for header parsing) | |
| $headerLine = rtrim($line, "\r"); | |
| if ($inHeaders) { | |
| if (trim($headerLine) === '') { | |
| // Only switch to body parsing if we've found an HTTP response line | |
| if ($foundHttpResponse) { | |
| $inHeaders = false; | |
| } | |
| continue; | |
| } | |
| if (strpos($headerLine, 'HTTP/') === 0) { | |
| $statusParts = explode(' ', $headerLine, 3); | |
| $statusCode = (array_key_exists(1, $statusParts) && $statusParts[1] !== null) ? $statusParts[1] : (string)HttpStatusCode::OK; | |
| $foundHttpResponse = true; | |
| continue; | |
| } | |
| // Only parse headers after we've found the HTTP response line | |
| if ($foundHttpResponse && strpos($headerLine, ':') !== false) { | |
| list($key, $value) = explode(':', $headerLine, 2); | |
| $responseHeaders[trim($key)] = trim($value); | |
| } | |
| } else { | |
| // Preserve original line endings in body | |
| $responseBody .= $line . $lineEnding; | |
| } | |
| } | |
| // Optionally trim only leading/trailing whitespace, not line endings | |
| $responseBody = trim($responseBody, "\r\n\t "); |
Copilot
AI
Nov 20, 2025
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.
Missing method docblock. Public method getBody() should include documentation describing that it returns an array of decoded bodies from all individual responses, following the documentation pattern used in the codebase (e.g., ODataResponse.php lines 102-106).
Copilot
AI
Nov 20, 2025
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.
Missing method docblock. Public method getRawBody() should include documentation, following the pattern in ODataResponse.php (lines 112-120).
Copilot
AI
Nov 20, 2025
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.
Missing method docblock. Public method getStatus() should include documentation, following the pattern in ODataResponse.php (lines 122-130).
Copilot
AI
Nov 20, 2025
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.
Missing method docblock. Public method getHeaders() should include documentation, following the pattern in ODataResponse.php (lines 132-140).
Copilot
AI
Nov 20, 2025
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.
Missing method docblock. Public method getResponses() should include documentation describing that it returns an array of ODataResponse objects parsed from the batch response.
Copilot
AI
Nov 20, 2025
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.
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,31 @@ | ||||||||
| <?php | ||||||||
|
|
||||||||
| namespace SaintSystems\OData; | ||||||||
|
|
||||||||
| class ODataResponseFactory | ||||||||
| { | ||||||||
| public static function create(IODataRequest $request, ?string $body = null, ?string $statusCode = null, array $headers = array()) | ||||||||
|
||||||||
| public static function create(IODataRequest $request, ?string $body = null, ?string $statusCode = null, array $headers = array()) | |
| public static function create(IODataRequest $request, ?string $body = null, ?string $statusCode = null, array $headers = array()): IODataResponse |
Copilot
AI
Nov 20, 2025
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 batch response detection logic is overly strict by checking for 'boundary=batchresponse_' specifically. According to the OData specification, any boundary string is valid as long as Content-Type is 'multipart/mixed'. The current implementation would fail for batch responses with custom boundary names that don't start with 'batchresponse_'. Consider checking only for 'multipart/mixed' content type and then letting the ODataBatchResponse class validate the boundary pattern internally.
| strpos($contentType, 'multipart/mixed') === 0 && | |
| strpos($contentType, 'boundary=batchresponse_') !== false) { | |
| strpos($contentType, 'multipart/mixed') === 0) { |
Copilot
AI
Nov 20, 2025
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.
Missing class-level and method docblocks. The factory class and its methods should include documentation describing their purpose, parameters, and return types, following the existing codebase convention seen in ODataResponse.php.
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.
Missing file-level and interface docblocks. The interface should include documentation describing its purpose and relationship to IODataResponse, following the codebase convention (e.g., as seen in other interface files).