fix(DockerContainer): Fix getContainerById to return undefined for non-existent containers
This commit is contained in:
26
changelog.md
26
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<DockerContainer | undefined>.
|
||||
- Updated DockerHost.getContainerById to return Promise<DockerContainer | undefined> 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<DockerContainer | undefined>` 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:**
|
||||
|
||||
@@ -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",
|
||||
|
||||
@@ -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<DockerContainer> {
|
||||
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<DockerContainer | undefined> {
|
||||
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<DockerContainer | undefined>`
|
||||
- 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
|
||||
|
||||
@@ -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;
|
||||
|
||||
|
||||
@@ -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.'
|
||||
}
|
||||
|
||||
@@ -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<DockerContainer> {
|
||||
const response = await dockerHostArg.request('GET', `/containers/${containerId}/json`);
|
||||
return new DockerContainer(dockerHostArg, response.body);
|
||||
): Promise<DockerContainer | undefined> {
|
||||
const containers = await this._list(dockerHostArg);
|
||||
return containers.find((container) => container.Id === containerId);
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -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<DockerContainer | undefined> {
|
||||
return await DockerContainer._fromId(this, containerId);
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user