feat: Add new GraphQL API for getting occurrences#62531
feat: Add new GraphQL API for getting occurrences#62531varungandhi-src merged 12 commits intomainfrom
Conversation
524d312 to
a268937
Compare
051c3d6 to
cf254ab
Compare
|
|
||
| EXPERIMENTAL: This API may change in the future. | ||
| """ | ||
| codeGraphData(filter: CodeGraphDataFilter): [CodeGraphData!] |
There was a problem hiding this comment.
I think we should tighten the result here even further -
| codeGraphData(filter: CodeGraphDataFilter): [CodeGraphData!] | |
| codeGraphData(filter: CodeGraphDataFilter): [CodeGraphData!]! |
Just to indicate that we will never return null here, and it will always be an object. Unless there's a situation where null here is an acceptable response?
There was a problem hiding this comment.
The exact meaning of [...!]! is covered in https://graphql.org/learn/schema/#object-types-and-fields (see appearsIn attribute)
There was a problem hiding this comment.
Making this non-null will affect error propagation; if there is an error getting the code graph data, then you won't be able to read other fields for GitBlob, which seems like a net negative from a client's POV.
There was a problem hiding this comment.
We can also strengthen this to become non-optional later, that would be backwards-compatible.
| input CodeGraphDataFilter { | ||
| """ | ||
| If this field is not set, then the codeGraphData API | ||
| will go through each provenance each provenance one by one |
There was a problem hiding this comment.
| will go through each provenance each provenance one by one | |
| will go through each provenance one by one |
I wonder if we should delegate this priority fallback process to the client instead, and make the filter required. This will simplify the implementation and can give better guarantees on execution time of a single API call.
There was a problem hiding this comment.
Delegating this to the client will likely cause redundant work, as the plan is to implement syntactic indexing internally using text search for a first-level filtering process.
There was a problem hiding this comment.
I like the idea of the server doing the hard work in order to make a simpler easier to use API.
There was a problem hiding this comment.
Oh wait, my comment here was wrong.
Delegating this to the client will likely cause redundant work, as the plan is to implement syntactic indexing internally using text search for a first-level filtering process.
The point above applies to the usagesForSymbol API, not here. However, given that both of them will end up being:
provenance: CodeGraphDataProvenanceComparator
It would be weird if the filter for usagesForSymbol followed Precise -> Syntactic -> SearchBased and the filter for codeGraphData didn't have fallback.
5447ff3 to
c4299f4
Compare
9cc6f3e to
e628bf2
Compare
e628bf2 to
77e3853
Compare
This reverts commit 24fe48326e1f55f03678087c2fa60ab3afa53ae6.
77e3853 to
bc43cec
Compare
20edb04 to
a10983f
Compare
a10983f to
62d26c2
Compare
| input CodeGraphDataFilter { | ||
| """ | ||
| If this field is not set, then the codeGraphData API | ||
| will go through each provenance each provenance one by one |
There was a problem hiding this comment.
I like the idea of the server doing the hard work in order to make a simpler easier to use API.
internal/codeintel/codenav/transport/graphql/root_resolver_code_graph.go
Outdated
Show resolved
Hide resolved
62d26c2 to
6c943aa
Compare
89fcc99 to
1c9adb7
Compare
| return &syntacticResolvers, err | ||
| } | ||
|
|
||
| // Enhancement idea: if a syntactic SCIP index is unavailable, |
There was a problem hiding this comment.
I think this is a good idea – I remember us discussing having a two-pronged approach, with relying on batch indexing for infrequently changing files, and doing online recalculation for missing data. Should be fast enough for a single file
There was a problem hiding this comment.
I think we should do it post MVP. If we start taking lockfiles and similar into account, then there will be 2 sub-flavors of syntactic (with and without versioning information), so we will need some fuzzy matching support to take advantage of it.
| /*document*/ nil, | ||
| /*documentRetrievalError*/ nil, |
There was a problem hiding this comment.
Why prefer this over explicit field naming?
There was a problem hiding this comment.
To make sure we get an error when adding new fields and forgetting to add something here. This is a constructor function, so it's extra important that updating it doesn't get missed.
There was a problem hiding this comment.
Huh. I had no idea about this behaviour.
// You can edit this code!
// Click here and start typing.
package main
import "fmt"
type Hello struct {
Name string
Age int
Nick string
}
func main() {
fmt.Println(Hello{"asd", 25}) // FAILS
fmt.Println(Hello{Name: "asd", Age: 25}) // OK
}This is an unpleasant surprise 😬
There was a problem hiding this comment.
Can I interest you in some blog posts?
https://typesanitizer.com/blog/go-experience-report.html
https://fasterthanli.me/articles/i-want-off-mr-golangs-wild-ride
There was a problem hiding this comment.
ohh I'm always down to read some fasterthanlime
keynmol
left a comment
There was a problem hiding this comment.
Phew, went through it.
Looks good!
This is the first stepping stone towards making our APIs more oriented around SCIP
rather than source locations.
Fixes GRAPH-126
Currently based on https://github.com/sourcegraph/sourcegraph/pull/62768 (base branch) and also https://github.com/sourcegraph/sourcegraph/pull/62789 (first commit)Base PRs were merged.Test plan
Bad document pathWill do in integration test