Repository: netlify/build
Pull request #3424 (2021-08-11 20:11:06)

fix: fix how redirects/headers check for duplicates
Follow-up on https://github.com/netlify/build/pull/3407#discussion_r686740695

The contents of both `netlify.toml` redirects and the `_redirects` file are concatenated in `netlifyConfig.redirects`. When a plugin modifies `netlifyConfig.redirects`, `@netlify/config` is called again in order to validate and normalize those new redirects. As part of this, it loads the `_redirects` file and concatenates it to `netlifyConfig.redirects`. 

However, this situation creates possible duplicate redirects inside `netlifyConfig.redirects`. Those are eventually persisted in `netlify.toml` at the end of the build, creating unnecessary performance issues and other problems.

The current solution is to detect whether `netlifyConfig.redirects` has the contents of `_redirects` file. We do this by checking if any redirect object is identical between `netlifyConfig.redirects` and `_redirects`.

However, this solution does not handle the following case: when `_redirects` and `netlify.toml` redirects are different but contain at least one identical redirects. When this happens, the current logic would ignore `_redirects`, which is wrong.

This PR opts for a different solution: filtering out duplicates explicitly.

All of the above also applies to headers.


Patch:
diff --git a/packages/config/src/headers.js b/packages/config/src/headers.js
index 05cde9ff6..a5eceb0a4 100644
--- a/packages/config/src/headers.js
+++ b/packages/config/src/headers.js
@@ -19,9 +19,10 @@ const HEADERS_FILENAME = '_headers'
 const addConfigHeaders = async function ({ headers: configHeaders = [], ...config }, headersPath, logs) {
   try {
     const normalizedConfigHeaders = normalizeHeaders(configHeaders)
-    const normalizedFileHeaders = await getFileHeaders(headersPath, normalizedConfigHeaders)
+    const normalizedFileHeaders = await getFileHeaders(headersPath)
     const headers = mergeHeaders({ fileHeaders: normalizedFileHeaders, configHeaders: normalizedConfigHeaders })
-    return { ...config, headers }
+    const uniqueHeaders = headers.filter(isUniqueHeader)
+    return { ...config, headers: uniqueHeaders }
     // @todo remove this failsafe once the code is stable
   } catch (error) {
     warnHeadersParsing(logs, error.message)
@@ -29,23 +30,19 @@ const addConfigHeaders = async function ({ headers: configHeaders = [], ...confi
   }
 }
 
-const getFileHeaders = async function (headersPath, normalizedConfigHeaders) {
+const getFileHeaders = async function (headersPath) {
   const fileHeaders = await parseFileHeaders(headersPath)
   const normalizedFileHeaders = normalizeHeaders(fileHeaders)
-  return hasMergedFileHeaders(normalizedFileHeaders, normalizedConfigHeaders) ? [] : normalizedFileHeaders
+  return normalizedFileHeaders
 }
 
 // `configHeaders` might contain the content of `_headers` already.
 // This happens when a plugin appends to `netlifyConfig.headers` which already
 // contains `_headers` rules and is transformed to an `inlineConfig` object,
 // which is itself parsed as `configHeaders`. In that case, we do not want
-// duplicate so we ignore `fileHeaders`.
-// The current logic works with the case where `_headers` is added both before
-// and after modifying `netlifyConfig.headers`.
-const hasMergedFileHeaders = function (normalizedFileHeaders, normalizedConfigHeaders) {
-  return normalizedConfigHeaders.some((configHeader) =>
-    normalizedFileHeaders.some((fileHeader) => isDeepStrictEqual(configHeader, fileHeader)),
-  )
+// duplicates.
+const isUniqueHeader = function (header, index, headers) {
+  return !headers.slice(index + 1).some((otherHeader) => isDeepStrictEqual(header, otherHeader))
 }
 
 module.exports = { addHeaders, addConfigHeaders }
diff --git a/packages/config/src/redirects.js b/packages/config/src/redirects.js
index acf19929a..2906b4570 100644
--- a/packages/config/src/redirects.js
+++ b/packages/config/src/redirects.js
@@ -19,12 +19,13 @@ const REDIRECTS_FILENAME = '_redirects'
 const addConfigRedirects = async function ({ redirects: configRedirects = [], ...config }, redirectsPath, logs) {
   try {
     const normalizedConfigRedirects = normalizeAllRedirects(configRedirects)
-    const normalizedFileRedirects = await getFileRedirects(redirectsPath, normalizedConfigRedirects)
+    const normalizedFileRedirects = await getFileRedirects(redirectsPath)
     const redirects = mergeRedirects({
       fileRedirects: normalizedFileRedirects,
       configRedirects: normalizedConfigRedirects,
     })
-    return { ...config, redirects }
+    const uniqueRedirects = redirects.filter(isUniqueRedirects)
+    return { ...config, redirects: uniqueRedirects }
     // @todo remove this failsafe once the code is stable
   } catch (error) {
     warnRedirectsParsing(logs, error.message)
@@ -32,10 +33,10 @@ const addConfigRedirects = async function ({ redirects: configRedirects = [], ..
   }
 }
 
-const getFileRedirects = async function (redirectsPath, normalizedConfigRedirects) {
+const getFileRedirects = async function (redirectsPath) {
   const fileRedirects = await parseFileRedirects(redirectsPath)
   const normalizedFileRedirects = normalizeAllRedirects(fileRedirects)
-  return hasMergedFileRedirects(normalizedFileRedirects, normalizedConfigRedirects) ? [] : normalizedFileRedirects
+  return normalizedFileRedirects
 }
 
 const normalizeAllRedirects = function (redirects) {
@@ -46,13 +47,9 @@ const normalizeAllRedirects = function (redirects) {
 // This happens when a plugin appends to `netlifyConfig.redirects` which already
 // contains `_redirects` rules and is transformed to an `inlineConfig` object,
 // which is itself parsed as `configRedirects`. In that case, we do not want
-// duplicate so we ignore `fileRedirects`.
-// The current logic works with the case where `_redirects` is added both before
-// and after modifying `netlifyConfig.redirects`.
-const hasMergedFileRedirects = function (normalizedFileRedirects, normalizedConfigRedirects) {
-  return normalizedConfigRedirects.some((configRedirect) =>
-    normalizedFileRedirects.some((fileRedirect) => isDeepStrictEqual(configRedirect, fileRedirect)),
-  )
+// duplicates.
+const isUniqueRedirects = function (redirect, index, redirects) {
+  return !redirects.slice(index + 1).some((otherRedirect) => isDeepStrictEqual(redirect, otherRedirect))
 }
 
 module.exports = { addRedirects, addConfigRedirects }


Test patch:
diff --git a/packages/config/tests/parse/fixtures/headers_both/netlify.toml b/packages/config/tests/parse/fixtures/headers_both/netlify.toml
index c95082985..fb633fff4 100644
--- a/packages/config/tests/parse/fixtures/headers_both/netlify.toml
+++ b/packages/config/tests/parse/fixtures/headers_both/netlify.toml
@@ -6,3 +6,8 @@ for = "/path"
   [headers.values]
   test = "override"
   testTwo = "two"
+
+[[headers]]
+for = "/path"
+  [headers.values]
+  testFour = "four"
diff --git a/packages/config/tests/parse/fixtures/headers_both/publish/_headers b/packages/config/tests/parse/fixtures/headers_both/publish/_headers
index c0ef5cd3e..b4696b508 100644
--- a/packages/config/tests/parse/fixtures/headers_both/publish/_headers
+++ b/packages/config/tests/parse/fixtures/headers_both/publish/_headers
@@ -1,3 +1,6 @@
 /path
   test: one
   testThree: three
+/path
+  test: override
+  testTwo: two
diff --git a/packages/config/tests/parse/fixtures/redirects_both/netlify.toml b/packages/config/tests/parse/fixtures/redirects_both/netlify.toml
index c6061cfdf..8ea5c4c30 100644
--- a/packages/config/tests/parse/fixtures/redirects_both/netlify.toml
+++ b/packages/config/tests/parse/fixtures/redirects_both/netlify.toml
@@ -4,3 +4,7 @@ publish = "publish"
 [[redirects]]
 from = "/from"
 to = "/to"
+
+[[redirects]]
+from = "/from"
+to = "/to_config"
diff --git a/packages/config/tests/parse/fixtures/redirects_both/publish/_redirects b/packages/config/tests/parse/fixtures/redirects_both/publish/_redirects
index 40af3d89a..5a03d862f 100644
--- a/packages/config/tests/parse/fixtures/redirects_both/publish/_redirects
+++ b/packages/config/tests/parse/fixtures/redirects_both/publish/_redirects
@@ -1,1 +1,2 @@
 /from /to_file
+/from /to
diff --git a/packages/config/tests/parse/snapshots/tests.js.md b/packages/config/tests/parse/snapshots/tests.js.md
index ccd5bc5b8..b0ff576ca 100644
--- a/packages/config/tests/parse/snapshots/tests.js.md
+++ b/packages/config/tests/parse/snapshots/tests.js.md
@@ -758,6 +758,12 @@ Generated by [AVA](https://ava.li).
               "test": "override",␊
               "testTwo": "two"␊
             }␊
+          },␊
+          {␊
+            "for": "/external/path",␊
+            "values": {␊
+              "testFour": "four"␊
+            }␊
           }␊
         ],␊
         "headersOrigin": "config",␊
@@ -2114,6 +2120,14 @@ Generated by [AVA](https://ava.li).
             "query": {},␊
             "to": "/external/path"␊
           },␊
+          {␊
+            "conditions": {},␊
+            "force": false,␊
+            "from": "/external/path",␊
+            "headers": {},␊
+            "query": {},␊
+            "to": "/external/path"␊
+          },␊
           {␊
             "conditions": {},␊
             "force": false,␊