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

Allow downloading multiple databases from GitHub #3073

Merged
merged 2 commits into from
Nov 21, 2023
Merged

Allow downloading multiple databases from GitHub #3073

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d0488dd
Allow downloading multiple databases from GitHub
koesie10 Nov 14, 2023
4f51445
Merge remote-tracking branch 'origin/main' into koesie10/download-mul…
koesie10 Nov 20, 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
102 changes: 68 additions & 34 deletions extensions/ql-vscode/src/databases/github-database-download.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,7 @@ import { window } from "vscode";
import { Octokit } from "@octokit/rest";
import { showNeverAskAgainDialog } from "../common/vscode/dialog";
import { getLanguageDisplayName } from "../common/query-language";
import {
downloadGitHubDatabaseFromUrl,
promptForLanguage,
} from "./database-fetcher";
import { downloadGitHubDatabaseFromUrl } from "./database-fetcher";
import { withProgress } from "../common/vscode/progress";
import { DatabaseManager } from "./local-databases";
import { CodeQLCliServer } from "../codeql-cli/cli";
Expand Down Expand Up @@ -66,40 +63,46 @@ export async function downloadDatabaseFromGitHub(
cliServer: CodeQLCliServer,
commandManager: AppCommandManager,
): Promise<void> {
const languages = databases.map((database) => database.language);

const language = await promptForLanguage(languages, undefined);
if (!language) {
const selectedDatabases = await promptForDatabases(databases);
if (selectedDatabases.length === 0) {
return;
}

const database = databases.find((database) => database.language === language);
if (!database) {
return;
}
await Promise.all(
selectedDatabases.map((database) =>
withProgress(
async (progress) => {
await downloadGitHubDatabaseFromUrl(
Copy link
Contributor

Choose a reason for hiding this comment

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

I found it slightly janky that it keeps selecting a new db when it has finished downloading. So when I download several
DBs I feel like I should be able to start immediately with the fastest one, but I dont think it is a big problem.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's hard to do that since the selection is not controlled by this function itself. It sounds sound like a good idea, but maybe we can see if there's any feedback about this from users and implement something else based on that.

database.url,
database.id,
database.created_at,
database.commit_oid ?? null,
owner,
repo,
octokit,
progress,
databaseManager,
storagePath,
cliServer,
true,
false,
);

await withProgress(async (progress) => {
await downloadGitHubDatabaseFromUrl(
database.url,
database.id,
database.created_at,
database.commit_oid ?? null,
owner,
repo,
octokit,
progress,
databaseManager,
storagePath,
cliServer,
true,
false,
);

await commandManager.execute("codeQLDatabases.focus");
void window.showInformationMessage(
`Downloaded ${getLanguageDisplayName(language)} database from GitHub.`,
);
});
await commandManager.execute("codeQLDatabases.focus");
void window.showInformationMessage(
`Downloaded ${getLanguageDisplayName(
database.language,
)} database from GitHub.`,
);
},
{
title: `Adding ${getLanguageDisplayName(
database.language,
)} database from GitHub`,
},
),
),
);
}

/**
Expand All @@ -126,3 +129,34 @@ function joinLanguages(languages: string[]): string {

return result;
}

async function promptForDatabases(
databases: CodeqlDatabase[],
): Promise<CodeqlDatabase[]> {
if (databases.length === 1) {
return databases;
}

const items = databases
.map((database) => {
const bytesToDisplayMB = `${(database.size / (1024 * 1024)).toFixed(
1,
)} MB`;

return {
label: getLanguageDisplayName(database.language),
description: bytesToDisplayMB,
database,
};
})
.sort((a, b) => a.label.localeCompare(b.label));

const selectedItems = await window.showQuickPick(items, {
title: "Select databases to download",
placeHolder: "Databases found in this repository",
ignoreFocusOut: true,
canPickMany: true,
});

return selectedItems?.map((selectedItem) => selectedItem.database) ?? [];
}
163 changes: 128 additions & 35 deletions ...sions/ql-vscode/test/vscode-tests/no-workspace/databases/github-database-download.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { faker } from "@faker-js/faker";
import { Octokit } from "@octokit/rest";
import { mockedObject } from "../../utils/mocking.helpers";
import { QuickPickItem, window } from "vscode";
import { mockedObject, mockedQuickPickItem } from "../../utils/mocking.helpers";
import {
askForGitHubDatabaseDownload,
downloadDatabaseFromGitHub,
Expand Down Expand Up @@ -103,15 +104,14 @@ describe("downloadDatabaseFromGitHub", () => {
created_at: faker.date.past().toISOString(),
commit_oid: faker.git.commitSha(),
language: "swift",
size: 27389673,
url: faker.internet.url({
protocol: "https",
}),
}),
];

let promptForLanguageSpy: jest.SpiedFunction<
typeof databaseFetcher.promptForLanguage
>;
let showQuickPickSpy: jest.SpiedFunction<typeof window.showQuickPick>;
let downloadGitHubDatabaseFromUrlSpy: jest.SpiedFunction<
typeof databaseFetcher.downloadGitHubDatabaseFromUrl
>;
Expand All @@ -121,9 +121,13 @@ describe("downloadDatabaseFromGitHub", () => {
databaseManager = mockedObject<DatabaseManager>({});
cliServer = mockedObject<CodeQLCliServer>({});

promptForLanguageSpy = jest
.spyOn(databaseFetcher, "promptForLanguage")
.mockResolvedValue(databases[0].language);
showQuickPickSpy = jest.spyOn(window, "showQuickPick").mockResolvedValue(
mockedQuickPickItem([
mockedObject<QuickPickItem & { database: CodeqlDatabase }>({
database: databases[0],
}),
]),
);
downloadGitHubDatabaseFromUrlSpy = jest
.spyOn(databaseFetcher, "downloadGitHubDatabaseFromUrl")
.mockResolvedValue(undefined);
Expand Down Expand Up @@ -157,28 +161,6 @@ describe("downloadDatabaseFromGitHub", () => {
true,
false,
);
expect(promptForLanguageSpy).toHaveBeenCalledWith(["swift"], undefined);
});

describe("when not selecting language", () => {
beforeEach(() => {
promptForLanguageSpy.mockResolvedValue(undefined);
});

it("does not download the database", async () => {
await downloadDatabaseFromGitHub(
octokit,
owner,
repo,
databases,
databaseManager,
storagePath,
cliServer,
commandManager,
);

expect(downloadGitHubDatabaseFromUrlSpy).not.toHaveBeenCalled();
});
});

describe("when there are multiple languages", () => {
Expand All @@ -189,6 +171,7 @@ describe("downloadDatabaseFromGitHub", () => {
created_at: faker.date.past().toISOString(),
commit_oid: faker.git.commitSha(),
language: "swift",
size: 27389673,
url: faker.internet.url({
protocol: "https",
}),
Expand All @@ -198,16 +181,23 @@ describe("downloadDatabaseFromGitHub", () => {
created_at: faker.date.past().toISOString(),
commit_oid: null,
language: "go",
size: 2930572385,
url: faker.internet.url({
protocol: "https",
}),
}),
];

promptForLanguageSpy.mockResolvedValue(databases[1].language);
});

it("downloads the correct database", async () => {
it("downloads a single selected language", async () => {
showQuickPickSpy.mockResolvedValue(
mockedQuickPickItem([
mockedObject<QuickPickItem & { database: CodeqlDatabase }>({
database: databases[1],
}),
]),
);

await downloadDatabaseFromGitHub(
octokit,
owner,
Expand Down Expand Up @@ -235,10 +225,113 @@ describe("downloadDatabaseFromGitHub", () => {
true,
false,
);
expect(promptForLanguageSpy).toHaveBeenCalledWith(
["swift", "go"],
undefined,
expect(showQuickPickSpy).toHaveBeenCalledWith(
[
expect.objectContaining({
label: "Go",
description: "2794.8 MB",
database: databases[1],
}),
expect.objectContaining({
label: "Swift",
description: "26.1 MB",
database: databases[0],
}),
],
expect.anything(),
);
});

it("downloads multiple selected languages", async () => {
showQuickPickSpy.mockResolvedValue(
mockedQuickPickItem([
mockedObject<QuickPickItem & { database: CodeqlDatabase }>({
database: databases[0],
}),
mockedObject<QuickPickItem & { database: CodeqlDatabase }>({
database: databases[1],
}),
]),
);

await downloadDatabaseFromGitHub(
octokit,
owner,
repo,
databases,
databaseManager,
storagePath,
cliServer,
commandManager,
);

expect(downloadGitHubDatabaseFromUrlSpy).toHaveBeenCalledTimes(2);
expect(downloadGitHubDatabaseFromUrlSpy).toHaveBeenCalledWith(
databases[0].url,
databases[0].id,
databases[0].created_at,
databases[0].commit_oid,
owner,
repo,
octokit,
expect.anything(),
databaseManager,
storagePath,
cliServer,
true,
false,
);
expect(downloadGitHubDatabaseFromUrlSpy).toHaveBeenCalledWith(
databases[1].url,
databases[1].id,
databases[1].created_at,
databases[1].commit_oid,
owner,
repo,
octokit,
expect.anything(),
databaseManager,
storagePath,
cliServer,
true,
false,
);
expect(showQuickPickSpy).toHaveBeenCalledWith(
[
expect.objectContaining({
label: "Go",
description: "2794.8 MB",
database: databases[1],
}),
expect.objectContaining({
label: "Swift",
description: "26.1 MB",
database: databases[0],
}),
],
expect.anything(),
);
});

describe("when not selecting language", () => {
beforeEach(() => {
showQuickPickSpy.mockResolvedValue(undefined);
});

it("does not download the database", async () => {
await downloadDatabaseFromGitHub(
octokit,
owner,
repo,
databases,
databaseManager,
storagePath,
cliServer,
commandManager,
);

expect(downloadGitHubDatabaseFromUrlSpy).not.toHaveBeenCalled();
});
});
});
});
Loading