Non destructive upgrades in vscode #679

alexet · 2020-11-13T19:14:48Z

This depends on a rebased copy of #639 and also on some unreleased cli features and shouldn't be merged until we know which version they will be released in.

This PR makes us use silent non-destructive upgrades. It also refactors the current upgrade mechanism in places so we have slightly fewer code paths.

There are a few identation changes so you may want to add ?w=1 to the url when reviewing to ignore those changes.

aeisenberg

This is a partial review. Looks good. Just some small things. I'll finish this off on Monday.

extensions/ql-vscode/src/pure/messages.ts

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

extensions/ql-vscode/src/upgrades.ts

extensions/ql-vscode/src/cli.ts

extensions/ql-vscode/src/pure/messages.ts

extensions/ql-vscode/src/upgrades.ts

alexet · 2021-01-11T18:12:35Z

You comment have made me realise that we can simplify the queryserver api, but it will mean that we need to wait for another codeql release to get this in.

alexet · 2021-01-25T16:50:23Z

Ok this should be ready for re-review. Sorry I was so slow at coming back to this.

aeisenberg

I think the only major issue is the missing await clause. Everything else is just wording and naming changes.

aeisenberg · 2021-01-25T17:14:30Z

extensions/ql-vscode/src/pure/messages.ts

+/**
+ * Compile an upgrade script to upgrade a dataset.
+ */
+export const compileUpgradeSequence = new rpc.RequestType<WithProgressId<CompileUpgradeSequenceParams>, CompiledUpgradeSequence, void, void>('compilation/compileUpgradeSequence');


Our naming convention is usually XxxParams and XxxResult. Here it's CompileUpgradeSequenceParams and CompiledUpgradeSequence. Can you follow the same convention here?

Hmmm...and I see below that EvaluateQueriesParams and EvaluationComplete does not follow the convention either. I still think it's useful to do so in order to know how these two interfaces are related.

I may fix that in a later PR.

aeisenberg · 2021-01-25T17:19:14Z

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

 }

+function reportNoUpgradePath(query: QueryInfo) {
+  throw new Error(`Query ${query.program.queryPath} expects database scheme ${query.queryDbscheme}, but the current database has a different scheme, and no database upgrades are available. The current database scheme may be newer than the CodeQL query libraries in your workspace. Please try using a newer version of the query libraries.`);


Nit: Consider emphasizing the proposed solution so it's not hidden in a wall of text.

Suggested change

throw new Error(`Query ${query.program.queryPath} expects database scheme ${query.queryDbscheme}, but the current database has a different scheme, and no database upgrades are available. The current database scheme may be newer than the CodeQL query libraries in your workspace. Please try using a newer version of the query libraries.`);

throw new Error(`Query ${query.program.queryPath} expects database scheme ${query.queryDbscheme}, but the current database has a different scheme, and no database upgrades are available. The current database scheme may be newer than the CodeQL query libraries in your workspace.\n\nPlease try using a newer version of the query libraries.`);

aeisenberg · 2021-01-25T17:23:25Z

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

-  await checkDbschemeCompatibility(cliServer, qs, query, progress, token);

-  let errors;
+  const upgradeDir = tmp.dirSync({ dir: upgradesTmpDir.name });


Avoid using sync functions in user code. You can use the tmp-promise package to make an async call to creating a tmp directory. After changing the import statement, all you need to do is this:

Suggested change

const upgradeDir = tmp.dirSync({ dir: upgradesTmpDir.name });

const upgradeDir = await tmp.dir({ dir: upgradesTmpDir.name });

aeisenberg · 2021-01-25T17:25:18Z

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

+      if (result.resultType !== messages.QueryResultType.SUCCESS) {
+        const message = result.message || 'Failed to run query';
+        logger.log(message);
+        helpers.showAndLogErrorMessage(message);


Nit: should already be imported directly.

Suggested change

helpers.showAndLogErrorMessage(message);

showAndLogErrorMessage(message);

aeisenberg · 2021-01-25T17:28:22Z

extensions/ql-vscode/src/upgrades.ts

-  }
-  finally {
-    qs.logger.log('Done checking database upgrade.');
+  if (!hasNondestructiveUpgradeCapabilities(qs)) {


Nice...code scanning worked here. This expression needs an await.

aeisenberg · 2021-01-25T17:31:16Z

extensions/ql-vscode/src/upgrades.ts

-  finally {
-    qs.logger.log('Done checking database upgrade.');
+  if (!hasNondestructiveUpgradeCapabilities(qs)) {
+    throw new Error('The version of codeql is to old to run non-destructive upgrades.');


Suggested change

throw new Error('The version of codeql is to old to run non-destructive upgrades.');

throw new Error('The version of codeql is to old too run non-destructive upgrades.');

aeisenberg · 2021-01-27T19:20:19Z

extensions/ql-vscode/src/upgrades.ts

  db: DatabaseItem,
  resolvedSequence: string[],
-  currentUpgradeTmp: tmp.DirResult,
+  currentUpgradeTmp: tmp.DirectoryResult,


I didn't realize that the type names were slightly different.

alexet requested a review from aeisenberg November 13, 2020 19:14

alexet marked this pull request as draft November 13, 2020 19:43

aeisenberg reviewed Nov 14, 2020

View reviewed changes

aeisenberg reviewed Nov 16, 2020

View reviewed changes

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

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

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

aeisenberg reviewed Nov 16, 2020

View reviewed changes

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

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

alexet force-pushed the non-destructive-upgrades branch from 22770d2 to 5335a84 Compare December 7, 2020 17:45

alexet force-pushed the non-destructive-upgrades branch from 5335a84 to 3c81e06 Compare December 16, 2020 16:51

alexet force-pushed the non-destructive-upgrades branch from 3c81e06 to 2cc06d4 Compare January 8, 2021 15:57

aeisenberg reviewed Jan 8, 2021

View reviewed changes

alexet added 10 commits January 25, 2021 16:36

Add support for new cli feature

788fc3d

QueryServer: Add new commands to client.

8ac3ee8

QueryServer: Use non-destructive upgrades where possible.

1d6d62a

Adress review comments

e316536

Fix rebase conflict

d81e846

Set codeql version to required version.

8dd67e2

Remove uneeded argument

8111a96

Adress comments on non-destructive upgrades.

f5be2d8

Simplify query server interface.

e0f5b95

Remove unused variable

bff5e2f

alexet marked this pull request as ready for review January 25, 2021 16:49

alexet force-pushed the non-destructive-upgrades branch from c1d906c to bff5e2f Compare January 25, 2021 16:50

aeisenberg requested changes Jan 25, 2021

View reviewed changes

alexet added 2 commits January 27, 2021 18:18

Adress comments

8a1946d

Use asycy tmp

907a094

aeisenberg approved these changes Jan 28, 2021

View reviewed changes

aeisenberg merged commit 89b8605 into github:main Jan 29, 2021

snyk-bot mentioned this pull request Feb 4, 2022

[Snyk] Fix for 2 vulnerabilities snitebobbie/vscode-codeql#20

Open

Dec	JAN	Feb
	01
2025	2026	2027

	throw new Error(`Query ${query.program.queryPath} expects database scheme ${query.queryDbscheme}, but the current database has a different scheme, and no database upgrades are available. The current database scheme may be newer than the CodeQL query libraries in your workspace. Please try using a newer version of the query libraries.`);
	throw new Error(`Query ${query.program.queryPath} expects database scheme ${query.queryDbscheme}, but the current database has a different scheme, and no database upgrades are available. The current database scheme may be newer than the CodeQL query libraries in your workspace.\n\nPlease try using a newer version of the query libraries.`);

	const upgradeDir = tmp.dirSync({ dir: upgradesTmpDir.name });
	const upgradeDir = await tmp.dir({ dir: upgradesTmpDir.name });

	helpers.showAndLogErrorMessage(message);
	showAndLogErrorMessage(message);

	throw new Error('The version of codeql is to old to run non-destructive upgrades.');
	throw new Error('The version of codeql is to old too run non-destructive upgrades.');

Non destructive upgrades in vscode #679

Non destructive upgrades in vscode #679

Uh oh!

Conversation

alexet commented Nov 13, 2020

Uh oh!

aeisenberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alexet commented Jan 11, 2021

Uh oh!

alexet commented Jan 25, 2021

Uh oh!

aeisenberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants