diff --git a/changelog.md b/changelog.md index a80ad44..b12b075 100644 --- a/changelog.md +++ b/changelog.md @@ -1,5 +1,14 @@ # Changelog +## 2025-11-24 - 5.0.2 - fix(DockerContainer) +Fix getContainerById to return undefined for non-existent containers + +- Prevented creation of an invalid DockerContainer from Docker API error responses when a container does not exist. +- Changed DockerContainer._fromId to use the list+find pattern and return Promise. +- Updated DockerHost.getContainerById to return Promise for type safety and consistent behavior. +- Added tests to verify undefined is returned for non-existent container IDs and that valid IDs return DockerContainer instances. +- Bumped package version to 5.0.1 and updated changelog and readme hints to document the fix. + ## 2025-11-24 - 5.0.0 - BREAKING CHANGE(DockerHost) Rename array-returning get* methods to list* on DockerHost and related resource classes; update docs, tests and changelog @@ -10,6 +19,23 @@ Rename array-returning get* methods to list* on DockerHost and related resource - Bumped package version to 4.0.0. - Migration note: replace calls to get*() with list*() for methods that return multiple items (arrays). Single-item getters such as getContainerById or getNetworkByName remain unchanged. +## 2025-11-24 - 5.0.1 - fix(DockerContainer) +Fix getContainerById() to return undefined instead of invalid container object when container doesn't exist + +**Bug Fixed:** +- `getContainerById()` was creating a DockerContainer object from error responses when a container didn't exist +- The error object `{ message: "No such container: ..." }` was being passed to the constructor +- Calling `.logs()` on this invalid container returned "[object Object]" instead of logs + +**Solution:** +- Changed `DockerContainer._fromId()` to use the list+filter pattern (consistent with all other resource getters) +- Now returns `undefined` when container is not found (matches DockerImage, DockerNetwork, DockerService, DockerSecret behavior) +- Updated return type to `Promise` for type safety +- Added tests to verify undefined is returned for non-existent containers + +**Migration:** +No breaking changes - users should already be checking for undefined/null based on TypeScript types and documentation. + ## 2025-11-24 - 4.0.0 - BREAKING CHANGE: Rename list methods for consistency **Breaking Changes:** diff --git a/package.json b/package.json index c6f9914..21768bd 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@apiclient.xyz/docker", - "version": "5.0.0", + "version": "5.0.1", "description": "Provides easy communication with Docker remote API from Node.js, with TypeScript support.", "private": false, "main": "dist_ts/index.js", diff --git a/readme.hints.md b/readme.hints.md index e481870..90adcfb 100644 --- a/readme.hints.md +++ b/readme.hints.md @@ -1,5 +1,52 @@ # Docker Module - Development Hints +## getContainerById() Bug Fix (2025-11-24 - v5.0.1) + +### Problem +The `getContainerById()` method had a critical bug where it would create a DockerContainer object from Docker API error responses when a container didn't exist. + +**Symptoms:** +- Calling `docker.getContainerById('invalid-id')` returned a DockerContainer object with `{ message: "No such container: invalid-id" }` +- Calling `.logs()` on this invalid container returned "[object Object]" instead of logs or throwing an error +- No way to detect the error state without checking for a `.message` property + +**Root Cause:** +The `DockerContainer._fromId()` method made a direct API call to `/containers/{id}/json` and blindly passed `response.body` to the constructor, even when the API returned a 404 error response. + +### Solution +Changed `DockerContainer._fromId()` to use the **list+filter pattern**, matching the behavior of all other resource getter methods (DockerImage, DockerNetwork, DockerService, DockerSecret): + +```typescript +// Before (buggy): +public static async _fromId(dockerHostArg: DockerHost, containerId: string): Promise { + const response = await dockerHostArg.request('GET', `/containers/${containerId}/json`); + return new DockerContainer(dockerHostArg, response.body); // Creates invalid object from error! +} + +// After (fixed): +public static async _fromId(dockerHostArg: DockerHost, containerId: string): Promise { + const containers = await this._list(dockerHostArg); + return containers.find((container) => container.Id === containerId); // Returns undefined if not found +} +``` + +**Benefits:** +- 100% consistent with all other resource classes +- Type-safe return signature: `Promise` +- Cannot create invalid objects - `.find()` naturally returns undefined +- Users can now properly check for non-existent containers + +**Usage:** +```typescript +const container = await docker.getContainerById('abc123'); +if (container) { + const logs = await container.logs(); + console.log(logs); +} else { + console.log('Container not found'); +} +``` + ## OOP Refactoring - Clean Architecture (2025-11-24) ### Architecture Changes diff --git a/test/test.nonci.node+deno.ts b/test/test.nonci.node+deno.ts index 4da6e57..c6aac0e 100644 --- a/test/test.nonci.node+deno.ts +++ b/test/test.nonci.node+deno.ts @@ -165,6 +165,22 @@ tap.test('should expose a working DockerImageStore', async () => { ); }); +// CONTAINER GETTERS +tap.test('should return undefined for non-existent container', async () => { + const container = await testDockerHost.getContainerById('invalid-container-id-12345'); + expect(container).toEqual(undefined); +}); + +tap.test('should return container for valid container ID', async () => { + const containers = await testDockerHost.listContainers(); + if (containers.length > 0) { + const validId = containers[0].Id; + const container = await testDockerHost.getContainerById(validId); + expect(container).toBeInstanceOf(docker.DockerContainer); + expect(container?.Id).toEqual(validId); + } +}); + // CONTAINER STREAMING FEATURES let testContainer: docker.DockerContainer; diff --git a/ts/00_commitinfo_data.ts b/ts/00_commitinfo_data.ts index 9df4d5c..026c413 100644 --- a/ts/00_commitinfo_data.ts +++ b/ts/00_commitinfo_data.ts @@ -3,6 +3,6 @@ */ export const commitinfo = { name: '@apiclient.xyz/docker', - version: '5.0.0', + version: '5.0.2', description: 'Provides easy communication with Docker remote API from Node.js, with TypeScript support.' } diff --git a/ts/classes.container.ts b/ts/classes.container.ts index 619cf43..79217db 100644 --- a/ts/classes.container.ts +++ b/ts/classes.container.ts @@ -28,13 +28,14 @@ export class DockerContainer extends DockerResource { /** * Internal: Get a container by ID * Public API: Use dockerHost.getContainerById(id) instead + * Returns undefined if container does not exist */ public static async _fromId( dockerHostArg: DockerHost, containerId: string, - ): Promise { - const response = await dockerHostArg.request('GET', `/containers/${containerId}/json`); - return new DockerContainer(dockerHostArg, response.body); + ): Promise { + const containers = await this._list(dockerHostArg); + return containers.find((container) => container.Id === containerId); } /** diff --git a/ts/classes.host.ts b/ts/classes.host.ts index c27e84c..f97f354 100644 --- a/ts/classes.host.ts +++ b/ts/classes.host.ts @@ -164,8 +164,9 @@ export class DockerHost { /** * Gets a container by ID + * Returns undefined if container does not exist */ - public async getContainerById(containerId: string) { + public async getContainerById(containerId: string): Promise { return await DockerContainer._fromId(this, containerId); }