apollo-server-integrations / apollo-server-integration-aws-lambda

An integration to use AWS Lambda as a hosting service with Apollo Server
MIT License
46 stars 9 forks source link

Bugfix, deduplicate querystring params #69

Closed emiloberg closed 1 year ago

emiloberg commented 1 year ago

This library merges event.queryStringParameters and event.multiValueQueryStringParameters. This means that if the same parameter is sent as a "normal" querystring and as multi value querystring (which they typically are), then we end up with duplicated search params, like

URLSearchParams { 'monkey' => 'ball', 'monkey' => 'ball' }

All fine and dandy until Apollo Server takes over and checks that a search param only exist once here: https://github.com/apollographql/apollo-server/blob/main/packages/server/src/runHttpQuery.ts#L35

This PR ensures that the params aren't duplicated.

On my setup, this manifests itself on AWS, and locally when doing an APQ query like this (APQ for query { __typename }):

curl --get http://localhost:5001/graphql \
  --header 'content-type: application/json' \
  --data-urlencode 'extensions={"persistedQuery":{"version":1,"sha256Hash":"ecf4edb46db40b5132295c0291d62fb65d6759a9eedfa4d5d612dd5ec54a6b38"}}'
trevor-scheer commented 1 year ago

This is being tested for (and passing) via the server integration test suite: https://github.com/apollographql/apollo-server/blob/35fa72bdd694ec3649708b34c89dda3c371389ea/packages/integration-testsuite/src/httpServerTests.ts#L764-L775

...which leads me to believe we're doing something wrong in our test setup here: https://github.com/apollo-server-integrations/apollo-server-integration-aws-lambda/blob/e3bc2dd97da02d01507a491fd61939ec9c49d5d2/src/__tests__/mockAPIGatewayV1Server.ts#L32-L49

Can you either: 1) confirm my understanding that "singular search params typically appear in both queryStringParameters and multiValueQueryStringParameters event objects" (so that I can correctly fix the test), OR 2) update the test I linked above (and its related V2 & ALB neighbors, if applicable) to construct those two objects on the event correctly.

Ultimately I need a test update somewhere to validate this change and prevent a future regression.

emiloberg commented 1 year ago

confirm my understanding that "singular search params typically appear in both queryStringParameters and multiValueQueryStringParameters event objects" (so that I can correctly fix the test)

Yes, I'm seeing singular search params both in queryStringParameters and multiValueQueryStringParameters. The content of both contains the same information.

(Sidenote: In an Apollo/GraphQL environment I can't really see where a multiValue query string could be used)

trevor-scheer commented 1 year ago

@emiloberg thanks! It looks like we've landed some fairly large changes since this PR was last updated. Would you please resolve conflicts and update the test setup accordingly so we can prevent this from regressing in the future?

BlenderDude commented 1 year ago

Superseded by #81

Some more work is required on the ALB request handler, but that would be a separate issue.