From f0e9d8b671c5dcb81e7fb0904a3eecf7e805443c Mon Sep 17 00:00:00 2001 From: Nicolas De Loof Date: Thu, 18 Sep 2025 10:34:25 +0200 Subject: [PATCH] refactored sendHTTPRequest by introducing an explicit httpRequest type Signed-off-by: Nicolas De Loof --- .github/workflows/ci.yml | 6 +- .prettierignore | 1 + package.json | 3 +- src/docker-client.ts | 58 ++++--- src/e2e.test.ts | 14 +- src/http.ts | 192 +++++++++++---------- src/models/ContainerStatsResponse.ts | 2 +- src/models/SystemVersionComponentsInner.ts | 2 +- src/multiplexed-stream.test.ts | 6 +- 9 files changed, 158 insertions(+), 126 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index cecaf45..4cf562f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -24,9 +24,9 @@ jobs: - name: Install run: | npm install + - name: Check + run: | + npm run check - name: Test run: | npm test - - name: Format - run: | - prettier --check diff --git a/.prettierignore b/.prettierignore index da0f246..3e50fc5 100644 --- a/.prettierignore +++ b/.prettierignore @@ -1,4 +1,5 @@ node_modules package.json +package-lock.json makefile swagger.yaml diff --git a/package.json b/package.json index 2ca0ee3..a54a4d6 100644 --- a/package.json +++ b/package.json @@ -45,7 +45,8 @@ "build": "npm run generate && tsup", "test": "vitest run", "lint": "oxlint --type-aware", - "format": "prettier --write ." + "format": "prettier --write .", + "check": "prettier --check ." }, "devDependencies": { "@openapitools/openapi-generator-cli": "^2.23.4", diff --git a/src/docker-client.ts b/src/docker-client.ts index 146e361..29acf85 100644 --- a/src/docker-client.ts +++ b/src/docker-client.ts @@ -5,8 +5,6 @@ import * as os from 'os'; import * as models from './models/index.js'; import { HTTPClient } from './http.js'; import { Filter } from './filter.js'; -import { Writable } from 'stream'; -import { FileInfo } from './models/FileInfo.js'; export class Credentials { username: string; @@ -242,9 +240,11 @@ export class DockerClient { ) { await this.api.sendHTTPRequest('GET', '/events', { params: options, - timeout: -1, - callback: (chunk: string) => - callback(JSON.parse(chunk) as models.EventMessage), + callback: (data: string) => { + data.split('\n').forEach((line) => { + callback(JSON.parse(line) as models.EventMessage); + }); + }, }); } @@ -345,16 +345,10 @@ export class DockerClient { stderr?: boolean; }, ): Promise { - return this.api.post( - `/containers/${id}/attach`, - options, - undefined, - undefined, - { - Connection: 'Upgrade', - Upgrade: 'tcp', - }, - ); + return this.api.post(`/containers/${id}/attach`, options, undefined, { + Connection: 'Upgrade', + Upgrade: 'tcp', + }); } /** @@ -684,7 +678,6 @@ export class DockerClient { `/containers/${id}/wait`, undefined, options, - -1, ); } @@ -885,17 +878,20 @@ export class DockerClient { * @param platform Platform in the format os[/arch[/variant]]. When used in combination with the 'fromImage' option, the daemon checks if the given image is present in the local image cache with the given OS and Architecture, and otherwise attempts to pull the image. If the option is not set, the host\'s native OS and Architecture are used. If the given image does not exist in the local image cache, the daemon attempts to pull the image with the host\'s native OS and Architecture. If the given image does exists in the local image cache, but its OS or architecture does not match, a warning is produced. When used with the 'fromSrc' option to import an image from an archive, this option sets the platform information for the imported image. If the option is not set, the host\'s native OS and Architecture are used for the imported image. * @param inputImage Image content if the value '-' has been specified in fromSrc query parameter */ - public async imageCreate(options?: { - fromImage?: string; - fromSrc?: string; - repo?: string; - tag?: string; - message?: string; - credentials?: Credentials | IdentityToken; - changes?: Array; - platform?: string; - inputImage?: string; - }): Promise { + public async imageCreate( + callback: (event: any) => void, + options?: { + fromImage?: string; + fromSrc?: string; + repo?: string; + tag?: string; + message?: string; + credentials?: Credentials | IdentityToken; + changes?: Array; + platform?: string; + inputImage?: string; + }, + ): Promise { const headers: Record = {}; if (options?.credentials) { @@ -917,8 +913,14 @@ export class DockerClient { inputImage: options?.inputImage, }, undefined, - undefined, headers, + (data: string) => { + data.split('\n').forEach((line) => { + if (line) { + callback(JSON.parse(line)); + } + }); + }, ); } diff --git a/src/e2e.test.ts b/src/e2e.test.ts index a95c5f1..3987f4d 100644 --- a/src/e2e.test.ts +++ b/src/e2e.test.ts @@ -59,11 +59,21 @@ test('container lifecycle should work end-to-end', async () => { let containerId: string | undefined; try { + await client.imageCreate( + (event) => { + console.log(event); + }, + { + fromImage: 'docker.io/library/nginx', + tag: 'latest', + }, + ); + console.log(' Creating nginx container...'); // Create container with label const createResponse = await client.containerCreate( { - Image: 'nginx:latest', + Image: 'docker.io/library/nginx', Labels: { 'test.type': 'e2e', }, @@ -173,7 +183,7 @@ test('container lifecycle should work end-to-end', async () => { } } } -}); +}, 30000); test('network lifecycle should work end-to-end', async () => { const client = await DockerClient.fromDockerConfig(); diff --git a/src/http.ts b/src/http.ts index 797d112..d8fdea4 100644 --- a/src/http.ts +++ b/src/http.ts @@ -36,18 +36,39 @@ function getErrorMessage( ): string { const contentType = headers['content-type']?.toLowerCase(); if (contentType?.includes('application/json') && body) { - try { - const jsonBody = JSON.parse(body); - if (jsonBody.message) { - return jsonBody.message; - } - } catch (parseError) { - // If JSON parsing fails, return the default message + const jsonBody = JSON.parse(body); + if (jsonBody.message) { + return jsonBody.message; } } return status; } +// Class to represent an HTTP request +export class HTTPRequest { + public method: string; + public uri: string; + public headers: { [key: string]: string }; + + constructor( + method: string, + uri: string, + headers: { [key: string]: string } = {}, + ) { + this.method = method; + this.uri = uri; + this.headers = headers; + } + + public addHeader(key: string, value: string): void { + this.headers[key] = value; + } + + public setHeaders(headers: { [key: string]: string }): void { + this.headers = { ...this.headers, ...headers }; + } +} + // Interface to represent an HTTP response export interface HTTPResponse { statusLine: string; @@ -152,20 +173,18 @@ export class HTTPClient { } // Callback called when data is received - private onDataReceived(data: string): void { + private onDataReceived(_: string): void { // This method can be overridden or modified as needed // By default, it does nothing more than logging } // Method to read a complete HTTP response public readHTTPResponse( - timeout: number = 10000, callback?: (chunk: string) => void, ): Promise { return new Promise((resolve, reject) => { let buffer = ''; let body = ''; - let timeoutId: NodeJS.Timeout; let resolved = false; let headersComplete = false; let expectedBodyLength = -1; @@ -253,7 +272,6 @@ export class HTTPClient { // Resolve immediately with upgrade response resolved = true; - clearTimeout(timeoutId); const response: HTTPResponse = { statusLine, @@ -306,7 +324,6 @@ export class HTTPClient { if (isComplete) { resolved = true; - clearTimeout(timeoutId); this.socket.off('data', dataHandler); let responseBody: string | undefined; @@ -352,16 +369,6 @@ export class HTTPClient { } }; - if (timeout > 0) { - timeoutId = setTimeout(() => { - if (!resolved) { - resolved = true; - this.socket.off('data', dataHandler); - reject(new Error('Timeout: incomplete HTTP response')); - } - }, timeout); - } - this.socket.on('data', dataHandler); }); } @@ -372,8 +379,7 @@ export class HTTPClient { uri: string, options?: { params?: Record; - body?: object; - timeout?: number; + data?: object; callback?: (data: string) => void; accept?: string; headers?: Record; @@ -381,48 +387,41 @@ export class HTTPClient { ): Promise { const { params, - body, - timeout = 10000, + data, callback, accept = 'application/json', - headers, + headers = {}, } = options || {}; + // Prepare HTTPRequest const queryString = this.buildQueryString(params); - const fullUri = `${uri}${queryString}`; - - let request = `${method} ${fullUri} HTTP/1.1 -Host: host -User-Agent: docker-ts/0.0.1 -Accept: ${accept} -`; - - // Add custom headers if provided - if (headers) { - Object.entries(headers).forEach(([key, value]) => { - request += `${key}: ${value}\r\n`; - }); - } + const httpRequest = new HTTPRequest(method, `${uri}${queryString}`, { + Host: 'host', + 'User-Agent': 'docker-ts/0.0.1', + }); + + // Add any custom headers + httpRequest.setHeaders(headers); + httpRequest.addHeader('Accept', accept); - let stream: NodeJS.ReadableStream = undefined; - if (body) { + // Prepare body data and headers + let body: string | NodeJS.ReadableStream | undefined; + if (data) { + // Check if body is a stream if ( - typeof body === 'object' && - 'read' in body && - typeof (body as any).read === 'function' + typeof data === 'object' && + 'read' in data && + typeof (data as any).read === 'function' ) { - stream = body as NodeJS.ReadableStream; // Use chunked transfer encoding for streams - request += `Transfer-Encoding: chunked\r\n\r\n`; + body = data as NodeJS.ReadableStream; + httpRequest.addHeader('Transfer-Encoding', 'chunked'); } else { - const json = JSON.stringify(body); - request += `Content-type: application/json -Content-length: ${json.length} - -${json}`; + // Convert to JSON string for objects + body = JSON.stringify(data); + httpRequest.addHeader('Content-Type', 'application/json'); + httpRequest.addHeader('Content-Length', body.length.toString()); } - } else { - request += '\r\n'; } return new Promise(async (resolve, reject) => { @@ -431,50 +430,69 @@ ${json}`; return; } + // Write HTTPRequest to socket try { - // Send the request headers - this.socket.write(request, 'utf8'); - - if (stream) { - // Handle streaming with chunked transfer encoding - await this.writeStreamChunked(stream); + let requestData = `${httpRequest.method} ${httpRequest.uri} HTTP/1.1\r\n`; + Object.entries(httpRequest.headers).forEach(([key, value]) => { + requestData += `${key}: ${value}\r\n`; + }); + requestData += '\r\n'; + this.socket.write(requestData, 'utf8'); + + if (body) { + if (typeof body === 'string') { + this.socket.write(body); + } else { + await this.writeStreamChunked(body).catch((error) => { + reject(error); + }); + } } - // Read the response - const response = await this.readHTTPResponse(timeout, callback); - resolve(response); + this.readHTTPResponse(callback) + .then((response) => { + resolve(response); + }) + .catch((error) => { + reject(error + ' ' + httpRequest.uri); + }); } catch (error) { reject(error); } }); } - private writeStreamChunked(stream: NodeJS.ReadableStream): void { - stream.on('data', (chunk: Buffer) => { - const chunkSize = chunk.length; - if (chunkSize > 0) { - // Write chunk size in hexadecimal followed by CRLF - this.socket.write(`${chunkSize.toString(16)}\r\n`); - // Write the chunk data followed by CRLF - this.socket.write(chunk); - this.socket.write('\r\n'); - } - }); + private async writeStreamChunked( + stream: NodeJS.ReadableStream, + ): Promise { + return new Promise((resolve, reject) => { + stream.on('data', (chunk: Buffer) => { + const chunkSize = chunk.length; + if (chunkSize > 0) { + // Write chunk size in hexadecimal followed by CRLF + this.socket.write(`${chunkSize.toString(16)}\r\n`); + // Write the chunk data followed by CRLF + this.socket.write(chunk); + this.socket.write('\r\n'); + } + }); - stream.on('end', () => { - // Write the final zero-length chunk to indicate end of stream - this.socket.write('0\r\n\r\n'); - }); + stream.on('end', () => { + // Write the final zero-length chunk to indicate end of stream + this.socket.write('0\r\n\r\n'); + resolve(); + }); - stream.on('error', (error) => { - throw error; + stream.on('error', (error) => { + reject(error); + }); }); } private handleResponse(response: HTTPResponse): T { const contentType = response.headers['content-type']?.toLowerCase(); - if (contentType?.includes('application/json')) { - const parsedBody = JSON.parse(response.body ?? ''); + if (contentType?.includes('application/json') && response.body) { + const parsedBody = JSON.parse(response.body); return parsedBody as T; } else { return response.body as T; @@ -522,14 +540,14 @@ ${json}`; uri: string, params?: Record, data?: object, - timeout?: number, headers?: Record, + callback?: (data: any) => void, ): Promise { return this.sendHTTPRequest('POST', uri, { params: params, - body: data, - timeout: timeout, + data: data, headers: headers, + callback: callback, }).then((response) => this.handleResponse(response)); } @@ -541,7 +559,7 @@ ${json}`; ): Promise { return this.sendHTTPRequest('PUT', uri, { params: params, - body: data, + data: data, headers: { 'Content-Type': type, }, diff --git a/src/models/ContainerStatsResponse.ts b/src/models/ContainerStatsResponse.ts index 3ce9198..fa4f0fb 100644 --- a/src/models/ContainerStatsResponse.ts +++ b/src/models/ContainerStatsResponse.ts @@ -49,5 +49,5 @@ export class ContainerStatsResponse { /** * Network statistics for the container per interface. This field is omitted if the container has no networking enabled. */ - 'Networks'?: any | null; + 'Networks'?: any; } diff --git a/src/models/SystemVersionComponentsInner.ts b/src/models/SystemVersionComponentsInner.ts index fb7769c..1444b03 100644 --- a/src/models/SystemVersionComponentsInner.ts +++ b/src/models/SystemVersionComponentsInner.ts @@ -22,5 +22,5 @@ export class SystemVersionComponentsInner { /** * Key/value pairs of strings with additional information about the component. These values are intended for informational purposes only, and their content is not defined, and not part of the API specification. These messages can be printed by the client as information to the user. */ - 'Details'?: any | null; + 'Details'?: any; } diff --git a/src/multiplexed-stream.test.ts b/src/multiplexed-stream.test.ts index 7ea047c..ce72073 100644 --- a/src/multiplexed-stream.test.ts +++ b/src/multiplexed-stream.test.ts @@ -85,7 +85,7 @@ test('should handle multiple messages in single chunk', () => { test('should handle incomplete messages across multiple chunks', () => { const { stream: stdout, data: stdoutData } = createMockStream(); - const { stream: stderr, data: stderrData } = createMockStream(); + const { stream: stderr, data: _ } = createMockStream(); const callback = createMultiplexedStreamCallback(stdout, stderr); const message = createMultiplexedMessage(1, 'Split message'); @@ -104,7 +104,7 @@ test('should handle incomplete messages across multiple chunks', () => { test('should handle empty content', () => { const { stream: stdout, data: stdoutData } = createMockStream(); - const { stream: stderr, data: stderrData } = createMockStream(); + const { stream: stderr, data: _ } = createMockStream(); const callback = createMultiplexedStreamCallback(stdout, stderr); const message = createMultiplexedMessage(1, ''); @@ -129,7 +129,7 @@ test('should handle very short incomplete chunks', () => { test('should handle large content', () => { const { stream: stdout, data: stdoutData } = createMockStream(); - const { stream: stderr, data: stderrData } = createMockStream(); + const { stream: stderr, data: _ } = createMockStream(); const callback = createMultiplexedStreamCallback(stdout, stderr); const largeContent = 'x'.repeat(10000);