-
Notifications
You must be signed in to change notification settings - Fork 103
add NDJSON Serialization class #730
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: main
Are you sure you want to change the base?
Changes from all commits
14b47e2
c1b66ff
66a5f61
509a0c6
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 |
---|---|---|
|
@@ -13,6 +13,7 @@ | |
use Meilisearch\Exceptions\JsonDecodingException; | ||
use Meilisearch\Exceptions\JsonEncodingException; | ||
use Meilisearch\Http\Serialize\Json; | ||
use Meilisearch\Http\Serialize\Ndjson; | ||
use Meilisearch\Meilisearch; | ||
use Psr\Http\Client\ClientExceptionInterface; | ||
use Psr\Http\Client\ClientInterface; | ||
|
@@ -30,7 +31,7 @@ class Client implements Http | |
/** @var array<string,string> */ | ||
private array $headers; | ||
private string $baseUrl; | ||
private Json $json; | ||
private Json|Ndjson $json; | ||
|
||
/** | ||
* @param array<int, string> $clientAgents | ||
|
@@ -53,7 +54,14 @@ public function __construct( | |
if (null !== $apiKey && '' !== $apiKey) { | ||
$this->headers['Authorization'] = \sprintf('Bearer %s', $apiKey); | ||
} | ||
$this->json = new Json(); | ||
} | ||
|
||
public function json(Json|Ndjson|null $json = null) { | ||
if ($json instanceof Json OR $json instanceof Ndjson) { | ||
return $this->json = $json; | ||
} else { | ||
return $this->json ??= new Json; | ||
} | ||
} | ||
|
||
/** | ||
|
@@ -83,9 +91,12 @@ public function post(string $path, $body = null, array $query = [], ?string $con | |
{ | ||
if (!\is_null($contentType)) { | ||
$this->headers['Content-type'] = $contentType; | ||
} elseif (str_ends_with($body, "}\n")) { | ||
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. Relying on str_ends_with($body, "}\n") to detect NDJSON payloads may be brittle. Consider implementing an explicit flag or configuration option to determine when to use NDJSON serialization. Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||
$this->headers['Content-type'] = 'application/x-ndjson'; | ||
$body = $this->json(new Ndjson)->serialize($body); | ||
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. I don't this is a good idea to switch format on the fly. Serializer should be preconfigured imho, or if contentType is manually set, then just instantiate other serializer without overriding the client state |
||
} else { | ||
$this->headers['Content-type'] = 'application/json'; | ||
$body = $this->json->serialize($body); | ||
$body = $this->json(new Json)->serialize($body); | ||
} | ||
$request = $this->requestFactory->createRequest( | ||
'POST', | ||
|
@@ -99,9 +110,12 @@ public function put(string $path, $body = null, array $query = [], ?string $cont | |
{ | ||
if (!\is_null($contentType)) { | ||
$this->headers['Content-type'] = $contentType; | ||
} elseif (str_ends_with($body, "}\n")) { | ||
$this->headers['Content-type'] = 'application/x-ndjson'; | ||
$body = $this->json(new Ndjson)->serialize($body); | ||
} else { | ||
$this->headers['Content-type'] = 'application/json'; | ||
$body = $this->json->serialize($body); | ||
$body = $this->json(new Json)->serialize($body); | ||
} | ||
$request = $this->requestFactory->createRequest( | ||
'PUT', | ||
|
@@ -119,11 +133,17 @@ public function put(string $path, $body = null, array $query = [], ?string $cont | |
*/ | ||
public function patch(string $path, $body = null, array $query = []) | ||
{ | ||
$this->headers['Content-type'] = 'application/json'; | ||
if (str_ends_with($body, "}\n")) { | ||
$this->headers['Content-type'] = 'application/x-ndjson'; | ||
$body = $this->json(new Ndjson)->serialize($body); | ||
} else { | ||
$this->headers['Content-type'] = 'application/json'; | ||
$body = $this->json(new Json)->serialize($body); | ||
} | ||
$request = $this->requestFactory->createRequest( | ||
'PATCH', | ||
$this->baseUrl.$path.$this->buildQueryString($query) | ||
)->withBody($this->streamFactory->createStream($this->json->serialize($body))); | ||
)->withBody($this->streamFactory->createStream($body)); | ||
|
||
return $this->execute($request); | ||
} | ||
|
@@ -181,25 +201,25 @@ private function parseResponse(ResponseInterface $response) | |
} | ||
|
||
if ($response->getStatusCode() >= 300) { | ||
$body = $this->json->unserialize((string) $response->getBody()) ?? $response->getReasonPhrase(); | ||
$body = $this->json()->unserialize((string) $response->getBody()) ?? $response->getReasonPhrase(); | ||
|
||
throw new ApiException($response, $body); | ||
} | ||
|
||
return $this->json->unserialize((string) $response->getBody()); | ||
return $this->json()->unserialize((string) $response->getBody()); | ||
} | ||
|
||
/** | ||
* Checks if any of the header values indicate a JSON response. | ||
* | ||
* @param array $headerValues the array of header values to check | ||
* | ||
* @return bool true if any header value contains 'application/json', otherwise false | ||
* @return bool true if any header value contains 'application/json' or 'application/x-ndjson', otherwise false | ||
*/ | ||
private function isJSONResponse(array $headerValues): bool | ||
{ | ||
$filteredHeaders = array_filter($headerValues, static function (string $headerValue) { | ||
return false !== strpos($headerValue, 'application/json'); | ||
return str_contains($headerValue, 'application/json') || str_contains($headerValue, 'application/x-ndjson'); | ||
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. str_contains is available only from php 8, this library supports php 7.4, so you cannot use it yet |
||
}); | ||
|
||
return \count($filteredHeaders) > 0; | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,36 @@ | ||||||
<?php | ||||||
|
||||||
declare(strict_types=1); | ||||||
|
||||||
namespace Meilisearch\Http\Serialize; | ||||||
|
||||||
use Meilisearch\Exceptions\JsonDecodingException; | ||||||
use Meilisearch\Exceptions\JsonEncodingException; | ||||||
|
||||||
class Ndjson implements SerializerInterface | ||||||
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.
Suggested change
|
||||||
{ | ||||||
private const NDJSON_ENCODE_ERROR_MESSAGE = 'Encoding payload to NDJSON failed: "%s".'; | ||||||
private const NDJSON_DECODE_ERROR_MESSAGE = 'Decoding payload to NDJSON failed: "%s".'; | ||||||
|
||||||
public function serialize($data) | ||||||
{ | ||||||
try { | ||||||
$encoded = json_encode($data, JSON_THROW_ON_ERROR); | ||||||
} catch (\JsonException $e) { | ||||||
throw new JsonEncodingException(\sprintf(self::NDJSON_ENCODE_ERROR_MESSAGE, $e->getMessage()), $e->getCode(), $e); | ||||||
} | ||||||
|
||||||
return $encoded."\n"; | ||||||
} | ||||||
|
||||||
public function unserialize(string $string) | ||||||
{ | ||||||
try { | ||||||
$decoded = json_decode(trim($string), true, 512, JSON_THROW_ON_ERROR); | ||||||
} catch (\JsonException $e) { | ||||||
throw new JsonDecodingException(\sprintf(self::NDJSON_DECODE_ERROR_MESSAGE, $e->getMessage()), $e->getCode(), $e); | ||||||
} | ||||||
|
||||||
return $decoded; | ||||||
} | ||||||
} |
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 should be
Meilisearch\Http\Serialize\SerializerInterface
instead.