Create a new octokit instance every time

I believe this doesn't change the user-visible behaviour at all. The user
won't be prompted to log in any more or less often than they would have
done before.

One benefit of this is that we can remove the registerListeners method
because we no longer need to know if the cached octokit is still valid.
Instead we just call vscode.authentication.getSession every time and it
will return the current session, which might be different from the last
time we called it. This might prompt the user to log in, but that would
have happened anyway because when the session changed we would have
overwritten our cached octokit instance.

Another benefit is that we no longer need the extension context and this
removed a surprisingly large amount of code where we are passing this
parameter around because we need it for the credentials.

The only downside I can see is that we call getSession more often and
create more javascript objects in general. I believe the performance
impact of this will be negligible and not worth worrying about.

Author: robertbrignull

extensions/ql-vscode/src/authentication.ts

@@ -13,42 +13,38 @@ const SCOPES = ["repo", "gist"];
  * Handles authentication to GitHub, using the VS Code [authentication API](https://code.visualstudio.com/api/references/vscode-api#authentication).
  */
 export class Credentials {
+  /**
+   * A specific octokit to return, otherwise a new authenticated octokit will be created when needed.
+   */
   private octokit: Octokit.Octokit | undefined;
 
   // Explicitly make the constructor private, so that we can't accidentally call the constructor from outside the class
   // without also initializing the class.
-  // eslint-disable-next-line @typescript-eslint/no-empty-function
-  private constructor() {}
+  private constructor(octokit?: Octokit.Octokit) {
+    this.octokit = octokit;
+  }
 
   /**
-   * Initializes an instance of credentials with an octokit instance.
-   *
-   * Do not call this method until you know you actually need an instance of credentials.
-   * since calling this method will require the user to log in.
+   * Initializes a Credentials instance. This will generate octokit instances
+   * authenticated as the user. If there is not already an authenticated GitHub
+   * session availabeT then the user will be prompted to log in.
    *
-   * @param context The extension context.
    * @returns An instance of credentials.
    */
-  static async initialize(
-    context: vscode.ExtensionContext,
-  ): Promise<Credentials> {
-    const c = new Credentials();
-    c.registerListeners(context);
-    return c;
+  static async initialize(): Promise<Credentials> {
+    return new Credentials();
   }
 
   /**
    * Initializes an instance of credentials with an octokit instance using
-   * a token from the user's GitHub account. This method is meant to be
-   * used non-interactive environments such as tests.
+   * a specific known token. This method is meant to be used non-interactive
+   * environments such as tests.
    *
    * @param overrideToken The GitHub token to use for authentication.
    * @returns An instance of credentials.
    */
   static async initializeWithToken(overrideToken: string) {
-    const c = new Credentials();
-    c.octokit = new Octokit.Octokit({ auth: overrideToken, retry });
-    return c;
+    return new Credentials(new Octokit.Octokit({ auth: overrideToken, retry }));
   }
 
   private async createOctokit(): Promise<Octokit.Octokit> {
@@ -64,26 +60,15 @@ export class Credentials {
     });
   }
 
-  registerListeners(context: vscode.ExtensionContext): void {
-    // Sessions are changed when a user logs in or logs out.
-    context.subscriptions.push(
-      vscode.authentication.onDidChangeSessions(async (e) => {
-        if (e.provider.id === GITHUB_AUTH_PROVIDER_ID) {
-          this.octokit = undefined;
-        }
-      }),
-    );
-  }
-
   /**
    * Creates or returns an instance of Octokit.
    *
    * @returns An instance of Octokit.
    */
   async getOctokit(): Promise<Octokit.Octokit> {
-    if (!this.octokit) {
-      this.octokit = await this.createOctokit();
+    if (this.octokit) {
+      return this.octokit;
     }
-    return this.octokit;
+    return await this.createOctokit();
   }
 }

extensions/ql-vscode/src/extension.ts

@@ -591,7 +591,7 @@ async function activateWithInstalledDistribution(
     qs,
     getContextStoragePath(ctx),
     ctx.extensionPath,
-    () => Credentials.initialize(ctx),
+    () => Credentials.initialize(),
   );
   databaseUI.init();
   ctx.subscriptions.push(databaseUI);
@@ -1236,7 +1236,7 @@ async function activateWithInstalledDistribution(
     commandRunner(
       "codeQL.exportRemoteQueryResults",
       async (queryId: string) => {
-        await exportRemoteQueryResults(qhm, rqm, ctx, queryId);
+        await exportRemoteQueryResults(qhm, rqm, queryId);
       },
     ),
   );
@@ -1251,7 +1251,6 @@ async function activateWithInstalledDistribution(
         filterSort?: RepositoriesFilterSortStateWithIds,
       ) => {
         await exportVariantAnalysisResults(
-          ctx,
           variantAnalysisManager,
           variantAnalysisId,
           filterSort,
@@ -1356,7 +1355,7 @@ async function activateWithInstalledDistribution(
       "codeQL.chooseDatabaseGithub",
       async (progress: ProgressCallback, token: CancellationToken) => {
         const credentials = isCanary()
-          ? await Credentials.initialize(ctx)
+          ? await Credentials.initialize()
           : undefined;
         await databaseUI.handleChooseDatabaseGithub(
           credentials,
@@ -1411,7 +1410,7 @@ async function activateWithInstalledDistribution(
        * Credentials for authenticating to GitHub.
        * These are used when making API calls.
        */
-      const credentials = await Credentials.initialize(ctx);
+      const credentials = await Credentials.initialize();
       const octokit = await credentials.getOctokit();
       const userInfo = await octokit.users.getAuthenticated();
       void showAndLogInformationMessage(

extensions/ql-vscode/src/query-history.ts

@@ -397,7 +397,7 @@ export class QueryHistoryManager extends DisposableObject {
     private readonly variantAnalysisManager: VariantAnalysisManager,
     private readonly evalLogViewer: EvalLogViewer,
     private readonly queryStorageDir: string,
-    private readonly ctx: ExtensionContext,
+    ctx: ExtensionContext,
     private readonly queryHistoryConfigListener: QueryHistoryConfig,
     private readonly labelProvider: HistoryItemLabelProvider,
     private readonly doCompareCallback: (
@@ -633,7 +633,7 @@ export class QueryHistoryManager extends DisposableObject {
   }
 
   private getCredentials() {
-    return Credentials.initialize(this.ctx);
+    return Credentials.initialize();
   }
 
   /**

extensions/ql-vscode/src/remote-queries/analyses-results-manager.ts

@@ -1,7 +1,7 @@
 import { pathExists } from "fs-extra";
 import { EOL } from "os";
 import { extname } from "path";
-import { CancellationToken, ExtensionContext } from "vscode";
+import { CancellationToken } from "vscode";
 
 import { Credentials } from "../authentication";
 import { Logger } from "../common";
@@ -26,7 +26,6 @@ export class AnalysesResultsManager {
   private readonly analysesResults: Map<string, AnalysisResults[]>;
 
   constructor(
-    private readonly ctx: ExtensionContext,
     private readonly cliServer: CodeQLCliServer,
     readonly storagePath: string,
     private readonly logger: Logger,
@@ -43,7 +42,7 @@ export class AnalysesResultsManager {
       return;
     }
 
-    const credentials = await Credentials.initialize(this.ctx);
+    const credentials = await Credentials.initialize();
 
     void this.logger.log(
       `Downloading and processing results for ${analysisSummary.nwo}`,
@@ -77,7 +76,7 @@ export class AnalysesResultsManager {
       (x) => !this.isAnalysisInMemory(x),
     );
 
-    const credentials = await Credentials.initialize(this.ctx);
+    const credentials = await Credentials.initialize();
 
     void this.logger.log("Downloading and processing analyses results");
 

extensions/ql-vscode/src/remote-queries/export-results.ts

@@ -4,7 +4,6 @@ import { ensureDir, writeFile } from "fs-extra";
 import {
   commands,
   CancellationToken,
-  ExtensionContext,
   Uri,
   ViewColumn,
   window,
@@ -74,7 +73,6 @@ export async function exportSelectedRemoteQueryResults(
 export async function exportRemoteQueryResults(
   queryHistoryManager: QueryHistoryManager,
   remoteQueriesManager: RemoteQueriesManager,
-  ctx: ExtensionContext,
   queryId: string,
 ): Promise<void> {
   const queryHistoryItem = queryHistoryManager.getRemoteQueryById(queryId);
@@ -107,7 +105,6 @@ export async function exportRemoteQueryResults(
   const exportedResultsDirectory = join(exportDirectory, "exported-results");
 
   await exportRemoteQueryAnalysisResults(
-    ctx,
     exportedResultsDirectory,
     query,
     analysesResults,
@@ -116,7 +113,6 @@ export async function exportRemoteQueryResults(
 }
 
 export async function exportRemoteQueryAnalysisResults(
-  ctx: ExtensionContext,
   exportedResultsPath: string,
   query: RemoteQuery,
   analysesResults: AnalysisResults[],
@@ -126,7 +122,6 @@ export async function exportRemoteQueryAnalysisResults(
   const markdownFiles = generateMarkdown(query, analysesResults, exportFormat);
 
   await exportResults(
-    ctx,
     exportedResultsPath,
     description,
     markdownFiles,
@@ -141,7 +136,6 @@ const MAX_VARIANT_ANALYSIS_EXPORT_PROGRESS_STEPS = 2;
  * The user is prompted to select the export format.
  */
 export async function exportVariantAnalysisResults(
-  ctx: ExtensionContext,
   variantAnalysisManager: VariantAnalysisManager,
   variantAnalysisId: number,
   filterSort: RepositoriesFilterSortStateWithIds | undefined,
@@ -255,7 +249,6 @@ export async function exportVariantAnalysisResults(
   );
 
   await exportVariantAnalysisAnalysisResults(
-    ctx,
     exportedResultsDirectory,
     variantAnalysis,
     getAnalysesResults(),
@@ -266,7 +259,6 @@ export async function exportVariantAnalysisResults(
 }
 
 export async function exportVariantAnalysisAnalysisResults(
-  ctx: ExtensionContext,
   exportedResultsPath: string,
   variantAnalysis: VariantAnalysis,
   analysesResults: AsyncIterable<
@@ -297,7 +289,6 @@ export async function exportVariantAnalysisAnalysisResults(
   );
 
   await exportResults(
-    ctx,
     exportedResultsPath,
     description,
     markdownFiles,
@@ -341,7 +332,6 @@ async function determineExportFormat(): Promise<"gist" | "local" | undefined> {
 }
 
 export async function exportResults(
-  ctx: ExtensionContext,
   exportedResultsPath: string,
   description: string,
   markdownFiles: MarkdownFile[],
@@ -354,7 +344,7 @@ export async function exportResults(
   }
 
   if (exportFormat === "gist") {
-    await exportToGist(ctx, description, markdownFiles, progress, token);
+    await exportToGist(description, markdownFiles, progress, token);
   } else if (exportFormat === "local") {
     await exportToLocalMarkdown(
       exportedResultsPath,
@@ -366,7 +356,6 @@ export async function exportResults(
 }
 
 export async function exportToGist(
-  ctx: ExtensionContext,
   description: string,
   markdownFiles: MarkdownFile[],
   progress?: ProgressCallback,
@@ -378,7 +367,7 @@ export async function exportToGist(
     message: "Creating Gist",
   });
 
-  const credentials = await Credentials.initialize(ctx);
+  const credentials = await Credentials.initialize();
 
   if (token?.isCancellationRequested) {
     throw new UserCancellationException("Cancelled");

extensions/ql-vscode/src/remote-queries/remote-queries-manager.ts

@@ -81,20 +81,19 @@ export class RemoteQueriesManager extends DisposableObject {
   private readonly view: RemoteQueriesView;
 
   constructor(
-    private readonly ctx: ExtensionContext,
+    ctx: ExtensionContext,
     private readonly cliServer: CodeQLCliServer,
     private readonly storagePath: string,
     logger: Logger,
   ) {
     super();
     this.analysesResultsManager = new AnalysesResultsManager(
-      ctx,
       cliServer,
       storagePath,
       logger,
     );
     this.view = new RemoteQueriesView(ctx, logger, this.analysesResultsManager);
-    this.remoteQueriesMonitor = new RemoteQueriesMonitor(ctx, logger);
+    this.remoteQueriesMonitor = new RemoteQueriesMonitor(logger);
 
     this.remoteQueryAddedEventEmitter = this.push(
       new EventEmitter<NewQueryEvent>(),
@@ -160,7 +159,7 @@ export class RemoteQueriesManager extends DisposableObject {
     progress: ProgressCallback,
     token: CancellationToken,
   ): Promise<void> {
-    const credentials = await Credentials.initialize(this.ctx);
+    const credentials = await Credentials.initialize();
 
     const {
       actionBranch,
@@ -218,7 +217,7 @@ export class RemoteQueriesManager extends DisposableObject {
     remoteQuery: RemoteQuery,
     cancellationToken: CancellationToken,
   ): Promise<void> {
-    const credentials = await Credentials.initialize(this.ctx);
+    const credentials = await Credentials.initialize();
 
     const queryWorkflowResult = await this.remoteQueriesMonitor.monitorQuery(
       remoteQuery,

extensions/ql-vscode/src/remote-queries/remote-queries-monitor.ts

@@ -16,16 +16,13 @@ export class RemoteQueriesMonitor {
   private static readonly maxAttemptCount = 17280;
   private static readonly sleepTime = 5000;
 
-  constructor(
-    private readonly extensionContext: vscode.ExtensionContext,
-    private readonly logger: Logger,
-  ) {}
+  constructor(private readonly logger: Logger) {}
 
   public async monitorQuery(
     remoteQuery: RemoteQuery,
     cancellationToken: vscode.CancellationToken,
   ): Promise<RemoteQueryWorkflowResult> {
-    const credentials = await Credentials.initialize(this.extensionContext);
+    const credentials = await Credentials.initialize();
 
     let attemptCount = 0;
 

extensions/ql-vscode/src/remote-queries/variant-analysis-manager.ts

@@ -106,7 +106,6 @@ export class VariantAnalysisManager
     super();
     this.variantAnalysisMonitor = this.push(
       new VariantAnalysisMonitor(
-        ctx,
         this.shouldCancelMonitorVariantAnalysis.bind(this),
       ),
     );
@@ -125,7 +124,7 @@ export class VariantAnalysisManager
     progress: ProgressCallback,
     token: CancellationToken,
   ): Promise<void> {
-    const credentials = await Credentials.initialize(this.ctx);
+    const credentials = await Credentials.initialize();
 
     const {
       actionBranch,
@@ -479,7 +478,7 @@ export class VariantAnalysisManager
 
     await this.onRepoStateUpdated(variantAnalysis.id, repoState);
 
-    const credentials = await Credentials.initialize(this.ctx);
+    const credentials = await Credentials.initialize();
 
     if (cancellationToken && cancellationToken.isCancellationRequested) {
       repoState.downloadStatus =
@@ -577,7 +576,7 @@ export class VariantAnalysisManager
       );
     }
 
-    const credentials = await Credentials.initialize(this.ctx);
+    const credentials = await Credentials.initialize();
 
     void showAndLogInformationMessage(
       "Cancelling variant analysis. This may take a while.",