Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
Contributor
📝 WalkthroughWalkthrough
ChangesBilling API Timeout and Error Handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Core/Resgrid.Services/SubscriptionsService.cs`:
- Around line 101-128: The catch block in the method that fetches plan counts
currently returns new DepartmentPlanCount() on exception, which signals zero
usage to downstream code instead of indicating that billing data is unavailable.
To maintain fail-closed semantics and align with the signal that
LimitsService.ValidateDepartmentIsWithinLimitsAsync expects (null indicates
billing unavailable), change the catch block to return null instead of new
DepartmentPlanCount(). This ensures that billing outages trigger denial of
service rather than allowing operations with assumed zero usage.
- Around line 319-346: The catch block in the GetPlanByIdAsync method is
returning null when an exception occurs, which masks errors (like timeouts from
the Billing API) and makes them indistinguishable from a legitimate not-found
response. This causes callers like SubscriptionController to dereference the
plan object without null checks, resulting in crashes. Instead of catching the
exception and returning null, either rethrow the exception after logging it or
throw a more meaningful custom exception so that calling code can properly
handle actual errors versus genuinely missing plans.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b9aa7bcd-3cd8-47d4-ada7-cb40bed12947
📒 Files selected for processing (1)
Core/Resgrid.Services/SubscriptionsService.cs
Comment on lines
+319
to
+346
| try | ||
| { | ||
| MaxTimeout = 200000 // ms | ||
| }; | ||
| var options = new RestClientOptions(Config.SystemBehaviorConfig.BillingApiBaseUrl) | ||
| { | ||
| MaxTimeout = BillingApiTimeoutMs // ms | ||
| }; | ||
|
|
||
| var client = new RestClient(options, configureSerialization: s => s.UseNewtonsoftJson()); | ||
| var request = new RestRequest($"/api/Billing/GetPlanById", Method.Get); | ||
| request.AddHeader("X-API-Key", Config.ApiConfig.BackendInternalApikey); | ||
| request.AddHeader("Content-Type", "application/json"); | ||
| request.AddParameter("planId", planId, ParameterType.QueryString); | ||
| var client = new RestClient(options, configureSerialization: s => s.UseNewtonsoftJson()); | ||
| var request = new RestRequest($"/api/Billing/GetPlanById", Method.Get); | ||
| request.AddHeader("X-API-Key", Config.ApiConfig.BackendInternalApikey); | ||
| request.AddHeader("Content-Type", "application/json"); | ||
| request.AddParameter("planId", planId, ParameterType.QueryString); | ||
|
|
||
| var response = await client.ExecuteAsync<GetCurrentPlanForDepartmentResult>(request); | ||
| var response = await client.ExecuteAsync<GetCurrentPlanForDepartmentResult>(request); | ||
|
|
||
| if (response.StatusCode == HttpStatusCode.NotFound) | ||
| return null; | ||
| if (response.StatusCode == HttpStatusCode.NotFound) | ||
| return null; | ||
|
|
||
| if (response.Data == null) | ||
| return null; | ||
| if (response.Data == null) | ||
| return null; | ||
|
|
||
| return response.Data.Data; | ||
| return response.Data.Data; | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| Framework.Logging.LogException(ex); | ||
| return null; | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Avoid null fallback in GetPlanByIdAsync on Billing exceptions.
Line 345 returns null after catch; a known caller (SubscriptionController, Line 595-599 in provided context) dereferences plan.Frequency without a null guard, which can crash requests when Billing times out/errors.
Suggested fix
catch (Exception ex)
{
Framework.Logging.LogException(ex);
- return null;
+ return await _plansRepository.GetPlanByPlanIdAsync(planId);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Core/Resgrid.Services/SubscriptionsService.cs` around lines 319 - 346, The
catch block in the GetPlanByIdAsync method is returning null when an exception
occurs, which masks errors (like timeouts from the Billing API) and makes them
indistinguishable from a legitimate not-found response. This causes callers like
SubscriptionController to dereference the plan object without null checks,
resulting in crashes. Instead of catching the exception and returning null,
either rethrow the exception after logging it or throw a more meaningful custom
exception so that calling code can properly handle actual errors versus
genuinely missing plans.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Core/Resgrid.Services/SubscriptionsService.cs (2)
68-90: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDon’t convert Billing timeouts into a free-plan response.
With the new 5s timeout, an empty/timed-out Billing response still hits lines 82-90 and returns
freePlan, which masks an outage as a free-tier downgrade. PreserveNotFound => freePlan, but returnnullfor unusable payloads/exceptions so callers can fail safely.Proposed fix
if (response.StatusCode == HttpStatusCode.NotFound) return freePlan; if (response.Data == null || response.Data.Data == null) - return freePlan; + return null; return response.Data.Data; } catch (Exception ex) { Framework.Logging.LogException(ex); - return freePlan; + return null; }As per coding guidelines, “When
SubscriptionsService.GetCurrentPlanForDepartmentAsync()receives a null response from the Billing API, it returns null instead of a fallback free plan.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Core/Resgrid.Services/SubscriptionsService.cs` around lines 68 - 90, The GetCurrentPlanForDepartmentAsync method currently returns freePlan whenever response.Data is null (lines 82-84), which masks timeout and outage scenarios as legitimate free-tier responses. Modify the null check for response.Data and response.Data.Data to return null instead of freePlan, while keeping the NotFound status code check intact so that only actual 404 responses return freePlan. This distinction allows callers to properly handle failures versus legitimate free plan scenarios.Source: Coding guidelines
30-33: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftApply the shared timeout to every Billing API client.
This constant says every Billing call should use the short timeout, but many call sites below still create
RestClientdirectly withoutRestClientOptions(for example lines 536, 727, 812, 1069, 1182, and 1248). Those paths can still hang for the old default duration.Proposed direction
// Billing API calls must fail fast. A slow/unresponsive Billing API previously blocked the // request thread for up to 200s (e.g. the Subscription and User Dashboard pages would hang // for ~3 minutes). Keep every Billing call on this short, shared timeout. private const int BillingApiTimeoutMs = 5000; + + private static RestClient CreateBillingClient() + { + var options = new RestClientOptions(Config.SystemBehaviorConfig.BillingApiBaseUrl) + { + MaxTimeout = BillingApiTimeoutMs + }; + + return new RestClient(options, configureSerialization: s => s.UseNewtonsoftJson()); + }Then replace each
new RestClient(Config.SystemBehaviorConfig.BillingApiBaseUrl, ...)Billing call withCreateBillingClient().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Core/Resgrid.Services/SubscriptionsService.cs` around lines 30 - 33, The BillingApiTimeoutMs constant is defined but not consistently applied across all Billing API calls. Create a helper method CreateBillingClient() that instantiates a RestClient with the BillingApiTimeoutMs timeout configured via RestClientOptions, then replace all direct instantiations of new RestClient(Config.SystemBehaviorConfig.BillingApiBaseUrl, ...) throughout the file with calls to this new CreateBillingClient() method to ensure the short timeout is applied uniformly across all Billing API operations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Core/Resgrid.Services/SubscriptionsService.cs`:
- Around line 623-629: The current validation throws an error when payment.Plan
is null without attempting to load it first. Before throwing the
InvalidOperationException that checks if payment?.Plan == null or plan == null,
add logic to hydrate payment.Plan using payment.PlanId when payment exists but
payment.Plan is null. Use the same approach already employed elsewhere in the
file to fetch the plan by ID, then re-evaluate the null conditions only after
attempting to load the missing Plan navigation property, so that valid payment
records with missing navigations can still be processed rather than failing
immediately.
In `@Web/Resgrid.Web/Areas/User/Controllers/SubscriptionController.cs`:
- Around line 608-611: The GetPlanAddonByIdAsync method call can return null,
but model.PlanAddon is being dereferenced without a guard check before the
existing plan null guard. Add a null guard immediately after the
GetPlanAddonByIdAsync call to check if the returned add-on is null, and only
proceed with dereferencing model.PlanAddon when it is not null. This guard
should be placed before the existing plan null check that protects
plan.Frequency dereferencing.
- Around line 769-773: Add a null guard for the `session` object returned from
the `CreateStripeSessionForSub` method call, similar to the existing guard for
`plan`. After obtaining the session, check if it is null and return a StatusCode
500 response with an appropriate error message before the code attempts to
dereference `session.CustomerId` and `session.SessionId`. This will prevent a
NullReferenceException when the Stripe session creation fails or returns null.
---
Outside diff comments:
In `@Core/Resgrid.Services/SubscriptionsService.cs`:
- Around line 68-90: The GetCurrentPlanForDepartmentAsync method currently
returns freePlan whenever response.Data is null (lines 82-84), which masks
timeout and outage scenarios as legitimate free-tier responses. Modify the null
check for response.Data and response.Data.Data to return null instead of
freePlan, while keeping the NotFound status code check intact so that only
actual 404 responses return freePlan. This distinction allows callers to
properly handle failures versus legitimate free plan scenarios.
- Around line 30-33: The BillingApiTimeoutMs constant is defined but not
consistently applied across all Billing API calls. Create a helper method
CreateBillingClient() that instantiates a RestClient with the
BillingApiTimeoutMs timeout configured via RestClientOptions, then replace all
direct instantiations of new
RestClient(Config.SystemBehaviorConfig.BillingApiBaseUrl, ...) throughout the
file with calls to this new CreateBillingClient() method to ensure the short
timeout is applied uniformly across all Billing API operations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1c7cacea-f072-45d0-93a6-49fc9eb6f129
📒 Files selected for processing (2)
Core/Resgrid.Services/SubscriptionsService.csWeb/Resgrid.Web/Areas/User/Controllers/SubscriptionController.cs
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Web/Resgrid.Web/Areas/User/Controllers/SubscriptionController.cs (1)
789-793: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winConsider validating session.SessionId to fully address the earlier comment.
While the session null guard prevents NRE, the session object might still have a null or empty
SessionIdfield, which would break the client checkout flow when returned at line 802. The past review comment suggested checkingstring.IsNullOrWhiteSpace(session.SessionId)in addition to the null guard.Suggested enhancement for session validation
var session = await _subscriptionsService.CreateStripeSessionForSub(DepartmentId, stripeCustomerId, plan.GetExternalKey(), plan.PlanId, user.Email, department.Name, count, discountCode); -// CreateStripeSessionForSub returns null when the Billing API is unavailable / Stripe session -// creation fails. Fail with a clear error instead of NRE'ing on session.CustomerId/SessionId below. -if (session == null) - return StatusCode(StatusCodes.Status500InternalServerError, "Unable to start the checkout session. Please try again."); +// CreateStripeSessionForSub returns null when the Billing API is unavailable / Stripe session +// creation fails. Validate both session and SessionId before returning to client. +if (session == null || string.IsNullOrWhiteSpace(session.SessionId)) + return StatusCode(StatusCodes.Status500InternalServerError, "Unable to start the checkout session. Please try again.");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Web/Resgrid.Web/Areas/User/Controllers/SubscriptionController.cs` around lines 789 - 793, The session null check in the SubscriptionController prevents NRE but does not validate that the session object has a valid SessionId value. Add an additional check using string.IsNullOrWhiteSpace() to validate that session.SessionId is not null or empty in the same if condition that checks for null session. This ensures the session object both exists and contains the necessary SessionId before it is used later in the response, preventing the client checkout flow from breaking due to a missing session identifier.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@Web/Resgrid.Web/Areas/User/Controllers/SubscriptionController.cs`:
- Around line 789-793: The session null check in the SubscriptionController
prevents NRE but does not validate that the session object has a valid SessionId
value. Add an additional check using string.IsNullOrWhiteSpace() to validate
that session.SessionId is not null or empty in the same if condition that checks
for null session. This ensures the session object both exists and contains the
necessary SessionId before it is used later in the response, preventing the
client checkout flow from breaking due to a missing session identifier.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6634b22d-5c08-49a8-90c2-30d90e49f2a6
📒 Files selected for processing (2)
Core/Resgrid.Services/SubscriptionsService.csWeb/Resgrid.Web/Areas/User/Controllers/SubscriptionController.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- Core/Resgrid.Services/SubscriptionsService.cs
Member
Author
|
Approve |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit