chore: update cache functionality to use nestjs-cache module #1360
Conversation
WalkthroughThis PR introduces caching capabilities and updates Docker-related API methods. The GraphQL schema’s Changes
Sequence Diagram(s)sequenceDiagram
participant Resolver
participant DockerService
participant CacheManager
participant DockerAPI
Resolver->>DockerService: getContainers({ skipCache: false })
DockerService->>CacheManager: get('containers')
alt Cached data found
CacheManager-->>DockerService: return cachedData
DockerService-->>Resolver: return cachedData
else No cached data
CacheManager-->>DockerService: undefined
DockerService->>DockerAPI: fetch container data
DockerAPI-->>DockerService: container data
DockerService->>CacheManager: set('containers', data)
DockerService-->>Resolver: return data
end
sequenceDiagram
participant EventService
participant DockerService
participant PubSub
participant Logger
EventService->>DockerService: clearContainerCache()
DockerService-->>EventService: acknowledgment
EventService->>DockerService: getAppInfo()
DockerService-->>EventService: return app info
EventService->>PubSub: publish(PUBSUB_CHANNEL.INFO, app info)
EventService->>Logger: log debug message
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
api/src/unraid-api/graph/resolvers/docker/docker-event.service.spec.ts (1)
81-82: Mock implementation duplicationThese mock implementations duplicate what's already defined in the global mock at lines 56-57. Consider consolidating to a single implementation to make future maintenance easier.
// Create a mock Docker service *instance* const mockDockerServiceImpl = { getDockerClient: vi.fn().mockReturnValue(mockDockerClient), - clearContainerCache: vi.fn(), - getAppInfo: vi.fn().mockResolvedValue({ info: { apps: { installed: 1, running: 1 } } }), + clearContainerCache: vi.fn(), + getAppInfo: vi.fn(), // No need to duplicate implementation, the imported mock will handle this };api/src/unraid-api/app/app.module.ts (1)
42-42: Consider providing a TTL or store configuration to theCacheModule.register().
Using the default configuration might be okay, but explicitly specifying options such as a defaultttlor a custom store can further optimize performance.imports: [ ... - CacheModule.register(), + CacheModule.register({ + // e.g. store: 'memory', + ttl: 60 + }), GraphModule,api/src/unraid-api/graph/resolvers/docker/docker.service.ts (1)
214-238: Stop logic and final state check.
Sleeping for 500ms up to 5 times might be too short for systems under heavy load. Consider making these retries configurable.- for (let i = 0; i < 5; i++) { - await sleep(500); + const maxRetries = parseInt(process.env.DOCKER_STOP_RETRIES || '5'); + const retryDelay = parseInt(process.env.DOCKER_STOP_DELAY || '500'); + for (let i = 0; i < maxRetries; i++) { + await sleep(retryDelay);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
api/generated-schema.graphql(1 hunks)api/package.json(2 hunks)api/src/unraid-api/app/app.module.ts(2 hunks)api/src/unraid-api/graph/resolvers/docker/docker-event.service.spec.ts(8 hunks)api/src/unraid-api/graph/resolvers/docker/docker-event.service.ts(2 hunks)api/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts(2 hunks)api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts(3 hunks)api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts(10 hunks)api/src/unraid-api/graph/resolvers/docker/docker.service.ts(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
api/src/unraid-api/graph/resolvers/docker/docker-event.service.ts (1)
api/src/core/pubsub.ts (1)
pubsub(25-25)
api/src/unraid-api/graph/resolvers/docker/docker-event.service.spec.ts (1)
api/src/core/pubsub.ts (1)
pubsub(25-25)
api/src/unraid-api/graph/resolvers/docker/docker.service.ts (3)
web/composables/gql/graphql.ts (3)
Docker(526-531)DockerContainer(533-551)DockerNetwork(571-588)api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts (2)
containers(38-40)networks(48-50)api/src/core/pubsub.ts (1)
pubsub(25-25)
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts (2)
api/src/core/pubsub.ts (1)
pubsub(25-25)web/composables/gql/graphql.ts (1)
DockerContainer(533-551)
🪛 GitHub Check: Test API
api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts
[failure] 38-38:
Replace @Args('skipCache',·{·defaultValue:·false,·type:·()·=>·Boolean·})·skipCache:·boolean with ⏎········@Args('skipCache',·{·defaultValue:·false,·type:·()·=>·Boolean·})·skipCache:·boolean⏎····
[failure] 48-48:
Replace @Args('skipCache',·{·defaultValue:·false,·type:·()·=>·Boolean·})·skipCache:·boolean with ⏎········@Args('skipCache',·{·defaultValue:·false,·type:·()·=>·Boolean·})·skipCache:·boolean⏎····
api/src/unraid-api/graph/resolvers/docker/docker-event.service.spec.ts
[failure] 59-59:
Delete ;⏎⏎//·Import·pubsub·for·use·in·tests⏎import·{·pubsub,·PUBSUB_CHANNEL·}·from·'@app/core/pubsub.js'
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts
[failure] 8-8:
Replace import·{⏎····ContainerState,⏎····DockerContainer,⏎ with //·Import·the·mocked·pubsub·parts⏎import·{·pubsub,·PUBSUB_CHANNEL·}·from·'@app/core/pubsub.js';⏎import·{·ContainerState,·DockerContainer·
🪛 GitHub Check: Build API
api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts
[failure] 38-38:
Replace @Args('skipCache',·{·defaultValue:·false,·type:·()·=>·Boolean·})·skipCache:·boolean with ⏎········@Args('skipCache',·{·defaultValue:·false,·type:·()·=>·Boolean·})·skipCache:·boolean⏎····
[failure] 48-48:
Replace @Args('skipCache',·{·defaultValue:·false,·type:·()·=>·Boolean·})·skipCache:·boolean with ⏎········@Args('skipCache',·{·defaultValue:·false,·type:·()·=>·Boolean·})·skipCache:·boolean⏎····
api/src/unraid-api/graph/resolvers/docker/docker-event.service.spec.ts
[failure] 59-59:
Delete ;⏎⏎//·Import·pubsub·for·use·in·tests⏎import·{·pubsub,·PUBSUB_CHANNEL·}·from·'@app/core/pubsub.js'
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts
[failure] 8-8:
Replace import·{⏎····ContainerState,⏎····DockerContainer,⏎ with //·Import·the·mocked·pubsub·parts⏎import·{·pubsub,·PUBSUB_CHANNEL·}·from·'@app/core/pubsub.js';⏎import·{·ContainerState,·DockerContainer·
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Web App
- GitHub Check: Cloudflare Pages
🔇 Additional comments (33)
api/src/unraid-api/graph/resolvers/docker/docker-event.service.spec.ts (4)
42-50: Good addition of pubsub mock for testingThe mock implementation correctly includes the publish function and PUBSUB_CHANNEL constant that will be needed for the tests. This aligns with the PR's objective of updating cache functionality.
56-57: Updated mock matches the new API changesThe DockerService mock has been properly updated to reflect the new cache-related methods, replacing the old
debouncedContainerCacheUpdatewithclearContainerCacheand addinggetAppInfo.
142-144: Correct update of test assertionsTest assertions have been properly updated to check that cache is cleared, app info is retrieved, and the update is published through pubsub when Docker events occur. This aligns with the new cache handling design.
164-166: All test cases updated consistentlyAll test cases have been consistently updated to verify the new behavior with
clearContainerCache,getAppInfo, andpubsub.publish. These changes properly test the new caching and notification mechanisms.Also applies to: 182-184, 200-202, 216-218, 233-235
api/package.json (2)
66-66: Added NestJS cache management dependency.The addition of
@nestjs/cache-manageris appropriate for implementing structured caching functionality within a NestJS application.
82-82: Added cache-manager dependency.The
cache-managerpackage is the underlying caching library that works with the NestJS wrapper. This is the correct version to pair with the NestJS cache manager.api/generated-schema.graphql (1)
1248-1248: Added cache control parameter to containers field.The GraphQL schema now allows clients to specify whether to use cache via the
useCacheparameter (defaulting to true). This provides flexibility for clients to request fresh data when needed.api/src/unraid-api/graph/resolvers/docker/docker.resolver.spec.ts (3)
22-22: Added missing mock for getNetworks method.This addition ensures the mock service properly implements all methods used by the resolver under test.
70-70: Updated test to use new parameter pattern.The test now explicitly passes
falseto the containers method to verify the correct behavior when cache is disabled.
72-72: Updated expectation to use skipCache instead of useCache.The test now verifies that
getContainersis called with{ skipCache: false }which reflects the parameter naming change fromuseCachetoskipCache.api/src/unraid-api/graph/resolvers/docker/docker-event.service.ts (2)
9-9: Added pubsub mechanism to broadcast container updates.Importing the pubsub module allows for real-time updates to be broadcast to subscribers when container events occur.
132-136: Enhanced Docker event handling with pubsub notifications.The event handler now:
- Clears the container cache (instead of updating it)
- Gets updated application info
- Publishes the info to subscribers
- Logs the update for debugging
This approach ensures that all subscribers immediately receive updated container information after Docker events.
api/src/unraid-api/app/app.module.ts (1)
4-4: The new import for the CacheModule is correct.
No issues identified.api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts (1)
1-1: The additional import from@nestjs/graphqllooks good.
No concerns.api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts (6)
3-3: The newCACHE_MANAGERimport is correct.
Consistent with usage inDockerService.
8-14: These additional imports for DockerContainer, ContainerState, and mocked pubsub look aligned.
No immediate issues.🧰 Tools
🪛 GitHub Check: Test API
[failure] 8-8:
Replaceimport·{⏎····ContainerState,⏎····DockerContainer,⏎with//·Import·the·mocked·pubsub·parts⏎import·{·pubsub,·PUBSUB_CHANNEL·}·from·'@app/core/pubsub.js';⏎import·{·ContainerState,·DockerContainer·🪛 GitHub Check: Build API
[failure] 8-8:
Replaceimport·{⏎····ContainerState,⏎····DockerContainer,⏎with//·Import·the·mocked·pubsub·parts⏎import·{·pubsub,·PUBSUB_CHANNEL·}·from·'@app/core/pubsub.js';⏎import·{·ContainerState,·DockerContainer·
78-83: Mocking the cacheManager is a good approach for testing.
This ensures that tests can isolate and verify caching behavior properly.
89-96: Resetting mocks before each test is a best practice.
Helps maintain a clean testing environment.
99-105: Providing DockerService and mockCacheManager in the test providers array is correct.
No concerns.
137-139: Skipping the cache for a direct-fetch test confirms the fallback logic.
Implementation is consistent with the newskipCacheapproach.api/src/unraid-api/graph/resolvers/docker/docker.service.ts (13)
1-2: Nice usage of@nestjs/cache-managerinjection.
Ensures the service is ready for caching as soon as it initializes.
5-5: Using theCacheinterface fromcache-manager.
This is beneficial for type safety and autocompletion.
23-27:skipCacheparameter is consistent.
ReplacinguseCachewithskipCachebetter clarifies the logic to bypass the cache.
36-38: Introducing container and network cache keys with a TTL.
Centralizing these constants supports consistency and maintainability.
40-40: Constructor injection ofCache.
Paves the way for consistent caching usage throughout the service.
50-59:getAppInfomethod.
Returns a quick metric of installed vs. running containers—a solid approach for summarizing Docker state.
62-68: Automatic Docker cache warming inonModuleInit.
Great for ensuring data is readily available at startup.
118-125:getContainersnow consistently respects theskipCacheoption.
Implementation logic appears sound.
126-131: Caching logic is correct.
Retrieving existing data from the cache before updating helps performance.
155-189:getNetworkswith caching.
Implementation mirrorsgetContainers, providing consistent caching across resources.
191-193:clearContainerCachemethod.
Explicit invalidation ensures the next retrieval won't read stale data.
199-208: Invalidation after starting containers.
Good logic to guarantee immediate reflection of updated container status.
143-148:❓ Verification inconclusive
Setting container cache.
Currently uses a 60-second TTL; consider verifying if this interval suits the production environment.
🏁 Script executed:
#!/bin/bash # Description: Investigate container usage patterns to confirm a 60s TTL is sufficient. rg "DockerService" -A 10Length of output: 43677
Revisit Container Cache TTL Setting
The container cache is set using a fixed 60-second TTL (i.e.
DockerService.CACHE_TTL_SECONDS) as implemented inapi/src/unraid-api/graph/resolvers/docker/docker.service.ts(lines 143–148). The tests and code confirm that the TTL is consistently applied (by multiplying the 60 seconds by 1000 to yield milliseconds). Please verify whether a 60-second cache interval remains optimal given production container update rates. If container states change more frequently or if performance requirements suggest a different timing, consider making the TTL configurable.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
api/src/unraid-api/graph/resolvers/docker/docker-event.service.spec.ts (1)
8-9: Move imports to the top of the fileThe imports for pubsub should be moved to the top of the file with the other imports to follow standard code organization practices.
- // Import pubsub for use in tests - import { pubsub, PUBSUB_CHANNEL } from '@app/core/pubsub.js'; // Move to the top with other imports + import { pubsub, PUBSUB_CHANNEL } from '@app/core/pubsub.js';
🧹 Nitpick comments (1)
api/src/unraid-api/graph/resolvers/docker/docker.service.ts (1)
157-158: TODO comment needs updatingThere's a TODO comment about implementing filtering/cache/proper typing for the getNetworks method, but caching has now been implemented. The TODO should be updated to reflect the current state.
- * @todo filtering / cache / proper typing + * @todo filtering / proper typing
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
api/src/unraid-api/app/app.module.ts(2 hunks)api/src/unraid-api/graph/resolvers/docker/docker-event.service.spec.ts(9 hunks)api/src/unraid-api/graph/resolvers/docker/docker-event.service.ts(2 hunks)api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts(3 hunks)api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts(10 hunks)api/src/unraid-api/graph/resolvers/docker/docker.service.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts
- api/src/unraid-api/app/app.module.ts
- api/src/unraid-api/graph/resolvers/docker/docker-event.service.ts
🧰 Additional context used
🧬 Code Graph Analysis (3)
api/src/unraid-api/graph/resolvers/docker/docker-event.service.spec.ts (1)
api/src/core/pubsub.ts (1)
pubsub(25-25)
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts (2)
api/src/core/pubsub.ts (1)
pubsub(25-25)web/composables/gql/graphql.ts (1)
DockerContainer(533-551)
api/src/unraid-api/graph/resolvers/docker/docker.service.ts (1)
api/src/unraid-api/graph/resolvers/docker/docker.resolver.ts (2)
containers(38-42)networks(50-54)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (38)
api/src/unraid-api/graph/resolvers/docker/docker-event.service.spec.ts (8)
44-52: Good implementation of pubsub mockThe mock implementation for pubsub is well structured with the necessary methods and constants needed for testing.
58-59: Updated DockerService mock to match new interfaceThe Docker service mock has been correctly updated to include the new methods
clearContainerCacheandgetAppInfothat replace the previousdebouncedContainerCacheUpdatemethod.Also applies to: 80-81
141-143: Correctly updated test assertions for container eventsThe test assertions have been properly updated to check for the new methods and pubsub usage, ensuring that container cache is cleared and app info is published when container events occur.
163-165: Properly testing ignored actionsThe test for ignored actions now correctly verifies that neither the cache clearing nor pubsub publishing occurs for non-watched actions.
180-183: Updated assertion counts for malformed JSON testThe test assertions for malformed JSON handling have been updated to verify the exact number of calls to the new methods, ensuring robust error handling.
199-201: Updated multi-event handling assertionsTest for handling multiple events now correctly verifies that cache clearing and pubsub publishing happens for each valid event.
215-217: Correct handling of mixed valid and invalid JSONThe test now properly verifies that cache clearing and pubsub operations only happen for valid JSON content.
232-234: Proper handling of empty linesTest now correctly verifies that empty lines in the data stream don't affect processing of valid events, with appropriate cache and pubsub operations.
api/src/unraid-api/graph/resolvers/docker/docker.service.spec.ts (14)
2-2: Added CACHE_MANAGER importThe CACHE_MANAGER import has been correctly added to support testing with the new cache functionality.
8-10: Added pubsub and type importsAppropriate imports for pubsub and container model types have been added to support testing the new functionality.
75-80: Added cache manager mockA complete mock implementation of the cache manager has been added with all the necessary methods (get, set, del) needed for testing.
86-94: Updated beforeEach with cache mock resetsThe beforeEach hook now correctly resets all mock functions, including the new cache manager functions, ensuring test isolation.
95-103: Added CACHE_MANAGER providerThe CACHE_MANAGER provider has been correctly added to the testing module configuration, allowing the DockerService to work with the mock cache.
134-137: Updated container test for cache functionalityThe container test now correctly simulates a cache miss and verifies that the cache is set after retrieving containers, confirming proper cache integration.
Also applies to: 164-164
190-192: Updated start container test with cache operationsThe start container test now verifies cache invalidation and checks that the cache is updated with the new container state, along with publishing to pubsub.
Also applies to: 215-222
248-249: Updated stop container test with cache operationsThe stop container test now verifies that the cache is properly invalidated after stopping a container and that the updated app info is published via pubsub.
Also applies to: 272-280
286-291: Error handling properly tests cache invalidationThe error handling test correctly verifies that cache is invalidated even when the start operation fails because the container is not found.
297-302: Stop error handling properly tests cache invalidationThe stop error handling test correctly verifies that cache is invalidated even when the stop operation fails because the container is not found.
344-346: Networks test verifies cache functionalityThe networks test now correctly verifies that the cache is set after retrieving networks when skipCache is true, ensuring proper caching behavior.
Also applies to: 388-388
393-399: Empty networks test verifies cache behaviorThe empty networks test correctly verifies that an empty array is cached when no networks are returned.
407-413: Network error test verifies cache is not setThe network error test correctly verifies that the cache is not set when an error occurs fetching networks, preventing caching of error states.
416-437: Added getAppInfo testA new test has been added for the getAppInfo method, verifying that it correctly calculates installed and running containers from cached data.
api/src/unraid-api/graph/resolvers/docker/docker.service.ts (16)
1-2: Added imports for cache functionalityThe necessary imports for the NestJS cache manager have been added, enabling caching capabilities in the service.
Also applies to: 5-5
19-21: Updated interface options to use skipCacheThe interface options have been updated to use
skipCacheinstead ofuseCache, which provides a more intuitive API where the default behavior is to use cache unless explicitly skipped.Also applies to: 23-25
33-36: Added cache constantsCache key constants and TTL values have been defined as static properties, making them easier to maintain and update.
37-39: Injected cache managerThe cache manager is now properly injected via constructor dependency injection, following NestJS best practices.
47-58: Implemented getAppInfo methodThe getAppInfo method has been implemented to calculate installed and running container counts, using the cached container data by default for better performance.
60-67: Updated onModuleInit to warm cacheThe onModuleInit method now properly warms the cache on startup and publishes initial app info to subscribers, improving initial performance and user experience.
117-133: Implemented cache checking in getContainersThe getContainers method now checks for cached data first, returning it if available and not explicitly skipped, reducing unnecessary Docker API calls.
147-152: Added cache storage for containersAfter fetching containers from Docker, the method now stores the results in cache with an appropriate TTL, enabling future cache hits.
160-169: Implemented cache checking in getNetworksThe getNetworks method now properly checks for cached data first, returning it if available and not explicitly skipped, reducing unnecessary Docker API calls.
194-199: Added cache storage for networksAfter fetching networks from Docker, the method now stores the results in cache with an appropriate TTL, enabling future cache hits.
202-205: Added clearContainerCache methodA new method has been added to explicitly clear the container cache, which is used when container state changes occur, ensuring fresh data on subsequent requests.
210-212: Added cache invalidation after container startThe start method now invalidates the container cache after starting a container, ensuring fresh data on subsequent requests, with appropriate logging.
217-219: Added pubsub publishing after container startThe start method now publishes updated app info via pubsub after starting a container, allowing subscribers to be notified of the container state change.
225-227: Added cache invalidation after container stopThe stop method now invalidates the container cache after stopping a container, ensuring fresh data on subsequent requests, with appropriate logging.
232-233: Using skipCache parameter in container stop retry logicThe container stop retry logic has been updated to explicitly skip cache when checking container status, ensuring fresh data during the retry process.
247-249: Added pubsub publishing after container stopThe stop method now publishes updated app info via pubsub after stopping a container, allowing subscribers to be notified of the container state change.
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
Summary by CodeRabbit
New Features
Refactor
Tests
Chores