Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't add database source archive folders by default #3047

Merged
merged 9 commits into from
Nov 13, 2023
+46 −12
Merged

Don't add database source archive folders by default #3047

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
3288572
Don't add database source archive folders by default
shati-patel Nov 1, 2023
2bf264a
Use the same setting for the model editor
shati-patel Nov 1, 2023
528af4a
Fix tests
shati-patel Nov 1, 2023
de10640
Rename global "databases" setting
shati-patel Nov 6, 2023
bb548ee
Revert "Use the same setting for the model editor"
shati-patel Nov 6, 2023
c771c05
Rename variable
shati-patel Nov 6, 2023
c111a74
Update changelog
shati-patel Nov 1, 2023
080fb9d
Add deprecation message for old setting
shati-patel Nov 13, 2023
1c79737
Update changelog
shati-patel Nov 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions extensions/ql-vscode/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@

## [UNRELEASED]

- When adding a CodeQL database, we no longer add the database source folder to the workspace by default (since this caused bugs in single-folder workspaces). [#3047](https://github.com/github/vscode-codeql/pull/3047)
- You can manually add individual database source folders to the workspace with the "Add Database Source to Workspace" right-click command in the databases view.
- To restore the old behavior of adding all database source folders by default, set the `codeQL.addingDatabases.addDatabaseSourceToWorkspace` setting to `true`.
- The "Sort by Language" action in the databases view now sorts by name within each language. [#3055](https://github.com/github/vscode-codeql/pull/3055)

## 1.9.4 - 6 November 2023
Expand Down
14 changes: 12 additions & 2 deletions extensions/ql-vscode/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,13 +372,23 @@
},
{
"type": "object",
"title": "Downloading databases",
"title": "Adding databases",
"order": 6,
"properties": {
"codeQL.addingDatabases.allowHttp": {
"type": "boolean",
"default": false,
"description": "Allow databases to be downloaded via HTTP. Warning: enabling this option will allow downloading from insecure servers."
},
"codeQL.databaseDownload.allowHttp": {
"type": "boolean",
"markdownDeprecationMessage": "**Deprecated**: Please use `#codeQL.addingDatabases.allowHttp#` instead.",
"deprecationMessage": "Deprecated: Please use codeQL.addingDatabases.allowHttp instead."
},
Comment on lines 383 to +387
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As mentioned offline, users who still use this old setting name will get a deprecation message:
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the record: we will no longer support the old setting name. This isn't the "formal" way of deprecating a setting, but this is a sufficiently niche/insecure setting that it shouldn't have much user impact.

In the case where a user did have this setting enabled: If they try to download a DB from an http location, they'll get the error message "Must use https for downloading a database.", which should point them to check their settings.

Copy link
Contributor

@robertbrignull robertbrignull Nov 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Discussed offline as well, but this should be fine. As said it's not the full proper way of renaming a setting, and we could aim to support both for a transition period, but we expect this setting to be almost (if not completely) unused. And as Shati points out, if someone is using this the error should point them towards checking their settings and seeing the deprecation notice. This along with a changelog entry should mean that anyone who is using it sees it and updates their settings.

"codeQL.addingDatabases.addDatabaseSourceToWorkspace": {
"type": "boolean",
"default": false,
"description": "Allow database to be downloaded via HTTP. Warning: enabling this option will allow downloading from insecure servers."
"markdownDescription": "When adding a CodeQL database, automatically add the database's source folder as a workspace folder. Warning: enabling this option in a single-folder workspace will cause the workspace to reload as a [multi-root workspace](https://code.visualstudio.com/docs/editor/multi-root-workspaces). This may cause query history and database lists to be reset."
}
}
},
Expand Down
13 changes: 11 additions & 2 deletions extensions/ql-vscode/src/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -641,14 +641,23 @@ export function isCodespacesTemplate() {
return !!CODESPACES_TEMPLATE.getValue<boolean>();
}

const DATABASE_DOWNLOAD_SETTING = new Setting("databaseDownload", ROOT_SETTING);
const ADDING_DATABASES_SETTING = new Setting("addingDatabases", ROOT_SETTING);

const ALLOW_HTTP_SETTING = new Setting("allowHttp", DATABASE_DOWNLOAD_SETTING);
const ALLOW_HTTP_SETTING = new Setting("allowHttp", ADDING_DATABASES_SETTING);

export function allowHttp(): boolean {
return ALLOW_HTTP_SETTING.getValue<boolean>() || false;
}

const ADD_DATABASE_SOURCE_TO_WORKSPACE_SETTING = new Setting(
"addDatabaseSourceToWorkspace",
ADDING_DATABASES_SETTING,
);

export function addDatabaseSourceToWorkspace(): boolean {
return ADD_DATABASE_SOURCE_TO_WORKSPACE_SETTING.getValue<boolean>() || false;
}

/**
* Parent setting for all settings related to the "Create Query" command.
*/
Expand Down
10 changes: 5 additions & 5 deletions extensions/ql-vscode/src/databases/database-fetcher.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import {
} from "../common/github-url-identifier-helper";
import { Credentials } from "../common/authentication";
import { AppCommandManager } from "../common/commands";
import { allowHttp } from "../config";
import { addDatabaseSourceToWorkspace, allowHttp } from "../config";
import { showAndLogInformationMessage } from "../common/logging";
import { AppOctokit } from "../common/octokit";
import { getLanguageDisplayName } from "../common/query-language";
Expand Down Expand Up @@ -99,7 +99,7 @@ export async function promptImportGithubDatabase(
cli?: CodeQLCliServer,
language?: string,
makeSelected = true,
addSourceArchiveFolder = true,
addSourceArchiveFolder = addDatabaseSourceToWorkspace(),
): Promise<DatabaseItem | undefined> {
const githubRepo = await askForGitHubRepo(progress);
if (!githubRepo) {
Expand Down Expand Up @@ -178,7 +178,7 @@ export async function downloadGitHubDatabase(
cli?: CodeQLCliServer,
language?: string,
makeSelected = true,
addSourceArchiveFolder = true,
addSourceArchiveFolder = addDatabaseSourceToWorkspace(),
): Promise<DatabaseItem | undefined> {
const nwo = getNwoFromGitHubUrl(githubRepo) || githubRepo;
if (!isValidGitHubNwo(nwo)) {
Expand Down Expand Up @@ -295,7 +295,7 @@ async function databaseArchiveFetcher(
progress: ProgressCallback,
cli?: CodeQLCliServer,
makeSelected = true,
addSourceArchiveFolder = true,
addSourceArchiveFolder = addDatabaseSourceToWorkspace(),
): Promise<DatabaseItem> {
progress({
message: "Getting database",
Expand Down Expand Up @@ -476,7 +476,7 @@ async function checkForFailingResponse(
return response;
}

// An error downloading the database. Attempt to extract the resaon behind it.
// An error downloading the database. Attempt to extract the reason behind it.
const text = await response.text();
let msg: string;
try {
Expand Down
5 changes: 3 additions & 2 deletions extensions/ql-vscode/src/databases/local-databases/database-manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { QueryRunner } from "../../query-server";
import * as cli from "../../codeql-cli/cli";
import { ProgressCallback, withProgress } from "../../common/vscode/progress";
import {
addDatabaseSourceToWorkspace,
getAutogenerateQlPacks,
isCodespacesTemplate,
setAutogenerateQlPacks,
Expand Down Expand Up @@ -135,7 +136,7 @@ export class DatabaseManager extends DisposableObject {
displayName?: string,
{
isTutorialDatabase = false,
addSourceArchiveFolder = true,
addSourceArchiveFolder = addDatabaseSourceToWorkspace(),
}: OpenDatabaseOptions = {},
): Promise<DatabaseItem> {
const databaseItem = await this.createDatabaseItem(uri, displayName);
Expand All @@ -158,7 +159,7 @@ export class DatabaseManager extends DisposableObject {
databaseItem: DatabaseItemImpl,
makeSelected: boolean,
isTutorialDatabase?: boolean,
addSourceArchiveFolder = true,
addSourceArchiveFolder = addDatabaseSourceToWorkspace(),
): Promise<DatabaseItem> {
const existingItem = this.findDatabaseItem(databaseItem.databaseUri);
if (existingItem !== undefined) {
Expand Down
12 changes: 11 additions & 1 deletion ...sions/ql-vscode/test/vscode-tests/minimal-workspace/local-queries/local-databases.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -729,7 +729,17 @@ describe("local databases", () => {
expect(setCurrentDatabaseItemSpy).toBeCalledTimes(1);
});

it("should add database source archive folder", async () => {
it("should not add database source archive folder when `codeQL.addingDatabases.addDatabaseSourceToWorkspace` is `false`", async () => {
jest.spyOn(config, "addDatabaseSourceToWorkspace").mockReturnValue(false);

await databaseManager.openDatabase(mockDbItem.databaseUri);

expect(addDatabaseSourceArchiveFolderSpy).toBeCalledTimes(0);
});

it("should add database source archive folder when `codeQL.addingDatabases.addDatabaseSourceToWorkspace` is `true`", async () => {
jest.spyOn(config, "addDatabaseSourceToWorkspace").mockReturnValue(true);

await databaseManager.openDatabase(mockDbItem.databaseUri);

expect(addDatabaseSourceArchiveFolderSpy).toBeCalledTimes(1);
Expand Down
Loading