Skip to content

perf: hoist regexp.MustCompile calls to package-level vars in validateExpressionForDangerousProps#20079

Merged
pelikhan merged 2 commits intomainfrom
copilot/fix-regexp-compilation
Mar 8, 2026
Merged

perf: hoist regexp.MustCompile calls to package-level vars in validateExpressionForDangerousProps#20079
pelikhan merged 2 commits intomainfrom
copilot/fix-regexp-compilation

Conversation

Copy link
Contributor

Copilot AI commented Mar 8, 2026

validateExpressionForDangerousProps was compiling two constant regex patterns on every call — one at function scope and one inside the for loop — accumulating unnecessary allocations since validateSingleExpression is recursive and runs on every expression in every compiled workflow.

Changes

  • pkg/workflow/expression_validation.go: Added exprPartSplitRe and exprNumericPartRe to the existing package-level var block; replaced the inline regexp.MustCompile calls with references to these vars.
// Before
parts := regexp.MustCompile(`[.\[\]]+`).Split(trimmed, -1)
for _, part := range parts {
    if part == "" || regexp.MustCompile(`^\d+$`).MatchString(part) {

// After
parts := exprPartSplitRe.Split(trimmed, -1)
for _, part := range parts {
    if part == "" || exprNumericPartRe.MatchString(part) {

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

…ExpressionForDangerousProps

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix regexp.MustCompile inside for loop in validateExpressionForDangerousProps perf: hoist regexp.MustCompile calls to package-level vars in validateExpressionForDangerousProps Mar 8, 2026
@pelikhan pelikhan marked this pull request as ready for review March 8, 2026 16:02
Copilot AI review requested due to automatic review settings March 8, 2026 16:02
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Optimizes expression validation performance by avoiding repeated compilation of constant regular expressions in validateExpressionForDangerousProps, which is invoked frequently during recursive expression validation.

Changes:

  • Hoists the expression-splitting regexp into a package-level precompiled variable.
  • Hoists the numeric-part-matching regexp into a package-level precompiled variable.
  • Updates validateExpressionForDangerousProps to reuse the precompiled regexps.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Comment on lines +80 to +83
// exprPartSplitRe splits expression strings on dot and bracket characters
exprPartSplitRe = regexp.MustCompile(`[.\[\]]+`)
// exprNumericPartRe matches purely numeric expression parts (array indices)
exprNumericPartRe = regexp.MustCompile(`^\d+$`)
Copy link

Copilot AI Mar 8, 2026

Choose a reason for hiding this comment

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

The package-level precompiled regexp vars in this file consistently use the ...Regex suffix (e.g., expressionRegex, needsStepsRegex, numberLiteralRegex). These new names use ...Re, which is inconsistent and makes it harder to scan for regexp vars. Consider renaming to exprPartSplitRegex / exprNumericPartRegex (or another ...Regex-suffixed convention used here).

Copilot uses AI. Check for mistakes.
@pelikhan pelikhan merged commit b221775 into main Mar 8, 2026
113 checks passed
@pelikhan pelikhan deleted the copilot/fix-regexp-compilation branch March 8, 2026 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[plan] Fix regexp.MustCompile inside for loop in validateExpressionForDangerousProps

3 participants