Skip to content

Commit

Permalink
馃悰 Fixed returning HTTP 500 response when recommendations check fails
Browse files Browse the repository at this point in the history
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
  • Loading branch information
daniellockyer committed Mar 12, 2024
1 parent 5fa4496 commit dea639e
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 17 deletions.
40 changes: 23 additions & 17 deletions ghost/recommendations/src/RecommendationMetadataService.ts
Expand Up @@ -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;
}
}

Expand Down
Expand Up @@ -32,6 +32,7 @@ describe('RecommendationMetadataService', function () {

afterEach(function () {
nock.cleanAll();
sinon.restore();
});

it('Returns metadata from the Ghost site', async function () {
Expand Down Expand Up @@ -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')
Expand Down

0 comments on commit dea639e

Please sign in to comment.