Usages API: implement surroundingContent

[camdencheek]

Instead of fetching the file for every node, this passes in a request-scoped cache to minimize the number of gitserver roundtrips, but does no fetching if surroundingContent is not requested by the caller.

Fixes GRAPH-724

Test plan

Manual testing by using this in the web client.

[camdencheek]

Unrelated, but it's most useful to me as a consumer to get a hard error if the API is disabled. Otherwise, I have no way of distinguishing "disabled" from "no results"

[kritzcreek]

Ahh I see. I was thinking that, because I can get the surrounding lines through the Search APIs (which I'm calling anyway, for non-precise usages), I'd just pass that data through, rather than going to back to gitserver to re-fetch the files.

But that would require knowing the number of surroundingLines at the "request" level for usagesForSymbol (which is why I made that comment on the issue)

[camdencheek]

But that would require knowing the number of surroundingLines at the "request" level for usagesForSymbol (which is why I made that comment on the issue)

One option is to wrap the LineGetter in another layer that uses the search result in the case the chunk in the search result chunk is largh enough to satisfy the request

[kritzcreek]

One option is to wrap the LineGetter in another layer that uses the search result in the case the chunk in the search result chunk is largh enough to satisfy the request

Varun said something like that as well. Use the size the frontend will request as a heuristic, and only re-fetch contents if the line count differs. Still feels a bit silly to me that the search API lets me specify exactly how many lines I want, and the requested count is specified in the request I'm currently fulfilling, but I can't get at that information because of the resolver structure GraphQL imposes.

[camdencheek]

feels a bit silly

Resolvers can, in fact, feel pretty silly 🙂 In particular, the GraphQL lib we use makes it impossible to inspect the request to determine ahead of time what data to fetch. See this issue that I've been subscribed to for years

[camdencheek]

Use the size the frontend will request as a heuristic

FWIW, I have no plans to make the frontend request more than just the containing line. We could probably simplify the API by only supporting the containing line for now. If we ever need more than that, it would be a backwards-compatible change to add an argument that allows growing the selection.

[kritzcreek]

FWIW, I have no plans to make the frontend request more than just the containing line. We could probably simplify the API by only supporting the containing line for now. If we ever need more than that, it would be a backwards-compatible change to add an argument that allows growing the selection.

Great! Let's do that. We might still end up needing the LineGetter for precise usages, but it should be easy enough to resurrect it from this PR.
If this PR lets you make progress in the meantime I'm happy to accept it and break the API in a follow-up PR.

[camdencheek]

If this PR lets you make progress in the meantime I'm happy to accept it and break the API in a follow-up PR.

Let's do that then 🙂 To be explicit, from my end, you can assume for now that I will never pass an arg to surroundingContext and just returning the containing line is A-OK