Conversation
robertbrignull
left a comment
robertbrignull
left a comment
There was a problem hiding this comment.
Thanks for splitting things up cleanly. All the changes look good to me.
norascheuch
left a comment
norascheuch
left a comment
There was a problem hiding this comment.
I have two small comments, otherwise looks good!
| import { window } from "vscode"; | ||
| import { glob } from "glob"; | ||
| import { basename } from "path"; | ||
| import { load } from "js-yaml"; | ||
| import { readFile } from "fs-extra"; | ||
| import { getQlPackPath } from "../pure/ql"; | ||
| import { CodeQLCliServer, QlpacksInfo } from "../codeql-cli/cli"; | ||
| import { extLogger } from "../common"; | ||
| import { getOnDiskWorkspaceFolders } from "../helpers"; | ||
|
|
||
| export interface QlPacksForLanguage { | ||
| /** The name of the pack containing the dbscheme. */ | ||
| dbschemePack: string; | ||
| /** `true` if `dbschemePack` is a library pack. */ | ||
| dbschemePackIsLibraryPack: boolean; | ||
| /** | ||
| * The name of the corresponding standard query pack. | ||
| * Only defined if `dbschemePack` is a library pack. | ||
| */ | ||
| queryPack?: string; | ||
| } | ||
|
|
||
| interface QlPackWithPath { | ||
| packName: string; | ||
| packDir: string | undefined; | ||
| } | ||
|
|
||
| async function findDbschemePack( | ||
| packs: QlPackWithPath[], | ||
| dbschemePath: string, | ||
| ): Promise<{ name: string; isLibraryPack: boolean }> { | ||
| for (const { packDir, packName } of packs) { | ||
| if (packDir !== undefined) { | ||
| const qlpackPath = await getQlPackPath(packDir); | ||
|
|
||
| if (qlpackPath !== undefined) { | ||
| const qlpack = load(await readFile(qlpackPath, "utf8")) as { | ||
| dbscheme?: string; | ||
| library?: boolean; | ||
| }; | ||
| if ( | ||
| qlpack.dbscheme !== undefined && | ||
| basename(qlpack.dbscheme) === basename(dbschemePath) | ||
| ) { | ||
| return { | ||
| name: packName, | ||
| isLibraryPack: qlpack.library === true, | ||
| }; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| throw new Error(`Could not find qlpack file for dbscheme ${dbschemePath}`); | ||
| } | ||
|
|
||
| function findStandardQueryPack( | ||
| qlpacks: QlpacksInfo, | ||
| dbschemePackName: string, | ||
| ): string | undefined { | ||
| const matches = dbschemePackName.match(/^codeql\/(?<language>[a-z]+)-all$/); | ||
| if (matches) { | ||
| const queryPackName = `codeql/${matches.groups!.language}-queries`; | ||
| if (qlpacks[queryPackName] !== undefined) { | ||
| return queryPackName; | ||
| } | ||
| } | ||
|
|
||
| // Either the dbscheme pack didn't look like one where the queries might be in the query pack, or | ||
| // no query pack was found in the search path. Either is OK. | ||
| return undefined; | ||
| } | ||
|
|
||
| export async function getQlPackForDbscheme( | ||
| cliServer: Pick<CodeQLCliServer, "resolveQlpacks">, | ||
| dbschemePath: string, | ||
| ): Promise<QlPacksForLanguage> { | ||
| const qlpacks = await cliServer.resolveQlpacks(getOnDiskWorkspaceFolders()); | ||
| const packs: QlPackWithPath[] = Object.entries(qlpacks).map( | ||
| ([packName, dirs]) => { | ||
| if (dirs.length < 1) { | ||
| void extLogger.log( | ||
| `In getQlPackFor ${dbschemePath}, qlpack ${packName} has no directories`, | ||
| ); | ||
| return { packName, packDir: undefined }; | ||
| } | ||
| if (dirs.length > 1) { | ||
| void extLogger.log( | ||
| `In getQlPackFor ${dbschemePath}, qlpack ${packName} has more than one directory; arbitrarily choosing the first`, | ||
| ); | ||
| } | ||
| return { | ||
| packName, | ||
| packDir: dirs[0], | ||
| }; | ||
| }, | ||
| ); | ||
| const dbschemePack = await findDbschemePack(packs, dbschemePath); | ||
| const queryPack = dbschemePack.isLibraryPack | ||
| ? findStandardQueryPack(qlpacks, dbschemePack.name) | ||
| : undefined; | ||
| return { | ||
| dbschemePack: dbschemePack.name, | ||
| dbschemePackIsLibraryPack: dbschemePack.isLibraryPack, | ||
| queryPack, | ||
| }; | ||
| } | ||
|
|
||
| export async function getPrimaryDbscheme( | ||
| datasetFolder: string, | ||
| ): Promise<string> { | ||
| const dbschemes = await glob("*.dbscheme", { | ||
| cwd: datasetFolder, | ||
| }); | ||
|
|
||
| if (dbschemes.length < 1) { | ||
| throw new Error( | ||
| `Can't find dbscheme for current database in ${datasetFolder}`, | ||
| ); | ||
| } | ||
|
|
||
| dbschemes.sort(); | ||
| const dbscheme = dbschemes[0]; | ||
|
|
||
| if (dbschemes.length > 1) { | ||
| void window.showErrorMessage( | ||
| `Found multiple dbschemes in ${datasetFolder} during quick query; arbitrarily choosing the first, ${dbscheme}, to decide what library to use.`, | ||
| ); | ||
| } | ||
| return dbscheme; | ||
| } |
There was a problem hiding this comment.
It seems this is only used for the quick query and tests? Can we move it somewhere more related to that? Even though it works with the db scheme I think it's fair to move it outside of the databases folder, e.g. the local-queries folder.
There was a problem hiding this comment.
It's also used in the query resolver (in language-support/contextual/query-resolver.ts), so I think it makes most sense to keep it in the databases folder. I don't think it's related to language-support or local-queries, but is about the database itself.
|
|
||
| /** | ||
| * Recursively walk a directory and return the full path to all files found. | ||
| * Symbolic links are ignored. | ||
| * | ||
| * @param dir the directory to walk | ||
| * | ||
| * @return An iterator of the full path to all files recursively found in the directory. | ||
| */ | ||
| export async function* walkDirectory( | ||
| dir: string, | ||
| ): AsyncIterableIterator<string> { | ||
| const seenFiles = new Set<string>(); | ||
| for await (const d of await opendir(dir)) { | ||
| const entry = join(dir, d.name); | ||
| seenFiles.add(entry); | ||
| if (d.isDirectory()) { | ||
| yield* walkDirectory(entry); | ||
| } else if (d.isFile()) { | ||
| yield entry; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
It looks like only the codeql cli is using this, can we move it into the codeql-cli folder? If not: since we want to phase out the pure folder we should move it into common at least?
There was a problem hiding this comment.
I think for now it makes the most sense to put it in this file, just so it's related to everything else. I agree that we should move it into the common folder if we're going to phase out the pure folder, but I don't think splitting up the file handling functions between two files makes this easier to read and understand.
There was a problem hiding this comment.
That makes sense, best to move the whole file to common!
There was a problem hiding this comment.
I agree, but I don't think it's part of this PR, I think deprecating/moving files out of the pure directory is a separate item.
3 participants
This is the first part in a series that will split the
helpers.tsfile. Each commit splits one or more methods to a separate file or moves it to a more appropriate existing file. This makes them easier to test and decreases the number of things thehelpers.tsfile is doing.Checklist
ready-for-doc-reviewlabel there.