codeintel: Validate package names before insertion.#39444
Conversation
|
Codenotify: Notifying subscribers in CODENOTIFY files for diff c664999...452e2dc.
|
| // applying any normalization or necessary transformations that lsif uploads | ||
| // require for internal consistency. | ||
| func newPackage(pkg shared.Package) precise.Package { | ||
| func newPackage(pkg shared.Package) (*precise.Package, error) { |
There was a problem hiding this comment.
I think its fine to return precise.Package here, as we can see theres a bit of extra work with *precise.Package, unless you really want the out: nil in tests
There was a problem hiding this comment.
It's not about the out: nil in the test case; returning nil prevents a caller from accidentally using an invalid pkg if there is an error (and if it gets used, there is a crash sooner rather than multiple steps later). I know it is unlikely in this specific case, but it seemed safer to do that rather than not.
There was a problem hiding this comment.
Opinion piece: There's precisely one case in Go where you are able to read the non-error value on a non-nil error return (and that's use of io.Reader).
Also it's a bit odd for a NewX method to fail. Could I interest you in a softer alternative such as MakeX?
There was a problem hiding this comment.
To clarify: is your suggestion that I rename the method or is the suggestion that I return a meaningless value in the err case?
There was a problem hiding this comment.
Sorry, those were two separate points. Returning precise.Package{}, err is idiomatic, and ALSO a NewX constructor returning an error catches me up (and I've heard others comment on it). Not sure if there is widespread hate but it's something that bleeps in my head each time I see it.
There was a problem hiding this comment.
OK, I'll change that in a follow-up PR.
There was a problem hiding this comment.
(It's also fairly nit, I just wanted to communicate it)
There was a problem hiding this comment.
Sorry, no take-backsies.
| case dependencies.JVMPackagesScheme: | ||
| p.Name = strings.TrimPrefix(p.Name, "maven/") | ||
| p.Name = strings.ReplaceAll(p.Name, "/", ":") | ||
| case dependencies.NpmPackagesScheme: |
There was a problem hiding this comment.
Why only do this for npm? Shouldn't we do it for all the package schemas? I assumed we could extend this table to have a name-validator function too and then validate every package we insert:
There was a problem hiding this comment.
You're right, I will fix this tomorrow, we can use the ParsePackageFromName API.
Unrelated to that: I also noticed that we have extra custom code here:
func (s *npmPackagesSyncer) ParsePackageFromName(name reposource.PackageName) (reposource.Package, error) {
return s.ParsePackageFromRepoName(api.RepoName("npm/" + strings.TrimPrefix(string(name), "@")))
}
This should be calling one of the npm-specific functions directly instead of modifying the text...
There was a problem hiding this comment.
As per Slack, I will do this cleanup after fixing https://github.com/sourcegraph/sourcegraph/issues/39489
Fixes https://github.com/sourcegraph/sourcegraph/issues/33251, suggested by @mrnugget.
Test plan
Added test case.