Skip to content
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

fix: exception handling for server errors with different content type header - fixes #132 #134

Merged
merged 4 commits into from
Jun 7, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .github/workflows/formats.yml
Original file line number Diff line number Diff line change
Expand Up @@ -45,3 +45,6 @@ jobs:

- name: Type Checks
run: composer test:types

- name: Type Coverage
run: composer test:type-coverage min=100
13 changes: 8 additions & 5 deletions composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -22,16 +22,17 @@
"psr/http-message": "^1.1.0|^2.0.0"
},
"require-dev": {
"guzzlehttp/guzzle": "^7.6.1",
"guzzlehttp/guzzle": "^7.7.0",
"guzzlehttp/psr7": "^2.5.0",
"laravel/pint": "^1.10.0",
"nunomaduro/collision": "^7.5.2",
"pestphp/pest": "^2.6.1",
"pestphp/pest-plugin-arch": "^2.1.2",
"pestphp/pest": "dev-develop as 2.6.2",
"pestphp/pest-plugin-arch": "^2.2.0",
"pestphp/pest-plugin-mock": "^2.0.0",
"pestphp/pest-plugin-type-coverage": "^2.0.0",
"phpstan/phpstan": "^1.10.15",
"rector/rector": "^0.14.8",
"symfony/var-dumper": "^6.2.10"
"rector/rector": "^0.16.0",
"symfony/var-dumper": "^6.3.0"
},
"autoload": {
"psr-4": {
Expand Down Expand Up @@ -62,11 +63,13 @@
"test:lint": "pint --test -v",
"test:refactor": "rector --dry-run",
"test:types": "phpstan analyse --ansi",
"test:type-coverage": "pest --type-coverage --min=100",
"test:unit": "pest --colors=always",
"test": [
"@test:lint",
"@test:refactor",
"@test:types",
"@test:type-coverage",
"@test:unit"
]
}
Expand Down
1 change: 1 addition & 0 deletions src/Enums/Transporter/ContentType.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,5 @@ enum ContentType: string
{
case JSON = 'application/json';
case MULTIPART = 'multipart/form-data';
case TEXT_PLAIN = 'text/plain';
}
2 changes: 1 addition & 1 deletion src/Responses/Chat/CreateStreamedResponseDelta.php
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,6 @@ public function toArray(): array
return array_filter([
'role' => $this->role,
'content' => $this->content,
], fn ($value): bool => ! is_null($value));
], fn (string|null $value): bool => ! is_null($value));
}
}
2 changes: 1 addition & 1 deletion src/Responses/Completions/CreateResponseChoice.php
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ public function toArray(): array
return [
'text' => $this->text,
'index' => $this->index,
'logprobs' => $this->logprobs !== null ? $this->logprobs->toArray() : null,
'logprobs' => $this->logprobs instanceof CreateResponseChoiceLogprobs ? $this->logprobs->toArray() : null,
'finish_reason' => $this->finishReason,
];
}
Expand Down
15 changes: 6 additions & 9 deletions src/Testing/ClientFake.php
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
/**
* @noRector Rector\Privatization\Rector\Class_\FinalizeClassesWithoutChildrenRector
*/
class ClientFake implements ClientContract
final class ClientFake implements ClientContract
{
/**
* @var array<array-key, TestRequest>
Expand All @@ -44,10 +44,7 @@ public function addResponses(array $responses): void
$this->responses = [...$this->responses, ...$responses];
}

/**
* @param callable|int|null $callback
*/
public function assertSent(string $resource, $callback = null): void
public function assertSent(string $resource, callable|int|null $callback = null): void
{
if (is_int($callback)) {
$this->assertSentTimes($resource, $callback);
Expand All @@ -61,7 +58,7 @@ public function assertSent(string $resource, $callback = null): void
);
}

protected function assertSentTimes(string $resource, int $times = 1): void
private function assertSentTimes(string $resource, int $times = 1): void
{
$count = count($this->sent($resource));

Expand All @@ -74,7 +71,7 @@ protected function assertSentTimes(string $resource, int $times = 1): void
/**
* @return mixed[]
*/
protected function sent(string $resource, callable $callback = null): array
private function sent(string $resource, callable $callback = null): array
{
if (! $this->hasSent($resource)) {
return [];
Expand All @@ -85,7 +82,7 @@ protected function sent(string $resource, callable $callback = null): array
return array_filter($this->resourcesOf($resource), fn (TestRequest $resource) => $callback($resource->method(), $resource->parameters()));
}

protected function hasSent(string $resource): bool
private function hasSent(string $resource): bool
{
return $this->resourcesOf($resource) !== [];
}
Expand All @@ -111,7 +108,7 @@ public function assertNothingSent(): void
/**
* @return array<array-key, TestRequest>
*/
protected function resourcesOf(string $type): array
private function resourcesOf(string $type): array
{
return array_filter($this->requests, fn (TestRequest $request): bool => $request->resource() === $type);
}
Expand Down
10 changes: 2 additions & 8 deletions src/Testing/Resources/Concerns/Testable.php
Original file line number Diff line number Diff line change
Expand Up @@ -23,18 +23,12 @@ protected function record(string $method, array|string|null $parameters = null):
return $this->fake->record(new TestRequest($this->resource(), $method, $parameters));
}

/**
* @param callable|int|null $callback
*/
public function assertSent($callback = null): void
public function assertSent(callable|int|null $callback = null): void
{
$this->fake->assertSent($this->resource(), $callback);
}

/**
* @param callable|int|null $callback
*/
public function assertNotSent(callable $callback = null): void
public function assertNotSent(callable|int|null $callback = null): void
{
$this->fake->assertNotSent($this->resource(), $callback);
}
Expand Down
5 changes: 3 additions & 2 deletions src/Transporters/HttpTransporter.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
use GuzzleHttp\Exception\ClientException;
use JsonException;
use OpenAI\Contracts\TransporterContract;
use OpenAI\Enums\Transporter\ContentType;
use OpenAI\Exceptions\ErrorException;
use OpenAI\Exceptions\TransporterException;
use OpenAI\Exceptions\UnserializableResponse;
Expand Down Expand Up @@ -48,7 +49,7 @@ public function requestObject(Payload $payload): array|string

$contents = $response->getBody()->getContents();

if ($response->getHeader('Content-Type')[0] === 'text/plain; charset=utf-8') {
if (str_contains($response->getHeaderLine('Content-Type'), ContentType::TEXT_PLAIN->value)) {
return $contents;
}

Expand Down Expand Up @@ -113,7 +114,7 @@ private function throwIfJsonError(ResponseInterface $response, string|ResponseIn
return;
}

if ($response->getheader('Content-Type')[0] !== 'application/json; charset=utf-8') {
if (! str_contains($response->getHeaderLine('Content-Type'), ContentType::JSON->value)) {
return;
}

Expand Down
2 changes: 1 addition & 1 deletion src/ValueObjects/Transporter/Payload.php
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ public function toRequest(BaseUri $baseUri, Headers $headers, QueryParams $query

$request = $psr17Factory->createRequest($this->method->value, $uri);

if ($body !== null) {
if ($body instanceof StreamInterface) {
$request = $request->withBody($body);
}

Expand Down
29 changes: 19 additions & 10 deletions tests/Arch.php
Original file line number Diff line number Diff line change
@@ -1,16 +1,25 @@
<?php

test('contracts')->expect('OpenAI\Contracts')->toOnlyUse([
'OpenAI\ValueObjects',
'OpenAI\Exceptions',
'OpenAI\Resources',
'Psr\Http\Message\ResponseInterface',
'OpenAI\Responses',
]);
test('contracts')
->expect('OpenAI\Contracts')
->toOnlyUse([
'OpenAI\ValueObjects',
'OpenAI\Exceptions',
'OpenAI\Resources',
'Psr\Http\Message\ResponseInterface',
'OpenAI\Responses',
])
->toBeInterfaces();

test('exceptions')->expect('OpenAI\Exceptions')->toOnlyUse([
'Psr\Http\Client',
]);
test('enums')
->expect('OpenAI\Enums')
->toBeEnums();

test('exceptions')
->expect('OpenAI\Exceptions')
->toOnlyUse([
'Psr\Http\Client',
])->toImplement(Throwable::class);

test('resources')->expect('OpenAI\Resources')->toOnlyUse([
'OpenAI\Contracts',
Expand Down
28 changes: 27 additions & 1 deletion tests/Transporters/HttpTransporter.php
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@
]);
});

test('request object server errors', function () {
test('request object server user errors', function () {
$payload = Payload::list('models');

$response = new Response(401, ['Content-Type' => 'application/json; charset=utf-8'], json_encode([
Expand All @@ -109,6 +109,32 @@
});
});

test('request object server errors', function () {
$payload = Payload::create('completions', ['model' => 'gpt-4']);

$response = new Response(401, ['Content-Type' => 'application/json'], json_encode([
'error' => [
'message' => 'That model is currently overloaded with other requests. You can ...',
'type' => 'server_error',
'param' => null,
'code' => null,
],
]));

$this->client
->shouldReceive('sendRequest')
->once()
->andReturn($response);

expect(fn () => $this->http->requestObject($payload))
->toThrow(function (ErrorException $e) {
expect($e->getMessage())->toBe('That model is currently overloaded with other requests. You can ...')
->and($e->getErrorMessage())->toBe('That model is currently overloaded with other requests. You can ...')
->and($e->getErrorCode())->toBeNull()
->and($e->getErrorType())->toBe('server_error');
});
});

test('error code may be null', function () {
$payload = Payload::create('completions', ['model' => 'gpt-42']);

Expand Down
Loading