-
Notifications
You must be signed in to change notification settings - Fork 27.5k
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: Contentful webhook body parse. #40732
Conversation
@@ -14,7 +14,7 @@ export default async function handler(req, res) { | |||
} | |||
|
|||
try { | |||
let postSlug = req.body.fields.slug['en-US'] | |||
let postSlug = JSON.parse(req.body).fields.slug['en-US'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the content-type: application/json
header not being sent with the Webhook request? If it is present it should already be parsing the req.body
to JSON automatically.
x-ref:
if (type === 'application/json' || type === 'application/ld+json') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't explicitly check headers but I've ran into this before with a webhook so I knew to try this pretty quickly. I can confirm that it goes from doesn't work
to works
with this change. 😬
I also quickly tried disabling bodyParser
through export const config = { ... }
to see if something else strange was going on but that also did not work as you'd expect.
I can go back and firm up the headers if needed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally the content-type
header is set from the Webhook although since we don't control it sounds alright to land
There’s a step in the README that tells you to update the header in Contentful to In other words, if you follow that then this is a breaking change. Spent a few hours today confused and trying to debug. The README should be updated, or the PR should be reverted. |
Good catch, added a note to that section of the readme instead here #40925 as it's definitely favorable to use the correct |
Bug
fixes #number
contributing.md
Feature
fixes #number
contributing.md
Documentation / Examples
pnpm lint