Improve aws-lambda webhookCallback adapter #418

chrisandrewcl · 2023-05-29T01:12:21Z

Changes:

feat: add aws-lambda-async webhookCallback adapter
fix: apply correct headers to aws-lambda webhookCallback adapter

I did not find any relevant test cases covering adapters, so I am opening this PR with just a few manual tests.

codecov · 2023-05-29T01:21:23Z

Codecov Report

Patch coverage: 3.44% and project coverage change: -0.14 ⚠️

Comparison is base (1505b92) 39.01% compared to head (5504c0c) 38.88%.

❗ Current head 5504c0c differs from pull request most recent head 60d8216. Consider uploading reports for the commit 60d8216 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #418      +/-   ##
==========================================
- Coverage   39.01%   38.88%   -0.14%     
==========================================
  Files          16       16              
  Lines        4831     4850      +19     
  Branches      194      194              
==========================================
+ Hits         1885     1886       +1     
- Misses       2944     2962      +18     
  Partials        2        2

Impacted Files	Coverage Δ
src/convenience/frameworks.ts	`7.88% <3.44%> (-0.20%)`	⬇️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

KnorpelSenf

Thanks! I assume that this closes #412?

Regarding the tests: it's very hard to write tests for adapters without just mocking all the APIs. But since they may change in unexpected ways anyway, there's little reason to test these few lines of code automatically. I trust your manual tests (and the community to complain if stuff breaks).

Did you test it by installing grammY from this branch using GitHub installs?

chrisandrewcl · 2023-05-29T02:12:24Z

I tested it on a node.js side project by switching grammy from the npm version to the fork I have on disk, which I built with deno task backport.

KnorpelSenf · 2023-05-29T06:59:30Z

I tested it on a node.js side project by switching grammy from the npm version to the fork I have on disk, which I built with deno task backport.

Could you try once more using npm install chrisandrewcl/grammY#bugfix/412-aws-lambda just to be extra sure? I'll merge as soon as you confirm that this works. It would be awesome if you could test secret tokens, too. (See https://deno.land/x/[email protected]/mod.ts?s=WebhookOptions#prop_secretToken if you weren't aware.)

chrisandrewcl · 2023-05-29T15:05:24Z

@KnorpelSenf Installing with npm directly from the branch doesn't seem to work because there is a build step to generate the ./out files needed on node_modules, so the installed package ends up unusable. I've tested locally by building with deno task backport and then installing it on my test project. I've double-checked that the installed module comes from my development version (e.g. by changing something in grammy and verifying it is reflected on my test project). Did I miss something in the process?

Regarding the secretToken: it should work, but it doesn't in my case because the handler is receiving the headers normalized to lowercase. I suspect this is by interference of another framework I am experimenting with (SST, which adds a transparent wrapper around the lambda to facilitate development) since the documented behavior for aws lambdas is to handle headers case-sensitive, so they should have been received as is (i.e. X-Telegram-Bot-Api-Secret-Token instead of x-telegram-bot-api-secret-token).

Let me know if you think more tests/changes are needed, and I will try to get to them in the coming days.

chrisandrewcl · 2023-05-30T01:52:04Z

Update regarding the secretToken: it seems I've misinterpreted the AWS docs. It doesn't seem to state anywhere that headers are delivered as is, just that they are "processed case-sensitive". To check it, I deployed a simple function that just print the received headers and during my test all headers were normalized lowercase. This behavior is not consistent with what I have previously encountered in other projects. I've also encountered similar confusion and multiple reports of conflicting and inconsistent behavior regarding how headers are passed thru lambda handlers on related repositories and other media.

In short, it seem we can't just pick one. I updated the PR to address this behavior.

Let me know what you think.

KnorpelSenf · 2023-05-30T12:08:47Z

@KnorpelSenf Installing with npm directly from the branch doesn't seem to work because there is a build step to generate the ./out files needed on node_modules, so the installed package ends up unusable. I've tested locally by building with deno task backport and then installing it on my test project. I've double-checked that the installed module comes from my development version (e.g. by changing something in grammy and verifying it is reflected on my test project). Did I miss something in the process?

That is very strange. If you install this package with npm from this repo, then it should run the prepare script after installation. This will run the backporting script. I'm surprised it doesn't work that way on your end …

Regarding the secretToken: it should work, but it doesn't in my case because the handler is receiving the headers normalized to lowercase. I suspect this is by interference of another framework I am experimenting with (SST, which adds a transparent wrapper around the lambda to facilitate development) since the documented behavior for aws lambdas is to handle headers case-sensitive, so they should have been received as is (i.e. X-Telegram-Bot-Api-Secret-Token instead of x-telegram-bot-api-secret-token).

Let me know if you think more tests/changes are needed, and I will try to get to them in the coming days.

So do you think we should add a new adapter for SST?

KnorpelSenf · 2023-05-30T12:20:39Z

src/convenience/frameworks.ts

+ let resolveResponse: (response: unknown) => void;
+ let handlerReturn: Promise<unknown> | undefined;
+
+ if (callback) {


It seems to me like you actually have two different adapters here that you're trying to support—one that passes a callback and one that doesn't. Why is this branching even necessary? Wouldn't it be much cleaner to have one adapter for every environment?

AWS lambdas support both ways, so that is not uncommon for middleware, plugins, and extensions aimed at it.

My intention was to keep the legacy behavior while adding support for the modern format.

That being said, separating it as a new adapter is also an alternative. Maybe call it aws-lambda-async?

Yes, that sounds good!

Just to confirm: you think it is best to have a new adapter called aws-lambda-async?

Yes. For every call signature there's exactly one adapter.

For comparison, check the two cloudflare adapters. One is for module workers, and one is for legacy workers.

I will look into it soon. Should all the changes be extracted to the new aws-lambda-async (leaving the current one unchanged) or are the headers changes being applied to the current one as well?

Does the current one have a problem with secret tokens? You can fix it if that's the case. If there's no problem, there's no need to touch the code.

chrisandrewcl · 2023-05-30T12:27:01Z

So do you think we should add a new adapter for SST?

No, not really. Since posting that comment I've done more tests and some research and posted my findings here.

KnorpelSenf · 2023-05-30T12:35:58Z

In short, it seem we can't just pick one. I updated the PR to address this behavior.

Let me know what you think.

Have you actually encountered a non-lowercase header for a grammY bot on AWS?

chrisandrewcl · 2023-05-30T12:39:49Z

Have you actually encountered a non-lowercase header for a grammY bot on AWS?

During my tests, no. If you think it is ok to look only for the lowercase version, fine by me, I was just avoiding introducing breaking behavior.

KnorpelSenf · 2023-05-30T12:42:57Z

It's good to be cautious about that. In this particular case, I'm fine with using the lowercase header name only. I see no way how it could change from request to request. If I'm wrong, it will be reliable to reproduce and simple to fix.

chrisandrewcl · 2023-05-30T15:29:13Z

That is very strange. If you install this package with npm from this repo, then it should run the prepare script after installation. This will run the backporting script. I'm surprised it doesn't work that way on your end …

Ah, I see what happened. Grammy's setup currently doesn't support being installed from git using yarn. I will switch to npm the next time I update the PR. Thanks!

KnorpelSenf · 2023-06-01T20:39:45Z

Can we merge? Is there anything left to test or to fix?

KnorpelSenf · 2023-06-01T20:40:48Z

Ah nvm, we're waiting for #418 (comment)

chrisandrewcl · 2023-06-03T19:11:34Z

Can we merge? Is there anything left to test or to fix?

All done!

KnorpelSenf

Thank you so much!

KnorpelSenf · 2023-06-05T13:19:48Z

@all-contributors add @chrisandrewcl for code

allcontributors · 2023-06-05T13:19:57Z

@KnorpelSenf

I've put up a pull request to add @chrisandrewcl! 🎉

KnorpelSenf approved these changes May 29, 2023

View reviewed changes

chrisandrewcl force-pushed the bugfix/412-aws-lambda branch from 7e30e21 to d992906 Compare May 29, 2023 01:50

chrisandrewcl mentioned this pull request May 29, 2023

bug: aws-lambda webhookCallback adapter doesn't work #412

Closed

chrisandrewcl force-pushed the bugfix/412-aws-lambda branch from 8720f8d to 9e9a10f Compare May 30, 2023 01:54

KnorpelSenf reviewed May 30, 2023

View reviewed changes

chrisandrewcl force-pushed the bugfix/412-aws-lambda branch from 9e9a10f to 5504c0c Compare May 30, 2023 12:33

chrisandrewcl added 2 commits June 3, 2023 16:03

feat: add aws-lambda-async webhookCallback adapter

0f68f02

fix: apply correct headers to aws-lambda webhookCallback adapter

60d8216

chrisandrewcl force-pushed the bugfix/412-aws-lambda branch from 5504c0c to 60d8216 Compare June 3, 2023 19:04

KnorpelSenf approved these changes Jun 5, 2023

View reviewed changes

KnorpelSenf merged commit 76b1b0f into grammyjs:main Jun 5, 2023
6 checks passed

allcontributors bot mentioned this pull request Jun 5, 2023

add chrisandrewcl as a contributor for code #430

Merged

KnorpelSenf mentioned this pull request Jun 14, 2023

NextJS framework secret_token header key should be lowercase #406

Closed

KnorpelSenf mentioned this pull request Jul 8, 2023

fix: correct case usage for X-Telegram-Bot-Api-Secret-Token header in… #464

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve aws-lambda webhookCallback adapter #418

Improve aws-lambda webhookCallback adapter #418

chrisandrewcl commented May 29, 2023 •

edited

Loading

codecov bot commented May 29, 2023 •

edited

Loading

KnorpelSenf left a comment

chrisandrewcl commented May 29, 2023

KnorpelSenf commented May 29, 2023

chrisandrewcl commented May 29, 2023 •

edited

Loading

chrisandrewcl commented May 30, 2023

KnorpelSenf commented May 30, 2023

KnorpelSenf May 30, 2023

chrisandrewcl May 30, 2023

KnorpelSenf May 30, 2023

chrisandrewcl May 30, 2023 •

edited

Loading

KnorpelSenf May 30, 2023

chrisandrewcl May 30, 2023

KnorpelSenf May 31, 2023

chrisandrewcl commented May 30, 2023

KnorpelSenf commented May 30, 2023

chrisandrewcl commented May 30, 2023

KnorpelSenf commented May 30, 2023

chrisandrewcl commented May 30, 2023 •

edited

Loading

KnorpelSenf commented Jun 1, 2023

KnorpelSenf commented Jun 1, 2023

chrisandrewcl commented Jun 3, 2023

KnorpelSenf left a comment

KnorpelSenf commented Jun 5, 2023

allcontributors bot commented Jun 5, 2023

Improve aws-lambda webhookCallback adapter #418

Improve aws-lambda webhookCallback adapter #418

Conversation

chrisandrewcl commented May 29, 2023 • edited Loading

codecov bot commented May 29, 2023 • edited Loading

Codecov Report

KnorpelSenf left a comment

Choose a reason for hiding this comment

chrisandrewcl commented May 29, 2023

KnorpelSenf commented May 29, 2023

chrisandrewcl commented May 29, 2023 • edited Loading

chrisandrewcl commented May 30, 2023

KnorpelSenf commented May 30, 2023

KnorpelSenf May 30, 2023

Choose a reason for hiding this comment

chrisandrewcl May 30, 2023

Choose a reason for hiding this comment

KnorpelSenf May 30, 2023

Choose a reason for hiding this comment

chrisandrewcl May 30, 2023 • edited Loading

Choose a reason for hiding this comment

KnorpelSenf May 30, 2023

Choose a reason for hiding this comment

chrisandrewcl May 30, 2023

Choose a reason for hiding this comment

KnorpelSenf May 31, 2023

Choose a reason for hiding this comment

chrisandrewcl commented May 30, 2023

KnorpelSenf commented May 30, 2023

chrisandrewcl commented May 30, 2023

KnorpelSenf commented May 30, 2023

chrisandrewcl commented May 30, 2023 • edited Loading

KnorpelSenf commented Jun 1, 2023

KnorpelSenf commented Jun 1, 2023

chrisandrewcl commented Jun 3, 2023

KnorpelSenf left a comment

Choose a reason for hiding this comment

KnorpelSenf commented Jun 5, 2023

allcontributors bot commented Jun 5, 2023

chrisandrewcl commented May 29, 2023 •

edited

Loading

codecov bot commented May 29, 2023 •

edited

Loading

chrisandrewcl commented May 29, 2023 •

edited

Loading

chrisandrewcl May 30, 2023 •

edited

Loading

chrisandrewcl commented May 30, 2023 •

edited

Loading