[deps] move cache impl to separate repo #198
Conversation
WalkthroughReplaces the repository's internal caching subsystem with the external github.com/go-core-fx/cachefx: removes local pkg/cache implementations, errors, options, typed helpers, and tests; updates imports and DI wiring; adds cachefx dependency and registers its FX module. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (22)
go.mod(2 hunks)internal/config/module.go(2 hunks)internal/sms-gateway/app.go(2 hunks)internal/sms-gateway/cache/config.go(0 hunks)internal/sms-gateway/cache/errors.go(0 hunks)internal/sms-gateway/cache/factory.go(0 hunks)internal/sms-gateway/cache/module.go(1 hunks)internal/sms-gateway/modules/messages/cache.go(1 hunks)internal/sms-gateway/modules/messages/module.go(2 hunks)internal/sms-gateway/modules/push/service.go(3 hunks)internal/sms-gateway/online/module.go(2 hunks)internal/sms-gateway/online/service.go(1 hunks)pkg/cache/cache.go(0 hunks)pkg/cache/errors.go(0 hunks)pkg/cache/memory.go(0 hunks)pkg/cache/memory_bench_test.go(0 hunks)pkg/cache/memory_concurrency_test.go(0 hunks)pkg/cache/memory_edge_test.go(0 hunks)pkg/cache/memory_profile_test.go(0 hunks)pkg/cache/memory_test.go(0 hunks)pkg/cache/options.go(0 hunks)pkg/cache/redis.go(0 hunks)
💤 Files with no reviewable changes (13)
- internal/sms-gateway/cache/config.go
- internal/sms-gateway/cache/errors.go
- pkg/cache/memory_bench_test.go
- internal/sms-gateway/cache/factory.go
- pkg/cache/memory_profile_test.go
- pkg/cache/cache.go
- pkg/cache/redis.go
- pkg/cache/errors.go
- pkg/cache/memory_edge_test.go
- pkg/cache/memory_test.go
- pkg/cache/memory_concurrency_test.go
- pkg/cache/memory.go
- pkg/cache/options.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: capcom6
Repo: android-sms-gateway/server PR: 178
File: pkg/cache/redis.go:63-75
Timestamp: 2025-09-10T23:53:42.006Z
Learning: In pkg/cache/redis.go, the Redis cache implementation uses Redis's native field-level TTL via HExpireAt, meaning expired fields are automatically removed by Redis itself. This differs from the memory implementation where the application tracks expiration times. As a result, operations like Drain that call HGetAll will only return non-expired items since expired fields are already removed by Redis.
Learnt from: capcom6
Repo: android-sms-gateway/server PR: 178
File: pkg/cache/redis.go:63-75
Timestamp: 2025-09-10T23:53:42.006Z
Learning: In pkg/cache/redis.go, the Redis cache implementation uses Redis's native field-level TTL via HExpireAt, meaning expired fields are automatically removed by Redis itself. This differs from the memory implementation where the application tracks expiration times. As a result, operations like Drain that call HGetAll will only return non-expired items since expired fields are already removed by Redis.
📚 Learning: 2025-09-10T23:45:45.502Z
Learnt from: capcom6
Repo: android-sms-gateway/server PR: 178
File: internal/sms-gateway/online/module.go:14-16
Timestamp: 2025-09-10T23:45:45.502Z
Learning: fx.Decorate in uber-go/fx creates module-scoped decorated dependencies, not global modifications. Using fx.Decorate(func(log *zap.Logger) *zap.Logger { return log.Named("module_name") }) is a correct pattern for creating module-specific loggers that are scoped only to that module's dependency graph.
Applied to files:
internal/sms-gateway/cache/module.gointernal/sms-gateway/online/module.go
🧬 Code graph analysis (6)
internal/config/module.go (1)
internal/config/config.go (1)
Config(10-19)
internal/sms-gateway/modules/messages/module.go (2)
internal/sms-gateway/cache/module.go (1)
Factory(9-9)internal/config/config.go (1)
Cache(75-77)
internal/sms-gateway/app.go (1)
internal/sms-gateway/cache/module.go (1)
Module(11-19)
internal/sms-gateway/cache/module.go (3)
internal/sms-gateway/app.go (1)
Module(34-61)internal/sms-gateway/modules/messages/module.go (1)
Module(11-26)internal/sms-gateway/online/module.go (1)
Module(12-37)
internal/sms-gateway/online/module.go (2)
internal/sms-gateway/cache/module.go (1)
Factory(9-9)internal/config/config.go (1)
Cache(75-77)
internal/sms-gateway/modules/push/service.go (1)
internal/sms-gateway/cache/module.go (1)
Factory(9-9)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Benchmark
- GitHub Check: Test
- GitHub Check: Lint
- GitHub Check: E2E
- GitHub Check: Analyze (go)
🔇 Additional comments (11)
internal/sms-gateway/online/module.go (1)
6-7: Clean migration to external cache implementation.The import alias pattern and factory usage are well-structured. The internal
appCache.Factoryacts as a wrapper that returns externalcache.Cacheinstances, maintaining a clean separation of concerns.Also applies to: 18-20
internal/sms-gateway/online/service.go (1)
10-10: LGTM!Simple import path update with no behavioral changes.
internal/sms-gateway/modules/messages/cache.go (1)
9-9: LGTM!Import path updated to external cachefx package. The API surface (Cache interface, WithTTL, AndSetTTL helpers) remains unchanged.
internal/sms-gateway/modules/messages/module.go (1)
6-6: Return type updated to external cache implementation.The
fx.Providereturn type changed fromcacheFactory.Cache(internal) tocacheImpl.Cache(external cachefx). This aligns with the migration and maintains type safety.Also applies to: 15-17
internal/config/module.go (1)
17-17: Config type migrated to external cachefx.The configuration provider now returns
cachefx.Configinstead of the internalcache.Config. The structure appears compatible with the sameURLfield being used.Also applies to: 124-128
internal/sms-gateway/cache/module.go (1)
9-9: Factory type alias now publicly exported.The
Factorytype is now a public alias ofcachefx.Factory, allowing other modules to reference it while maintaining the ability to swap implementations if needed in the future.go.mod (1)
22-22: Review redis upgrade for breaking changes before merging.The PR bumps
go-redisfrom v9.9.0 to v9.17.2—a significant upgrade with multiple breaking/behavioral changes that require code review:
- Typed errors: Error handling changed from string-matching to typed errors (use
errors.As/errors.Isinstead of string comparisons)- Buffer sizes: Default read/write buffers increased from 4KiB to 32KiB (may affect memory; set explicitly if needed)
- Connection setup: CLIENT SETINFO behavior and timing changed (affects connection identity/DisableIdentity option)
- Panic to error returns: Several operations now return errors instead of panicking (review error handling for assumptions about panics)
- New commands and RESP protocol behavior: Verify your server version supports new Redis 8.x commands and RESP protocol expectations match your configuration
The zap v1.27.0→v1.27.1 upgrade is safe (minor patch, no security advisories). However,
github.com/go-core-fx/cachefxis a new, unmaintained dependency (0 stars, 1 commit, minimal activity)—consider if an established cache library is preferable.Run full integration tests, especially for connection pooling and error handling paths.
internal/sms-gateway/app.go (1)
40-43: Module initialization order is correct for Uber fx dependency injection.The
cachefx.Module()declaration beforeappconfig.Module()does not create a dependency issue. In Uber fx, module declaration order does not determine dependency resolution—fx resolves dependencies by matching parameter types across all registered providers. Sinceappconfig.Module()providescachefx.Config(line 124 ininternal/config/module.go) andcache.Module()depends oncachefx.Factoryfromcachefx.Module(), the dependency graph resolves correctly regardless of declaration order. The recent commit moving cache implementation to the externalcachefxlibrary (v0.0.0-20251204095227-c346b7da87db) confirms this approach is working without initialization errors.internal/sms-gateway/modules/push/service.go (3)
8-10: LGTM! Import changes align with external cache migration.The import structure correctly separates:
- Internal cache factory wrapper (line 8, aliased as
cacheFactory)- External cachefx package for cache types and options (line 10)
The aliasing improves code clarity by distinguishing the internal wrapper from the external implementation.
46-46: LGTM! Parameter correctly uses the internal cache factory.The parameter type
cacheFactory.Factoryappropriately uses the internal wrapper, which maintains the abstraction layer while leveraging the external cachefx implementation underneath.
210-210: WithTTL usage is correct and consistent with existing patterns.The
cache.WithTTLoption is already proven to work elsewhere in the codebase (seeinternal/sms-gateway/modules/messages/cache.go). The cachefx package properly supports TTL across both memory and Redis backends through its abstracted factory pattern. No further verification needed.
🤖 Pull request artifacts
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/sms-gateway/modules/auth/service.go (1)
13-13: Update cache import to usego-core-fx/cachefx.The codebase has been migrated to
github.com/go-core-fx/cachefxacross most modules, but this file still imports the oldgithub.com/capcom6/go-helpers/cachepackage. The modulesmodules/auth/andmodules/devices/have not been updated. Align this import with the rest of the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/sms-gateway/modules/auth/passwords.go(1 hunks)internal/sms-gateway/modules/auth/service.go(3 hunks)internal/sms-gateway/modules/messages/service.go(0 hunks)pkg/health/health.go(1 hunks)
💤 Files with no reviewable changes (1)
- internal/sms-gateway/modules/messages/service.go
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: capcom6
Repo: android-sms-gateway/server PR: 178
File: pkg/cache/redis.go:63-75
Timestamp: 2025-09-10T23:53:42.006Z
Learning: In pkg/cache/redis.go, the Redis cache implementation uses Redis's native field-level TTL via HExpireAt, meaning expired fields are automatically removed by Redis itself. This differs from the memory implementation where the application tracks expiration times. As a result, operations like Drain that call HGetAll will only return non-expired items since expired fields are already removed by Redis.
Learnt from: capcom6
Repo: android-sms-gateway/server PR: 178
File: pkg/cache/redis.go:63-75
Timestamp: 2025-09-10T23:53:42.006Z
Learning: In pkg/cache/redis.go, the Redis cache implementation uses Redis's native field-level TTL via HExpireAt, meaning expired fields are automatically removed by Redis itself. This differs from the memory implementation where the application tracks expiration times. As a result, operations like Drain that call HGetAll will only return non-expired items since expired fields are already removed by Redis.
🧬 Code graph analysis (1)
internal/sms-gateway/modules/auth/service.go (1)
internal/sms-gateway/modules/auth/passwords.go (2)
MakeBCryptHash(14-20)CompareBCryptHash(22-27)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: E2E
- GitHub Check: Benchmark
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
internal/sms-gateway/modules/auth/passwords.go (1)
1-1: LGTM: Package rename improves cohesion.Moving bcrypt helpers into the
authpackage where they're consumed improves module cohesion and eliminates the need for cross-package references.internal/sms-gateway/modules/auth/service.go (1)
99-99: LGTM: Function calls updated correctly.All bcrypt helper calls have been consistently updated to reference the local functions after the package rename.
Also applies to: 166-166, 198-198, 202-202
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/go.yml(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: capcom6
Repo: android-sms-gateway/server PR: 178
File: pkg/cache/redis.go:63-75
Timestamp: 2025-09-10T23:53:42.006Z
Learning: In pkg/cache/redis.go, the Redis cache implementation uses Redis's native field-level TTL via HExpireAt, meaning expired fields are automatically removed by Redis itself. This differs from the memory implementation where the application tracks expiration times. As a result, operations like Drain that call HGetAll will only return non-expired items since expired fields are already removed by Redis.
Learnt from: capcom6
Repo: android-sms-gateway/server PR: 178
File: pkg/cache/redis.go:63-75
Timestamp: 2025-09-10T23:53:42.006Z
Learning: In pkg/cache/redis.go, the Redis cache implementation uses Redis's native field-level TTL via HExpireAt, meaning expired fields are automatically removed by Redis itself. This differs from the memory implementation where the application tracks expiration times. As a result, operations like Drain that call HGetAll will only return non-expired items since expired fields are already removed by Redis.
🪛 actionlint (1.7.9)
.github/workflows/go.yml
78-78: condition "false" is always evaluated to false. remove the if: section
(if-cond)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: E2E
- GitHub Check: Lint
- GitHub Check: Analyze (go)
capcom6
force-pushed
the
deps/external-cache-impl
branch
from
938b02c to
0c24995
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.github/workflows/go.yml (1)
75-130: Avoid keeping a permanently disabled benchmark job
if: falsemakes the entirebenchmarkjob dead code and keeps actionlint complaining. Either remove this job entirely, or replace the condition with something meaningful (e.g., an input flag, label, or schedule) and/or add a short comment explaining when/how it should be re‑enabled.
🧹 Nitpick comments (2)
internal/sms-gateway/modules/push/service.go (1)
8-10: Consider renaming to avoid shadowing the import alias.The parameter
cacheFactory(line 46) shadows the import aliascacheFactory(line 8). While this works correctly—cacheFactory.New(...)calls the method on theFactoryinstance—it can be confusing to readers.Consider renaming either the import alias or the parameter for clarity:
- cacheFactory "github.com/android-sms-gateway/server/internal/sms-gateway/cache" + pushCache "github.com/android-sms-gateway/server/internal/sms-gateway/cache"- cacheFactory cacheFactory.Factory, + cacheFactory pushCache.Factory,Or alternatively, rename the parameter:
- cacheFactory cacheFactory.Factory, + cf cacheFactory.Factory,And update usages at lines 50 and 55.
Also applies to: 46-46
internal/config/module.go (1)
124-128: cachefx.Config wiring from app config looks reasonable; verify URL/TTL semanticsMapping
cfg.Cache.URLintocachefx.Config{URL: ...}is straightforward and aligns with the move to cachefx. Please double‑check that:
cfg.Cache.URLis in the format thatcachefx.Configexpects (e.g., Redis DSN, memory URL, etc.).- Any previous cache behavior that relied on Redis field‑level TTL and Drain/HGetAll semantics is preserved or explicitly adjusted under cachefx. Based on learnings, the old Redis impl depended on Redis removing expired hash fields automatically before Drain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (27)
.github/workflows/go.yml(1 hunks)go.mod(2 hunks)internal/config/module.go(2 hunks)internal/sms-gateway/app.go(2 hunks)internal/sms-gateway/cache/config.go(0 hunks)internal/sms-gateway/cache/errors.go(0 hunks)internal/sms-gateway/cache/factory.go(0 hunks)internal/sms-gateway/cache/module.go(1 hunks)internal/sms-gateway/modules/auth/passwords.go(1 hunks)internal/sms-gateway/modules/auth/service.go(3 hunks)internal/sms-gateway/modules/messages/cache.go(1 hunks)internal/sms-gateway/modules/messages/module.go(2 hunks)internal/sms-gateway/modules/messages/service.go(0 hunks)internal/sms-gateway/modules/push/service.go(3 hunks)internal/sms-gateway/online/module.go(2 hunks)internal/sms-gateway/online/service.go(1 hunks)pkg/cache/cache.go(0 hunks)pkg/cache/errors.go(0 hunks)pkg/cache/memory.go(0 hunks)pkg/cache/memory_bench_test.go(0 hunks)pkg/cache/memory_concurrency_test.go(0 hunks)pkg/cache/memory_edge_test.go(0 hunks)pkg/cache/memory_profile_test.go(0 hunks)pkg/cache/memory_test.go(0 hunks)pkg/cache/options.go(0 hunks)pkg/cache/redis.go(0 hunks)pkg/health/health.go(1 hunks)
💤 Files with no reviewable changes (14)
- pkg/cache/memory_test.go
- pkg/cache/memory_bench_test.go
- internal/sms-gateway/cache/factory.go
- pkg/cache/memory_profile_test.go
- pkg/cache/cache.go
- pkg/cache/memory_edge_test.go
- internal/sms-gateway/modules/messages/service.go
- internal/sms-gateway/cache/errors.go
- pkg/cache/memory_concurrency_test.go
- pkg/cache/errors.go
- pkg/cache/memory.go
- pkg/cache/redis.go
- pkg/cache/options.go
- internal/sms-gateway/cache/config.go
🚧 Files skipped from review as they are similar to previous changes (10)
- internal/sms-gateway/modules/messages/cache.go
- pkg/health/health.go
- go.mod
- internal/sms-gateway/modules/messages/module.go
- internal/sms-gateway/modules/auth/service.go
- internal/sms-gateway/modules/auth/passwords.go
- internal/sms-gateway/cache/module.go
- internal/sms-gateway/online/service.go
- internal/sms-gateway/app.go
- internal/sms-gateway/online/module.go
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: capcom6
Repo: android-sms-gateway/server PR: 178
File: pkg/cache/redis.go:63-75
Timestamp: 2025-09-10T23:53:42.006Z
Learning: In pkg/cache/redis.go, the Redis cache implementation uses Redis's native field-level TTL via HExpireAt, meaning expired fields are automatically removed by Redis itself. This differs from the memory implementation where the application tracks expiration times. As a result, operations like Drain that call HGetAll will only return non-expired items since expired fields are already removed by Redis.
Learnt from: capcom6
Repo: android-sms-gateway/server PR: 178
File: pkg/cache/redis.go:63-75
Timestamp: 2025-09-10T23:53:42.006Z
Learning: In pkg/cache/redis.go, the Redis cache implementation uses Redis's native field-level TTL via HExpireAt, meaning expired fields are automatically removed by Redis itself. This differs from the memory implementation where the application tracks expiration times. As a result, operations like Drain that call HGetAll will only return non-expired items since expired fields are already removed by Redis.
🧬 Code graph analysis (2)
internal/config/module.go (1)
internal/config/config.go (1)
Config(10-19)
internal/sms-gateway/modules/push/service.go (1)
internal/sms-gateway/cache/module.go (1)
Factory(9-9)
🪛 actionlint (1.7.9)
.github/workflows/go.yml
78-78: condition "false" is always evaluated to false. remove the if: section
(if-cond)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: E2E
- GitHub Check: Lint
- GitHub Check: Test
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
internal/sms-gateway/modules/push/service.go (1)
210-210: LGTM!The
cache.WithTTLoption is correctly imported from the externalcachefxpackage, maintaining the same TTL behavior for blacklisted tokens.internal/config/module.go (1)
17-17: New cachefx dependency importImporting
github.com/go-core-fx/cachefxhere matches the PR’s goal of externalizing the cache implementation. Just ensure go.mod/go.sum are updated consistently and thatgo mod tidyhas been run so this import lines up with the resolved version.
capcom6
force-pushed
the
deps/external-cache-impl
branch
2 times, most recently
from
aec5654 to
034bb76
Compare
capcom6
force-pushed
the
deps/external-cache-impl
branch
from
034bb76 to
8e0618c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
.github/workflows/go.yml (1)
78-78: Duplicate: Benchmark job is permanently disabled.This issue has already been flagged in a previous review. The job with
if: falseshould either be removed or replaced with a clear explanatory comment.internal/sms-gateway/cache/module.go (1)
14-14: Duplicate: Logger pattern inconsistency across modules.This has been flagged previously. The codebase shows inconsistency: some modules use
logger.WithNamedLogger("name")(e.g., messages module) while others usefx.Decorate(func(log *zap.Logger) *zap.Logger { return log.Named("name") })(e.g., online module). Consider standardizing on one pattern for consistency.
🧹 Nitpick comments (1)
internal/sms-gateway/cache/module.go (1)
15-17: Consider aligning factory name with module name.The factory is named
"sms-gateway"viaWithName("sms-gateway"), but the fx module is named"cache"(line 13). For consistency and clarity, consider usingfactory.WithName("cache")unless there's a specific reason for the"sms-gateway"namespace.Apply this diff if the naming should match the module:
- fx.Provide(func(factory cachefx.Factory) Factory { - return factory.WithName("sms-gateway") - }), + fx.Provide(func(factory cachefx.Factory) Factory { + return factory.WithName("cache") + }),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (25)
.github/workflows/go.yml(1 hunks)go.mod(2 hunks)internal/config/module.go(2 hunks)internal/sms-gateway/app.go(2 hunks)internal/sms-gateway/cache/config.go(0 hunks)internal/sms-gateway/cache/errors.go(0 hunks)internal/sms-gateway/cache/factory.go(0 hunks)internal/sms-gateway/cache/module.go(1 hunks)internal/sms-gateway/modules/messages/cache.go(1 hunks)internal/sms-gateway/modules/messages/module.go(2 hunks)internal/sms-gateway/modules/push/service.go(3 hunks)internal/sms-gateway/online/module.go(2 hunks)internal/sms-gateway/online/service.go(1 hunks)internal/sms-gateway/users/cache.go(1 hunks)internal/sms-gateway/users/service.go(1 hunks)pkg/cache/cache.go(0 hunks)pkg/cache/errors.go(0 hunks)pkg/cache/memory.go(0 hunks)pkg/cache/memory_bench_test.go(0 hunks)pkg/cache/memory_concurrency_test.go(0 hunks)pkg/cache/memory_edge_test.go(0 hunks)pkg/cache/memory_profile_test.go(0 hunks)pkg/cache/memory_test.go(0 hunks)pkg/cache/options.go(0 hunks)pkg/cache/redis.go(0 hunks)
💤 Files with no reviewable changes (13)
- internal/sms-gateway/cache/errors.go
- pkg/cache/memory_profile_test.go
- pkg/cache/memory_edge_test.go
- pkg/cache/memory_bench_test.go
- pkg/cache/errors.go
- pkg/cache/memory.go
- pkg/cache/redis.go
- internal/sms-gateway/cache/factory.go
- pkg/cache/options.go
- internal/sms-gateway/cache/config.go
- pkg/cache/cache.go
- pkg/cache/memory_concurrency_test.go
- pkg/cache/memory_test.go
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/sms-gateway/modules/push/service.go
- internal/sms-gateway/online/service.go
- internal/sms-gateway/modules/messages/cache.go
- internal/config/module.go
- internal/sms-gateway/app.go
- internal/sms-gateway/modules/messages/module.go
- internal/sms-gateway/online/module.go
- go.mod
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-10T23:45:45.502Z
Learnt from: capcom6
Repo: android-sms-gateway/server PR: 178
File: internal/sms-gateway/online/module.go:14-16
Timestamp: 2025-09-10T23:45:45.502Z
Learning: fx.Decorate in uber-go/fx creates module-scoped decorated dependencies, not global modifications. Using fx.Decorate(func(log *zap.Logger) *zap.Logger { return log.Named("module_name") }) is a correct pattern for creating module-specific loggers that are scoped only to that module's dependency graph.
Applied to files:
internal/sms-gateway/cache/module.go
🧬 Code graph analysis (1)
internal/sms-gateway/cache/module.go (3)
internal/sms-gateway/app.go (1)
Module(36-65)internal/sms-gateway/modules/messages/module.go (1)
Module(11-26)internal/sms-gateway/online/module.go (1)
Module(12-37)
🪛 actionlint (1.7.9)
.github/workflows/go.yml
78-78: condition "false" is always evaluated to false. remove the if: section
(if-cond)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test
- GitHub Check: Lint
- GitHub Check: E2E
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
internal/sms-gateway/users/service.go (1)
8-8: LGTM! Import path updated consistently.The cache import path has been updated to use the external cachefx library, consistent with the migration in
cache.go. The usage ofcache.ErrKeyNotFoundon line 71 should be covered by the API compatibility verification requested forusers/cache.go.internal/sms-gateway/cache/module.go (2)
4-5: LGTM! Imports aligned with cachefx migration.The new imports for
cachefxandloggerare correctly added to support the external cache implementation.
9-9: LGTM! Type alias provides clean module boundary.The
Factorytype alias appropriately exposescachefx.Factoryas this module's public API, maintaining clear dependency boundaries with other modules.internal/sms-gateway/users/cache.go (1)
12-12: API compatibility confirmed - cachefx migration is sound.The external library
github.com/go-core-fx/cachefx/cacheprovides all required APIs:cache.Typed,cache.NewTyped,cache.AndSetTTL,cache.WithTTL,cache.ErrKeyNotFound, andcache.ErrKeyExpired. TheloginCacheWrappertype correctly implements the requiredIteminterface withMarshal()andUnmarshal()methods, making the migration compatible.
capcom6
force-pushed
the
deps/external-cache-impl
branch
from
8e0618c to
dc6c5bc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/go.yml (1)
75-82: Remove the permanently disabledbenchmarkjob (or document + gate it properly).
if: falseat Line 78 makes this job un-runnable; either delete the whole job block, or replace with a real condition (e.g.,workflow_dispatchinput / label / schedule) and add a brief comment explaining when it should run.
🧹 Nitpick comments (2)
internal/sms-gateway/cache/module.go (1)
15-17: Clarify the factory naming inconsistency.The module is named "cache" and the logger is named "cache", but the factory is named "sms-gateway". This creates confusion about the purpose and scope of the factory. Consider using
factory.WithName("cache")for consistency, or document why "sms-gateway" is the appropriate namespace for this factory.internal/sms-gateway/online/module.go (1)
15-16: Consider migrating to the logger.WithNamedLogger pattern for consistency.This module still uses
fx.Decorate(func(log *zap.Logger) *zap.Logger { return log.Named("online") }), while other modules in this PR (cache, messages) uselogger.WithNamedLogger("online"). Both patterns are functionally equivalent, but consistency across the codebase improves maintainability.Apply this diff to align with the emerging pattern:
func Module() fx.Option { return fx.Module( "online", - fx.Decorate(func(log *zap.Logger) *zap.Logger { - return log.Named("online") - }), + logger.WithNamedLogger("online"), fx.Provide(func(factory appCache.Factory) (cache.Cache, error) { return factory.New("online") }, fx.Private),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (26)
.github/workflows/go.yml(1 hunks)go.mod(2 hunks)internal/config/module.go(2 hunks)internal/sms-gateway/app.go(2 hunks)internal/sms-gateway/cache/config.go(0 hunks)internal/sms-gateway/cache/errors.go(0 hunks)internal/sms-gateway/cache/factory.go(0 hunks)internal/sms-gateway/cache/module.go(1 hunks)internal/sms-gateway/modules/messages/cache.go(1 hunks)internal/sms-gateway/modules/messages/module.go(2 hunks)internal/sms-gateway/modules/push/service.go(3 hunks)internal/sms-gateway/online/module.go(2 hunks)internal/sms-gateway/online/service.go(1 hunks)internal/sms-gateway/users/cache.go(1 hunks)internal/sms-gateway/users/service.go(1 hunks)pkg/cache/cache.go(0 hunks)pkg/cache/errors.go(0 hunks)pkg/cache/memory.go(0 hunks)pkg/cache/memory_bench_test.go(0 hunks)pkg/cache/memory_concurrency_test.go(0 hunks)pkg/cache/memory_edge_test.go(0 hunks)pkg/cache/memory_profile_test.go(0 hunks)pkg/cache/memory_test.go(0 hunks)pkg/cache/options.go(0 hunks)pkg/cache/redis.go(0 hunks)pkg/cache/typed.go(0 hunks)
💤 Files with no reviewable changes (14)
- internal/sms-gateway/cache/errors.go
- pkg/cache/memory_concurrency_test.go
- pkg/cache/memory_test.go
- pkg/cache/errors.go
- pkg/cache/cache.go
- pkg/cache/redis.go
- internal/sms-gateway/cache/config.go
- pkg/cache/options.go
- pkg/cache/typed.go
- pkg/cache/memory_bench_test.go
- pkg/cache/memory.go
- pkg/cache/memory_edge_test.go
- pkg/cache/memory_profile_test.go
- internal/sms-gateway/cache/factory.go
✅ Files skipped from review due to trivial changes (1)
- internal/sms-gateway/online/service.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/sms-gateway/modules/push/service.go
- internal/sms-gateway/modules/messages/cache.go
- internal/sms-gateway/app.go
- internal/sms-gateway/users/service.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-10T23:45:45.502Z
Learnt from: capcom6
Repo: android-sms-gateway/server PR: 178
File: internal/sms-gateway/online/module.go:14-16
Timestamp: 2025-09-10T23:45:45.502Z
Learning: fx.Decorate in uber-go/fx creates module-scoped decorated dependencies, not global modifications. Using fx.Decorate(func(log *zap.Logger) *zap.Logger { return log.Named("module_name") }) is a correct pattern for creating module-specific loggers that are scoped only to that module's dependency graph.
Applied to files:
internal/sms-gateway/cache/module.gointernal/sms-gateway/online/module.go
🧬 Code graph analysis (4)
internal/sms-gateway/cache/module.go (3)
internal/sms-gateway/app.go (1)
Module(37-67)internal/sms-gateway/modules/messages/module.go (1)
Module(11-26)internal/sms-gateway/online/module.go (1)
Module(12-37)
internal/sms-gateway/modules/messages/module.go (2)
internal/sms-gateway/cache/module.go (1)
Factory(9-9)internal/config/config.go (1)
Cache(79-81)
internal/sms-gateway/online/module.go (2)
internal/sms-gateway/cache/module.go (1)
Factory(9-9)internal/config/config.go (1)
Cache(79-81)
internal/config/module.go (2)
internal/config/config.go (1)
Config(12-23)internal/sms-gateway/pubsub/config.go (1)
Config(4-7)
🪛 actionlint (1.7.9)
.github/workflows/go.yml
78-78: condition "false" is always evaluated to false. remove the if: section
(if-cond)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Lint
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
internal/sms-gateway/users/cache.go (1)
12-12: LGTM!The import path change is straightforward and maintains the same API usage throughout the file.
internal/sms-gateway/modules/messages/module.go (1)
6-6: LGTM!The provider correctly uses the internal factory to create a cache instance that implements the external cache interface. The pattern aligns with the migration strategy.
Also applies to: 15-17
internal/config/module.go (1)
126-130: LGTM!The configuration provider correctly migrates to the external cachefx.Config type while maintaining the same configuration structure.
internal/sms-gateway/online/module.go (1)
18-20: LGTM!The factory provider correctly uses the internal cache factory alias to create a cache instance for the online module.
go.mod (2)
28-28: Zap upgrade to v1.27.1 is safe. The patch version includes only bug fixes: prevention of Object panicking on nils and a race condition fix in WithLazy. No breaking changes are introduced.
24-24: Safe to upgrade; includes critical bug fix in connection pool management.The upgrade from v9.9.0 to v9.17.2 introduces no breaking changes. v9.17.2 fixes a critical race condition in the connection pool that could cause connection leaks when dial goroutines complete after request timeout, and improves context timeout calculation. No new security vulnerabilities are introduced in this version range.
capcom6
force-pushed
the
deps/external-cache-impl
branch
from
a31dc1b to
de73ae7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/go.yml (1)
78-78: Duplicate: Benchmark job permanently disabled.This concern has already been raised in a previous review comment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (29)
.github/workflows/go.yml(1 hunks)go.mod(2 hunks)internal/config/module.go(2 hunks)internal/sms-gateway/app.go(2 hunks)internal/sms-gateway/cache/config.go(0 hunks)internal/sms-gateway/cache/errors.go(0 hunks)internal/sms-gateway/cache/factory.go(0 hunks)internal/sms-gateway/cache/module.go(1 hunks)internal/sms-gateway/modules/messages/cache.go(1 hunks)internal/sms-gateway/modules/messages/module.go(2 hunks)internal/sms-gateway/modules/push/service.go(3 hunks)internal/sms-gateway/online/module.go(2 hunks)internal/sms-gateway/online/service.go(1 hunks)internal/sms-gateway/otp/module.go(2 hunks)internal/sms-gateway/otp/service.go(1 hunks)internal/sms-gateway/otp/storage.go(1 hunks)internal/sms-gateway/users/cache.go(1 hunks)internal/sms-gateway/users/service.go(1 hunks)pkg/cache/cache.go(0 hunks)pkg/cache/errors.go(0 hunks)pkg/cache/memory.go(0 hunks)pkg/cache/memory_bench_test.go(0 hunks)pkg/cache/memory_concurrency_test.go(0 hunks)pkg/cache/memory_edge_test.go(0 hunks)pkg/cache/memory_profile_test.go(0 hunks)pkg/cache/memory_test.go(0 hunks)pkg/cache/options.go(0 hunks)pkg/cache/redis.go(0 hunks)pkg/cache/typed.go(0 hunks)
💤 Files with no reviewable changes (14)
- pkg/cache/memory_test.go
- pkg/cache/memory_edge_test.go
- pkg/cache/memory.go
- pkg/cache/errors.go
- pkg/cache/cache.go
- internal/sms-gateway/cache/errors.go
- internal/sms-gateway/cache/factory.go
- pkg/cache/typed.go
- pkg/cache/redis.go
- pkg/cache/memory_bench_test.go
- internal/sms-gateway/cache/config.go
- pkg/cache/options.go
- pkg/cache/memory_profile_test.go
- pkg/cache/memory_concurrency_test.go
🚧 Files skipped from review as they are similar to previous changes (11)
- internal/sms-gateway/modules/messages/module.go
- internal/sms-gateway/online/module.go
- internal/sms-gateway/cache/module.go
- internal/sms-gateway/online/service.go
- internal/sms-gateway/app.go
- internal/sms-gateway/users/service.go
- internal/config/module.go
- internal/sms-gateway/users/cache.go
- go.mod
- internal/sms-gateway/modules/messages/cache.go
- internal/sms-gateway/otp/storage.go
🧰 Additional context used
🧬 Code graph analysis (2)
internal/sms-gateway/modules/push/service.go (1)
internal/sms-gateway/cache/module.go (1)
Factory(9-9)
internal/sms-gateway/otp/module.go (2)
internal/sms-gateway/cache/module.go (1)
Factory(9-9)internal/config/config.go (1)
Cache(79-81)
🪛 actionlint (1.7.9)
.github/workflows/go.yml
78-78: condition "false" is always evaluated to false. remove the if: section
(if-cond)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: E2E
- GitHub Check: Lint
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
internal/sms-gateway/modules/push/service.go (2)
8-10: LGTM: Import reorganization supports cache migration.The import aliasing appropriately distinguishes between the internal factory (cacheFactory) and external cache operations, facilitating the migration to the external library.
43-49: LGTM: Constructor signature updated correctly.The factory parameter type is correctly updated to use the aliased cacheFactory.Factory, maintaining consistency with the import changes.
internal/sms-gateway/otp/module.go (1)
10-23: LGTM: Module wiring correctly migrated.The fx module properly integrates with cachefx.Factory, creating a namespaced cache instance via
factory.New("otp"). The dependency injection pattern is correctly maintained.internal/sms-gateway/otp/service.go (1)
10-10: LGTM: Import path updated correctly.The cache import has been cleanly migrated to the external cachefx library. The usage of
cache.WithValidUntilat line 72 remains unchanged, demonstrating API compatibility.
Summary by CodeRabbit
Chores
Breaking Changes
✏️ Tip: You can customize this high-level summary in your review settings.