Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(gatsby-source-contentful): Improve network error handling #30257

Merged
merged 18 commits into from
Mar 29, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
8b454e9
fix: do not log empty values when logging requests
axe312ger Mar 15, 2021
3568845
fix: remove progress total as the real total is not available
axe312ger Mar 15, 2021
227754c
fix: allow passing of any option to Contentful SDK again
axe312ger Mar 15, 2021
8946b74
fix: pretty print Contentful errors after all retry attempts failed
axe312ger Mar 15, 2021
1226330
Merge branch 'master' into fix/contentful-improve-network-error-handling
axe312ger Mar 16, 2021
6da14b6
docs: add reintroduced contentfulClientConfig to the plugin validation
axe312ger Mar 16, 2021
935f1d6
docs: add missing quote
axe312ger Mar 16, 2021
9b03227
fix: allow error responses with JSON strings
axe312ger Mar 16, 2021
f2a6e7b
fix: do not pass error object to reporter.panic to display custom mes…
axe312ger Mar 16, 2021
7f0b22a
style: clean up error message creation function
axe312ger Mar 16, 2021
2d2dc07
Update packages/gatsby-source-contentful/src/gatsby-node.js
axe312ger Mar 16, 2021
e013695
test: revert change to unrelated snapshot
axe312ger Mar 22, 2021
4a8ac8d
style: improve code style
axe312ger Mar 22, 2021
ab66282
style: simplify optional chaining
axe312ger Mar 22, 2021
5a289ff
refactor(contentful): catch string only error responses and simplify
axe312ger Mar 22, 2021
8dca766
style: optional chaning is required for images
axe312ger Mar 23, 2021
dab281c
Merge branch 'master' into fix/contentful-improve-network-error-handling
axe312ger Mar 23, 2021
d4716d5
revert normalize changes
wardpeet Mar 23, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions packages/gatsby-source-contentful/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,12 @@ Number of entries to retrieve from Contentful at a time. Due to some technical l

Number of workers to use when downloading Contentful assets. Due to technical limitations, opening too many concurrent requests can cause stalled downloads. If you encounter this issue you can set this param to a lower number than 50, e.g 25.

**`contentfulClientConfig`** [object][optional] [default: `{}`]

Additional config which will get passed to [Contentfuls JS SDK](https://github.com/contentful/contentful.js#configuration).
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add retryLimit in the SDK readme?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but if you do, pls make sure to add the other potential missing config options there as well :)


Use this with caution, you might override values this plugin does set for you to connect to Contentful.

## Notes on Contentful Content Models

There are currently some things to keep in mind when building your content models at Contentful.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,219 @@
/**
* @jest-environment node
*/

import nock from "nock"
import fetchData from "../fetch"
import { createPluginConfig } from "../plugin-options"

nock.disableNetConnect()

const host = `localhost`
const options = {
spaceId: `12345`,
accessToken: `67890`,
host,
contentfulClientConfig: {
retryLimit: 2,
},
}
const baseURI = `https://${host}`

const start = jest.fn()
const end = jest.fn()
const mockActivity = {
start,
end,
tick: jest.fn(),
done: end,
}

const reporter = {
info: jest.fn(),
verbose: jest.fn(),
panic: jest.fn(e => {
throw e
}),
activityTimer: jest.fn(() => mockActivity),
createProgress: jest.fn(() => mockActivity),
}

const pluginConfig = createPluginConfig(options)

describe(`fetch-retry`, () => {
afterEach(() => {
nock.cleanAll()
reporter.verbose.mockClear()
reporter.panic.mockClear()
})

test(`request retries when network timeout happens`, async () => {
const scope = nock(baseURI)
// Space
.get(`/spaces/${options.spaceId}/`)
.reply(200, { items: [] })
// Locales
.get(`/spaces/${options.spaceId}/environments/master/locales`)
.reply(200, { items: [{ code: `en`, default: true }] })
// Sync
.get(
`/spaces/${options.spaceId}/environments/master/sync?initial=true&limit=100`
)
.times(1)
.replyWithError({ code: `ETIMEDOUT` })
.get(
`/spaces/${options.spaceId}/environments/master/sync?initial=true&limit=100`
)
.reply(200, { items: [] })
// Content types
.get(
`/spaces/${options.spaceId}/environments/master/content_types?skip=0&limit=100&order=sys.createdAt`
)
.reply(200, { items: [] })

await fetchData({ pluginConfig, reporter })

expect(reporter.panic).not.toBeCalled()
expect(scope.isDone()).toBeTruthy()
})

test(`request should fail after to many retries`, async () => {
// Due to the retries, this can take up to 10 seconds
jest.setTimeout(10000)

const scope = nock(baseURI)
// Space
.get(`/spaces/${options.spaceId}/`)
.reply(200, { items: [] })
// Locales
.get(`/spaces/${options.spaceId}/environments/master/locales`)
.reply(200, { items: [{ code: `en`, default: true }] })
// Sync
.get(
`/spaces/${options.spaceId}/environments/master/sync?initial=true&limit=100`
)
.times(3)
.reply(
500,
{
sys: {
type: `Error`,
id: `MockedContentfulError`,
},
message: `Mocked message of Contentful error`,
},
{ [`x-contentful-request-id`]: `123abc` }
)

try {
await fetchData({ pluginConfig, reporter })
jest.fail()
} catch (e) {
const msg = expect(e.context.sourceMessage)
msg.toEqual(
expect.stringContaining(
`Fetching contentful data failed: 500 MockedContentfulError`
)
)
msg.toEqual(expect.stringContaining(`Request ID: 123abc`))
msg.toEqual(
expect.stringContaining(`The request was sent with 3 attempts`)
)
}
expect(reporter.panic).toBeCalled()
expect(scope.isDone()).toBeTruthy()
})
})

describe(`fetch-network-errors`, () => {
test(`catches plain network error`, async () => {
const scope = nock(baseURI)
// Space
.get(`/spaces/${options.spaceId}/`)
.replyWithError({ code: `ECONNRESET` })
try {
await fetchData({
pluginConfig: createPluginConfig({
...options,
contentfulClientConfig: { retryOnError: false },
}),
reporter,
})
jest.fail()
} catch (e) {
expect(e.context.sourceMessage).toEqual(
expect.stringContaining(
`Accessing your Contentful space failed: ECONNRESET`
)
)
}

expect(reporter.panic).toBeCalled()
expect(scope.isDone()).toBeTruthy()
})

test(`catches error with response string`, async () => {
const scope = nock(baseURI)
// Space
.get(`/spaces/${options.spaceId}/`)
.reply(502, `Bad Gateway`)

try {
await fetchData({
pluginConfig: createPluginConfig({
...options,
contentfulClientConfig: { retryOnError: false },
}),
reporter,
})
jest.fail()
} catch (e) {
expect(e.context.sourceMessage).toEqual(
expect.stringContaining(
`Accessing your Contentful space failed: Bad Gateway`
)
)
}

expect(reporter.panic).toBeCalled()
expect(scope.isDone()).toBeTruthy()
})

test(`catches error with response object`, async () => {
const scope = nock(baseURI)
// Space
.get(`/spaces/${options.spaceId}/`)
.reply(429, {
sys: {
type: `Error`,
id: `MockedContentfulError`,
},
message: `Mocked message of Contentful error`,
requestId: `123abc`,
})

try {
await fetchData({
pluginConfig: createPluginConfig({
...options,
contentfulClientConfig: { retryOnError: false },
}),
reporter,
})
jest.fail()
} catch (e) {
const msg = expect(e.context.sourceMessage)

msg.toEqual(
expect.stringContaining(
`Accessing your Contentful space failed: MockedContentfulError`
)
)
msg.toEqual(expect.stringContaining(`Mocked message of Contentful error`))
msg.toEqual(expect.stringContaining(`Request ID: 123abc`))
}

expect(reporter.panic).toBeCalled()
expect(scope.isDone()).toBeTruthy()
})
})
Loading