Skip to content

making query history persistent#21544

Merged
aasimkhan30 merged 8 commits intomainfrom
aasim/fix/queryHistory
Mar 18, 2026
Merged

making query history persistent#21544
aasimkhan30 merged 8 commits intomainfrom
aasim/fix/queryHistory

Conversation

@aasimkhan30
Copy link
Contributor

@aasimkhan30 aasimkhan30 commented Mar 10, 2026

Description

Partially address: #21196

Query history is now restored from and written to an AES-256-GCM encrypted file in extension global storage, with the encryption key kept in SecretStorage.

Code Changes Checklist

  • New or updated unit tests added
  • All existing tests pass (npm run test)
  • Code follows contributing guidelines
  • Telemetry/logging updated if relevant
  • No regressions or UX breakage

Reviewers: Please read our reviewer guidelines

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds persistence for the MSSQL Query History tree so entries can be restored across VS Code sessions (partial progress toward #21196).

Changes:

  • Persist/restore query history via ExtensionContext.secrets in QueryHistoryProvider, with versioned payload + bounds on stored nodes/query length.
  • Wire ExtensionContext into QueryHistoryProvider from MainController and add a dedicated SecretStorage key constant.
  • Add unit tests covering persistence/restore behavior and edge cases; expose isSuccess getter on QueryHistoryNode.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
extensions/mssql/src/queryHistory/queryHistoryProvider.ts Implements query history persistence/restore and adjusts node handling/sorting.
extensions/mssql/src/controllers/mainController.ts Passes ExtensionContext into QueryHistoryProvider.
extensions/mssql/src/constants/constants.ts Adds queryHistorySecretStorageKey constant for persistence.
extensions/mssql/src/queryHistory/queryHistoryNode.ts Adds isSuccess getter used by persistence/tests.
extensions/mssql/test/unit/queryHistoryProvider.test.ts Adds unit test coverage for persistence/restore and related behaviors.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@github-actions
Copy link

github-actions bot commented Mar 10, 2026

PR Changes

Category Target Branch PR Branch Difference
vscode-mssql VSIX 6452 KB 6411 KB ⚪ -41 KB ( 0% )
sql-database-projects VSIX 7062 KB 7061 KB ⚪ -1 KB ( 0% )
data-workspace VSIX 535 KB 535 KB ⚪ 0 KB ( 0% )

@codecov-commenter
Copy link

codecov-commenter commented Mar 10, 2026

Codecov Report

❌ Patch coverage is 76.47059% with 76 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.80%. Comparing base (a41239d) to head (bd83c7b).
⚠️ Report is 61 commits behind head on main.

Files with missing lines Patch % Lines
...ons/mssql/src/queryHistory/queryHistoryProvider.ts 71.60% 71 Missing ⚠️
extensions/mssql/src/utils/encryptionUtils.ts 93.93% 4 Missing ⚠️
extensions/mssql/src/controllers/mainController.ts 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #21544      +/-   ##
==========================================
+ Coverage   72.67%   72.80%   +0.12%     
==========================================
  Files         331      332       +1     
  Lines       98489    98802     +313     
  Branches     5462     5495      +33     
==========================================
+ Hits        71580    71934     +354     
+ Misses      26909    26868      -41     
Files with missing lines Coverage Δ
extensions/mssql/src/constants/constants.ts 100.00% <100.00%> (ø)
...ensions/mssql/src/queryHistory/queryHistoryNode.ts 96.90% <100.00%> (+48.52%) ⬆️
extensions/mssql/src/controllers/mainController.ts 33.92% <0.00%> (-0.02%) ⬇️
extensions/mssql/src/utils/encryptionUtils.ts 93.93% <93.93%> (ø)
...ons/mssql/src/queryHistory/queryHistoryProvider.ts 67.28% <71.60%> (+29.52%) ⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Aasim Khan added 2 commits March 10, 2026 10:46
- Added encryptionUtils.ts for data encryption and decryption using AES-256-GCM.
- Implemented functions to generate encryption keys, encrypt data, and decrypt data.
- Created unit tests for encryption utilities to ensure functionality and security.
- Updated queryHistoryProvider tests to utilize encrypted storage for query history.
- Ensured sensitive data, such as passwords, are handled securely during storage and retrieval.
Copilot AI review requested due to automatic review settings March 10, 2026 18:09
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +407 to +411
const persistedCredentials = { ...credentials };
if ((credentials as IConnectionProfile).savePassword === false) {
persistedCredentials.password = "";
}

Copy link

Copilot AI Mar 10, 2026

Choose a reason for hiding this comment

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

sanitizeCredentialsForPersistence currently persists the entire IConnectionInfo object (including fields like azureAccountToken/expiresOn and potentially connectionString, which may embed credentials). Even though the file is encrypted, persisting access tokens/connection strings extends the lifetime of secrets and increases exposure risk. Consider persisting a minimal, explicit allow-list (e.g., server/database/authenticationType/user and only password when savePassword=true), and explicitly omit azureAccountToken/expiresOn/connectionString (or scrub secrets from it) before writing.

Suggested change
const persistedCredentials = { ...credentials };
if ((credentials as IConnectionProfile).savePassword === false) {
persistedCredentials.password = "";
}
const savePassword = (credentials as IConnectionProfile).savePassword;
const passwordToPersist =
savePassword === false ? "" : (credentials as vscodeMssql.IConnectionInfo).password;
// Explicitly allow-list non-sensitive fields needed to restore context.
// Do not persist access tokens, expiry times, or connection strings.
const persistedCredentials = {
server: credentials.server,
database: credentials.database,
authenticationType: credentials.authenticationType,
user: (credentials as vscodeMssql.IConnectionInfo).user,
password: passwordToPersist,
} as vscodeMssql.IConnectionInfo;

Copilot uses AI. Check for mistakes.
@aasimkhan30 aasimkhan30 changed the title making query history persistent across session making query history persistent Mar 18, 2026
kburtram
kburtram previously approved these changes Mar 18, 2026
lewis-sanchez
lewis-sanchez previously approved these changes Mar 18, 2026
Copilot AI review requested due to automatic review settings March 18, 2026 23:03
@aasimkhan30 aasimkhan30 dismissed stale reviews from lewis-sanchez and kburtram via bd83c7b March 18, 2026 23:03
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

@aasimkhan30 aasimkhan30 merged commit 539d3e2 into main Mar 18, 2026
7 checks passed
@aasimkhan30 aasimkhan30 deleted the aasim/fix/queryHistory branch March 18, 2026 23:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants