Skip to content

Commit

Permalink
Issue #1965 - Fix label resolution for non-captured regex (#1969)
Browse files Browse the repository at this point in the history
<!-- Thanks for sending a PR! Before submitting:

1. If this is your first PR, please read CONTRIBUTING.md and sign the
CLA
   first. We cannot review code without a signed CLA.
2. Please file an issue *first*. All features and most bug fixes should
have
an associated issue with a design discussed and decided upon. Small bug
   fixes and documentation improvements don't need issues.
3. New features and bug fixes must have tests. Documentation may need to
be updated. If you're unsure what to update, send the PR, and we'll
discuss
   in review.
-->

**What type of PR is this?**
Bug fix


**What package or component does this PR mostly affect?**
resolve

**What does this PR do? Why is it needed?**
This change resolves an issue with regex_replace where the default
behavior attempts to perform a string replacement using regex, but
results in an invalid label.

**Which issues(s) does this PR fix?**
Fixes #1965

**Other notes for review**
  • Loading branch information
alandonham authored Nov 5, 2024
1 parent da0ad18 commit 3c129b3
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 3 deletions.
7 changes: 6 additions & 1 deletion resolve/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,12 @@ func (o regexpOverrideSpec) matches(imp ImportSpec, lang string) bool {
}

func (o regexpOverrideSpec) resolveRegexpDep(imp ImportSpec) label.Label {

// If "$" is found in the dependency string, then backreferences exist and
// ReplaceAllString() should be run on the string to substitute in the
// correct replacement strings to build the label.
if !strings.Contains(o.dep.String(), "$") {
return o.dep
}
resolvedDepWithRegex := o.ImpRegex.ReplaceAllString(imp.Imp, o.dep.String())
resolvedLabel, err := label.Parse(resolvedDepWithRegex)
if err != nil {
Expand Down
16 changes: 14 additions & 2 deletions resolve/resolve_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@ func TestFindRuleWithOverride_ParentTraversal(t *testing.T) {
{Key: "resolve_regexp", Value: "go ^github.com/foo/(.*)/(.*)$ @com_example//$1/bar_sub_dir/$2:replacement"},
}, rootCfg)

nonCapturedRegexpCfg := getConfig(t, "", []rule.Directive{
{Key: "resolve_regexp", Value: "py github.com/\\.* @com_example//regexp:no_replacement"},
}, nil)

tests := []struct {
name string
cfg *config.Config
Expand Down Expand Up @@ -103,13 +107,21 @@ func TestFindRuleWithOverride_ParentTraversal(t *testing.T) {
wantFound: true,
},
{
name: "Target resolves to label populated by multipe captured regexp",
name: "Target resolves to label populated by multiple captured regexp",
cfg: multipleExpDualResolveRegexpCfg,
importSpec: ImportSpec{Lang: "go", Imp: "github.com/foo/foo_package/baz"},
lang: "go",
want: getTestLabel(t, "@com_example//foo_package/bar_sub_dir/baz:replacement"),
wantFound: true,
},
},
{
name: "Target resolves to label populated by the configuration with no captured regex",
cfg: nonCapturedRegexpCfg,
importSpec: ImportSpec{Lang: "py", Imp: "github.com/root/repo"},
lang: "py",
want: getTestLabel(t, "@com_example//regexp:no_replacement"),
wantFound: true,
},
}

for _, tt := range tests {
Expand Down

0 comments on commit 3c129b3

Please sign in to comment.