-
Notifications
You must be signed in to change notification settings - Fork 123
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
feat: fault tolerant body handling #747
Conversation
This is not true, it's still a property on the adapters rather than a function |
@@ -239,6 +243,7 @@ export type WorktopAdapter = (req: { | |||
|
|||
/** AWS lambda serverless functions */ | |||
const awsLambda: LambdaAdapter = (event, _context, callback) => ({ | |||
// TODO: add safe parse workaround |
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.
Making it a function will btw fix this
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 just commited, but again late for 2 secs before you merged:)
function safeJsonParse(value: string, fallback = {} as any): any {
try {
return JSON.parse(value);
} catch (_err) {
return fallback;
}
}
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.
Ah
but function wrapper is not needed now |
The issue was originally described here: #613
Short Summary:
We assume that the body will be valid JSON. However, it can be empty or contain invalid JSON in some unusual cases. This can cause the server to crash due to an uncaught error triggered by JSON parsing in some of the adapters.
It was suggested to wrap it in a function, but I think that is unnecessary. We already have a promise and just need to safely resolve it to a value that we can then validate.
I suggest catching all promises where a crash can occur and returning an empty object. In
webhookCallback
, we can perform basic validation by checking theupdate_id
field. In the case of an invalid body, we should return a 400 status to the client. Here, we need to add an extra callback to the adapter interface.I added some e2e test cases to check for 400 and 401 errors, but it takes some time (~300-350ms on my machine).
As of
TODO: investigate deno bug that happens when this console logging is removed
- I didn't notice any issues. Maybe it's fixed now?Closes: #697