From dea639e3f6d404650781441a77f1eeb5a994734f Mon Sep 17 00:00:00 2001 From: Daniel Lockyer Date: Tue, 12 Mar 2024 11:49:24 +0100 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20Fixed=20returning=20HTTP=20500?= =?UTF-8?q?=20response=20when=20recommendations=20check=20fails?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ref ENG-737 ref https://linear.app/tryghost/issue/ENG-737/http-500-errors-from-recommendations-check-endpoint - it's still possible for `this.#externalRequest.get` to throw, like if DNS resolution fails - we want to try-catch this so we don't throw from this function and return a HTTP 500 to the user - instead, we can just return `undefined`, which is the fallback - adds a breaking test too --- .../src/RecommendationMetadataService.ts | 40 +++++++++++-------- .../RecommendationMetadataService.test.ts | 8 ++++ 2 files changed, 31 insertions(+), 17 deletions(-) diff --git a/ghost/recommendations/src/RecommendationMetadataService.ts b/ghost/recommendations/src/RecommendationMetadataService.ts index 9d6e5f947c..5006c584f1 100644 --- a/ghost/recommendations/src/RecommendationMetadataService.ts +++ b/ghost/recommendations/src/RecommendationMetadataService.ts @@ -40,25 +40,31 @@ export class RecommendationMetadataService { } async #fetchJSON(url: URL, options?: {timeout?: number}) { - // default content type is application/x-www-form-encoded which is what we need for the webmentions spec - const response = await this.#externalRequest.get(url.toString(), { - throwHttpErrors: false, - maxRedirects: 10, - followRedirect: true, - timeout: 15000, - retry: { - // Only retry on network issues, or specific HTTP status codes - limit: 3 - }, - ...options - }); + // Even though we have throwHttpErrors: false, we still need to catch DNS errors + // that can arise from externalRequest, otherwise we'll return a HTTP 500 to the user + try { + // default content type is application/x-www-form-encoded which is what we need for the webmentions spec + const response = await this.#externalRequest.get(url.toString(), { + throwHttpErrors: false, + maxRedirects: 10, + followRedirect: true, + timeout: 15000, + retry: { + // Only retry on network issues, or specific HTTP status codes + limit: 3 + }, + ...options + }); - if (response.statusCode >= 200 && response.statusCode < 300) { - try { - return JSON.parse(response.body); - } catch (e) { - return undefined; + if (response.statusCode >= 200 && response.statusCode < 300) { + try { + return JSON.parse(response.body); + } catch (e) { + return undefined; + } } + } catch (e) { + return undefined; } } diff --git a/ghost/recommendations/test/RecommendationMetadataService.test.ts b/ghost/recommendations/test/RecommendationMetadataService.test.ts index dd58cbffea..7d6b1f8f50 100644 --- a/ghost/recommendations/test/RecommendationMetadataService.test.ts +++ b/ghost/recommendations/test/RecommendationMetadataService.test.ts @@ -32,6 +32,7 @@ describe('RecommendationMetadataService', function () { afterEach(function () { nock.cleanAll(); + sinon.restore(); }); it('Returns metadata from the Ghost site', async function () { @@ -181,6 +182,13 @@ describe('RecommendationMetadataService', function () { }); }); + it('does not throw an error even if fetching throws an error', async function () { + // TODO: simulate DNS resolution failures if possible + sinon.stub(got, 'get').rejects(new Error('Failed to fetch')); + + await service.fetch(new URL('https://exampleghostsite.com/subdirectory')); + }); + it('Nullifies empty oembed data', async function () { nock('https://exampleghostsite.com') .get('/subdirectory/members/api/site')