Move shared HTTP client into extension-toolkit#22539
Open
aasimkhan30 wants to merge 3 commits into
Open
Conversation
Both the mssql and sql-database-projects extensions carried near-identical copies of the HTTP client, proxy resolution and file download logic. This consolidates them into a single implementation owned by extension-toolkit. - Add `extension-toolkit/base` HttpClient: portable HTTP + download with proxy/tunnel support, a generic `onProgress` callback and typed `IHttpResponse` / `IDownloadFileResult` results. - Add `extension-toolkit/vscode` VscodeHttpClient adapter wiring VS Code's `http.proxy` / `http.proxyStrictSSL` settings and warning messages. - Add `extension-toolkit/base/common/errors.ts` with `getErrorMessage`, so the client no longer needs a formatter injected by callers. - Keep axios out of the public API; the toolkit owns its own response types. - Localized strings stay in the extensions and are injected as `messages`. - Move axios/tunnel dependencies to the toolkit and drop the duplicated http/ folders and tests from both extensions.
aasimkhan30
requested review from
Benjin,
allancascante,
caohai,
kburtram,
laurenastrid1,
lewis-sanchez and
ssreerama
as code owners
July 24, 2026 23:47
Contributor
There was a problem hiding this comment.
Pull request overview
This PR consolidates duplicated HTTP client / proxy validation / file download logic from the mssql and sql-database-projects extensions into a single shared implementation in packages/extension-toolkit, with a VS Code adapter (VscodeHttpClient) and a VS Code-independent core (HttpClient).
Changes:
- Introduces
extension-toolkit/baseHttpClient(proxy + download + progress callbacks) andextension-toolkit/vscodeVscodeHttpClientadapter. - Updates both extensions (and unit tests) to consume the shared client and shared response/progress shapes.
- Moves
axios/tunneldependencies intoextension-toolkitand removes per-extension copies and tests.
Reviewed changes
Copilot reviewed 28 out of 31 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/extension-toolkit/src/vscode/index.ts | Re-exports VS Code HTTP adapter entrypoint. |
| packages/extension-toolkit/src/vscode/http/vscodeHttpClient.ts | Adds VscodeHttpClient wrapper wiring VS Code proxy settings + warnings. |
| packages/extension-toolkit/src/vscode/http/index.ts | Barrel export for VS Code HTTP adapter. |
| packages/extension-toolkit/src/base/index.ts | Exposes new common + http modules from toolkit base. |
| packages/extension-toolkit/src/base/http/index.ts | Barrel export for base HTTP client. |
| packages/extension-toolkit/src/base/http/httpClient.ts | New shared HTTP client + download/progress APIs. |
| packages/extension-toolkit/src/base/common/index.ts | Barrel export for shared base helpers. |
| packages/extension-toolkit/src/base/common/errors.ts | Adds shared getErrorMessage() helper. |
| packages/extension-toolkit/package.json | Adds axios + tunnel dependencies (and @types/tunnel). |
| packages/extension-toolkit/package-lock.json | Lockfile updates for new toolkit dependencies. |
| extensions/sql-database-projects/test/httpClient.test.ts | Deletes duplicate HTTP client tests (migrated to mssql suite). |
| extensions/sql-database-projects/test/buildHelper.test.ts | Updates tests to stub VscodeHttpClient.downloadFile. |
| extensions/sql-database-projects/src/tools/buildHelper.ts | Switches downloads to shared VscodeHttpClient and new progress callback. |
| extensions/sql-database-projects/src/http/httpClientCore.ts | Deletes duplicated core HTTP client. |
| extensions/sql-database-projects/src/http/httpClient.ts | Deletes duplicated VS Code HTTP client wrapper. |
| extensions/sql-database-projects/src/controllers/mainController.ts | Switches proxy warning call to VscodeHttpClient. |
| extensions/sql-database-projects/src/common/logger.ts | Updates logger documentation to reflect new HTTP client usage. |
| extensions/sql-database-projects/package.json | Removes direct axios/tunnel deps; relies on toolkit. |
| extensions/sql-database-projects/package-lock.json | Lockfile updates reflecting dependency move. |
| extensions/mssql/test/unit/mainController.test.ts | Updates proxy-warning test to spy on VscodeHttpClient. |
| extensions/mssql/test/unit/languageService.test.ts | Updates download progress test to use toolkit progress type. |
| extensions/mssql/test/unit/httpClient.test.ts | Migrates HTTP client unit tests to toolkit HttpClient API. |
| extensions/mssql/test/unit/fabricHelper.test.ts | Updates Fabric tests to use toolkit response type and client stub. |
| extensions/mssql/test/unit/extension.test.ts | Updates proxy-warning stub to VscodeHttpClient. |
| extensions/mssql/src/languageservice/downloadHelper.ts | Uses toolkit HttpClient.downloadFile with unified progress callback. |
| extensions/mssql/src/fabric/fabricHelper.ts | Switches Fabric REST calls to VscodeHttpClient + toolkit response types. |
| extensions/mssql/src/controllers/mainController.ts | Switches activation-time proxy warning to VscodeHttpClient. |
| extensions/mssql/src/azure/utils.ts | Switches Graph calls to VscodeHttpClient. |
| extensions/mssql/src/azure/msal/msalAzureAuth.ts | Switches tenant calls to VscodeHttpClient. |
| extensions/mssql/package.json | Removes direct axios/tunnel deps; relies on toolkit. |
| extensions/mssql/package-lock.json | Lockfile updates reflecting dependency move. |
Files not reviewed (3)
- extensions/mssql/package-lock.json: Generated file
- extensions/sql-database-projects/package-lock.json: Generated file
- packages/extension-toolkit/package-lock.json: Generated file
Comments suppressed due to low confidence (7)
packages/extension-toolkit/src/base/http/httpClient.ts:159
makeGetRequestreturns the Axios response object via a type cast toIHttpResponse<T>, butIHttpResponse.headersis declared asRecord<string, string>. Axios headers are not guaranteed to already be normalized strings, so this is an unsound contract and can break consumers relying on the type.
This issue also appears on line 187 of the same file.
packages/extension-toolkit/src/base/http/httpClient.ts:187
makePostRequestreturns the Axios response object via a type cast toIHttpResponse<T>, butIHttpResponse.headersis declared asRecord<string, string>. Axios headers are not guaranteed to already be normalized strings, so this is an unsound contract and can break consumers relying on the type.
packages/extension-toolkit/src/base/http/httpClient.ts:283downloadToFileDescriptorusesgetContentLength()which currently returnsundefinedwhencontent-lengthis0. That makes an empty download indistinguishable from an unknown size, contradicting the API intent oftotalBytesrepresenting a known header value when available.
packages/extension-toolkit/src/base/http/httpClient.ts:322- Progress percentage calculation divides by
totalByteswithout guarding against0. Ifcontent-lengthis present and0(valid empty response), this results in a division-by-zero bug (Infinity/NaN).
packages/extension-toolkit/src/base/http/httpClient.ts:292 IDownloadFileResult.headersis typed asRecord<string, string>, but this returns Axios headers via a type cast. This can leak non-string header values (or arrays) through a string-only contract.
This issue also appears on line 340 of the same file.
packages/extension-toolkit/src/base/http/httpClient.ts:343
IDownloadFileResult.headersis typed asRecord<string, string>, but this returns Axios headers via a type cast. This can leak non-string header values (or arrays) through a string-only contract.
packages/extension-toolkit/src/vscode/http/vscodeHttpClient.ts:40parseUriSchemeusesvscode.Uri.parse(value).scheme, which returns a non-empty scheme for values likelocalhost:3128(e.g.localhost). That can preventwarnOnInvalidProxySettings()from warning about a missing protocol even though later proxy agent creation will throw onnew URL(proxy).
Comment on lines
48
to
50
| * Logger that writes formatted, timestamped messages to a VS Code OutputChannel. | ||
| * Implements ILogger so it can be passed directly to HttpClient / HttpClientCore. | ||
| * Implements the logger contract used by the shared HttpClient. | ||
| * |
PR Changes
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #22539 +/- ##
==========================================
- Coverage 75.78% 75.74% -0.04%
==========================================
Files 407 402 -5
Lines 130542 129266 -1276
Branches 8382 8351 -31
==========================================
- Hits 98933 97917 -1016
+ Misses 31609 31349 -260
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 50 out of 53 changed files in this pull request and generated 3 comments.
Files not reviewed (3)
- extensions/mssql/package-lock.json: Generated file
- extensions/sql-database-projects/package-lock.json: Generated file
- packages/extension-toolkit/package-lock.json: Generated file
Comment on lines
+9
to
+17
| export function getErrorMessage(error: unknown): string { | ||
| return error instanceof Error | ||
| ? typeof error.message === "string" | ||
| ? error.message | ||
| : "" | ||
| : typeof error === "string" | ||
| ? error | ||
| : `${JSON.stringify(error, undefined, "\t")}`; | ||
| } |
Comment on lines
+458
to
+463
| const parsedRetryAfter = retryAfter === undefined ? Number.NaN : parseInt(retryAfter, 10); | ||
| const retryAfterInMs = | ||
| Number.isFinite(parsedRetryAfter) && parsedRetryAfter > 0 | ||
| ? parsedRetryAfter | ||
| : this.defaultRetryInMs; | ||
|
|
Comment on lines
+71
to
+82
| /** Progress reported while a download is being written. */ | ||
| export interface IDownloadProgress { | ||
| /** Number of bytes received so far. */ | ||
| readonly downloadedBytes: number; | ||
|
|
||
| /** | ||
| * Total number of bytes to expect. | ||
| * | ||
| * `undefined` means the length is unknown; `0` means the response is known to be empty. | ||
| */ | ||
| readonly totalBytes: number | undefined; | ||
| } |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 50 out of 53 changed files in this pull request and generated 4 comments.
Files not reviewed (3)
- extensions/mssql/package-lock.json: Generated file
- extensions/sql-database-projects/package-lock.json: Generated file
- packages/extension-toolkit/package-lock.json: Generated file
Comments suppressed due to low confidence (1)
packages/extension-toolkit/src/base/common/errors.ts:16
- getErrorMessage falls back to JSON.stringify for non-Error, non-string values, but JSON.stringify itself can throw (e.g., circular structures or BigInt). That can cause logging/cleanup paths to throw while handling another error.
Comment on lines
+755
to
+763
| function createFileDescriptorWritable(destinationFd: number): Writable { | ||
| return new Writable({ | ||
| write(chunk: Buffer, _encoding, callback) { | ||
| fs.write(destinationFd, chunk, 0, chunk.length, null, (error) => { | ||
| callback(error ?? null); | ||
| }); | ||
| }, | ||
| }); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Both the
mssqlandsql-database-projectsextensions carried near-identical copies of the HTTP client, proxy resolution, and file download logic. This PR consolidates them into a single implementation owned byextension-toolkit.Net effect: 978 insertions, 1731 deletions — one HTTP client instead of two.
What moved
extension-toolkit/base→HttpClientextensions/mssql/src/http/httpClientCore.ts,extensions/sql-database-projects/src/http/httpClientCore.tsextension-toolkit/vscode→VscodeHttpClientextensions/mssql/src/http/httpClient.ts,extensions/sql-database-projects/src/http/httpClient.tsextension-toolkit/base→getErrorMessageDesign notes
baseis VS Code independent.HttpClienttakes an optionalIHttpClientDependenciesfor proxy config, URI parsing, and warning display.VscodeHttpClientis the thin adapter that wires those tohttp.proxy/http.proxyStrictSSLandvscode.window.showWarningMessage.IHttpResponse<T>/IHttpHeadersso consumers aren't coupled to the implementation library.messages; the toolkit never callsl10n.downloadFiletakes anonProgresscallback delivering{ downloadedBytes, totalBytes?, percentage? }, so an extension can render it however it likes (status bar, output channel, notification). The oldonHeaders/onDatapair and the per-extensioncontent-lengthparsing are gone —totalBytesis nowundefinedrather than0when the size is unknown, so "unknown size" is distinguishable from "empty".autoClose: false) and completion resolves onfinish.axios/tunnel/@types/tunnelmoved to the toolkit'spackage.jsonand dropped from both extensions.Testing
extensions/mssql/test/unit/httpClient.test.ts— 18/18 passing, now the single suite covering the shared client (the duplicate DB Projects copy was deleted).extensions/mssql/test/unit/fabricHelper.test.ts— 8/8 passing; unsafeas unknown as IHttpResponse<...>casts replaced with a typed factory.extension-toolkit,mssql, andsql-database-projects.