fix(task) #35 #36 #37 #38 #39 RequestTask and Client fixes, #31 add missing tests

CHANGELOG.md

@@ -1,6 +1,16 @@
 Latest
 ------
 
+### Changes
+* [#31](https://github.com/cleverage/rest-process-bundle/issues/31) Add missing tests: RequestTask and Client with a mocked HTTP client, MissingClientException, bundle and DI extension.
+* [#39](https://github.com/cleverage/rest-process-bundle/issues/39) Add `Client::getUri()`, deprecate the misspelled `geUri()`; type `MissingClientException::create()`. Update documentation, add tests.
+
+### Fixes
+* [#35](https://github.com/cleverage/rest-process-bundle/issues/35) Fix RequestTask: a `3xx` / `4xx` / `5xx` status code listed in `valid_response_code` outputs the response body (the task still failed). Update documentation, add tests.
+* [#36](https://github.com/cleverage/rest-process-bundle/issues/36) Fix RequestTask: log transport errors (the log was lost, reading the response details threw again). Update documentation, add tests.
+* [#37](https://github.com/cleverage/rest-process-bundle/issues/37) Fix RequestTask `log_response`: log the requested URL, and the status code, headers and content of the response (instead of the configured URL and the response object). Update documentation, add tests.
+* [#38](https://github.com/cleverage/rest-process-bundle/issues/38) Fix RequestTask: throw an explicit `\UnexpectedValueException` on a non-array input; Client: convert scalar URL parameters to strings (a `TypeError` was triggered). Update documentation, add tests.
+
 v3.1
 ------
 

docs/reference/client.md

@@ -46,8 +46,9 @@ Any other key throws a `Symfony\Component\OptionsResolver\Exception\UndefinedOpt
 
 The request is then built this way:
 - **URL**: `<base URI>/<url>`, the leading `/` of `url` being removed. Then each `{key}` of `url_parameters` is
-  replaced by its value, encoded with [`rawurlencode()`](https://www.php.net/manual/en/function.rawurlencode.php)
-  (values must be strings).
+  replaced by its value, converted to a string and encoded with
+  [`rawurlencode()`](https://www.php.net/manual/en/function.rawurlencode.php) (a non-scalar value throws an
+  `\UnexpectedValueException`).
 - **Method**: must be one of `HEAD`, `GET`, `POST`, `PUT`, `DELETE`, `OPTIONS`, `TRACE`, `PATCH` (case-sensitive),
   otherwise a `CleverAge\RestProcessBundle\Exception\RestRequestException` (`<method> is not an HTTP method`) is thrown.
 - **Headers**: `headers`, plus `Content-Type: <sends>` and `Accept: <expects>` when these options are not empty
@@ -81,14 +82,14 @@ A client must implement `ClientInterface`:
 | Method                                                | Description                                                                                  |
 |-------------------------------------------------------|----------------------------------------------------------------------------------------------|
 | `getCode(): string`                                   | Code of the client, used by the `client` option of the task. Must be unique                  |
-| `geUri(): string`                                     | Base URI of the API (the method name is misspelled, `geUri`, in the interface)               |
+| `geUri(): string`                                     | **Deprecated** (misspelled): base URI, use `getUri()` of the default `Client`                |
 | `setUri(string $uri): void`                           | Change the base URI                                                                          |
 | `call(array $options = []): ResponseInterface`        | Send the request described by the request options, return a Symfony HttpClient response      |
 
 The simplest way is to extend the default `Client` and override one of its protected methods:
 - `configureOptions(OptionsResolver $resolver)`: request options accepted by `call()`
 - `getRequestOptions(array $options)`: Symfony HttpClient options (headers, `json`, `query`, `body`...)
-- `getApiUrl()`: base URI used to build the request URL (defaults to `geUri()`)
+- `getApiUrl()`: base URI used to build the request URL (defaults to `getUri()`)
 - `constructUri(array $options)` / `replaceParametersInUri(string $uri, array $options)`: URL construction
 
 Examples

docs/reference/tasks/request_task.md

@@ -16,13 +16,14 @@ Accepted inputs
 
 `array` or empty value (`null`, `[]`...): request options overriding the task options (shallow merge, the input wins).
 Allowed keys are `url`, `method`, `headers`, `url_parameters`, `sends`, `expects` and `data`: any other key is passed to
-the client, which rejects it (the default client throws an `UndefinedOptionsException`).
+the client, which rejects it (the default client throws an `UndefinedOptionsException`). Any other non-empty input
+(e.g. a `string`) throws an `\UnexpectedValueException`.
 
 Possible outputs
 ----------------
 
 `string`: the body of the response (empty string for a `204 No Content`), as returned by
-`Symfony\Contracts\HttpClient\ResponseInterface::getContent()`. It is not decoded: chain a
+`Symfony\Contracts\HttpClient\ResponseInterface::getContent(false)`. It is not decoded: chain a
 [DeserializerTask](https://github.com/cleverage/process-bundle/blob/main/docs/reference/tasks/deserializer_task.md) or a
 [TransformerTask](https://github.com/cleverage/process-bundle/blob/main/docs/reference/tasks/transformer_task.md) to
 decode it.
@@ -39,12 +40,12 @@ Options
 | `url`                 | `string`                  |  **X**   |                    | Path of the endpoint, appended to the client base URI (a leading `/` is optional). May contain `{placeholders}` replaced by `url_parameters`   |
 | `method`              | `string`                  |  **X**   |                    | HTTP method, in uppercase, among `HEAD`, `GET`, `POST`, `PUT`, `DELETE`, `OPTIONS`, `TRACE`, `PATCH` (checked by the default client)          |
 | `headers`             | `array`                   |          | `[]`               | HTTP headers, as `name => value`                                                                                                               |
-| `url_parameters`      | `array`                   |          | `[]`               | List of `placeholder => value`: each `{placeholder}` of the URL is replaced by the URL-encoded value (values must be strings)                  |
+| `url_parameters`      | `array`                   |          | `[]`               | List of `placeholder => value`: each `{placeholder}` of the URL is replaced by the URL-encoded value (scalar values, converted to strings)     |
 | `data`                | `array`, `string`, `null` |          | `null`             | Payload of the request, sent as JSON body, query string or raw body depending on `method` and `sends` (see [REST client](../client.md#request-options)) |
 | `sends`               | `string`                  |          | `application/json` | Value of the `Content-Type` header (not sent if empty)                                                                                         |
 | `expects`             | `string`                  |          | `application/json` | Value of the `Accept` header (not sent if empty)                                                                                               |
 | `valid_response_code` | `array`                   |          | `[200, 201, 204]`  | List of the [HTTP status codes](https://en.wikipedia.org/wiki/List_of_HTTP_status_codes) considered as a success                               |
-| `log_response`        | `bool`                    |          | `false`            | Log the request options and the response object (`debug` level) once the response is received                                                 |
+| `log_response`        | `bool`                    |          | `false`            | Log the requested URL, the request options, and the status code, headers and content of the response (`debug` level)                           |
 
 Options are resolved once per process execution: [contextual values](https://github.com/cleverage/process-bundle/blob/main/docs/01-quick_start.md#contextual-values)
 like `'{{ code }}'` (passed with `-c code:"'value'"`) are allowed in any option. Use the input to change the request for
@@ -164,9 +165,10 @@ Notes
 * When the status code is not in `valid_response_code`, the task sets the raw response body as error output, then
   fails with an `Invalid response code` exception, logged with the response headers and body. The process then follows
   the task `error_strategy`: `skip` goes on with the next item, `stop` stops the process.
-* The response content is read with `getContent()`, which throws for `3xx`, `4xx` and `5xx` status codes: adding
-  such a code to `valid_response_code` only prevents the error log, the task still fails. Redirections are followed by
-  the HTTP client, so a `3xx` status is only received when redirections are disabled or exceeded.
-* A transport error (DNS failure, timeout...) or an unknown `method` makes the task fail as well.
+* A `3xx`, `4xx` or `5xx` status code listed in `valid_response_code` is handled as a success: the response body is
+  output. Redirections are followed by the HTTP client, so a `3xx` status is only received when redirections are
+  disabled or exceeded.
+* A transport error (DNS failure, timeout...) makes the task fail as well: it is logged (`REST request failed`, without
+  the response headers and body) then thrown. An unknown `method` makes the task fail too.
 * A `client` that is not registered throws a `CleverAge\RestProcessBundle\Exception\MissingClientException`
   (`No rest client with code : <code>`) on the first execution of the task.

src/Client/Client.php

@@ -42,11 +42,19 @@ public function getCode(): string
         return $this->code;
     }
 
-    public function geUri(): string
+    public function getUri(): string
     {
         return $this->uri;
     }
 
+    /**
+     * @deprecated typo, use getUri() instead
+     */
+    public function geUri(): string
+    {
+        return $this->getUri();
+    }
+
     public function setUri(string $uri): void
     {
         $this->uri = $uri;
@@ -182,7 +190,7 @@ protected function constructUri(array $options): string
 
     protected function getApiUrl(): string
     {
-        return $this->geUri();
+        return $this->getUri();
     }
 
     /**
@@ -199,13 +207,15 @@ static function (&$item, $key) {
                     $item = '{'.$item.'}';
                 }
             );
-            /** @var array<string> $replace */
-            $replace = array_values($options['url_parameters']);
-            array_walk(
-                $replace,
-                static function (&$item, $key) {
-                    $item = rawurlencode($item);
-                }
+            $replace = array_map(
+                static function (mixed $value): string {
+                    if (!\is_scalar($value)) {
+                        throw new \UnexpectedValueException(\sprintf('URL parameters must be scalar values, %s given', get_debug_type($value)));
+                    }
+
+                    return rawurlencode((string) $value);
+                },
+                array_values($options['url_parameters'])
             );
 
             $uri = str_replace($search, $replace, $uri);

src/Client/ClientInterface.php

@@ -25,6 +25,9 @@ interface ClientInterface
      */
     public function getCode(): string;
 
+    /**
+     * @deprecated typo, implement and use getUri() instead (it will replace this method in the next major version)
+     */
     public function geUri(): string;
 
     public function setUri(string $uri): void;

src/Exception/MissingClientException.php

@@ -20,12 +20,7 @@
  */
 class MissingClientException extends RestException
 {
-    /**
-     * @param string $code
-     *
-     * @return MissingClientException
-     */
-    public static function create($code)
+    public static function create(string $code): self
     {
         $errorStr = "No rest client with code : {$code}";
 

src/Task/RequestTask.php

@@ -26,6 +26,7 @@
 use Symfony\Contracts\HttpClient\Exception\RedirectionExceptionInterface;
 use Symfony\Contracts\HttpClient\Exception\ServerExceptionInterface;
 use Symfony\Contracts\HttpClient\Exception\TransportExceptionInterface;
+use Symfony\Contracts\HttpClient\ResponseInterface;
 
 /**
  * @phpstan-type Options array{
@@ -76,19 +77,22 @@ public function execute(ProcessState $state): void
             ['requestOptions' => $requestOptions]
         );
         $response = $this->registry->getClient($options['client'])->call($requestOptions);
-        if ($options['log_response']) {
-            $this->logger->debug(
-                "Response received from '{$options['url']}'",
-                [
-                    'requestOptions' => $requestOptions,
-                    'result' => $response,
-                ]
-            );
-        }
 
-        // Handle empty results
         try {
-            if (!\in_array($response->getStatusCode(), $options['valid_response_code'], false)) {
+            $statusCode = $response->getStatusCode();
+            if ($options['log_response']) {
+                $this->logger->debug(
+                    "Response received from '{$requestOptions['url']}'",
+                    [
+                        'requestOptions' => $requestOptions,
+                        'status_code' => $statusCode,
+                        'headers' => $response->getHeaders(false),
+                        'content' => $response->getContent(false),
+                    ]
+                );
+            }
+
+            if (!\in_array($statusCode, $options['valid_response_code'], false)) {
                 $state->setErrorOutput($response->getContent(false));
 
                 if (TaskConfiguration::STRATEGY_SKIP === $state->getTaskConfiguration()->getErrorStrategy()) {
@@ -100,38 +104,41 @@ public function execute(ProcessState $state): void
                 throw new \Exception('Invalid response code');
             }
 
-            $state->setOutput($response->getContent());
+            // The status code is valid: do not throw for 3xx / 4xx / 5xx codes listed in valid_response_code
+            $state->setOutput($response->getContent(false));
         } catch (\Throwable $e) {
-            $allowRedirectionException = false;
-            $allowClientException = false;
-            foreach ($options['valid_response_code'] as $code) {
-                if ($code >= 300 && $code < 400) {
-                    $allowRedirectionException = true;
-                }
-                if ($code >= 400 && $code < 500) {
-                    $allowClientException = true;
-                }
-            }
-            if ((!$allowRedirectionException || !$e instanceof RedirectionExceptionInterface)
-                && (!$allowClientException || !$e instanceof ClientExceptionInterface)
-            ) {
-                $this->logger->error(
-                    'REST request failed',
-                    [
-                        'client' => $options['client'],
-                        'options' => $options,
-                        'request_options' => $requestOptions,
-                        'message' => $e->getMessage(),
-                        'raw_headers' => $response->getHeaders(false),
-                        'raw_body' => $response->getContent(false),
-                    ]
-                );
-            }
+            $this->logger->error(
+                'REST request failed',
+                [
+                    'client' => $options['client'],
+                    'options' => $options,
+                    'request_options' => $requestOptions,
+                    'message' => $e->getMessage(),
+                    ...$this->getResponseDetails($response),
+                ]
+            );
 
             throw $e;
         }
     }
 
+    /**
+     * Headers and body of the response, for the error log: not available after a transport error.
+     *
+     * @return array{raw_headers?: array<string, list<string>>, raw_body?: string}
+     */
+    protected function getResponseDetails(ResponseInterface $response): array
+    {
+        try {
+            return [
+                'raw_headers' => $response->getHeaders(false),
+                'raw_body' => $response->getContent(false),
+            ];
+        } catch (TransportExceptionInterface) {
+            return [];
+        }
+    }
+
     /**
      * @throws UndefinedOptionsException
      * @throws AccessException
@@ -182,8 +189,10 @@ protected function getRequestOptions(ProcessState $state): array
             'data' => $options['data'],
         ];
 
-        /** @var array<mixed> $input */
         $input = $state->getInput() ?: [];
+        if (!\is_array($input)) {
+            throw new \UnexpectedValueException(\sprintf('RequestTask expects an array or empty input, %s given', get_debug_type($input)));
+        }
 
         /** @var RequestOptions $mergedOptions */
         $mergedOptions = array_merge($requestOptions, $input);

tests/CleverAgeRestProcessBundleTest.php

@@ -0,0 +1,58 @@
+<?php
+
+declare(strict_types=1);
+
+/*
+ * This file is part of the CleverAge/RestProcessBundle package.
+ *
+ * Copyright (c) Clever-Age
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace CleverAge\RestProcessBundle\Tests;
+
+use CleverAge\RestProcessBundle\CleverAgeRestProcessBundle;
+use CleverAge\RestProcessBundle\Client\Client;
+use CleverAge\RestProcessBundle\Registry\ClientRegistry;
+use PHPUnit\Framework\Attributes\CoversClass;
+use PHPUnit\Framework\Attributes\UsesClass;
+use PHPUnit\Framework\TestCase;
+use Psr\Log\NullLogger;
+use Symfony\Component\DependencyInjection\ContainerBuilder;
+use Symfony\Component\DependencyInjection\Definition;
+use Symfony\Component\HttpClient\MockHttpClient;
+
+#[CoversClass(CleverAgeRestProcessBundle::class)]
+#[UsesClass(ClientRegistry::class)]
+#[UsesClass(Client::class)]
+class CleverAgeRestProcessBundleTest extends TestCase
+{
+    public function testPathIsTheBundleRoot(): void
+    {
+        $path = (new CleverAgeRestProcessBundle())->getPath();
+
+        self::assertSame(\dirname(__DIR__), $path);
+        self::assertDirectoryExists($path.'/config/services');
+    }
+
+    public function testTaggedClientsAreRegistered(): void
+    {
+        $container = new ContainerBuilder();
+        (new CleverAgeRestProcessBundle())->build($container);
+        $container->setDefinition('cleverage_rest_process.registry.client', new Definition(ClientRegistry::class))
+            ->setPublic(true);
+        $container->setDefinition('app.client', new Definition(Client::class, [
+            new Definition(MockHttpClient::class),
+            new Definition(NullLogger::class),
+            'api',
+            'https://example.com/api',
+        ]))->addTag('cleverage.rest.client');
+        $container->compile(true);
+
+        /** @var ClientRegistry $registry */
+        $registry = $container->get('cleverage_rest_process.registry.client');
+        self::assertSame('api', $registry->getClient('api')->getCode());
+    }
+}