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.

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

feat: remove feature flag for PCRE engine #580

Merged
merged 3 commits into from
Feb 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
11 changes: 4 additions & 7 deletions node/declaration.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { test, expect } from 'vitest'

import { FunctionConfig } from './config.js'
import { Declaration, mergeDeclarations, parsePattern } from './declaration.js'
import { Declaration, mergeDeclarations, normalizePattern } from './declaration.js'

const deployConfigDeclarations: Declaration[] = []

Expand Down Expand Up @@ -164,22 +164,19 @@ test('Does not escape front slashes in a regex pattern if they are already escap
const regexPattern = '^(?:\\/(_next\\/data\\/[^/]{1,}))?(?:\\/([^/.]{1,}))\\/shows(?:\\/(.*))(.json)?[\\/#\\?]?$'
const expected = '^(?:\\/(_next\\/data\\/[^/]{1,}))?(?:\\/([^/.]{1,}))\\/shows(?:\\/(.*))(.json)?[\\/#\\?]?$'

expect(parsePattern(regexPattern, false)).toEqual(expected)
expect(parsePattern(regexPattern, true)).toEqual(expected)
expect(normalizePattern(regexPattern)).toEqual(expected)
})

test('Escapes front slashes in a regex pattern', () => {
const regexPattern = '^(?:/(_next/data/[^/]{1,}))?(?:/([^/.]{1,}))/shows(?:/(.*))(.json)?[/#\\?]?$'
const expected = '^(?:\\/(_next\\/data\\/[^/]{1,}))?(?:\\/([^/.]{1,}))\\/shows(?:\\/(.*))(.json)?[/#\\?]?$'

expect(parsePattern(regexPattern, false)).toEqual(expected)
expect(parsePattern(regexPattern, true)).toEqual(expected)
expect(normalizePattern(regexPattern)).toEqual(expected)
})

test('Ensures pattern match on the whole path', () => {
const regexPattern = '/foo/.*/bar'
const expected = '^\\/foo\\/.*\\/bar$'

expect(parsePattern(regexPattern, false)).toEqual(expected)
expect(parsePattern(regexPattern, true)).toEqual(expected)
expect(normalizePattern(regexPattern)).toEqual(expected)
})
40 changes: 3 additions & 37 deletions node/declaration.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
import regexpAST from 'regexp-tree'

import { FunctionConfig, HTTPMethod, Path } from './config.js'
import { FeatureFlags } from './feature_flags.js'

