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: passing body for graphql requests generated from aws-appsync-simulator #16

Merged
merged 2 commits into from
Oct 28, 2024

Conversation

sc0ttdav3y
Copy link
Contributor

Hi,

I'm running a local version of AppSync (via servereless-offline)+ API Gateway (via this package) + Bref:

[ Client ] ---> [ AppSync ] ---> [ HTTP Resolver ] ---> [ API Gateway] ---> [ Bref ]

I was finding all of my GraphQL queries and mutations were missing the body part of the request. I looked into it and found local-api-gateway was suppressing the body of the request.

I have removed that conditional in this PR to pass the body whenever it's available, and all is working for my use case now.

I get the intent of the code, and I suspect it's not working because the payload, at least in my case, has method in request.requestContext.method, and not request.method. But I think it's not terrible to simplify things and just pass body if it's supplied, especially given the aims of this project to be minimally useful.

I also added a tidy on L64 to cast request.ip as a string, because the Typescript indicates request.ip is string|null while the AWS payload must be a string.

Note I tried to spin up the tests and contribute a unit test, but I couldn't work out the dev steps to get them going. If you want me to work on some tests, give me some hints on how to get started and I'd be happy to do so.

Thanks, Scott

@mnapoli
Copy link
Member

mnapoli commented Oct 24, 2024

Thanks, on paper that makes sense.

However the tests are failing, can you fix those?

To run the tests you need to run npm run test (run npm i before ofc).

@sc0ttdav3y
Copy link
Contributor Author

My missing knowledge in running the tests was having to set the environment variable TARGET="localhost:9000" beforehand. With that done I've fixed the failing test.

@mnapoli
Copy link
Member

mnapoli commented Oct 25, 2024

Oh cool, might be worth adding to the docs or even the npm run test script, but let's not block this PR 👍

@mnapoli mnapoli merged commit ee31b94 into brefphp:main Oct 28, 2024
5 checks passed
@sc0ttdav3y
Copy link
Contributor Author

A good medal to you @mnapoli 🏅 — I can confirm this fix is working well via the new docker image, and many thanks for the work you do on this project and Bref generally.

@mnapoli
Copy link
Member

mnapoli commented Oct 30, 2024

Awesome! btw I'd love to learn more how you use Bref, I've never seen that architecture done with Bref before. I posted a link to my calendar here, feel free to book some time if you want: https://twitter.com/matthieunapoli/status/1849780007998259673

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants