Repository: netlify/cli
Pull request #5797 (2023-06-13 10:51:42)

fix: preserve duplicate query parameters in functions
#### Summary

Fixes a regression introduced in https://github.com/netlify/cli/pull/5782, and reported in https://github.com/netlify/cli/issues/5790, which swallows duplicate query parameters from functions.

Closes https://github.com/netlify/cli/issues/5790.

github-actions[bot]: ## 📊 Benchmark results

*Comparing with 4c6dc34dcf06844722d9a277122247b9a0e29add*

- **Dependency count**: 1,238 (no change)
- **Package size**: 235 MB ⬇️ **0.00% decrease** vs. 4c6dc34dcf06844722d9a277122247b9a0e29add
<!--delta:post-package-size@{}-->
danez: Could you check the `200.command.dev › should handle query params in redirects` test?
eduardoboucas: > chore: fix test

I fixed the test in https://github.com/netlify/cli/pull/5797/commits/669d593fd63e08402f16ebefacbc09870e689a32. I verified in production that the expected behaviour is for both parameters to be received, so the test was wrong.

Patch:
diff --git a/.eslintrc.cjs b/.eslintrc.cjs
index 36c13b3b..681f131c 100644
--- a/.eslintrc.cjs
+++ b/.eslintrc.cjs
@@ -23,7 +23,7 @@ module.exports = {
     'max-statements': 0,
     'no-param-reassign': 0,
     'no-process-exit': 0,
-    'fp/no-loops': 'error',
+    'fp/no-loops': 'off',
     'import/max-dependencies': 0,
     'import/extensions': [2, 'ignorePackages'],
     'n/no-process-exit': 0,
diff --git a/functions/error-reporting.ts b/functions/error-reporting.ts
index 26808603..f2418ce3 100755
--- a/functions/error-reporting.ts
+++ b/functions/error-reporting.ts
@@ -30,7 +30,7 @@ export const handler: Handler = async ({ body }) => {
         version: cliVersion,
         type: 'netlify-cli',
       }
-      // eslint-disable-next-line fp/no-loops
+
       for (const [section, values] of Object.entries(metadata)) {
         event.addMetadata(section, values as Record<string, any>)
       }
diff --git a/scripts/prepare-for-publish.mjs b/scripts/prepare-for-publish.mjs
index a694655d..575ff702 100644
--- a/scripts/prepare-for-publish.mjs
+++ b/scripts/prepare-for-publish.mjs
@@ -22,7 +22,6 @@ delete pkgJson.devDependencies
 delete pkgJson.ava
 delete pkgJson.config
 
-// eslint-disable-next-line fp/no-loops
 for (const scriptName in pkgJson.scripts) {
   if (preserveScripts.has(scriptName)) continue
 
diff --git a/src/lib/functions/runtimes/js/builders/netlify-lambda.mjs b/src/lib/functions/runtimes/js/builders/netlify-lambda.mjs
index 9b7e44d0..206fd2eb 100644
--- a/src/lib/functions/runtimes/js/builders/netlify-lambda.mjs
+++ b/src/lib/functions/runtimes/js/builders/netlify-lambda.mjs
@@ -16,7 +16,6 @@ export const detectNetlifyLambda = async function ({ packageJson } = {}) {
 
   const matchingScripts = Object.entries(scripts).filter(([, script]) => script.match(/netlify-lambda\s+build/))
 
-  // eslint-disable-next-line fp/no-loops
   for (const [key, script] of matchingScripts) {
     // E.g. ["netlify-lambda", "build", "functions/folder"]
     const match = minimist(script.split(' '), {
diff --git a/src/lib/functions/server.mjs b/src/lib/functions/server.mjs
index 1ac9d951..38bd72b1 100644
--- a/src/lib/functions/server.mjs
+++ b/src/lib/functions/server.mjs
@@ -87,7 +87,14 @@ export const createHandler = function (options) {
 
     let requestQuery = request.query
     if (request.header('x-netlify-original-search')) {
-      requestQuery = Object.fromEntries(new URLSearchParams(request.header('x-netlify-original-search')))
+      const newRequestQuery = {}
+      const searchParams = new URLSearchParams(request.header('x-netlify-original-search'))
+
+      for (const key of searchParams.keys()) {
+        newRequestQuery[key] = searchParams.getAll(key)
+      }
+
+      requestQuery = newRequestQuery
       delete request.headers['x-netlify-original-search']
     }
 
diff --git a/src/utils/dev.mjs b/src/utils/dev.mjs
index af18351c..00a021b4 100644
--- a/src/utils/dev.mjs
+++ b/src/utils/dev.mjs
@@ -167,7 +167,6 @@ export const getDotEnvVariables = async ({ devConfig, env, site }) => {
  * @return {void}
  */
 export const injectEnvVariables = (env) => {
-  // eslint-disable-next-line fp/no-loops
   for (const [key, variable] of Object.entries(env)) {
     const existsInProcess = process.env[key] !== undefined
     const [usedSource, ...overriddenSources] = existsInProcess ? ['process', ...variable.sources] : variable.sources


Test patch:
diff --git a/tests/integration/200.command.dev.test.cjs b/tests/integration/200.command.dev.test.cjs
index 22ef4b37..5e74e0c4 100644
--- a/tests/integration/200.command.dev.test.cjs
+++ b/tests/integration/200.command.dev.test.cjs
@@ -63,7 +63,7 @@ testMatrix.forEach(({ args }) => {
           ])
 
         // query params should be taken from redirect rule for functions
-        t.deepEqual(fromFunction.multiValueQueryStringParameters, { bar: ['2'], foo: ['2'] })
+        t.deepEqual(fromFunction.multiValueQueryStringParameters, { bar: ['1', '2'], foo: ['1', '2'] })
 
         // query params should be passed through from the request
         t.is(queryPassthrough.headers.location, '/?foo=1&foo=2&bar=1&bar=2')
diff --git a/tests/integration/commands/dev/redirects.test.ts b/tests/integration/commands/dev/redirects.test.ts
index efda99de..ecbf98c3 100644
--- a/tests/integration/commands/dev/redirects.test.ts
+++ b/tests/integration/commands/dev/redirects.test.ts
@@ -18,5 +18,20 @@ describe('redirects', () => {
       expect(result.queryStringParameters).toHaveProperty('param2', 'world')
       expect(result.queryStringParameters).toHaveProperty('other', '1')
     })
+
+    test<FixtureTestContext>('should send original query params to functions when using duplicate parameters', async ({
+      devServer,
+    }) => {
+      const response = await got(`http://localhost:${devServer.port}/api/echo?param=hello&param=world`, {
+        throwHttpErrors: false,
+        retry: { limit: 0 },
+      })
+
+      expect(response.statusCode).toBe(200)
+
+      const result = JSON.parse(response.body)
+      expect(result.queryStringParameters).toHaveProperty('param', 'hello, world')
+      expect(result.multiValueQueryStringParameters).toHaveProperty('param', ['hello', 'world'])
+    })
   })
 })
diff --git a/tests/integration/utils/dev-server.cjs b/tests/integration/utils/dev-server.cjs
index faa67069..6f820bf6 100644
--- a/tests/integration/utils/dev-server.cjs
+++ b/tests/integration/utils/dev-server.cjs
@@ -116,7 +116,7 @@ const startServer = async ({
 
 const startDevServer = async (options, expectFailure) => {
   const maxAttempts = 5
-  // eslint-disable-next-line fp/no-loops
+
   for (let attempt = 1; attempt <= maxAttempts; attempt++) {
     try {
       // do not use destruction, as we use getters which otherwise would be evaluated here
diff --git a/tests/integration/utils/site-builder.cjs b/tests/integration/utils/site-builder.cjs
index 4d759979..378a69eb 100644
--- a/tests/integration/utils/site-builder.cjs
+++ b/tests/integration/utils/site-builder.cjs
@@ -217,7 +217,6 @@ class SiteBuilder {
   }
 
   async build() {
-    // eslint-disable-next-line fp/no-loops
     for (const task of this.tasks) {
       await task()
     }