feat: remove provider from Redis queues when availability is set to false #547
Conversation
- Add functionality to remove providers from Redis queues when their availability is set to false. - Introduce a new method, removeProviderFromRedisQueues, to handle the removal process. - Enhance tests to verify correct behavior when updating provider availability, including scenarios for both true and false settings. - Ensure graceful handling of Redis errors during profile updates. This update improves the management of provider data in Redis, ensuring that inactive providers are properly removed from relevant queues.
WalkthroughAdds ProfileController.removeProviderFromRedisQueues and calls it from UpdateProviderProfile after a successful DB commit when IsAvailable is set to false; introduces Redis-backed tests (miniredis) covering removal, no-op, error-tolerance, and absent-provider scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant PC as ProfileController
participant DB as Database
participant R as Redis
C->>PC: PATCH /settings/provider (IsAvailable toggled)
PC->>DB: Update provider profile (tx)
DB-->>PC: Commit success
alt IsAvailable set to false
note over PC,R #DFF2DF: Post-commit Redis cleanup (new)
PC->>R: removeProviderFromRedisQueues(providerID, currency)
alt Removal succeeds
R-->>PC: OK
PC-->>C: 200 OK
else Removal error
PC->>PC: Log error (non-blocking)
PC-->>C: 200 OK
end
else IsAvailable true or unchanged
PC-->>C: 200 OK
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
controllers/accounts/profile.go(2 hunks)controllers/accounts/profile_test.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
controllers/accounts/profile_test.go (8)
storage/redis.go (1)
RedisClient(13-13)ent/providerprofile/where.go (2)
IDEQ(19-21)ID(14-16)ent/provisionbucket/where.go (4)
IDEQ(20-22)ID(15-17)MinAmount(60-62)MaxAmount(65-67)ent/providercurrencies_query.go (1)
ProviderCurrenciesQuery(22-34)types/types.go (1)
ProviderProfilePayload(265-272)ent/providercurrencies/where.go (1)
IsAvailable(76-78)utils/token/token.go (1)
GenerateAccessJWT(21-29)utils/test/test.go (1)
PerformRequest(14-25)
controllers/accounts/profile.go (8)
ent/providerprofile/where.go (2)
ID(14-16)IDEQ(19-21)utils/logger/logger.go (2)
WithFields(76-109)Fields(73-73)storage/database.go (1)
Client(21-21)ent/providercurrencies_query.go (1)
ProviderCurrenciesQuery(22-34)ent/provisionbucket_query.go (1)
ProvisionBucketQuery(24-37)ent/providercurrencies.go (2)
ProviderCurrencies(20-40)ProviderCurrencies(76-97)ent/schema/providercurrencies.go (4)
ProviderCurrencies(15-17)ProviderCurrencies(20-36)ProviderCurrencies(39-50)ProviderCurrencies(53-58)storage/redis.go (1)
RedisClient(13-13)
⏰ 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). (1)
- GitHub Check: build-and-test
- Add currency-specific filtering to prevent removing provider from all currencies - Fix Redis client management in tests to prevent connection pollution - Update removeProviderFromRedisQueues to accept currencyCode parameter
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
controllers/accounts/profile.go (1)
872-931: Consider simplifying the nested loop structure.The outer loop over
ProviderCurrenciesappears redundant since it only validates that the currency exists for the provider, but the actual filtering happens in the inner loop overProvisionBuckets. You could streamline this by iterating directly overprovider.Edges.ProvisionBucketsand filtering by currency code, eliminating the outer loop entirely.Apply this diff to simplify the logic:
- // Remove provider from Redis queues for the specific currency only - for _, pc := range provider.Edges.ProviderCurrencies { - if pc.Edges.Currency == nil || pc.Edges.Currency.Code != currencyCode { - continue - } - for _, bucket := range provider.Edges.ProvisionBuckets { - if bucket.Edges.Currency != nil && bucket.Edges.Currency.Code == currencyCode { + // Remove provider from Redis queues for the specific currency only + for _, bucket := range provider.Edges.ProvisionBuckets { + if bucket.Edges.Currency != nil && bucket.Edges.Currency.Code == currencyCode { + redisKey := fmt.Sprintf("bucket_%s_%s_%s", bucket.Edges.Currency.Code, bucket.MinAmount, bucket.MaxAmount) + + // Remove provider from Redis queue using the provided algorithm + for index := -1; ; index-- { + providerData, err := storage.RedisClient.LIndex(ctx, redisKey, int64(index)).Result() + if err != nil { + break + } + + parts := strings.Split(providerData, ":") + if len(parts) != 5 { + logger.Errorf("invalid provider data format: %s", providerData) + continue + } + + if parts[0] == providerID { + placeholder := "DELETED_PROVIDER" + _, err := storage.RedisClient.LSet(ctx, redisKey, int64(index), placeholder).Result() + if err != nil { + logger.Errorf("failed to set placeholder at index %d: %v", index, err) + } + + _, err = storage.RedisClient.LRem(ctx, redisKey, 0, placeholder).Result() + if err != nil { + logger.Errorf("failed to remove placeholder from circular queue: %v", err) + } + + break + } + } + } + }Alternatively, if the intent is to fail fast when the provider doesn't have the specified currency configured, add an early validation check before iterating buckets:
// Verify provider has the specified currency configured hasCurrency := false for _, pc := range provider.Edges.ProviderCurrencies { if pc.Edges.Currency != nil && pc.Edges.Currency.Code == currencyCode { hasCurrency = true break } } if !hasCurrency { return fmt.Errorf("provider does not have currency %s configured", currencyCode) } // Process buckets for the specified currency for _, bucket := range provider.Edges.ProvisionBuckets { // ... rest of logic }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
controllers/accounts/profile.go(2 hunks)controllers/accounts/profile_test.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
controllers/accounts/profile_test.go (5)
storage/redis.go (1)
RedisClient(13-13)storage/database.go (1)
Client(21-21)types/types.go (1)
ProviderProfilePayload(265-272)utils/token/token.go (1)
GenerateAccessJWT(21-29)utils/test/test.go (1)
PerformRequest(14-25)
controllers/accounts/profile.go (9)
ent/providerprofile/where.go (2)
ID(14-16)IDEQ(19-21)utils/logger/logger.go (2)
WithFields(76-109)Fields(73-73)storage/database.go (1)
Client(21-21)ent/providerprofile.go (2)
ProviderProfile(20-43)ProviderProfile(136-153)ent/provisionbucket_query.go (1)
ProvisionBucketQuery(24-37)ent/providercurrencies.go (2)
ProviderCurrencies(20-40)ProviderCurrencies(76-97)ent/schema/providercurrencies.go (4)
ProviderCurrencies(15-17)ProviderCurrencies(20-36)ProviderCurrencies(39-50)ProviderCurrencies(53-58)ent/provisionbucket/where.go (2)
MinAmount(60-62)MaxAmount(65-67)storage/redis.go (1)
RedisClient(13-13)
⏰ 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). (1)
- GitHub Check: build-and-test
🔇 Additional comments (4)
controllers/accounts/profile.go (2)
624-634: LGTM! Redis removal is correctly integrated.The call to
removeProviderFromRedisQueuesis appropriately placed after the transaction commit and only executes when availability is being set to false. Redis errors are logged but don't prevent the profile update from succeeding, which aligns with the requirement that Redis failures should be non-blocking.
899-925: Verify behavior for duplicate provider entries.The current implementation breaks after removing the first occurrence of the provider from each bucket's queue. If duplicate entries exist in the same Redis queue (perhaps from a previous bug or race condition), only the first occurrence will be removed.
Confirm whether this single-removal behavior is intentional or if all occurrences should be removed. If all occurrences should be removed, modify the logic to continue searching instead of breaking:
if parts[0] == providerID { placeholder := "DELETED_PROVIDER" _, err := storage.RedisClient.LSet(ctx, redisKey, int64(index), placeholder).Result() if err != nil { logger.Errorf("failed to set placeholder at index %d: %v", index, err) } _, err = storage.RedisClient.LRem(ctx, redisKey, 0, placeholder).Result() if err != nil { logger.Errorf("failed to remove placeholder from circular queue: %v", err) } - - break } }Note: Removing the
breakwill cause the loop to continue iterating backwards through the entire list, removing all occurrences. Consider performance implications if queues are large.controllers/accounts/profile_test.go (2)
860-1187: Excellent test coverage!The test suite comprehensively covers the Redis queue removal feature with four well-structured scenarios:
- ✅ Happy path: Provider removed when
IsAvailable=false- ✅ Negative case: Provider preserved when
IsAvailable=true- ✅ Error resilience: Profile update succeeds despite Redis failures
- ✅ Edge case: Graceful handling when provider not in queue
The Redis client restoration pattern (
prevClient := db.RedisClient+ defer) correctly addresses the past review concern and prevents connection leakage between subtests.
919-920: Tests correctly mirror production Redis enqueue format
Production code usesfmt.Sprintf("%s:%s:%s:%s:%s", provider.ID, token.Symbol, rate, minAmount, maxAmount), matching the test’sproviderID:USDC:550:minAmount:maxAmountconstruction.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
controllers/accounts/profile_test.go (1)
860-1187: Consider extracting common setup logic into helper functions.The four subtests share significant setup code (miniredis initialization, Redis client creation, currency fetching, bucket creation). While test isolation and clarity often justify some duplication, extracting the common setup into helper functions could improve maintainability without sacrificing readability.
For example:
setupTestRedis(t) (*miniredis.Miniredis, *redis.Client, func())for Redis setup and cleanupgetProviderCurrency(ctx, providerID) (*ent.FiatCurrency, error)for currency fetchingcreateTestBucket(ctx, currency, min, max) (*ent.ProvisionBucket, error)for bucket creationThis refactoring can be deferred if test clarity is prioritized.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
controllers/accounts/profile_test.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
controllers/accounts/profile_test.go (9)
storage/redis.go (1)
RedisClient(13-13)storage/database.go (1)
Client(21-21)ent/providerprofile/where.go (2)
IDEQ(19-21)ID(14-16)ent/providercurrencies_query.go (1)
ProviderCurrenciesQuery(22-34)ent/providercurrencies.go (2)
ProviderCurrencies(20-40)ProviderCurrencies(76-97)ent/provisionbucket.go (2)
ProvisionBucket(19-34)ProvisionBucket(79-96)types/types.go (1)
ProviderProfilePayload(265-272)utils/token/token.go (1)
GenerateAccessJWT(21-29)utils/test/test.go (1)
PerformRequest(14-25)
⏰ 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). (1)
- GitHub Check: build-and-test
🔇 Additional comments (4)
controllers/accounts/profile_test.go (4)
6-6: LGTM!The new imports (
fmt,miniredis,redis) are appropriate for the Redis queue removal tests.Also applies to: 19-19, 33-33
861-964: LGTM!This test correctly verifies that providers are removed from Redis queues when
IsAvailableis set to false. The test setup properly:
- Uses the actual currency code from the database in both Redis keys and provider data
- Adds providers to multiple bucket queues
- Verifies removal from all relevant queues while preserving other providers
1047-1112: LGTM!This test correctly verifies that Redis errors are handled gracefully without blocking the profile update. Closing the Redis client before the update simulates a realistic failure scenario, and the assertion that the profile update still returns HTTP 200 confirms the non-blocking requirement from the PR acceptance criteria.
916-920: All Redis key and data formats match production code Thebucket_%s_%s_%skey andproviderID:currencyCode:rate:minAmount:maxAmountdata prefix used in tests align withremoveProviderFromRedisQueuesinprofile.go(which only inspectsparts[0]).
| providerData := testCtx.providerProfile.ID + ":USDC:550:" + bucket.MinAmount.String() + ":" + bucket.MaxAmount.String() | ||
| redisClient.RPush(ctx, redisKey, providerData, "other_provider:USDC:550:"+bucket.MinAmount.String()+":"+bucket.MaxAmount.String()) |
There was a problem hiding this comment.
Fix currency code inconsistency in provider data.
The provider data hardcodes "USDC" while the Redis key (line 1013) uses currency.Code (which is "KES" from the database). This creates a mismatch between the key and the data format that doesn't reflect production usage. Test 1 correctly uses currency.Code in the provider data (line 919), and this test should follow the same pattern.
Apply this diff to use the actual currency code:
- providerData := testCtx.providerProfile.ID + ":USDC:550:" + bucket.MinAmount.String() + ":" + bucket.MaxAmount.String()
- redisClient.RPush(ctx, redisKey, providerData, "other_provider:USDC:550:"+bucket.MinAmount.String()+":"+bucket.MaxAmount.String())
+ providerData := testCtx.providerProfile.ID + ":" + currency.Code + ":550:" + bucket.MinAmount.String() + ":" + bucket.MaxAmount.String()
+ redisClient.RPush(ctx, redisKey, providerData, "other_provider:"+currency.Code+":550:"+bucket.MinAmount.String()+":"+bucket.MaxAmount.String())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| providerData := testCtx.providerProfile.ID + ":USDC:550:" + bucket.MinAmount.String() + ":" + bucket.MaxAmount.String() | |
| redisClient.RPush(ctx, redisKey, providerData, "other_provider:USDC:550:"+bucket.MinAmount.String()+":"+bucket.MaxAmount.String()) | |
| providerData := testCtx.providerProfile.ID + ":" + currency.Code + ":550:" + bucket.MinAmount.String() + ":" + bucket.MaxAmount.String() | |
| redisClient.RPush(ctx, redisKey, providerData, "other_provider:"+currency.Code+":550:"+bucket.MinAmount.String()+":"+bucket.MaxAmount.String()) |
🤖 Prompt for AI Agents
In controllers/accounts/profile_test.go around lines 1014 to 1015, the provider
data string hardcodes "USDC" while the Redis key uses currency.Code (e.g.,
"KES"), causing inconsistency; update the test to use currency.Code instead of
the literal "USDC" for both providerData and the second argument passed to
redisClient.RPush so the currency in the pushed provider data matches the key
and mirrors the pattern used earlier in the file.
|
|
||
| // Create Redis queue with different provider (provider not found scenario) | ||
| redisKey := fmt.Sprintf("bucket_%s_%s_%s", currency.Code, bucket.MinAmount, bucket.MaxAmount) | ||
| redisClient.RPush(ctx, redisKey, "other_provider:USDC:550:"+bucket.MinAmount.String()+":"+bucket.MaxAmount.String()) |
There was a problem hiding this comment.
Fix currency code inconsistency in provider data.
Similar to the issue in test 2, this line hardcodes "USDC" while the Redis key (line 1161) uses currency.Code (which is "KES"). Even though this test is verifying the "provider not found" scenario, the data format should remain consistent with production usage.
Apply this diff:
- redisClient.RPush(ctx, redisKey, "other_provider:USDC:550:"+bucket.MinAmount.String()+":"+bucket.MaxAmount.String())
+ redisClient.RPush(ctx, redisKey, "other_provider:"+currency.Code+":550:"+bucket.MinAmount.String()+":"+bucket.MaxAmount.String())📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| redisClient.RPush(ctx, redisKey, "other_provider:USDC:550:"+bucket.MinAmount.String()+":"+bucket.MaxAmount.String()) | |
| redisClient.RPush(ctx, redisKey, "other_provider:"+currency.Code+":550:"+bucket.MinAmount.String()+":"+bucket.MaxAmount.String()) |
🤖 Prompt for AI Agents
In controllers/accounts/profile_test.go around line 1162, the test pushes
provider data with a hardcoded "USDC" while the Redis key uses currency.Code
("KES"); update the RPush payload to use the same currency.Code variable (e.g.,
build the string with currency.Code instead of the literal "USDC") so the test
data format matches production usage and remains consistent with the Redis key.
2 participants
Description
This PR implements automatic Redis queue management for provider profiles when their availability status changes. The feature ensures that providers are immediately removed from all relevant Redis queues when their
IsAvailablestatus is set tofalse, preventing the system from routing orders to unavailable providers.Key Changes:
removeProviderFromRedisQueues()function to handle Redis queue removalUpdateProviderProfilefunctionTechnical Implementation:
Impact:
References
This PR addresses the user story: #435
Acceptance Criteria Met:
IsAvailableis set tofalseTesting
Test Coverage Added:
IsAvailableset tofalseIsAvailableset totrueManual Testing Steps:
IsAvailable: falseChecklist
mainFiles Modified:
controllers/accounts/profile.go- Added Redis queue removal logiccontrollers/accounts/profile_test.go- Added comprehensive test coverageKey Features:
By submitting a PR, I agree to Paycrest's Contributor Code of Conduct and Contribution Guide.
Summary by CodeRabbit
New Features
Tests