fix(docker): remove aggressive caching to ensure data correctness #1864
Conversation
WalkthroughSystematic removal of caching infrastructure from Docker services, including elimination of CACHE_MANAGER dependency, clearContainerCache method, skipCache parameters across service implementations, test files, and resolvers. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| this.logger.debug(`Invalidated container caches after stopping ${id}`); | ||
|
|
||
| let containers = await this.getContainers({ skipCache: true }); | ||
| let containers = await this.getContainers(); |
Check warning
Code scanning / CodeQL
Useless assignment to local variable Warning
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 24 days ago
In general, to fix a “useless assignment to local variable” you either (a) remove the unnecessary assignment if the value is truly unused, or (b) start using the initially assigned value in the logic if it was meant to be used but wasn’t. You must ensure that any required side effects from the right-hand side expression are still executed somewhere if they matter.
Here, the initial assignment let containers = await this.getContainers(); is never read before containers is reassigned in the loop. The method’s logic only relies on the values retrieved inside the loop. The simplest and safest fix is to declare containers without an initial value, and continue to assign to it inside the loop. That way, we preserve the same number and timing of this.getContainers() calls (still called once per loop iteration after a sleep) while eliminating the useless initial call and assignment.
Concretely, in api/src/unraid-api/graph/resolvers/docker/docker.service.ts, in the stop method around line 239, change let containers = await this.getContainers(); to let containers: DockerContainer[] = []; (or just let containers: DockerContainer[]; if you prefer leaving it undefined until first use). This keeps type information clear and avoids any unused initial value. No new imports or additional methods are required.
| @@ -236,7 +236,7 @@ | ||
| const container = this.client.getContainer(id); | ||
| await container.stop({ t: 10 }); | ||
|
|
||
| let containers = await this.getContainers(); | ||
| let containers: DockerContainer[]; | ||
| let updatedContainer: DockerContainer | undefined; | ||
| for (let i = 0; i < 5; i++) { | ||
| await sleep(500); |
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1864 +/- ##
==========================================
- Coverage 46.49% 46.44% -0.06%
==========================================
Files 954 954
Lines 59788 59730 -58
Branches 5552 5541 -11
==========================================
- Hits 27799 27741 -58
Misses 31870 31870
Partials 119 119 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts:
- Around line 78-82: The container resolver currently fetches all containers via
dockerService.getContainers() and doesn't pass the Docker API size option when
the GraphQL fields sizeRootFs/sizeRw are requested; update the container method
to inspect the GraphQLResolveInfo (add an info parameter) to detect if
sizeRootFs or sizeRw are requested and then call a single-container fetch that
accepts the size flag (e.g., dockerService.getContainer(id, { size: true }) or
dockerService.getContainers({ id, size: true }) depending on available service
methods) so the Docker API returns populated sizeRootFs/sizeRw fields; keep
returning the found container or null.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
api/src/unraid-api/graph/resolvers/docker/docker-event.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker-event.service.tsapi/src/unraid-api/graph/resolvers/docker/docker-network.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker-network.service.tsapi/src/unraid-api/graph/resolvers/docker/docker-template-scanner.service.tsapi/src/unraid-api/graph/resolvers/docker/docker.module.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.tsapi/src/unraid-api/graph/resolvers/docker/docker.service.integration.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.service.tsapi/src/unraid-api/graph/resolvers/docker/organizer/docker-organizer.service.ts
💤 Files with no reviewable changes (1)
- api/src/unraid-api/graph/resolvers/docker/docker-event.service.spec.ts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Always use TypeScript imports with.jsextensions for ESM compatibility
Never add comments unless they are needed for clarity of function
Never add comments for obvious things, and avoid commenting when starting and ending code blocks
Files:
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker-network.service.tsapi/src/unraid-api/graph/resolvers/docker/organizer/docker-organizer.service.tsapi/src/unraid-api/graph/resolvers/docker/docker.service.integration.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker-network.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker-template-scanner.service.tsapi/src/unraid-api/graph/resolvers/docker/docker-event.service.tsapi/src/unraid-api/graph/resolvers/docker/docker.module.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.service.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts
api/**/*
📄 CodeRabbit inference engine (CLAUDE.md)
Prefer adding new files to the NestJS repo located at
api/src/unraid-api/instead of the legacy code
Files:
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker-network.service.tsapi/src/unraid-api/graph/resolvers/docker/organizer/docker-organizer.service.tsapi/src/unraid-api/graph/resolvers/docker/docker.service.integration.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker-network.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker-template-scanner.service.tsapi/src/unraid-api/graph/resolvers/docker/docker-event.service.tsapi/src/unraid-api/graph/resolvers/docker/docker.module.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.service.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Never use theanytype. Always prefer proper typing
Avoid using casting whenever possible, prefer proper typing from the start
Files:
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker-network.service.tsapi/src/unraid-api/graph/resolvers/docker/organizer/docker-organizer.service.tsapi/src/unraid-api/graph/resolvers/docker/docker.service.integration.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker-network.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker-template-scanner.service.tsapi/src/unraid-api/graph/resolvers/docker/docker-event.service.tsapi/src/unraid-api/graph/resolvers/docker/docker.module.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.service.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts
api/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
cache-manager v7 expects TTL values in milliseconds, not seconds (e.g., 600000 for 10 minutes, not 600)
Files:
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker-network.service.tsapi/src/unraid-api/graph/resolvers/docker/organizer/docker-organizer.service.tsapi/src/unraid-api/graph/resolvers/docker/docker.service.integration.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker-network.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker-template-scanner.service.tsapi/src/unraid-api/graph/resolvers/docker/docker-event.service.tsapi/src/unraid-api/graph/resolvers/docker/docker.module.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.service.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts
**/*
📄 CodeRabbit inference engine (.cursor/rules/default.mdc)
Never add comments unless they are needed for clarity of function
Files:
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker-network.service.tsapi/src/unraid-api/graph/resolvers/docker/organizer/docker-organizer.service.tsapi/src/unraid-api/graph/resolvers/docker/docker.service.integration.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker-network.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker-template-scanner.service.tsapi/src/unraid-api/graph/resolvers/docker/docker-event.service.tsapi/src/unraid-api/graph/resolvers/docker/docker.module.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.service.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts
🧠 Learnings (16)
📚 Learning: 2025-11-24T17:52:26.907Z
Learnt from: CR
Repo: unraid/api PR: 0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-11-24T17:52:26.907Z
Learning: Applies to **/*.test.ts : Mock external services and API calls
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.service.integration.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker-network.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.module.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts
📚 Learning: 2025-11-24T17:51:46.348Z
Learnt from: CR
Repo: unraid/api PR: 0
File: .cursor/rules/api-rules.mdc:0-0
Timestamp: 2025-11-24T17:51:46.348Z
Learning: Applies to api/**/*.test.{ts,tsx} : Prefer to not mock simple dependencies
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.service.integration.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker-network.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.module.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts
📚 Learning: 2025-11-24T17:52:26.908Z
Learnt from: CR
Repo: unraid/api PR: 0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-11-24T17:52:26.908Z
Learning: Applies to **/__test__/store/**/*.ts : Mock external dependencies appropriately in Pinia store tests
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.service.integration.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker-network.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.module.spec.ts
📚 Learning: 2025-11-24T17:52:26.907Z
Learnt from: CR
Repo: unraid/api PR: 0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-11-24T17:52:26.907Z
Learning: Applies to **/*.test.ts : Use `vi.mock()` for module-level mocks
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.service.integration.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker-network.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.module.spec.ts
📚 Learning: 2025-08-09T01:03:29.676Z
Learnt from: elibosley
Repo: unraid/api PR: 1575
File: packages/unraid-shared/src/services/socket-config.service.spec.ts:10-13
Timestamp: 2025-08-09T01:03:29.676Z
Learning: Vitest is used for all testing across all repositories in the unraid organization, not Jest. Always use `vi` for mocking utilities, not `jest`.
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker-network.service.spec.ts
📚 Learning: 2025-11-24T17:52:26.907Z
Learnt from: CR
Repo: unraid/api PR: 0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-11-24T17:52:26.907Z
Learning: Applies to **/*.test.ts : Reset mocks between tests with `vi.clearAllMocks()`
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.service.integration.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker-network.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.module.spec.ts
📚 Learning: 2025-11-24T17:52:26.907Z
Learnt from: CR
Repo: unraid/api PR: 0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-11-24T17:52:26.907Z
Learning: Applies to **/__test__/components/**/*.ts : Mock external dependencies and services in Vue component tests
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.service.integration.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker-network.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.module.spec.ts
📚 Learning: 2025-11-24T17:52:26.908Z
Learnt from: CR
Repo: unraid/api PR: 0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-11-24T17:52:26.908Z
Learning: Applies to **/__test__/store/**/*.ts : Use factory functions for module mocks in Pinia store tests to avoid hoisting issues
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.service.integration.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker-network.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.module.spec.ts
📚 Learning: 2025-11-24T17:52:26.908Z
Learnt from: CR
Repo: unraid/api PR: 0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-11-24T17:52:26.908Z
Learning: Applies to **/__test__/store/**/*.ts : Test Pinia store getter dependencies are properly mocked
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.service.integration.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker-network.service.spec.tsapi/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts
📚 Learning: 2025-11-24T17:52:26.908Z
Learnt from: CR
Repo: unraid/api PR: 0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-11-24T17:52:26.908Z
Learning: Applies to **/__test__/store/**/*.ts : Use `createPinia()` instead of `createTestingPinia()` for most Pinia store tests
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker.service.integration.spec.ts
📚 Learning: 2025-11-24T17:51:37.915Z
Learnt from: CR
Repo: unraid/api PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T17:51:37.915Z
Learning: Applies to web/__test__/**/*.test.{ts,tsx} : Use `createPinia()` and `setActivePinia` when testing Store files, only use `createTestingPinia` if you specifically need its testing features
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker.service.integration.spec.ts
📚 Learning: 2025-11-24T17:52:26.907Z
Learnt from: CR
Repo: unraid/api PR: 0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-11-24T17:52:26.907Z
Learning: This is a Nuxt.js app but tests are run with vitest outside of the Nuxt environment
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker-network.service.spec.ts
📚 Learning: 2025-11-24T17:51:46.348Z
Learnt from: CR
Repo: unraid/api PR: 0
File: .cursor/rules/api-rules.mdc:0-0
Timestamp: 2025-11-24T17:51:46.348Z
Learning: Applies to api/**/*.test.{ts,tsx} : Use Vitest for the test suite, not Jest
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker-network.service.spec.ts
📚 Learning: 2025-11-24T17:52:26.907Z
Learnt from: CR
Repo: unraid/api PR: 0
File: .cursor/rules/web-testing-rules.mdc:0-0
Timestamp: 2025-11-24T17:52:26.907Z
Learning: Applies to **/__test__/components/**/*.ts : Use `createTestingPinia()` for mocking stores in Vue components
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker-network.service.spec.ts
📚 Learning: 2025-04-07T14:34:47.255Z
Learnt from: elibosley
Repo: unraid/api PR: 1334
File: api/src/unraid-api/graph/resolvers/docker/docker-event.service.ts:63-66
Timestamp: 2025-04-07T14:34:47.255Z
Learning: In DockerEventService, the chokidar file watcher is configured with `ignoreInitial: false` to ensure that existing files (like the Docker socket) are detected and processed at application startup, not just when files change after the watcher is initialized.
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker-event.service.ts
📚 Learning: 2024-11-06T20:59:25.809Z
Learnt from: mdatelle
Repo: unraid/api PR: 942
File: api/src/unraid-api/graph/resolvers/auth/auth.resolver.ts:111-113
Timestamp: 2024-11-06T20:59:25.809Z
Learning: In the Unraid API project, error handling for mutations is handled at the service level rather than in the GraphQL resolvers. Specifically, in `api/src/unraid-api/graph/resolvers/auth/auth.resolver.ts`, methods like `removeRoleFromApiKey` rely on service-level error handling.
Applied to files:
api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts
🧬 Code graph analysis (4)
api/src/unraid-api/graph/resolvers/docker/docker-network.service.ts (3)
api/src/unraid-api/graph/resolvers/docker/utils/docker-client.ts (1)
getDockerClient(5-12)api/src/unraid-api/cli/generated/graphql.ts (1)
DockerNetwork(900-917)web/src/composables/gql/graphql.ts (1)
DockerNetwork(924-941)
api/src/unraid-api/graph/resolvers/docker/organizer/docker-organizer.service.ts (1)
api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts (1)
organizer(178-187)
api/src/unraid-api/graph/resolvers/docker/docker.service.integration.spec.ts (1)
api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts (2)
containers(89-122)networks(145-154)
api/src/unraid-api/graph/resolvers/docker/docker-template-scanner.service.ts (1)
api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts (1)
containers(89-122)
🪛 GitHub Check: CodeQL
api/src/unraid-api/graph/resolvers/docker/docker.service.ts
[warning] 239-239: Useless assignment to local variable
The initial value of containers is unused, since it is always overwritten.
🔇 Additional comments (22)
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts (1)
276-276: LGTM! Tests correctly updated to reflect cache removal.The tests now verify direct Docker API interactions via
mockListContainersinstead of cache-related behavior, which aligns with the PR objective to eliminate the 60-second TTL cache.Also applies to: 319-345
api/src/unraid-api/graph/resolvers/docker/docker.service.ts (4)
1-1: LGTM! Cache manager successfully removed from service.The removal of
CACHE_MANAGERinjection and related imports correctly implements the PR objective to eliminate Docker caching infrastructure.Also applies to: 32-41
128-160: Signature change ensures fresh data on every call.The updated
getContainerssignature now usesDocker.ContainerListOptionsdirectly (with defaultsall=true,size=false) and removes the cache abstraction. This guarantees API queries return live Docker state, addressing the stale-data issues mentioned in the PR objectives.
162-165: Port conflicts and networks now fetch fresh data.Both
getPortConflictsandgetNetworksmethods now always fetch live data without cache parameters, ensuring changes from external tools (like Community Applications) are immediately visible.Also applies to: 182-184
239-239: CodeQL false positive can be safely ignored.The static analysis warning about "useless assignment" at line 239 is a false positive. The
containersvariable is intentionally reassigned in the retry loop (line 243) to poll for updated container state after the stop command.api/src/unraid-api/graph/resolvers/docker/docker-template-scanner.service.ts (1)
90-90: Template scanner now uses live container data.Calling
getContainers()withoutskipCacheensures the template scanner sees the most current container state, preventing mismatches when containers are added/removed externally.api/src/unraid-api/graph/resolvers/docker/docker.module.spec.ts (1)
76-76: Mock correctly updated to reflect removed method.The DockerService mock no longer includes
clearContainerCache, which aligns with its removal from the actual service implementation.api/src/unraid-api/graph/resolvers/docker/docker-event.service.ts (1)
127-136: Event handler now publishes fresh app info.Removing the cache-clearing step and directly calling
getAppInfo()ensures Docker events (start, stop, etc.) trigger immediate fresh data fetches. This guarantees the published app info reflects the actual current state.api/src/unraid-api/graph/resolvers/docker/docker.service.integration.spec.ts (2)
70-81: LGTM - Cache manager correctly removed from test setup.The test module no longer provides CACHE_MANAGER, which aligns with the service's removal of cache dependencies. The setup now includes only the necessary service providers for integration testing.
94-94: LGTM - Method calls updated to reflect no-cache API.All Docker service method calls have been correctly updated to omit the
skipCacheparameter, ensuring integration tests validate direct Docker daemon interaction as intended by this PR.Also applies to: 104-104, 113-113, 145-145
api/src/unraid-api/graph/resolvers/docker/docker-network.service.spec.ts (3)
22-30: LGTM - Test module simplified correctly.The test setup has been streamlined to remove the CACHE_MANAGER provider, reflecting the service's removal of cache dependencies. The module now contains only the essential DockerNetworkService provider.
37-52: LGTM - Test correctly validates direct Docker fetch.The test has been appropriately updated to:
- Call
getNetworks()without cache-related parameters- Verify the camelCase property
name(correctly mapped from Docker'sName)- Focus on direct Docker API interaction
54-60: LGTM - Good edge case coverage.The test appropriately validates the empty array case when no Docker networks exist, ensuring the service handles this scenario correctly without cache fallback.
api/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts (2)
163-163: LGTM - Test expectations correctly updated.All test expectations have been properly updated to reflect the removal of
skipCachefrom thegetContainersmethod signature. The tests now validate calls with only thesizeparameter, which is correct.Also applies to: 193-193, 207-207, 242-242, 256-256, 268-268
271-281: LGTM - Excellent deprecation test.This test effectively documents that the
skipCacheparameter is now deprecated and ignored, providing clear evidence of the API behavior change while maintaining backward compatibility.api/src/unraid-api/graph/resolvers/docker/docker-network.service.ts (1)
1-42: LGTM - Clean cache removal implementation.The service has been successfully refactored to remove all caching infrastructure:
- Eliminated CACHE_MANAGER dependency
- Simplified
getNetworks()to always fetch live data- Updated logging to reflect direct Docker API calls
This ensures Docker network queries always return current state, addressing the stale-data issues described in the PR objectives.
api/src/unraid-api/graph/resolvers/docker/organizer/docker-organizer.service.ts (1)
57-90: LGTM - Organizer service correctly updated.The organizer service methods have been properly refactored to remove
skipCacheparameters throughout the call chain:
getResourcesnow passes options directly todockerService.getContainerswithout cache-related spreadingsyncAndGetOrganizerandresolveOrganizerhave simplified signatures without cache options- The changes maintain functionality while removing cache complexity
api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts (5)
89-122: LGTM! Clean deprecation with correct service integration.The
skipCacheparameter is properly deprecated with a clear reason, and the service call correctly removes caching while preserving the conditionalsizeoption based on requested fields. The field inspection logic via@Info()remains intact and appropriate.
145-154: LGTM! Consistent deprecation pattern.The deprecation and service call update align with the caching removal objective.
161-170: LGTM! Consistent deprecation pattern.The deprecation and service call update align with the caching removal objective.
178-187: LGTM! Consistent deprecation pattern.The deprecation and service call update align with the caching removal objective.
363-374: LGTM! Cache clearing removed as intended.The removal of the
clearContainerCache()call is consistent with the PR objective to eliminate caching infrastructure. The mutation still correctly resets template mappings to defaults.
| @ResolveField(() => DockerContainer, { nullable: true }) | ||
| public async container(@Args('id', { type: () => PrefixedID }) id: string) { | ||
| const containers = await this.dockerService.getContainers({ skipCache: false }); | ||
| const containers = await this.dockerService.getContainers(); | ||
| return containers.find((c) => c.id === id) ?? null; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add size field handling for consistency with the containers method.
The container resolver doesn't check whether size fields (sizeRootFs, sizeRw) are requested, unlike the containers method (lines 98-103). This means if a client queries docker.container(id: "...") { sizeRootFs }, the field won't be populated correctly because the Docker API requires an explicit size option.
🔧 Suggested fix to match `containers` behavior
@ResolveField(() => DockerContainer, { nullable: true })
-public async container(@Args('id', { type: () => PrefixedID }) id: string) {
- const containers = await this.dockerService.getContainers();
+public async container(
+ @Args('id', { type: () => PrefixedID }) id: string,
+ @Info() info: GraphQLResolveInfo
+) {
+ const requestsRootFsSize = GraphQLFieldHelper.isFieldRequested(info, 'sizeRootFs');
+ const requestsRwSize = GraphQLFieldHelper.isFieldRequested(info, 'sizeRw');
+ const containers = await this.dockerService.getContainers({
+ size: requestsRootFsSize || requestsRwSize,
+ });
return containers.find((c) => c.id === id) ?? null;
}🤖 Prompt for AI Agents
In @api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts around lines 78
- 82, The container resolver currently fetches all containers via
dockerService.getContainers() and doesn't pass the Docker API size option when
the GraphQL fields sizeRootFs/sizeRw are requested; update the container method
to inspect the GraphQLResolveInfo (add an info parameter) to detect if
sizeRootFs or sizeRw are requested and then call a single-container fetch that
accepts the size flag (e.g., dockerService.getContainer(id, { size: true }) or
dockerService.getContainers({ id, size: true }) depending on available service
methods) so the Docker API returns populated sizeRootFs/sizeRw fields; keep
returning the found container or null.
Summary
Test plan
pnpm type-check)🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Deprecations
✏️ Tip: You can customize this high-level summary in your review settings.