Expand Down Expand Up @@ -118,10 +116,9 @@ const createDeclarationsFromFunctionConfigs = (

/**
* Normalizes a regular expression, ensuring it has a leading `^` and trailing
* `$` characters. It also converts the regular expression from PCRE to RE2 if
* needed.
* `$` characters.
*/
export const parsePattern = (pattern: string, pcreRegexpEngine: boolean) => {
export const normalizePattern = (pattern: string) => {
let enclosedPattern = pattern

if (!pattern.startsWith('^')) {
Expand All @@ -133,38 +130,7 @@ export const parsePattern = (pattern: string, pcreRegexpEngine: boolean) => {
}

const regexp = new RegExp(enclosedPattern)
const regexpString = pcreRegexpEngine ? regexp.toString() : transformPCRERegexp(regexp)

// Strip leading and forward slashes.
return regexpString.slice(1, -1)
}

/**
* Transforms a PCRE regular expression into a RE2 expression, compatible
* with the Go engine used in our edge nodes.
*/
const transformPCRERegexp = (regexp: RegExp) => {
const newRegexp = regexpAST.transform(regexp, {
Assertion(path) {
// Lookaheads are not supported. If we find one, throw an error.
if (path.node.kind === 'Lookahead') {
throw new Error('Regular expressions with lookaheads are not supported')
}
},

Group(path) {
// Named captured groups in JavaScript use a different syntax than in Go.
// If we find one, convert it to an unnamed capture group, which is valid
// in both engines.
if ('name' in path.node && path.node.name !== undefined) {
path.replace({
...path.node,
name: undefined,
nameRaw: undefined,
})
}
},
})

return newRegexp.toString()
return regexp.toString().slice(1, -1)
}
4 changes: 1 addition & 3 deletions node/feature_flags.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
const defaultFlags = {
edge_bundler_pcre_regexp: false,
}
const defaultFlags = {}

type FeatureFlag = keyof typeof defaultFlags
type FeatureFlags = Partial<Record<FeatureFlag, boolean>>
Expand Down
22 changes: 3 additions & 19 deletions node/manifest.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -433,28 +433,12 @@ test('Generates a manifest with layers', () => {
expect(manifest2.layers).toEqual(layers)
})

test('Throws an error if the regular expression contains a negative lookahead and support for the PCRE engine is disabled', () => {
const functions = [{ name: 'func-1', path: '/path/to/func-1.ts' }]
const declarations = [{ function: 'func-1', pattern: '^\\/((?!api|_next\\/static|_next\\/image|favicon.ico).*)$' }]

expect(() =>
generateManifest({
bundles: [],
declarations,
functions,
}),
).toThrowError(
/^Could not parse path declaration of function 'func-1': Regular expressions with lookaheads are not supported$/,
)
})

test('Accepts regular expressions with lookaheads if support for the PCRE engine is enabled', () => {
test('Accepts regular expressions with lookaheads', () => {
ascorbic marked this conversation as resolved.
Show resolved Hide resolved
const functions = [{ name: 'func-1', path: '/path/to/func-1.ts' }]
const declarations = [{ function: 'func-1', pattern: '^\\/((?!api|_next\\/static|_next\\/image|favicon.ico).*)$' }]
const { manifest } = generateManifest({
bundles: [],
declarations,
featureFlags: { edge_bundler_pcre_regexp: true },
functions,
})
const [route] = manifest.routes
Expand All @@ -464,12 +448,12 @@ test('Accepts regular expressions with lookaheads if support for the PCRE engine
expect(regexp.test('/_next/static/foo')).toBeFalsy()
})

test('Converts named capture groups to unnamed capture groups in regular expressions', () => {
test('Accepts regular expressions with named capture groups', () => {
const functions = [{ name: 'func-1', path: '/path/to/func-1.ts' }]
const declarations = [{ function: 'func-1', pattern: '^/(?<name>\\w+)$' }]
const { manifest } = generateManifest({ bundles: [], declarations, functions })

expect(manifest.routes).toEqual([{ function: 'func-1', pattern: '^/(\\w+)$', excluded_patterns: [] }])
expect(manifest.routes).toEqual([{ function: 'func-1', pattern: '^/(?<name>\\w+)$', excluded_patterns: [] }])
})

test('Returns functions without a declaration and unrouted functions', () => {
Expand Down
15 changes: 7 additions & 8 deletions node/manifest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { join } from 'path'
import type { Bundle } from './bundle.js'
import { wrapBundleError } from './bundle_error.js'
import { Cache, FunctionConfig, Path } from './config.js'
import { Declaration, parsePattern } from './declaration.js'
import { Declaration, normalizePattern } from './declaration.js'
import { EdgeFunction } from './edge_function.js'
import { FeatureFlags } from './feature_flags.js'
import { Layer } from './layer.js'
Expand Down Expand Up @@ -108,7 +108,6 @@ const normalizeMethods = (method: unknown, name: string): string[] | undefined =
const generateManifest = ({
bundles = [],
declarations = [],
featureFlags,
functions,
userFunctionConfig = {},
internalFunctionConfig = {},
Expand Down Expand Up @@ -154,7 +153,7 @@ const generateManifest = ({
return
}

const pattern = getRegularExpression(declaration, Boolean(featureFlags?.edge_bundler_pcre_regexp))
const pattern = getRegularExpression(declaration)

// If there is no `pattern`, the declaration will never be triggered, so we
// can discard it.
Expand All @@ -164,7 +163,7 @@ const generateManifest = ({

routedFunctions.add(declaration.function)

const excludedPattern = getExcludedRegularExpressions(declaration, Boolean(featureFlags?.edge_bundler_pcre_regexp))
const excludedPattern = getExcludedRegularExpressions(declaration)
const route: Route = {
function: func.name,
pattern: serializePattern(pattern),
Expand Down Expand Up @@ -226,10 +225,10 @@ const pathToRegularExpression = (path: string) => {
}
}

const getRegularExpression = (declaration: Declaration, pcreRegexpEngine: boolean) => {
const getRegularExpression = (declaration: Declaration) => {
if ('pattern' in declaration) {
try {
return parsePattern(declaration.pattern, pcreRegexpEngine)
return normalizePattern(declaration.pattern)
} catch (error: unknown) {
throw wrapBundleError(
new Error(
Expand All @@ -242,15 +241,15 @@ const getRegularExpression = (declaration: Declaration, pcreRegexpEngine: boolea
return pathToRegularExpression(declaration.path)
}

const getExcludedRegularExpressions = (declaration: Declaration, pcreRegexpEngine: boolean): string[] => {
const getExcludedRegularExpressions = (declaration: Declaration): string[] => {
if ('excludedPattern' in declaration && declaration.excludedPattern) {
const excludedPatterns: string[] = Array.isArray(declaration.excludedPattern)
? declaration.excludedPattern
: [declaration.excludedPattern]

return excludedPatterns.map((excludedPattern) => {
try {
return parsePattern(excludedPattern, pcreRegexpEngine)
return normalizePattern(excludedPattern)
} catch (error: unknown) {
throw wrapBundleError(
new Error(
Expand Down
5 changes: 3 additions & 2 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,6 @@
"p-retry": "^5.1.1",
"p-wait-for": "^4.1.0",
"path-key": "^4.0.0",
"regexp-tree": "^0.1.24",
"semver": "^7.3.8",
"tmp-promise": "^3.0.3",
"urlpattern-polyfill": "8.0.2",
Expand Down
Loading