Add query items immediately #1094

aeisenberg · 2022-01-24T17:27:44Z

This is a large commit and includes all the changes to add query
history items immediately. This also includes some smaller related
changes that were hit while cleaning this area up.

The major part of this change is a refactoring of what we store in
the query history list. Previously, the CompletedQuery was stored.
Previously, objects of this type include all information about a query that was run
including:

Its source file and text range (if a quick eval)
Its database
Its label
The query results itself
Metrics about the query run
Metadata about the query itself

Now, the item stored is called a FullQueryInfo, which has two
properties:

InitialQueryInfo: all the data about the query that we know before
the query completes, eg- its source file and text range, database, and
label
CompletedQueryInfo: all the data about the query that we can only
learn after the query completes. This is an optional property.

There is also a failureReason property, which is an optional string
describing why the query failed.

There is also a FullCompletedQueryInfo type, which only exists to
help with stronger typing. It is a FullQueryInfo with a non-optional
CompletedQueryInfo.

Most of the changes are around changing how the query history accesses
its history list.

There are some other smaller changes included here:

New icon for completed query (previously, completed queries had no
icons).
New spinning icon for in progress queries.
Better error handling in the logger to handle log messages when the
extension is shutting down. This mostly helps clean up the output
during tests.
Add more disposables to subscriptions to be disposed of when the
extension shuts down.

Checklist

CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
Issues have been created for any UI or other user-facing changes made by this pull request.
[Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

This is a large commit and includes all the changes to add query history items immediately. This also includes some smaller related changes that were hit while cleaning this area up. The major part of this change is a refactoring of what we store in the query history list. Previously, the `CompletedQuery` was stored. Previously, objects of this type include all information about a query that was run including: - Its source file and text range (if a quick eval) - Its database - Its label - The query results itself - Metrics about the query run - Metadata about the query itself Now, the item stored is called a `FullQueryInfo`, which has two properties: - InitialQueryInfo: all the data about the query that we know _before_ the query completes, eg- its source file and text range, database, and label - CompletedQueryInfo: all the data about the query that we can only learn _after_ the query completes. This is an optional property. There is also a `failureReason` property, which is an optional string describing why the query failed. There is also a `FullCompletedQueryInfo` type, which only exists to help with stronger typing. It is a `FullQueryInfo` with a non-optional `CompletedQueryInfo`. Most of the changes are around changing how the query history accesses its history list. There are some other smaller changes included here: - New icon for completed query (previously, completed queries had no icons). - New spinning icon for in progress queries. - Better error handling in the logger to handle log messages when the extension is shutting down. This mostly helps clean up the output during tests. - Add more disposables to subscriptions to be disposed of when the extension shuts down.

extensions/ql-vscode/src/extension.ts

aeisenberg · 2022-01-25T17:48:29Z

extensions/ql-vscode/src/query-history.ts

+    if (!finalSingleItem.completedQuery) {
+      throw new Error('Select a completed query.');
+    }


This may be annoying. Do we want to silently fail in this case?

Yes, better to fail silently.

I'll keep the explicit error in findOtherQueryToCompare since that may be confusing therwise.

aeisenberg · 2022-01-25T17:50:03Z

extensions/ql-vscode/src/query-history.ts

  ) {
    if (!this.assertSingleQuery(multiSelect)) {
      return;
    }

-    if (singleItem.logFileLocation) {
-      await this.tryOpenExternalFile(singleItem.logFileLocation);
+    if (!singleItem.completedQuery) {


I think.

Suggested change

if (!singleItem.completedQuery) {

if (!singleItem.isCompleted()) {

No...that won't work. If I do this, then this causes a compile error later on.

extensions/ql-vscode/src/query-history.ts

aeisenberg · 2022-01-25T17:53:05Z

extensions/ql-vscode/src/query-history.ts

-      return queryHistoryItem.options.queryText;
-    } else if (queryHistoryItem.query.quickEvalPosition) {
+  async getQueryText(queryHistoryItem: FullQueryInfo): Promise<string> {
+    if (queryHistoryItem.initialInfo.queryText) {


Wait...why would this ever be falsy??

No. It can't. Removing.

aeisenberg · 2022-01-25T17:53:22Z

extensions/ql-vscode/src/query-history.ts

+    }
+
+    if (!queryHistoryItem.completedQuery) {
+      return '<No label>';


I think this should be somethng different. (If we can even get here)

aeisenberg · 2022-01-25T17:54:21Z

extensions/ql-vscode/src/query-results.ts

+ */
+export interface InitialQueryInfo {
+  userSpecifiedLabel?: string; // if missing, use a default label
+  readonly queryText?: string; // text of the selected file


Why will this ever be falsy???

No...but need to fix up some other types.

aeisenberg · 2022-01-25T17:57:07Z

extensions/ql-vscode/src/query-results.ts

+  }
+
+  get time() {
+    return this.initialInfo.start.toLocaleString(env.language);


Note that "time" is now the start time, not the end time.

aeisenberg · 2022-01-25T18:01:52Z

extensions/ql-vscode/src/query-results.ts

+   * If there is a completed query, then check if didRunSuccessfully.
+   * If true, then this query has completed successfully, otherwise it has failed.
+   */
+  get status(): QueryStatus {


Make sure that query cancelling works as expected. Should have a red X (ie- should be considered a failure).

aeisenberg · 2022-01-25T18:04:42Z

extensions/ql-vscode/src/vscode-tests/no-workspace/query-results.test.ts

+    // interpolate
+    // time
+    // label
+    // getShortLabel
+    // getQueryFileName
+    // getQueryName
+
+    // status


Same with "Open query that produced these results". In order to do this, needed to move the query id generation into the InitialQueryInfo.

aeisenberg · 2022-01-26T22:12:53Z

All comments addressed. I love having conversations with myself.

There are some things this PR does not do, but we should probably implement later:

Ability to cancel in progress queries from the query history view
Ability to view in progress logs for queries.

I'll create issues for these.

shati-patel

Thanks for this PR (and for walking me through the changes ✨)

Looks great overall - I've just left a few comments where there's some unexpected behaviour!

shati-patel · 2022-01-27T12:23:12Z

extensions/ql-vscode/src/run-queries.ts

+    id: queryId++,
+    start: new Date(),
+    ... (isQuickEval ? {
+      queryText: quickEvalText!, // if this query is quick eval, it must have quick eval text


I noticed that "Show Query Text" in the Query History doesn't work for quick evals. As in, the quickEvalText seems to be an empty string. Possibly a bug in determineSelectedQuery?

Hmmm...I'm not able to reproduce. There's probably some case I'm not handling. Can you show me an example query and selection that isn't working?

I've been testing with quick-evaling https://github.com/github/codeql/blob/lgtm.com/javascript/ql/lib/semmle/javascript/BasicBlocks.qll#L13 on a github/vscode-codeql database:

With the current version of the extension, I get:

Let me know if you still can't reproduce!

😱 The plot thickens! I ran the exact same quick eval a few times in a row. The first one has "query text", but anything later seems to be broken 😮 I'm officially confused 😅

extensions/ql-vscode/src/logging.ts

extensions/ql-vscode/src/compare/compare-interface.ts

extensions/ql-vscode/src/run-queries.ts

shati-patel · 2022-01-27T12:57:36Z

extensions/ql-vscode/src/query-history.ts

+    return element ? [] : this.history.sort((h1, h2) => {
+      const q1 = h1.completedQuery;
+      const q2 = h2.completedQuery;
+
      switch (this.sortOrder) {


Not sure where in the code to put this comment, but sorting is a bit funny at the moment 🤔 I see the following:

Run a query. The query is at the bottom of the query history list

Once the query finishes, it jumps to the top of the list

Yes...I see that too. Thanks for pointing this out.

Thanks for fixing!

This commit fixes two related issues with the history view. 1. Sort order was changing after a query item completed. The fix is a change in how we fire off the `onDidChangeTreeData` event. When the event is fired with a single item, that item is pushed to the top of the list. I'm not exactly sure why this wasn't happening before, but I suspect it was because we were refreshing the list at the same time as we were inserting the new item. The solution here is to always refresh the entire list, instead of single items. This is fine since re building the list is a trivial operation. See the `refreshTreeView()` method. With this change, the sort order is now stable. 2. Originally reported here: #1093 The problem is that the internal treeView selection was not being updated when a new item was being added. Due to some oddities with the way selection works in the tree view (ie- the visible selection does not always match the internal selection). The solution is to use the current item from the `treeDataProvider` in `determineSelection`. Also, this change makes the sorting more precise and fixes some typos.

aeisenberg · 2022-01-28T00:30:02Z

All comments addressed and problems fixed. Could you take another look, @shati-patel?

aeisenberg · 2022-01-28T02:19:54Z

Actually, there's still the issue with the quick eval. Let's chat about that tomorrow.

shati-patel

Thanks for the changes! 🎉✨

Looks good, just left an example of the quick eval query text bug 😅

shati-patel · 2022-01-28T12:39:14Z

extensions/ql-vscode/src/run-queries.ts

+    id: queryId++,
+    start: new Date(),
+    ... (isQuickEval ? {
+      queryText: quickEvalText!, // if this query is quick eval, it must have quick eval text


I've been testing with quick-evaling https://github.com/github/codeql/blob/lgtm.com/javascript/ql/lib/semmle/javascript/BasicBlocks.qll#L13 on a github/vscode-codeql database:

With the current version of the extension, I get:

Let me know if you still can't reproduce!

aeisenberg

Thanks for the explanation now. I think I see the problem.

aeisenberg · 2022-01-29T18:09:23Z

As far as I can tell, the only difference between the way quick eval text is displayed before and after this PR is that when the quick eval selection is not a selection, but just a single caret location, the entire line is shown as part of the quick eval text.

Both before and after, if the quick eval selection is a block of text, exactly that text is shown. This is not ideal, but since it is the same as the old behaviour, I don't think we should change. We could consider being more intelligent about grabbing surrounding text in a future change.

aeisenberg · 2022-01-29T18:25:29Z

And looking at how this was implemented in the past, I don't really think this is correct. The logic is that if there is no text saved with the query when it was created, then look at the quick eval location. Expand the location to capture all selected lines.

This will be incorrect if the query file has changed after the query has run. I consider this a bug. We should be capturing the query text at the time the query was run.

Fixes a bug where quick eval was showing empty query text. Previously, `getQueryText` was looking up the query text when it was called if the specified text was empty. This was removed with the recent changes to query history. It was also a bug since the query file could have changed after the query was run. This change ensures that if the quick eval position is empty, the entire line is returned as the quick eval location.

shati-patel

Thanks! ✨

Agree that there seems to be some general weirdness around quick eval, but that's out of scope for this PR 😅

aeisenberg requested a review from a team as a code owner January 24, 2022 17:27

aeisenberg linked an issue Jan 24, 2022 that may be closed by this pull request

Create Query History Items Immediately #400

Closed

aeisenberg force-pushed the aeisenberg/query-history branch from 16c3e66 to 43f5c37 Compare January 24, 2022 20:10

Update changelog

a69dbd7

aeisenberg force-pushed the aeisenberg/query-history branch from 43f5c37 to a69dbd7 Compare January 24, 2022 21:29

aeisenberg commented Jan 25, 2022

View reviewed changes

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

aeisenberg commented Jan 25, 2022

View reviewed changes

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

aeisenberg commented Jan 25, 2022

View reviewed changes

Ensure Open Query Text works for in progress queries

db962a4

Same with "Open query that produced these results". In order to do this, needed to move the query id generation into the InitialQueryInfo.

This was referenced Jan 26, 2022

Ability to cancel in progress queries from the query history view #1101

Closed

Ability to view in progress logs for queries #1102

Open

shati-patel reviewed Jan 27, 2022

View reviewed changes

aeisenberg mentioned this pull request Jan 27, 2022

Can't delete "Quick evaluation" result #1093

Closed

Add tests for sort order and selection

dd0a21a

aeisenberg force-pushed the aeisenberg/query-history branch from 3b62f0f to dd0a21a Compare January 28, 2022 04:30

shati-patel reviewed Jan 28, 2022

View reviewed changes

aeisenberg commented Jan 28, 2022

View reviewed changes

shati-patel approved these changes Feb 1, 2022

View reviewed changes

aeisenberg merged commit 0672133 into main Feb 1, 2022

aeisenberg deleted the aeisenberg/query-history branch February 1, 2022 14:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add query items immediately #1094

Add query items immediately #1094

aeisenberg commented Jan 24, 2022 •

edited

Loading

aeisenberg Jan 25, 2022

aeisenberg Jan 26, 2022

aeisenberg Jan 26, 2022

aeisenberg Jan 25, 2022

aeisenberg Jan 26, 2022

aeisenberg Jan 25, 2022

aeisenberg Jan 26, 2022

aeisenberg Jan 25, 2022

aeisenberg Jan 26, 2022

aeisenberg Jan 25, 2022

aeisenberg Jan 26, 2022

aeisenberg Jan 25, 2022

aeisenberg Jan 26, 2022

aeisenberg Jan 25, 2022

aeisenberg Jan 26, 2022

aeisenberg Jan 25, 2022

aeisenberg commented Jan 26, 2022

shati-patel left a comment

shati-patel Jan 27, 2022

aeisenberg Jan 27, 2022

shati-patel Jan 28, 2022

shati-patel Jan 28, 2022

shati-patel Jan 27, 2022

aeisenberg Jan 27, 2022

shati-patel Jan 28, 2022

aeisenberg commented Jan 28, 2022

aeisenberg commented Jan 28, 2022

shati-patel left a comment

shati-patel Jan 28, 2022

aeisenberg left a comment

aeisenberg commented Jan 29, 2022

aeisenberg commented Jan 29, 2022

shati-patel left a comment

	if (!singleItem.completedQuery) {
	if (!singleItem.isCompleted()) {

Add query items immediately #1094

Add query items immediately #1094

Conversation

aeisenberg commented Jan 24, 2022 • edited Loading

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeisenberg commented Jan 26, 2022

shati-patel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeisenberg commented Jan 28, 2022

aeisenberg commented Jan 28, 2022

shati-patel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeisenberg left a comment

Choose a reason for hiding this comment

aeisenberg commented Jan 29, 2022

aeisenberg commented Jan 29, 2022

shati-patel left a comment

Choose a reason for hiding this comment

aeisenberg commented Jan 24, 2022 •

edited

Loading