Integrate codeql database unbundle

[marcnjaramillo]

Fixes #1660

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.
• @github/docs-content-codeql has been cc'd in all issues for UI or other user-facing changes made by this pull request.

[aeisenberg]

This is looking really good. There are still a few things left. Mostly, we can simplify things now that we are can handle both the old and new behaviour.

[aeisenberg]

This check is redundant. It is already taking place in readAndUnzip.

Suggested change

	if (this.queryServer && await this.queryServer.cliServer.cliConstraints.supportsDatabaseUnbundle()) {
	return await promptImportInternetDatabase(
	this.databaseManager,
	this.storagePath,
	progress,
	token,
	this.queryServer.cliServer
	);
	} else {
	return await promptImportInternetDatabase(
	this.databaseManager,
	this.storagePath,
	progress,
	token
	);
	}
	return await promptImportInternetDatabase(
	this.databaseManager,
	this.storagePath,
	progress,
	token,
	this.queryServer.cliServer
	);

[marcnjaramillo]

Making this change gives me an error, specifically that queryServer could be undefined.

  handleChooseDatabaseInternet = async (
    progress: ProgressCallback,
    token: CancellationToken
  ): Promise<DatabaseItem | undefined> => {
    return await promptImportInternetDatabase(
      this.databaseManager,
      this.storagePath,
      progress,
      token,
      this.queryServer.cliServer
    );
  };

Doing it this way gives no error:

  handleChooseDatabaseInternet = async (
    progress: ProgressCallback,
    token: CancellationToken
  ): Promise<DatabaseItem | undefined> => {
    return await promptImportInternetDatabase(
      this.databaseManager,
      this.storagePath,
      progress,
      token,
      this.queryServer?.cliServer
    );
  };

[aeisenberg]

Same here

Suggested change

	if (this.queryServer && await this.queryServer.cliServer.cliConstraints.supportsDatabaseUnbundle()) {
	return await promptImportLgtmDatabase(
	this.databaseManager,
	this.storagePath,
	progress,
	token,
	this.queryServer.cliServer
	);
	} else {
	return await promptImportLgtmDatabase(
	this.databaseManager,
	this.storagePath,
	progress,
	token
	);
	}
	if (this.queryServer && await this.queryServer.cliServer.cliConstraints.supportsDatabaseUnbundle()) {
	return await promptImportLgtmDatabase(
	this.databaseManager,
	this.storagePath,
	progress,
	token,
	this.queryServer.cliServer
	);

[marcnjaramillo]

When I remove the else block completely, I get a warning that not all code paths return a value.

  handleChooseDatabaseLgtm = async (
    progress: ProgressCallback,
    token: CancellationToken
  ): Promise<DatabaseItem | undefined> => {

    if (this.queryServer && await this.queryServer.cliServer.cliConstraints.supportsDatabaseUnbundle()) {
      return await promptImportLgtmDatabase(
        this.databaseManager,
        this.storagePath,
        progress,
        token,
        this.queryServer.cliServer
      );
    }
  };

Is that what you'd like me to do? Or should it look like this:

  handleChooseDatabaseLgtm = async (
    progress: ProgressCallback,
    token: CancellationToken
  ): Promise<DatabaseItem | undefined> => {
    if (this.queryServer && await this.queryServer.cliServer.cliConstraints.supportsDatabaseUnbundle()) {
      return await promptImportLgtmDatabase(
        this.databaseManager,
        this.storagePath,
        progress,
        token,
        this.queryServer.cliServer
      );
    }
    return await promptImportLgtmDatabase(
      this.databaseManager,
      this.storagePath,
      progress,
      token
    );
  };

[aeisenberg]

Oops...should be the same as above. You don't need the if statement at all.

  handleChooseDatabaseLgtm = async (
    progress: ProgressCallback,
    token: CancellationToken
  ): Promise<DatabaseItem | undefined> => {
    return await promptImportLgtmDatabase(
      this.databaseManager,
      this.storagePath,
      progress,
      token,
      this?.queryServer.cliServer
    );
  };

This works because you are testing for the cliServer later when you actually try to call unbundle.

[aeisenberg]

Now, this check is no longer needed. You can just fall back to the old behaviour if hte query server doesn't exist.

Suggested change

	if (!this.queryServer) {
	throw new Error('Query server not started');
	}

[aeisenberg]

Suggested change

	this.queryServer.cliServer
	this?.queryServer.cliServer

[aeisenberg]

See comment below, this is no longer needed

[aeisenberg]

Looks good. Let's ship it.