Skip to content
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.

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

feat: Add new GraphQL API for getting occurrences #62531

Merged
merged 12 commits into from May 24, 2024

Conversation

varungandhi-src
Copy link
Contributor

@varungandhi-src varungandhi-src commented May 8, 2024

This is the first stepping stone towards making our APIs more oriented around SCIP
rather than source locations.

Fixes GRAPH-126

Currently based on #62768 (base branch) and also #62789 (first commit) Base PRs were merged.

Test plan

  • Add basic resolver test cases
    • All results in single page
    • Results split over multiple pages
  • [-] Graceful error handling
    • [-] Bad document path Will do in integration test
    • Bad cursor

@cla-bot cla-bot bot added the cla-signed label May 8, 2024
@github-actions github-actions bot added team/graph Graph Team (previously Code Intel/Language Tools/Language Platform) team/product-platform labels May 8, 2024
Base automatically changed from vg/fix-candidates-bug to main May 8, 2024 10:28
@varungandhi-src varungandhi-src force-pushed the vg/occurrences-api branch 5 times, most recently from 524d312 to a268937 Compare May 15, 2024 04:50
@varungandhi-src varungandhi-src force-pushed the vg/occurrences-api branch 3 times, most recently from 051c3d6 to cf254ab Compare May 16, 2024 12:01

EXPERIMENTAL: This API may change in the future.
"""
codeGraphData(filter: CodeGraphDataFilter): [CodeGraphData!]
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should tighten the result here even further -

Suggested change
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?

Copy link
Contributor

Choose a reason for hiding this comment

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

The exact meaning of [...!]! is covered in https://graphql.org/learn/schema/#object-types-and-fields (see appearsIn attribute)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the idea of the server doing the hard work in order to make a simpler easier to use API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@varungandhi-src varungandhi-src force-pushed the vg/occurrences-api branch 8 times, most recently from 5447ff3 to c4299f4 Compare May 20, 2024 10:55
@varungandhi-src varungandhi-src force-pushed the vg/occurrences-api branch 2 times, most recently from 9cc6f3e to e628bf2 Compare May 21, 2024 03:46
@varungandhi-src varungandhi-src changed the base branch from main to vg/simplify-3 May 21, 2024 03:46
@varungandhi-src varungandhi-src marked this pull request as ready for review May 21, 2024 03:48
Base automatically changed from vg/simplify-3 to main May 21, 2024 13:16
@mmanela mmanela requested review from fkling and peterguy May 21, 2024 16:45
Copy link
Member

@eseliger eseliger left a comment

Choose a reason for hiding this comment

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

GQL schema LGTM :)

input CodeGraphDataFilter {
"""
If this field is not set, then the codeGraphData API
will go through each provenance each provenance one by one
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the idea of the server doing the hard work in order to make a simpler easier to use API.

internal/codeintel/resolvers/codenav.go Show resolved Hide resolved
return &syntacticResolvers, err
}

// Enhancement idea: if a syntactic SCIP index is unavailable,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Comment on lines +295 to +296
/*document*/ nil,
/*documentRetrievalError*/ nil,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why prefer this over explicit field naming?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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 😬

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

ohh I'm always down to read some fasterthanlime

Copy link
Contributor

@keynmol keynmol left a comment

Choose a reason for hiding this comment

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

Phew, went through it.

Looks good!

@varungandhi-src varungandhi-src merged commit f06262e into main May 24, 2024
12 checks passed
@varungandhi-src varungandhi-src deleted the vg/occurrences-api branch May 24, 2024 01:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed team/graph Graph Team (previously Code Intel/Language Tools/Language Platform) team/product-platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants