Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add excludedPath support to ISC & TOML #402

Merged
merged 9 commits into from
May 24, 2023
+127 −71
Merged

Add excludedPath support to ISC & TOML #402

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e0659f8
feat: support mutliple excludedPath declarations
Skn0tt May 22, 2023
ec67991
feat: add excluded_patterns field to routes
Skn0tt May 22, 2023
49a941b
feat: move declaration-level excludedPaths into route field
Skn0tt May 22, 2023
d8f6b46
chore: add test reproducing design example
Skn0tt May 22, 2023
00a41b3
fix: add leading slash to make typescript happy
Skn0tt May 22, 2023
f972a69
fix: another slash missing
Skn0tt May 23, 2023
65435cf
fix: types
Skn0tt May 23, 2023
3476b72
fix: rename getExcludedRegularExpression to plural
Skn0tt May 24, 2023
9e92fc8
fix: allow excludedPattern to be array
Skn0tt May 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions node/bundler.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,14 +342,15 @@ test('Loads declarations and import maps from the deploy configuration and in-so

const manifestFile = await readFile(resolve(distPath, 'manifest.json'), 'utf8')
const manifest = JSON.parse(manifestFile)
const { bundles, function_config: functionConfig } = manifest
const { bundles, function_config: functionConfig, routes } = manifest
expect(bundles.length).toBe(1)
expect(bundles[0].format).toBe('eszip2')
expect(generatedFiles.includes(bundles[0].asset)).toBe(true)

// respects excludedPath from deploy config
expect(routes[0].excluded_patterns).toEqual(['^/func2/skip/?$'])

expect(functionConfig.func2).toEqual({
excluded_patterns: ['^/func2/skip/?$'],
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Thinking aloud as I follow along to make sure I understand what's happening)
So, previously the excluded_patterns was part of the functionConfig, but with this change it moves to the routes

... unless the user sets it in the ISC, in which case it'll still be set in the functionConfig, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right! Edge Functions can serve different routes, and so we make the distinction between "route config" and "function config". Route Config only applies to executions of a function that match that route, while function config will be applied to all executions, no matter the route.

name: 'Function two',
generator: '@netlify/[email protected]',
})
Expand Down
14 changes: 7 additions & 7 deletions node/config.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,15 +193,15 @@ test('Loads function paths from the in-source `config` function', async () => {
expect(generatedFiles.includes(bundles[0].asset)).toBe(true)

expect(routes.length).toBe(6)
expect(routes[0]).toEqual({ function: 'framework-func2', pattern: '^/framework-func2/?$' })
expect(routes[1]).toEqual({ function: 'user-func2', pattern: '^/user-func2/?$' })
expect(routes[2]).toEqual({ function: 'framework-func1', pattern: '^/framework-func1/?$' })
expect(routes[3]).toEqual({ function: 'user-func1', pattern: '^/user-func1/?$' })
expect(routes[4]).toEqual({ function: 'user-func3', pattern: '^/user-func3/?$' })
expect(routes[5]).toEqual({ function: 'user-func5', pattern: '^/user-func5/.*/?$' })
expect(routes[0]).toEqual({ function: 'framework-func2', pattern: '^/framework-func2/?$', excluded_patterns: [] })
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if the user doesn't configure an excluded_pattern, it'll be included in each route as an empty array.

expect(routes[1]).toEqual({ function: 'user-func2', pattern: '^/user-func2/?$', excluded_patterns: [] })
expect(routes[2]).toEqual({ function: 'framework-func1', pattern: '^/framework-func1/?$', excluded_patterns: [] })
expect(routes[3]).toEqual({ function: 'user-func1', pattern: '^/user-func1/?$', excluded_patterns: [] })
expect(routes[4]).toEqual({ function: 'user-func3', pattern: '^/user-func3/?$', excluded_patterns: [] })
expect(routes[5]).toEqual({ function: 'user-func5', pattern: '^/user-func5/.*/?$', excluded_patterns: [] })

expect(postCacheRoutes.length).toBe(1)
expect(postCacheRoutes[0]).toEqual({ function: 'user-func4', pattern: '^/user-func4/?$' })
expect(postCacheRoutes[0]).toEqual({ function: 'user-func4', pattern: '^/user-func4/?$', excluded_patterns: [] })

expect(Object.keys(functionConfig)).toHaveLength(1)
expect(functionConfig['user-func5']).toEqual({
Expand Down
4 changes: 2 additions & 2 deletions node/declaration.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,12 @@ interface BaseDeclaration {

type DeclarationWithPath = BaseDeclaration & {
path: Path
excludedPath?: Path
excludedPath?: Path | Path[]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

excludedPath can either be a Path string or and array of Path strings; If it's just a string, it'll be converted to an array before being added to Routes (here)

}

type DeclarationWithPattern = BaseDeclaration & {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be: excludedPattern?: string | []string? Asking because here we're checking to see if declaration.excludedPattern is an array

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

absolutely! done in 9e92fc8

pattern: string
excludedPattern?: string
excludedPattern?: string | string[]
}

export type Declaration = DeclarationWithPath | DeclarationWithPattern
Expand Down
88 changes: 65 additions & 23 deletions node/manifest.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ test('Generates a manifest with different bundles', () => {
{ asset: bundle1.hash + bundle1.extension, format: bundle1.format },
{ asset: bundle2.hash + bundle2.extension, format: bundle2.format },
]
const expectedRoutes = [{ function: 'func-1', pattern: '^/f1/?$' }]
const expectedRoutes = [{ function: 'func-1', pattern: '^/f1/?$', excluded_patterns: [] }]

expect(manifest.bundles).toEqual(expectedBundles)
expect(manifest.routes).toEqual(expectedRoutes)
Expand All @@ -46,7 +46,7 @@ test('Generates a manifest with display names', () => {
}
const manifest = generateManifest({ bundles: [], declarations, functions, internalFunctionConfig })

const expectedRoutes = [{ function: 'func-1', pattern: '^/f1/.*/?$' }]
const expectedRoutes = [{ function: 'func-1', pattern: '^/f1/.*/?$', excluded_patterns: [] }]
expect(manifest.function_config).toEqual({
'func-1': { name: 'Display Name' },
})
Expand All @@ -65,7 +65,7 @@ test('Generates a manifest with a generator field', () => {
}
const manifest = generateManifest({ bundles: [], declarations, functions, internalFunctionConfig })

const expectedRoutes = [{ function: 'func-1', pattern: '^/f1/.*/?$' }]
const expectedRoutes = [{ function: 'func-1', pattern: '^/f1/.*/?$', excluded_patterns: [] }]
const expectedFunctionConfig = { 'func-1': { generator: '@netlify/[email protected]' } }
expect(manifest.routes).toEqual(expectedRoutes)
expect(manifest.function_config).toEqual(expectedFunctionConfig)
Expand All @@ -79,22 +79,18 @@ test('Generates a manifest with excluded paths and patterns', () => {
]
const declarations: Declaration[] = [
{ function: 'func-1', path: '/f1/*', excludedPath: '/f1/exclude' },
{ function: 'func-2', pattern: '^/f2/.*/?$', excludedPattern: '^/f2/exclude$' },
{ function: 'func-2', pattern: '^/f2/.*/?$', excludedPattern: ['^/f2/exclude$', '^/f2/exclude-as-well$'] },
{ function: 'func-3', path: '/*', excludedPath: '/**/*.html' },
]
const manifest = generateManifest({ bundles: [], declarations, functions })
const expectedRoutes = [
{ function: 'func-1', pattern: '^/f1/.*/?$' },
{ function: 'func-2', pattern: '^/f2/.*/?$' },
{ function: 'func-3', pattern: '^/.*/?$' },
{ function: 'func-1', pattern: '^/f1/.*/?$', excluded_patterns: ['^/f1/exclude/?$'] },
{ function: 'func-2', pattern: '^/f2/.*/?$', excluded_patterns: ['^/f2/exclude$', '^/f2/exclude-as-well$'] },
{ function: 'func-3', pattern: '^/.*/?$', excluded_patterns: ['^/.*/.*\\.html/?$'] },
]

expect(manifest.routes).toEqual(expectedRoutes)
expect(manifest.function_config).toEqual({
'func-1': { excluded_patterns: ['^/f1/exclude/?$'] },
'func-2': { excluded_patterns: ['^/f2/exclude$'] },
'func-3': { excluded_patterns: ['^/.*/.*\\.html/?$'] },
})
expect(manifest.function_config).toEqual({})
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since excluded_patterns have been moved into routes, the function_config is now empty.

expect(manifest.bundler_version).toBe(env.npm_package_version as string)

const matcher = getRouteMatcher(manifest)
Expand All @@ -111,7 +107,7 @@ test('TOML-defined paths can be combined with ISC-defined excluded paths', () =>
}
const manifest = generateManifest({ bundles: [], declarations, functions, userFunctionConfig })

const expectedRoutes = [{ function: 'func-1', pattern: '^/f1/.*/?$' }]
const expectedRoutes = [{ function: 'func-1', pattern: '^/f1/.*/?$', excluded_patterns: [] }]

expect(manifest.routes).toEqual(expectedRoutes)
expect(manifest.function_config).toEqual({
Expand Down Expand Up @@ -171,6 +167,52 @@ test('Filters out internal in-source configurations in user created functions',
})
})

test('excludedPath from ISC goes into function_config, TOML goes into routes', () => {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test matches the scenario described in the proposal here

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Furthermore, /checkout/scrooge-mc-duck-animation.css and /showcases/boho-style/expensive-chair.jpg are skipped as well, because they were listed in ISC, and that takes precedence over excludedPaths in TOML declarations.

So the excludedPath from the ISC will still be stored in the function_config, but the excludedPaths defined in the TOML will be stored in routes. When deciding whether or not to run a function for a path, both configurations will be taken into account, with excludedPath in the function_config taking precedence.

const functions = [{ name: 'customisation', path: '/path/to/customisation.ts' }]
const declarations: Declaration[] = [
{ function: 'customisation', path: '/showcases/*' },
{ function: 'customisation', path: '/checkout/*', excludedPath: ['/*/terms-and-conditions'] },
]
const userFunctionConfig: Record<string, FunctionConfig> = {
customisation: {
excludedPath: ['/*.css', '/*.jpg'],
},
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the user configures excludedPath, does it get saved as an excluded_pattern? According to this test, it does: both the declaration and the function config have an excludedPath that get converted to excluded_pattern. Is it correct to assume that whether the user sets excludedPath or excludedPattern, it will ultimately be treated the same way?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've understood it the same way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's correct. Context in #402 (comment).

}
const internalFunctionConfig: Record<string, FunctionConfig> = {}
const manifest = generateManifest({
bundles: [],
declarations,
functions,
userFunctionConfig,
internalFunctionConfig,
})
expect(manifest.routes).toEqual([
{
function: 'customisation',
pattern: '^/showcases/.*/?$',
excluded_patterns: [],
},
{
function: 'customisation',
pattern: '^/checkout/.*/?$',
excluded_patterns: ['^/.*/terms-and-conditions/?$'],
},
])
expect(manifest.function_config).toEqual({
customisation: {
excluded_patterns: ['^/.*\\.css/?$', '^/.*\\.jpg/?$'],
},
})

const matcher = getRouteMatcher(manifest)

expect(matcher('/showcases/boho-style')).toBeDefined()
expect(matcher('/checkout/address')).toBeDefined()
expect(matcher('/checkout/terms-and-conditions')).toBeUndefined()
expect(matcher('/checkout/scrooge-mc-duck-animation.css')).toBeUndefined()
expect(matcher('/showcases/boho-style/expensive-chair.jpg')).toBeUndefined()
})

test('Includes failure modes in manifest', () => {
const functions = [
{ name: 'func-1', path: '/path/to/func-1.ts' },
Expand Down Expand Up @@ -204,7 +246,7 @@ test('Excludes functions for which there are function files but no matching conf
const declarations: Declaration[] = [{ function: 'func-1', path: '/f1' }]
const manifest = generateManifest({ bundles: [bundle1], declarations, functions })

const expectedRoutes = [{ function: 'func-1', pattern: '^/f1/?$' }]
const expectedRoutes = [{ function: 'func-1', pattern: '^/f1/?$', excluded_patterns: [] }]

expect(manifest.routes).toEqual(expectedRoutes)
})
Expand All @@ -222,7 +264,7 @@ test('Excludes functions for which there are config declarations but no matching
]
const manifest = generateManifest({ bundles: [bundle1], declarations, functions })

const expectedRoutes = [{ function: 'func-2', pattern: '^/f2/?$' }]
const expectedRoutes = [{ function: 'func-2', pattern: '^/f2/?$', excluded_patterns: [] }]

expect(manifest.routes).toEqual(expectedRoutes)
})
Expand All @@ -232,7 +274,7 @@ test('Generates a manifest without bundles', () => {
const declarations: Declaration[] = [{ function: 'func-1', path: '/f1' }]
const manifest = generateManifest({ bundles: [], declarations, functions })

const expectedRoutes = [{ function: 'func-1', pattern: '^/f1/?$' }]
const expectedRoutes = [{ function: 'func-1', pattern: '^/f1/?$', excluded_patterns: [] }]

expect(manifest.bundles).toEqual([])
expect(manifest.routes).toEqual(expectedRoutes)
Expand Down Expand Up @@ -267,10 +309,10 @@ test('Generates a manifest with pre and post-cache routes', () => {
{ asset: bundle2.hash + bundle2.extension, format: bundle2.format },
]
const expectedPreCacheRoutes = [
{ function: 'func-1', name: undefined, pattern: '^/f1/?$' },
{ function: 'func-2', name: undefined, pattern: '^/f2/?$' },
{ function: 'func-1', name: undefined, pattern: '^/f1/?$', excluded_patterns: [] },
{ function: 'func-2', name: undefined, pattern: '^/f2/?$', excluded_patterns: [] },
]
const expectedPostCacheRoutes = [{ function: 'func-3', name: undefined, pattern: '^/f3/?$' }]
const expectedPostCacheRoutes = [{ function: 'func-3', name: undefined, pattern: '^/f3/?$', excluded_patterns: [] }]

expect(manifest.bundles).toEqual(expectedBundles)
expect(manifest.routes).toEqual(expectedPreCacheRoutes)
Expand All @@ -288,8 +330,8 @@ test('Generates a manifest with layers', () => {
{ function: 'func-2', path: '/f2/*' },
]
const expectedRoutes = [
{ function: 'func-1', pattern: '^/f1/.*/?$' },
{ function: 'func-2', pattern: '^/f2/.*/?$' },
{ function: 'func-1', pattern: '^/f1/.*/?$', excluded_patterns: [] },
{ function: 'func-2', pattern: '^/f2/.*/?$', excluded_patterns: [] },
]
const layers = [
{
Expand Down Expand Up @@ -323,7 +365,7 @@ test('Shows a warning if the regular expression contains a negative lookahead',

console.warn = consoleWarn

expect(manifest.routes).toEqual([{ function: 'func-1', pattern: '^/\\w+(?=\\d)$' }])
expect(manifest.routes).toEqual([{ function: 'func-1', pattern: '^/\\w+(?=\\d)$', excluded_patterns: [] }])
expect(mockConsoleWarn).toHaveBeenCalledOnce()
expect(mockConsoleWarn).toHaveBeenCalledWith(
"Function 'func-1' uses an unsupported regular expression and will not be invoked: Regular expressions with lookaheads are not supported",
Expand Down Expand Up @@ -351,5 +393,5 @@ test('Converts named capture groups to unnamed capture groups in regular express
const declarations = [{ function: 'func-1', pattern: '^/(?<name>\\w+)$' }]
const manifest = generateManifest({ bundles: [], declarations, functions })

expect(manifest.routes).toEqual([{ function: 'func-1', pattern: '^/(\\w+)$' }])
expect(manifest.routes).toEqual([{ function: 'func-1', pattern: '^/(\\w+)$', excluded_patterns: [] }])
})
58 changes: 32 additions & 26 deletions node/manifest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import { nonNullable } from './utils/non_nullable.js'
interface Route {
function: string
pattern: string
excluded_patterns: string[]
}

interface EdgeFunctionConfig {
Expand Down Expand Up @@ -129,17 +130,15 @@ const generateManifest = ({
}

const pattern = getRegularExpression(declaration, featureFlags?.edge_functions_fail_unsupported_regex)
const route: Route = {
function: func.name,
pattern: serializePattern(pattern),
}
const excludedPattern = getExcludedRegularExpression(
const excludedPattern = getExcludedRegularExpressions(
declaration,
featureFlags?.edge_functions_fail_unsupported_regex,
)

if (excludedPattern) {
manifestFunctionConfig[func.name].excluded_patterns.push(serializePattern(excludedPattern))
const route: Route = {
function: func.name,
pattern: serializePattern(pattern),
excluded_patterns: excludedPattern.map(serializePattern),
}

if (declaration.cache === Cache.Manual) {
Expand Down Expand Up @@ -178,7 +177,7 @@ const pathToRegularExpression = (path: string) => {
return normalizedSource
}

const getRegularExpression = (declaration: Declaration, failUnsupportedRegex = false) => {
const getRegularExpression = (declaration: Declaration, failUnsupportedRegex = false): string => {
if ('pattern' in declaration) {
try {
return parsePattern(declaration.pattern)
Expand All @@ -203,31 +202,38 @@ const getRegularExpression = (declaration: Declaration, failUnsupportedRegex = f
return pathToRegularExpression(declaration.path)
}

const getExcludedRegularExpression = (declaration: Declaration, failUnsupportedRegex = false) => {
const getExcludedRegularExpressions = (declaration: Declaration, failUnsupportedRegex = false): string[] => {
if ('excludedPattern' in declaration && declaration.excludedPattern) {
try {
return parsePattern(declaration.excludedPattern)
} catch (error: unknown) {
// eslint-disable-next-line max-depth
if (failUnsupportedRegex) {
throw new Error(
`Could not parse path declaration of function '${declaration.function}': ${(error as Error).message}`,
const excludedPatterns: string[] = Array.isArray(declaration.excludedPattern)
? declaration.excludedPattern
: [declaration.excludedPattern]
return excludedPatterns.map((excludedPattern) => {
try {
return parsePattern(excludedPattern)
} catch (error: unknown) {
if (failUnsupportedRegex) {
throw new Error(
`Could not parse path declaration of function '${declaration.function}': ${(error as Error).message}`,
)
}

console.warn(
`Function '${
declaration.function
}' uses an unsupported regular expression and will therefore not be invoked: ${(error as Error).message}`,
)
}

console.warn(
`Function '${declaration.function}' uses an unsupported regular expression and will therefore not be invoked: ${
(error as Error).message
}`,
)

return declaration.excludedPattern
}
return excludedPattern
}
})
}

if ('path' in declaration && declaration.excludedPath) {
return pathToRegularExpression(declaration.excludedPath)
const paths = Array.isArray(declaration.excludedPath) ? declaration.excludedPath : [declaration.excludedPath]
return paths.map(pathToRegularExpression)
}

return []
}

interface WriteManifestOptions extends GenerateManifestOptions {
Expand Down
21 changes: 12 additions & 9 deletions node/validation/manifest/schema.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,16 @@ const bundlesSchema = {
additionalProperties: false,
}

const excludedPatternsSchema = {
type: 'array',
items: {
type: 'string',
format: 'regexPattern',
errorMessage:
'excluded_patterns must be an array of regex that starts with ^ and ends with $ (e.g. ^/blog/[d]{4}$)',
},
}

const routesSchema = {
type: 'object',
required: ['function', 'pattern'],
Expand All @@ -19,6 +29,7 @@ const routesSchema = {
format: 'regexPattern',
errorMessage: 'pattern must be a regex that starts with ^ and ends with $ (e.g. ^/blog/[d]{4}$)',
},
excluded_patterns: excludedPatternsSchema,
generator: { type: 'string' },
},
additionalProperties: false,
Expand All @@ -28,15 +39,7 @@ const functionConfigSchema = {
type: 'object',
required: [],
properties: {
excluded_patterns: {
type: 'array',
items: {
type: 'string',
format: 'regexPattern',
errorMessage:
'excluded_patterns must be an array of regex that starts with ^ and ends with $ (e.g. ^/blog/[d]{4}$)',
},
},
excluded_patterns: excludedPatternsSchema,
on_error: { type: 'string' },
},
}
Expand Down
8 changes: 6 additions & 2 deletions test/util.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,12 @@ const getRouteMatcher = (manifest: Manifest) => (candidate: string) =>
return false
}

const excludedPattern = manifest.function_config[route.function].excluded_patterns
const isExcluded = excludedPattern.some((pattern) => new RegExp(pattern).test(candidate))
if (route.excluded_patterns.some((pattern) => new RegExp(pattern).test(candidate))) {
return false
}

const excludedPatterns = manifest.function_config[route.function]?.excluded_patterns ?? []
const isExcluded = excludedPatterns.some((pattern) => new RegExp(pattern).test(candidate))
Copy link

@lukasholzer lukasholzer May 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the regex is invalid then it will throw here with an Uncaught SyntaxError.

Should we handle this case, or is the parsePattern taking care of it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm unsure about that. This is unrelated to the PR, so let's open an issue and address it in another PR :)


return !isExcluded
})
Expand Down