Issue 687: Throw error when backend error in /router/translate-path

[marcorcau]

to avoid re-generation with 404.

This pull request is for: (mark with an "x")

• examples/*
• modules/next
• packages/next-drupal
• starters/basic-starter
• starters/graphql-starter
• Other

GitHub Issue: #687

• I need help adding tests. (mark with an "x")

Describe your changes

When /router/translate-path?path=xxx response is "Internal server error" (starts with "5") throw Error instead of returning the same as if it was 404 Not Found to avoid re-generation with 404.

[vercel]

Someone is attempting to deploy a commit to the Chapter Three Team on Vercel.

A member of the Team first needs to authorize it.

[JohnAlbin]

Please move this code block inside the if (!response?.ok) { code block below. We don't need to check this if the response is ok.

No need for response.status && since response.status is going to be a number.

Actually, the only "acceptable" error here is 404 not found, right?

We should check

if (response.status === 404) {
 return null
}

And otherwise throw the error. Right?

[marcorcau]

Done !! Makes more sense this way, thanks!

[JohnAlbin]

Why not throw an error that contains info about the server error?

Please add status code and the error message from the JSON body.

Something like:

let errorMessage
try {
  errorMessage = `${response.status} ${(await response.json())?.message}`
} catch (e) {
  errorMessage = `${response.status} ${response.statusText}`
}
throw new Error(errorMessage)

We need to wrap the response.json() in a try/catch block because a 5xx error may cause Drupal to not respond with JSON.

[marcorcau]

Done !! Would this.throwError be preferable instead of throw?

[JohnAlbin]

I just refactored the Jest tests. And the translatePath() tests are now found in packages/next-drupal/tests/DrupalClient/fetch-related-methods.test.ts.

[marcorcau]

That was actually resource-methods.test.ts, right?

[JohnAlbin]

When fetch receives a 500 response, it doesn't throw an error; it simply returns the response with the error information. You've mocked fetch throwing an error (which can happen if you don't pass the right arguments, etc.); see https://developer.mozilla.org/en-US/docs/Web/API/fetch#exceptions

You need to mock a failing response. Fortunately, I just added a test helper function that makes these easy to do.

spyOnFetch({ responseBody: { message: "mocked internal server error" }, status: 500 })

[marcorcau]

Ah yeah. Sorry. I'm not that familiar with it. Fixed.

But I was unable of adding test code for the catch part in translatePath. fetchMockImplementation ensures that is valid json response. Any suggestion or just leave it with the c8 ignore.

[JohnAlbin]

I just noticed that getResourceByPath() and its tests will also need to be updated for this bug.

[marcorcau]

I just noticed that getResourceByPath() and its tests will also need to be updated for this bug.

I've added this to the MR and also a test for it, but now there is a bit of code duplication:

      let errorMessage: string
      try {
        const responseJson = await response.json()
        errorMessage = `${response.status} ${responseJson?.message}`
      } catch (e) {
        /* c8 ignore next 2 */
        errorMessage = `${response.status} ${response.statusText}`
      }
      throw new Error(errorMessage)

Should I refactor that in a common function or wejust accept it?

[JohnAlbin]

Looking at this more, I realize that the fix is a lot more complicated then "just update getResourceByPath()".

I've taken the code from the PR and added to it to make a new PR. #743

Thanks for reporting this bug and thanks for all your effort in fixing it, @marcorcau!