sst / open-next

Open source Next.js serverless adapter
https://open-next.js.org
MIT License
3.73k stars 113 forks source link

change querystring format for multi value parameters (#319) #320

Closed thoaltmann closed 7 months ago

thoaltmann commented 7 months ago

Changes querystrings with multi-value parameters from "?key=value1,value2" to ?key=value1&key=value2 since NextJS understands this format and converts it to an array when reading search params.

changeset-bot[bot] commented 7 months ago

⚠️ No Changeset found

Latest commit: db97a0fc6d79d9d085827826c2e4ec13ab5712ff

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

vercel[bot] commented 7 months ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
open-next ❌ Failed (Inspect) Nov 28, 2023 8:15am
thoaltmann commented 7 months ago

I may have an idea but I'm not sure why my change would cause the behavior:

in getUrlParts, the ? character is replaced with %3F and then returned as part of the pathname.

So, some middleware functions add a parameter with e.g. url.searchParams.set("rewritten", "true"). Does that mean, that in handleRewrites, the rewrittenUrl is actually /middleware-rewrite%3Frewritten=true and when other parameters are added, it results in /middleware-rewrite%F3rewritten=true?otherParam=hello which somehow gets decoded somewhere so that the final pathname is /middleware-rewrite?rewritten=true?otherParam=hello?

But that would have been the behavior before too I think, so I'm not really sure if that's actually the problem. I'm not that familiar with which events contain which fields and when a request ends up in one or the other function, so I'm mostly guessing. Do you have any other ideas?

thoaltmann commented 7 months ago

With the last commit I changed how the query for the internalEvent is built. It now results in a record like multiValueQueryStringParameters from the APIGatewayProxyEvent when converting from other events to ensure one unified structure. Now the internal event always has a query object that contains an array of values. e2e-tests are happy too