Autodetect what language a query is for by shati-patel · Pull Request #915 · github/vscode-codeql

shati-patel · 2021-07-26T11:46:50Z

Adds a findLanguage function that uses codeql resolve queries --format bylanguage to detect a query's language. This might not work for some queries, so I've added a manual selection as a fallback.

This is used in two places:

When running a query on multiple databases (we now only show the databases that correspond to the query language)
When running a remote query (we can now autodetect a language when it isn't specified in the .repositories config. This config file is going to be replaced by a different UI soon.)

Question: Does this need tests? I wasn't sure where/how to go about that!

Checklist

CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
- Only the "multiple DBs" part is user-facing for now
Issues have been created for any UI or other user-facing changes made by this pull request.
@github/docs-content-codeql has been cc'd in all issues for UI or other user-facing changes made by this pull request.
- No docs update required (beyond pending update from Docs: Running query on multiple DBs in CodeQL for VS Code codeql#6322)

extensions/ql-vscode/src/cli.ts

aeisenberg

This looks right to me!

extensions/ql-vscode/src/cli.ts

edoardopirovano

Mostly looks good apart from a minor comment inline. I'll hold off on approving it, though, since we paired on this for a bit so it should probably have another set of eyes.

extensions/ql-vscode/src/run-remote-query.ts

adityasharad

Looks good! I think the code is mostly ready to go, with one outstanding suggestion from Edoardo about return types, and one from me about logging.

I've added some suggestions about tests, but I think those are fine to add later, since it will require some upfront work to set up tests for this part of the code.

Have we been adding changelog entries for this feature?

extensions/ql-vscode/src/run-remote-query.ts

adityasharad · 2021-08-02T19:24:02Z

extensions/ql-vscode/src/cli.ts

+   * Resolves the language for a query.
+   * @param queryUri The URI of the query
+   */
+  async resolveQueryByLanguage(workspaces: string[], queryUri: Uri): Promise<QueryInfoByLanguage> {


Since you asked about tests:

extensions/ql-vscode/src/vscode-tests/cli-integration/run-cli.test.ts is a good place to add integration tests for the resolveQueryByLanguage and resolveLanguages functions.

You can use the existing tests as an example.

resolveLanguages is probably easier to write a test for because there's no query as input.

resolveQueryByLanguage will require a test query: there are some examples in the data folders, but as far as I can tell none of them depend on an actual language. So you may need to add one, or save this test for later.

adityasharad · 2021-08-02T19:35:05Z

extensions/ql-vscode/src/run-remote-query.ts

+ * Finds the language that a query targets.
+ * If it can't be autodetected, prompt the user to specify the language manually.
+ */
+export async function findLanguage(


Since you asked about tests: I agree that having tests for this function would be handy, and give you confidence in its behaviour. However we don't yet have a test module for run-remote-query due to the experimental nature of the entire feature, so adding a test requires some upfront work. (In future it will be easier :) )

I'll record some pointers here about how you could create those tests, but I'm not expecting you to do it in this PR. We can look at test coverage together after this PR.

For integration tests (which use a real query and actually run the underlying commands), copy extensions/ql-vscode/src/vscode-tests/cli-integration/queries.test.ts as a template. I think it would make sense to have:

one test function that runs findLanguage on a query with a real language dependency and identifies the language

one test function that runs it on a dummy query and returns undefined

any other cases you can think of

For unit tests (which don't run the actual commands, but simulate the underlying behaviour), use extensions/ql-vscode/src/vscode-tests/no-workspace/query-results.test.ts as a template. Those tests mock the behaviour of the CLI server.

Also use autodection in relevant places - When running on multiple databases - When running a remote query

- Tweak return types + logging - Update changelog

shati-patel · 2021-08-04T15:29:20Z

Thank you for the reviews ✨ I've addressed the main comments in 83c68ec and added a minimal changelog entry that covers the user-facing part of this.

I'll look at adding tests in a separate PR!

edoardopirovano

A couple more minor comments, but otherwise looks good!

edoardopirovano · 2021-08-04T15:36:54Z

extensions/ql-vscode/src/extension.ts

+            return;
+          }
+        }
+        const quickPickItems = filteredDBs.map<DatabaseQuickPickItem>(dbItem => (


Note that if the user has no databases and doesn't pick a language when prompted, we try to display this prompt with nothing in it. There should probably be an extra check to display an error in this case and return, similarly to what happens with the filtered list above.

Woops, good point! Thanks 😄

edoardopirovano · 2021-08-04T15:37:20Z

extensions/ql-vscode/src/run-remote-query.ts

  const repositories = config.repositories;

+  if (!language) {
+    return;


I would add a brief comment stating that we don't need an error message here because findLanguage already displays one.

edoardopirovano

Thanks for addressing all my comments, this LGTM 👍🏼

shati-patel commented Jul 26, 2021

View reviewed changes

extensions/ql-vscode/src/cli.ts Outdated Show resolved Hide resolved

shati-patel force-pushed the language-detection branch from c17caba to dac4201 Compare July 26, 2021 15:52

shati-patel marked this pull request as ready for review July 26, 2021 16:32

shati-patel requested a review from a team as a code owner July 26, 2021 16:32

edoardopirovano reviewed Jul 26, 2021

View reviewed changes

extensions/ql-vscode/src/cli.ts Outdated Show resolved Hide resolved

edoardopirovano reviewed Jul 26, 2021

View reviewed changes

extensions/ql-vscode/src/cli.ts Outdated Show resolved Hide resolved

shati-patel marked this pull request as draft July 26, 2021 17:46

aeisenberg reviewed Jul 26, 2021

View reviewed changes

extensions/ql-vscode/src/cli.ts Outdated Show resolved Hide resolved

shati-patel changed the title ~~[WIP] Attempt to autodetect what language a query is for~~ Autodetect what language a query is for Jul 28, 2021

shati-patel marked this pull request as ready for review July 28, 2021 14:53

edoardopirovano reviewed Aug 2, 2021

View reviewed changes

extensions/ql-vscode/src/run-remote-query.ts Show resolved Hide resolved

adityasharad reviewed Aug 2, 2021

View reviewed changes

shati-patel added 2 commits August 4, 2021 16:15

Autodetect language using "resolve queries"

1aef089

Also use autodection in relevant places - When running on multiple databases - When running a remote query

Address review comments

83c68ec

- Tweak return types + logging - Update changelog

shati-patel force-pushed the language-detection branch from b8a90b7 to 83c68ec Compare August 4, 2021 15:16

edoardopirovano reviewed Aug 4, 2021

View reviewed changes

adityasharad approved these changes Aug 4, 2021

View reviewed changes

Don't show empty list of DBs

ad4ff84

edoardopirovano approved these changes Aug 4, 2021

View reviewed changes

shati-patel merged commit 2bfcd11 into github:main Aug 4, 2021

shati-patel deleted the language-detection branch August 4, 2021 19:00

shati-patel mentioned this pull request Aug 31, 2021

Tidy and add tests for resolveLanguages commands #937

Merged

3 tasks

Conversation

shati-patel commented Jul 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aeisenberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

edoardopirovano left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adityasharad left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

adityasharad Aug 2, 2021

Choose a reason for hiding this comment

Uh oh!

adityasharad Aug 2, 2021

Choose a reason for hiding this comment

Uh oh!

shati-patel commented Aug 4, 2021

Uh oh!

edoardopirovano left a comment

Choose a reason for hiding this comment

Uh oh!

edoardopirovano Aug 4, 2021

Choose a reason for hiding this comment

Uh oh!

shati-patel Aug 4, 2021

Choose a reason for hiding this comment

Uh oh!

edoardopirovano Aug 4, 2021

Choose a reason for hiding this comment

Uh oh!

edoardopirovano left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

shati-patel commented Jul 26, 2021 •

edited

Loading