From 4c0174b5c15557a0629ffe36c37877591a16d4f4 Mon Sep 17 00:00:00 2001 From: Melroy van den Berg Date: Thu, 19 Sep 2024 21:28:48 +0200 Subject: [PATCH 1/9] Reduce error pollution even more on post request failures --- src/Service/ActivityPub/ApHttpClient.php | 48 ++++++++++++++---------- 1 file changed, 28 insertions(+), 20 deletions(-) diff --git a/src/Service/ActivityPub/ApHttpClient.php b/src/Service/ActivityPub/ApHttpClient.php index 42ff00850..60b017fda 100644 --- a/src/Service/ActivityPub/ApHttpClient.php +++ b/src/Service/ActivityPub/ApHttpClient.php @@ -101,8 +101,8 @@ private function getActivityObjectImpl(string $url): ?string $statusCode = $r->getStatusCode(); // Accepted status code are 2xx or 410 (used Tombstone types) if (!str_starts_with((string) $statusCode, '2') && 410 !== $statusCode) { - // Do NOT include the content in the error message, this will be often a full HTML page - throw new InvalidApPostException("Invalid status code while getting: $url : $statusCode"); + // Do NOT include the response content in the error message, this will be often a full HTML page + throw new InvalidApPostException("Invalid status code while getting: $url, status code: $statusCode"); } // Read also non-OK responses (like 410) by passing 'false' @@ -317,8 +317,8 @@ private function getCollectionObjectImpl(string $apAddress): ?string $statusCode = $response->getStatusCode(); // Accepted status code are 2xx or 410 (used Tombstone types) if (!str_starts_with((string) $statusCode, '2') && 410 !== $statusCode) { - // Do NOT include the content in the error message, this will be often a full HTML page - throw new InvalidApPostException("Invalid status code while getting: $apAddress : $statusCode"); + // Do NOT include the response content in the error message, this will be often a full HTML page + throw new InvalidApPostException("Invalid status code while getting: $apAddress, status code: $statusCode"); } } catch (\Exception $e) { $this->logRequestException($response, $apAddress, 'ApHttpClient:getCollectionObject', $e); @@ -379,27 +379,35 @@ public function post(string $url, User|Magazine $actor, ?array $body = null): vo return; } + $jsonBody = json_encode($body ?? []); + $this->logger->debug("ApHttpClient:post:url: $url"); - $this->logger->debug('ApHttpClient:post:body '.json_encode($body ?? [])); + $this->logger->debug("ApHttpClient:post:body $jsonBody"); // Set-up request - $client = new CurlHttpClient(); - $response = $client->request('POST', $url, [ - 'max_duration' => self::MAX_DURATION, - 'timeout' => self::TIMEOUT, - 'body' => json_encode($body), - 'headers' => $this->getHeaders($url, $actor, $body), - ]); + try { + $client = new CurlHttpClient(); + $response = $client->request('POST', $url, [ + 'max_duration' => self::MAX_DURATION, + 'timeout' => self::TIMEOUT, + 'body' => $jsonBody, + 'headers' => $this->getHeaders($url, $actor, $body), + ]); - if (!str_starts_with((string) $response->getStatusCode(), '2')) { - throw new InvalidApPostException("Post fail: $url, ".substr($response->getContent(false), 0, 1000).' '.json_encode($body)); - } + $statusCode = $response->getStatusCode(); + if (!str_starts_with((string) $statusCode, '2')) { + // Do NOT include the response content in the error message, this will be often a full HTML page + throw new InvalidApPostException("Post failed: $url, status code: $statusCode, request body: $jsonBody"); + } - // build cache - $item = $this->cache->getItem($cacheKey); - $item->set(true); - $item->expiresAt(new \DateTime('+45 minutes')); - $this->cache->save($item); + // build cache + $item = $this->cache->getItem($cacheKey); + $item->set(true); + $item->expiresAt(new \DateTime('+45 minutes')); + $this->cache->save($item); + } catch (\Exception $e) { + $this->logRequestException($response, $url, 'ApHttpClient:post', $e); + } } public function fetchInstanceNodeInfoEndpoints(string $domain, bool $decoded = true): array|string|null From df86ed45557abd386f4c91a10725be3c39cf3529 Mon Sep 17 00:00:00 2001 From: Melroy van den Berg Date: Thu, 19 Sep 2024 21:32:15 +0200 Subject: [PATCH 2/9] Putting the build cache outside of the try/cache --- src/Service/ActivityPub/ApHttpClient.php | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/Service/ActivityPub/ApHttpClient.php b/src/Service/ActivityPub/ApHttpClient.php index 60b017fda..9cd4b3dd7 100644 --- a/src/Service/ActivityPub/ApHttpClient.php +++ b/src/Service/ActivityPub/ApHttpClient.php @@ -399,15 +399,16 @@ public function post(string $url, User|Magazine $actor, ?array $body = null): vo // Do NOT include the response content in the error message, this will be often a full HTML page throw new InvalidApPostException("Post failed: $url, status code: $statusCode, request body: $jsonBody"); } - - // build cache - $item = $this->cache->getItem($cacheKey); - $item->set(true); - $item->expiresAt(new \DateTime('+45 minutes')); - $this->cache->save($item); } catch (\Exception $e) { $this->logRequestException($response, $url, 'ApHttpClient:post', $e); } + + // build cache + $item = $this->cache->getItem($cacheKey); + $item->set(true); + $item->expiresAt(new \DateTime('+45 minutes')); + $this->cache->save($item); + } public function fetchInstanceNodeInfoEndpoints(string $domain, bool $decoded = true): array|string|null From 9cfe44b0bbcbc2b195394ac33a857a87ea187373 Mon Sep 17 00:00:00 2001 From: Melroy van den Berg Date: Thu, 19 Sep 2024 21:33:46 +0200 Subject: [PATCH 3/9] Remove empty line --- src/Service/ActivityPub/ApHttpClient.php | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Service/ActivityPub/ApHttpClient.php b/src/Service/ActivityPub/ApHttpClient.php index 9cd4b3dd7..60ee3ac29 100644 --- a/src/Service/ActivityPub/ApHttpClient.php +++ b/src/Service/ActivityPub/ApHttpClient.php @@ -408,7 +408,6 @@ public function post(string $url, User|Magazine $actor, ?array $body = null): vo $item->set(true); $item->expiresAt(new \DateTime('+45 minutes')); $this->cache->save($item); - } public function fetchInstanceNodeInfoEndpoints(string $domain, bool $decoded = true): array|string|null From d526643de9389b07436b950f53e9ac076883cd25 Mon Sep 17 00:00:00 2001 From: Melroy van den Berg Date: Thu, 19 Sep 2024 21:40:04 +0200 Subject: [PATCH 4/9] Reduce scope of var, just like the rest of the code --- src/Service/ActivityPub/ApHttpClient.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Service/ActivityPub/ApHttpClient.php b/src/Service/ActivityPub/ApHttpClient.php index 60ee3ac29..dda560a53 100644 --- a/src/Service/ActivityPub/ApHttpClient.php +++ b/src/Service/ActivityPub/ApHttpClient.php @@ -90,8 +90,8 @@ private function getActivityObjectImpl(string $url): ?string { $this->logger->debug("ApHttpClient:getActivityObject:url: $url"); - $client = new CurlHttpClient(); try { + $client = new CurlHttpClient(); $r = $client->request('GET', $url, [ 'max_duration' => self::MAX_DURATION, 'timeout' => self::TIMEOUT, From 73d9c8277c4f8b7033295ae4798cc562afe5e033 Mon Sep 17 00:00:00 2001 From: Melroy van den Berg Date: Thu, 19 Sep 2024 21:52:27 +0200 Subject: [PATCH 5/9] Small refactoring --- src/Service/ActivityPub/ApHttpClient.php | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/Service/ActivityPub/ApHttpClient.php b/src/Service/ActivityPub/ApHttpClient.php index dda560a53..df0f3d475 100644 --- a/src/Service/ActivityPub/ApHttpClient.php +++ b/src/Service/ActivityPub/ApHttpClient.php @@ -89,16 +89,16 @@ public function getActivityObject(string $url, bool $decoded = true): array|stri private function getActivityObjectImpl(string $url): ?string { $this->logger->debug("ApHttpClient:getActivityObject:url: $url"); - + $content = null; try { $client = new CurlHttpClient(); - $r = $client->request('GET', $url, [ + $response = $client->request('GET', $url, [ 'max_duration' => self::MAX_DURATION, 'timeout' => self::TIMEOUT, 'headers' => $this->getInstanceHeaders($url), ]); - $statusCode = $r->getStatusCode(); + $statusCode = $response->getStatusCode(); // Accepted status code are 2xx or 410 (used Tombstone types) if (!str_starts_with((string) $statusCode, '2') && 410 !== $statusCode) { // Do NOT include the response content in the error message, this will be often a full HTML page @@ -106,10 +106,10 @@ private function getActivityObjectImpl(string $url): ?string } // Read also non-OK responses (like 410) by passing 'false' - $content = $r->getContent(false); + $content = $response->getContent(false); $this->logger->debug('ApHttpClient:getActivityObject:url: {url} - content: {content}', ['url' => $url, 'content' => $content]); } catch (\Exception $e) { - $this->logRequestException($r, $url, 'ApHttpClient:getActivityObject', $e); + $this->logRequestException($response, $url, 'ApHttpClient:getActivityObject', $e); } return $content; @@ -171,19 +171,19 @@ public function getWebfingerObject(string $url): ?array private function getWebfingerObjectImpl(string $url): ?string { $this->logger->debug("ApHttpClient:getWebfingerObject:url: $url"); - $r = null; + $response = null; try { $client = new CurlHttpClient(); - $r = $client->request('GET', $url, [ + $response = $client->request('GET', $url, [ 'max_duration' => self::MAX_DURATION, 'timeout' => self::TIMEOUT, 'headers' => $this->getInstanceHeaders($url, null, 'get', ApRequestType::WebFinger), ]); } catch (\Exception $e) { - $this->logRequestException($r, $url, 'ApHttpClient:getWebfingerObject', $e); + $this->logRequestException($response, $url, 'ApHttpClient:getWebfingerObject', $e); } - return $r->getContent(); + return $response->getContent(); } private function getActorCacheKey(string $apProfileId): string @@ -354,7 +354,7 @@ private function logRequestException(?ResponseInterface $response, string $reque 'content' => $content, ]); } - throw $e; + throw $e; // re-throw the exception } /** From b37124dab88bc64b2e1c8826fb39cbea1df4ef23 Mon Sep 17 00:00:00 2001 From: Melroy van den Berg Date: Fri, 20 Sep 2024 17:09:50 +0200 Subject: [PATCH 6/9] Add code docs + remove the word "get". Since it can both be a get or post request now. --- src/Service/ActivityPub/ApHttpClient.php | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/Service/ActivityPub/ApHttpClient.php b/src/Service/ActivityPub/ApHttpClient.php index df0f3d475..6115b64cb 100644 --- a/src/Service/ActivityPub/ApHttpClient.php +++ b/src/Service/ActivityPub/ApHttpClient.php @@ -328,6 +328,16 @@ private function getCollectionObjectImpl(string $apAddress): ?string return $response->getContent(); } + /** + * Helper function for logging get/post/.. requests to the error log with additional info. + * + * @param ResponseInterface|null $response Optional response object + * @param string $requestUrl Full URL of the request + * @param string $requestType an additional string where the error happened in the code + * @param \Exception $e Error object + * + * @throws InvalidArgumentException + */ private function logRequestException(?ResponseInterface $response, string $requestUrl, string $requestType, \Exception $e): void { if (null !== $response) { @@ -341,7 +351,7 @@ private function logRequestException(?ResponseInterface $response, string $reque // Often 400, 404 errors just return the full HTML page, so we don't want to log the full content of them // We truncate the content to 200 characters max. - $this->logger->error('{type} get fail: {address}, ex: {e}: {msg}. Truncated content: {content}', [ + $this->logger->error('{type} failed: {address}, ex: {e}: {msg}. Truncated content: {content}', [ 'type' => $requestType, 'address' => $requestUrl, 'e' => \get_class($e), From 2202c6c3150af1faac59c5478c5ffa3b02631df4 Mon Sep 17 00:00:00 2001 From: Melroy van den Berg Date: Fri, 20 Sep 2024 17:10:41 +0200 Subject: [PATCH 7/9] And debug log --- src/Service/ActivityPub/ApHttpClient.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Service/ActivityPub/ApHttpClient.php b/src/Service/ActivityPub/ApHttpClient.php index 6115b64cb..e88595320 100644 --- a/src/Service/ActivityPub/ApHttpClient.php +++ b/src/Service/ActivityPub/ApHttpClient.php @@ -329,7 +329,7 @@ private function getCollectionObjectImpl(string $apAddress): ?string } /** - * Helper function for logging get/post/.. requests to the error log with additional info. + * Helper function for logging get/post/.. requests to the error & debug log with additional info. * * @param ResponseInterface|null $response Optional response object * @param string $requestUrl Full URL of the request From 3c0d4b25efd6cb31fac4733ce8bc412fbc4ca57e Mon Sep 17 00:00:00 2001 From: Melroy van den Berg Date: Fri, 20 Sep 2024 17:11:41 +0200 Subject: [PATCH 8/9] Update ApHttpClient.php --- src/Service/ActivityPub/ApHttpClient.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Service/ActivityPub/ApHttpClient.php b/src/Service/ActivityPub/ApHttpClient.php index e88595320..2956f508d 100644 --- a/src/Service/ActivityPub/ApHttpClient.php +++ b/src/Service/ActivityPub/ApHttpClient.php @@ -374,7 +374,7 @@ private function logRequestException(?ResponseInterface $response, string $reque * @param User|Magazine $actor The actor initiating the request, either a User or Magazine object * @param array|null $body (Optional) The body of the POST request. Defaults to null. * - * @throws InvalidApPostException if the POST request fails with a non-2xx response status code + * @throws InvalidApPostException rethrows the error */ public function post(string $url, User|Magazine $actor, ?array $body = null): void { From 48bf9005b5faca0f5ffc962f09783fc574eb9f51 Mon Sep 17 00:00:00 2001 From: Melroy van den Berg Date: Fri, 20 Sep 2024 17:14:41 +0200 Subject: [PATCH 9/9] Fix throw docs --- src/Service/ActivityPub/ApHttpClient.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Service/ActivityPub/ApHttpClient.php b/src/Service/ActivityPub/ApHttpClient.php index 2956f508d..d5139fe1c 100644 --- a/src/Service/ActivityPub/ApHttpClient.php +++ b/src/Service/ActivityPub/ApHttpClient.php @@ -336,7 +336,7 @@ private function getCollectionObjectImpl(string $apAddress): ?string * @param string $requestType an additional string where the error happened in the code * @param \Exception $e Error object * - * @throws InvalidArgumentException + * @throws InvalidApPostException rethrows the error */ private function logRequestException(?ResponseInterface $response, string $requestUrl, string $requestType, \Exception $e): void { @@ -374,7 +374,7 @@ private function logRequestException(?ResponseInterface $response, string $reque * @param User|Magazine $actor The actor initiating the request, either a User or Magazine object * @param array|null $body (Optional) The body of the POST request. Defaults to null. * - * @throws InvalidApPostException rethrows the error + * @throws InvalidApPostException if the POST request fails with a non-2xx response status code */ public function post(string $url, User|Magazine $actor, ?array $body = null): void {