Skip to content

fix(gopackagesdriver): Improve wildcard package query matching #4288

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix(gopackagesdriver): Improve wildcard package query matching #4288

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
60ab876
Omit only start of string matching from wildcard package queries
Aaronkala Mar 6, 2025
23be4f0
escape literal importpath when used as a regular expression
Aaronkala Mar 6, 2025
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
2 changes: 1 addition & 1 deletion go/tools/gopackagesdriver/bazel_json_builder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ func (b *BazelJSONBuilder) adjustToRelativePathIfPossible(request string) string

func (b *BazelJSONBuilder) packageQuery(importPath string) string {
if strings.HasSuffix(importPath, "/...") {
importPath = fmt.Sprintf(`^%s(/.+)?$`, strings.TrimSuffix(importPath, "/..."))
importPath = fmt.Sprintf(`%s(/.+)?$`, strings.TrimSuffix(importPath, "/..."))
Copy link
Author

@Aaronkala Aaronkala Mar 6, 2025

Choose a reason for hiding this comment

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

The pattern supplied being treated as a regular expression can result in different behaviour if the input happens to contain regexp special characters. Leaving it as is here

Copy link
Member

@fmeum fmeum Mar 6, 2025

Choose a reason for hiding this comment

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

If we change this, let's fix that as well. Could you wrap the import path into regexp.QuoteMeta?

Copy link
Author

@Aaronkala Aaronkala Mar 6, 2025

Choose a reason for hiding this comment

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

I pushed a commit that escapes both the wild card case and the default case: 23be4f0

But this essentially means that the ./something query won't work at least against the path that we are seeing //something as \./something won't match //something. I'm unsure if this is correct, and/or if the //something paths are only specific to our implementation.

Copy link
Author

@Aaronkala Aaronkala Mar 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Okay the importpaths it's actually matching against are in the form of github.com/orgname/reponame/something/foo/bar as defined in go_library() importpath

I'll test if importpath_aliases aliases fixes this for us with something like ./something/foo/bar as the alias

Copy link
Author

@Aaronkala Aaronkala Mar 7, 2025

Choose a reason for hiding this comment

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

Hmm but the query is targetting only importpath 🤔

Copy link
Member

@fmeum fmeum May 6, 2025

Choose a reason for hiding this comment

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

Sorry for getting back to this so late, where are we at? Should I take another look?

}

return fmt.Sprintf(
Expand Down