change querystring format for multi value parameters (#319)

[thoaltmann]

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]

⚠️ No Changeset found

Latest commit: db97a0f

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]

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

[khuezy]

Thanks!

[conico974]

4 E2E tests are failing for me:

The redirect doesn't work because there is an additional ? at the end.
Not sure why the multi query doesn't work

[thoaltmann]

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?

[conico974]

The issue might be here, query here could be {}, this will evaluate to true.
This does not explain why query.test.ts fails though.
My bet is that it comes from here

open-next/packages/open-next/src/adapters/event-mapper.ts

Line 126 in 8bc075b

query: removeUndefinedFromQuery(event.queryStringParameters ?? {}),

You should probably try deploying and add a bunch of console.logs to try to understand why it fails

[thoaltmann]

Good point, I've changed this and moved the ternary logic to the utility-function direclty. i got it deployed and still have one failing e2e test in the query.test.ts, unfortunately with exactly the same error we currently have. But I'll just log everything I can and see where I end up

[thoaltmann]

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

[conico974]

LGTM.
Thanks