Skip to content

Conversation

@wikando-dt
Copy link
Contributor

@wikando-dt wikando-dt commented Nov 20, 2025

No description provided.

Copy link

Copilot AI left a 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, enabling the library to handle both single and changeset-grouped batch operations. This addresses a key capability gap for clients needing to process multiple OData operations in a single request/response cycle.

Key changes:

  • Introduced interface hierarchy separating entity-specific methods (IODataEntityResponse) from general response methods (IODataResponse) to support both regular and batch responses
  • Added ODataResponseFactory to automatically detect and instantiate the appropriate response type based on Content-Type headers
  • Implemented ODataBatchResponse class with full parsing logic for multipart/mixed batch responses including nested changesets

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 17 comments.

Show a summary per file
File Description
src/IODataResponse.php Removed entity-specific methods to create a more general base interface
src/IODataEntityResponse.php New interface extending IODataResponse with entity-specific methods (getResponseAsObject, getSkipToken, getId)
src/ODataResponse.php Updated to implement IODataEntityResponse instead of IODataResponse
src/ODataResponseFactory.php New factory class for automatic response type detection and instantiation based on Content-Type
src/ODataRequest.php Modified to use ODataResponseFactory instead of direct ODataResponse instantiation
src/ODataBatchResponse.php New class implementing full batch response parsing with support for boundaries, changesets, and mixed success/error responses
tests/ODataBatchResponseTest.php Comprehensive test suite covering standard batch responses, changesets, and mixed success/error scenarios

Comment on lines +195 to +198
public function getRawBody(): ?string
{
return $this->body;
}
Copy link

Copilot AI Nov 20, 2025

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 uses AI. Check for mistakes.
Comment on lines +141 to +175
$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";
}
}
Copy link

Copilot AI Nov 20, 2025

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 uses AI. Check for mistakes.
Comment on lines +13 to +14
strpos($contentType, 'multipart/mixed') === 0 &&
strpos($contentType, 'boundary=batchresponse_') !== false) {
Copy link

Copilot AI Nov 20, 2025

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.

Suggested change
strpos($contentType, 'multipart/mixed') === 0 &&
strpos($contentType, 'boundary=batchresponse_') !== false) {
strpos($contentType, 'multipart/mixed') === 0) {

Copilot uses AI. Check for mistakes.
Comment on lines +186 to +193
public function getBody(): array
{
$bodies = array();
foreach ($this->responses as $response) {
$bodies[] = $response->getBody();
}
return $bodies;
}
Copy link

Copilot AI Nov 20, 2025

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 uses AI. Check for mistakes.
Comment on lines +215 to +218
public function getResponse(int $index): ?ODataResponse
{
return (array_key_exists($index, $this->responses) && $this->responses[$index] !== null) ? $this->responses[$index] : null;
}
Copy link

Copilot AI Nov 20, 2025

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 getResponse() should include documentation describing the @param and @return values.

Copilot uses AI. Check for mistakes.
Comment on lines +205 to +208
public function getHeaders(): array
{
return $this->headers;
}
Copy link

Copilot AI Nov 20, 2025

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 uses AI. Check for mistakes.
Comment on lines +210 to +213
public function getResponses(): array
{
return $this->responses;
}
Copy link

Copilot AI Nov 20, 2025

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 uses AI. Check for mistakes.
Comment on lines +1 to +19
<?php

namespace SaintSystems\OData;

class ODataResponseFactory
{
public static function create(IODataRequest $request, ?string $body = null, ?string $statusCode = null, array $headers = array())
{
$contentType = self::getContentType($headers);

// Batch response detection - Microsoft uses multipart/mixed with batchresponse_ boundary
if ($contentType !== null &&
strpos($contentType, 'multipart/mixed') === 0 &&
strpos($contentType, 'boundary=batchresponse_') !== false) {
return new ODataBatchResponse($request, $body, $statusCode, $headers);
}

return new ODataResponse($request, $body, $statusCode, $headers);
}
Copy link

Copilot AI Nov 20, 2025

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.

Copilot uses AI. Check for mistakes.
Comment on lines +27 to +31
if ($contentType !== null && $contentType !== '' && preg_match('/boundary=(["\']?)([^"\';]+)\1/', $contentType, $matches)) {
$boundary = $matches[2];

if (strpos(strtolower($boundary), 'batchresponse') !== false) {
return $boundary;
Copy link

Copilot AI Nov 20, 2025

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.

Suggested change
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 uses AI. Check for mistakes.
Comment on lines +2 to +4

namespace SaintSystems\OData;

Copy link

Copilot AI Nov 20, 2025

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).

Suggested change
namespace SaintSystems\OData;
/**
* IODataEntityResponse interface file.
*
* Defines the contract for OData entity response objects, which represent
* responses containing single entities or collections of entities from an OData service.
* Extends IODataResponse to provide additional methods for entity-specific
* operations such as object conversion, skip token retrieval, and ID access.
*
* @package SaintSystems\OData
*/
namespace SaintSystems\OData;
/**
* Interface for OData entity response objects.
*
* Extends IODataResponse to provide methods for converting the response
* to SDK objects, retrieving skip tokens for pagination, and accessing
* the entity ID after insert operations. Used by ODataClient to represent
* responses containing entities or entity sets from an OData service.
*
* @see IODataResponse
*/

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant