Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

chore(search): update search API call sites to set the version explicitly#63782

Merged
stefanhengl merged 2 commits into
mainfrom
stefan-splf-106-update-api-calls-to-set-the-version-explicitly
Jul 12, 2024
Merged

chore(search): update search API call sites to set the version explicitly#63782
stefanhengl merged 2 commits into
mainfrom
stefan-splf-106-update-api-calls-to-set-the-version-explicitly

Conversation

@stefanhengl

@stefanhengl stefanhengl commented Jul 11, 2024

Copy link
Copy Markdown
Member

I went through all call sites of the 3 search APIs (Stream API, GQL API, SearchClient (internal)) and made sure that the query syntax version is set to "V3".

Why?

Without this change, a new default search syntax version might have caused a change in behavior for some of the call sites.

Test plan

  • No functional change, so relying mostly on CI
  • The codeintel GQL queries set the patternType explicitly, so this change is a NOP.

I tested manually

  • search based code intel sends GQL requests with version "V3"
  • repo badge still works
  • compute GQL returns results

@cla-bot cla-bot Bot added the cla-signed label Jul 11, 2024
@github-actions github-actions Bot added team/product-platform team/search-platform Issues owned by the search platform team labels Jul 11, 2024
SavedSearchResult,
} from '../graphql-operations'

export function fetchReposByQuery(query: string): Observable<{ name: string; url: string }[]> {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This was unused

expect(receivedQuery).toEqual('r:golang/oauth2 test f:travis')
expect(receivedOptions).toEqual({
version: 'V3',
version: LATEST_VERSION,

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is consistent with the rest of the web client code.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you explain the reasoning for using LATEST_VERSION for web client code, while explicitly pinning to V3 elsewhere? Would it be simpler/ more consistent to use one or the other everywhere?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Answering my own question: this constant is only available in the web client :)

const countGoImportersGraphQLQuery = `
query CountGoImporters($query: String!) {
search(query: $query) { results { matchCount } }
query CountGoImporters($query: String!, $version: SearchVersion!) {

@stefanhengl stefanhengl Jul 11, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This supports a rather obscure feature for .com I didn't know about. See here.

@stefanhengl stefanhengl requested a review from a team July 11, 2024 12:58
@stefanhengl stefanhengl marked this pull request as ready for review July 11, 2024 12:59

@jtibshirani jtibshirani left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks good to me. The test coverage at this layer is definitely light, but the change feels low risk.

@stefanhengl stefanhengl merged commit b77809c into main Jul 12, 2024
@stefanhengl stefanhengl deleted the stefan-splf-106-update-api-calls-to-set-the-version-explicitly branch July 12, 2024 08:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla-signed team/product-platform team/search-platform Issues owned by the search platform team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants