Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (29)
📝 WalkthroughWalkthroughAdds a Stats API: new resource, interfaces, models, implementation, tests, example project, fixtures, and README/CHANGELOG updates to support fetching and grouping email sending statistics. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Account as AccountResource
participant Stats as StatsResource
participant HTTP as HttpClient
participant JSON as JsonParser
Client->>Account: Stats()
Account-->>Client: IStatsResource
Client->>Stats: GetStats(filter)
Stats->>Stats: BuildFilterUri(baseUri, filter)
Stats->>HTTP: GET /accounts/{id}/stats?...
HTTP-->>Stats: 200 OK + JSON
Stats->>JSON: Deserialize to SendingStats
JSON-->>Stats: SendingStats
Stats-->>Client: SendingStats
Client->>Stats: ByDomains(filter)
Stats->>HTTP: GET /accounts/{id}/stats/domains?...
HTTP-->>Stats: 200 OK + JSON array
Stats->>JSON: Map items -> SendingStatGroup[]
JSON-->>Stats: IList<SendingStatGroup>
Stats-->>Client: IList<SendingStatGroup>
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
267-310:⚠️ Potential issue | 🟡 MinorStats API missing from "Supported functionality & Examples" section.
The PR adds Stats API functionality (per PR objectives and AI summary), but the README's "Supported functionality & Examples" section doesn't include a Stats API entry or link to the Stats example (
examples/Mailtrap.Example.Stats). This creates an inconsistency with the AI summary which states "README updated with Stats API reference."Consider adding an entry similar to:
### Stats API - Email sending statistics – [`examples/Mailtrap.Example.Stats`](examples/Mailtrap.Example.Stats/)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 267 - 310, Add a "Stats API" entry to the "Supported functionality & Examples" section of README.md to reflect the new functionality; specifically, insert a header "### Stats API" and an item like "Email sending statistics – [`examples/Mailtrap.Example.Stats`](examples/Mailtrap.Example.Stats/)" alongside the other example links so the README lists examples/Mailtrap.Example.Stats and matches the PR summary. Ensure the new entry follows the same formatting as adjacent sections (e.g., other "###" headers and bullet link style).
🧹 Nitpick comments (2)
src/Mailtrap/Stats/StatsResource.cs (1)
67-74: Duplicate property access and missing error handling.
item.GetProperty(groupKey)is called twice per iteration, andGetProperty("stats")will throwKeyNotFoundExceptionif the JSON is malformed.Consider caching the property value and adding defensive handling:
♻️ Proposed refactor
return response.Select(item => { + var groupValue = item.GetProperty(groupKey); return new SendingStatGroup { Name = groupKey, - Value = item.GetProperty(groupKey).ValueKind == JsonValueKind.Number - ? item.GetProperty(groupKey).GetInt64() - : item.GetProperty(groupKey).GetString() ?? string.Empty, - Stats = item.GetProperty("stats").Deserialize<SendingStats>() ?? new SendingStats() + Value = groupValue.ValueKind == JsonValueKind.Number + ? groupValue.GetInt64() + : groupValue.GetString() ?? string.Empty, + Stats = item.TryGetProperty("stats", out var statsElement) + ? statsElement.Deserialize<SendingStats>() ?? new SendingStats() + : new SendingStats() }; }).ToList<SendingStatGroup>();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Mailtrap/Stats/StatsResource.cs` around lines 67 - 74, Cache the result of item.GetProperty(groupKey) into a local variable to avoid duplicate lookups when building each SendingStatGroup and use item.TryGetProperty("stats", out var statsProp) to defensively handle missing or malformed "stats" (fall back to new SendingStats()). Also ensure the value extraction checks prop.ValueKind once and reads either GetInt64() or GetString() with a string.Empty fallback; update the LINQ projection that constructs SendingStatGroup to use these cached locals.tests/Mailtrap.IntegrationTests/Stats/StatsIntegrationTests.cs (1)
29-68: Consider testing full query string or additional filter parameters.The test sets both
StartDateandEndDatebut the mock only verifies thestart_dateparameter via partial matching. IfBuildFilterUriproduced an incorrect key forend_date(or other parameters), this test would still pass.Consider either:
- Using exact query string matching to validate all parameters
- Adding test cases that exercise the array filter parameters (
SendingDomainIds,SendingStreams,Categories,EmailServiceProviders)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Mailtrap.IntegrationTests/Stats/StatsIntegrationTests.cs` around lines 29 - 68, The test Get_Success only asserts the presence of the start_date query parameter via ConfigureWithQueryAndCreateClient, leaving EndDate and other filter fields unverified; update the test to either (a) use exact query-string matching for the request URI returned by BuildFilterUri so both start_date and end_date (and any other keys) are validated, or (b) add additional test cases in StatsIntegrationTests (e.g., new tests calling Account(...).Stats().GetStats with StatsFilter values for SendingDomainIds, SendingStreams, Categories, EmailServiceProviders) that assert the full expected query string or parameters are present, using MockHttpMessageHandler/ConfigureWithQueryAndCreateClient to verify the complete set of query parameters rather than just the single start_date key.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Line 41: The README command referencing "dotnet add package Mailtrap -v 3.1.0
-s github-mailtrap" will fail unless a git tag matching the MinVerTagPrefix (v)
exists; create and push the git tag v3.1.0 before or at merge time, or update
the README to use an existing published version (or remove the explicit -v flag)
so the "dotnet add package" command resolves; look for the README line
containing the dotnet command and the project MinVer configuration
(MinVerTagPrefix = v) to make the change.
In `@src/Mailtrap.Abstractions/Stats/Models/StatsFilter.cs`:
- Around line 12-17: StatsFilter is constructible with empty StartDate/EndDate
which later breaks BuildFilterUri when it calls AppendQueryParameter; make the
date range required or validate it before building the URI. Update the
StatsFilter class so StartDate and EndDate cannot be empty (e.g., remove the
string.Empty defaults and force values via constructor or mark them required),
or add explicit validation in BuildFilterUri to throw a clear exception if
StartDate/EndDate are null/empty, and reference StartDate, EndDate, StatsFilter
and BuildFilterUri/AppendQueryParameter to locate where to enforce the check.
- Around line 27-37: The string filter collections (SendingStreams, Categories,
EmailServiceProviders) can contain characters that must be percent-encoded
before being appended to a query; update StatsResource.BuildFilterUri to
percent-encode each value passed to UriExtensions.AppendQueryParameter (e.g.,
via Uri.EscapeDataString or by calling an encoding helper) so spaces, +, &, etc.
are encoded, or alternatively update UriExtensions.AppendQueryParameter to
URL-encode the value it receives; locate usages in StatsResource.BuildFilterUri
and ensure every value from SendingStreams, Categories, and
EmailServiceProviders is encoded before appending.
- Around line 22-23: The SendingDomainIds property in StatsFilter is typed as
IList<int> which can't represent IDs above Int32.MaxValue; change the property
signature to use IList<long> and initialize it with a long-capable collection
(e.g., new List<long>()) so it matches the rest of the SDK (see
AccountResource/SendingDomain(long)) and preserves compatibility with existing
long domain IDs.
---
Outside diff comments:
In `@README.md`:
- Around line 267-310: Add a "Stats API" entry to the "Supported functionality &
Examples" section of README.md to reflect the new functionality; specifically,
insert a header "### Stats API" and an item like "Email sending statistics –
[`examples/Mailtrap.Example.Stats`](examples/Mailtrap.Example.Stats/)" alongside
the other example links so the README lists examples/Mailtrap.Example.Stats and
matches the PR summary. Ensure the new entry follows the same formatting as
adjacent sections (e.g., other "###" headers and bullet link style).
---
Nitpick comments:
In `@src/Mailtrap/Stats/StatsResource.cs`:
- Around line 67-74: Cache the result of item.GetProperty(groupKey) into a local
variable to avoid duplicate lookups when building each SendingStatGroup and use
item.TryGetProperty("stats", out var statsProp) to defensively handle missing or
malformed "stats" (fall back to new SendingStats()). Also ensure the value
extraction checks prop.ValueKind once and reads either GetInt64() or GetString()
with a string.Empty fallback; update the LINQ projection that constructs
SendingStatGroup to use these cached locals.
In `@tests/Mailtrap.IntegrationTests/Stats/StatsIntegrationTests.cs`:
- Around line 29-68: The test Get_Success only asserts the presence of the
start_date query parameter via ConfigureWithQueryAndCreateClient, leaving
EndDate and other filter fields unverified; update the test to either (a) use
exact query-string matching for the request URI returned by BuildFilterUri so
both start_date and end_date (and any other keys) are validated, or (b) add
additional test cases in StatsIntegrationTests (e.g., new tests calling
Account(...).Stats().GetStats with StatsFilter values for SendingDomainIds,
SendingStreams, Categories, EmailServiceProviders) that assert the full expected
query string or parameters are present, using
MockHttpMessageHandler/ConfigureWithQueryAndCreateClient to verify the complete
set of query parameters rather than just the single start_date key.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: be8d7ef4-308f-4141-bb71-4bffecc0f6f3
📒 Files selected for processing (29)
CHANGELOG.mdMailtrap.slnREADME.mdexamples/Mailtrap.Example.Stats/Mailtrap.Example.Stats.csprojexamples/Mailtrap.Example.Stats/Properties/launchSettings.jsonexamples/Mailtrap.Example.Stats/Stats.csexamples/Mailtrap.Example.Stats/appsettings.jsonsrc/Mailtrap.Abstractions/Accounts/IAccountResource.cssrc/Mailtrap.Abstractions/GlobalUsings.cssrc/Mailtrap.Abstractions/Stats/IStatsResource.cssrc/Mailtrap.Abstractions/Stats/Models/SendingStatGroup.cssrc/Mailtrap.Abstractions/Stats/Models/SendingStats.cssrc/Mailtrap.Abstractions/Stats/Models/StatsFilter.cssrc/Mailtrap/Accounts/AccountResource.cssrc/Mailtrap/Core/Constants/UrlSegments.cssrc/Mailtrap/GlobalUsings.cssrc/Mailtrap/Stats/StatsResource.cstests/Mailtrap.IntegrationTests/GlobalUsings.cstests/Mailtrap.IntegrationTests/Stats/ByCategories_Success.jsontests/Mailtrap.IntegrationTests/Stats/ByDate_Success.jsontests/Mailtrap.IntegrationTests/Stats/ByDomains_Success.jsontests/Mailtrap.IntegrationTests/Stats/ByEmailServiceProviders_Success.jsontests/Mailtrap.IntegrationTests/Stats/Get_Forbidden.jsontests/Mailtrap.IntegrationTests/Stats/Get_Success.jsontests/Mailtrap.IntegrationTests/Stats/StatsIntegrationTests.cstests/Mailtrap.IntegrationTests/TestConstants/UrlSegmentsTestConstants.cstests/Mailtrap.UnitTests/GlobalUsings.cstests/Mailtrap.UnitTests/Stats/StatsResourceTests.cstests/Mailtrap.UnitTests/TestConstants/UrlSegmentsTestConstants.cs
README.md
Outdated
|
|
||
| ```bash | ||
| dotnet add package Mailtrap -v 3.0.0 -s github-mailtrap | ||
| dotnet add package Mailtrap -v 3.1.0 -s github-mailtrap |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for version 3.1.0 tag
echo "=== Checking for version tags ==="
git tag -l | grep -E '^v?3\.1\.0$' && echo "Tag found" || echo "Tag not found"
echo -e "\n=== Recent tags (last 10) ==="
git tag -l --sort=-v:refname | head -10
echo -e "\n=== Checking for project files with version/MinVer config ==="
fd -e "csproj" -e "props" -e "Directory.Build.props" | head -20Repository: mailtrap/mailtrap-dotnet
Length of output: 1166
🏁 Script executed:
#!/bin/bash
# Find and examine project files for MinVer configuration
echo "=== Searching for MinVer configuration ==="
rg -i "minver" --type xml --type csproj -lRepository: mailtrap/mailtrap-dotnet
Length of output: 158
🏁 Script executed:
#!/bin/bash
# Look at the README to see what's being documented
echo "=== README.md content around lines 41-47 ==="
sed -n '35,50p' README.mdRepository: mailtrap/mailtrap-dotnet
Length of output: 461
🏁 Script executed:
#!/bin/bash
# Check for release/CI workflow files
echo "=== Looking for release/CI processes ==="
fd -e "yml" -e "yaml" | xargs grep -l "3.1.0\|release\|tag" 2>/dev/null | head -10Repository: mailtrap/mailtrap-dotnet
Length of output: 108
🏁 Script executed:
#!/bin/bash
# Check Directory.Build.props for MinVer or versioning configuration
echo "=== Directory.Build.props ==="
cat Directory.Build.props | head -50Repository: mailtrap/mailtrap-dotnet
Length of output: 544
🏁 Script executed:
#!/bin/bash
# Check the NuGet-related build props
echo "=== build/mailtrap-nuget.props ==="
cat build/mailtrap-nuget.propsRepository: mailtrap/mailtrap-dotnet
Length of output: 2066
🏁 Script executed:
#!/bin/bash
# Search for any version-related configuration
echo "=== Searching for Version property ==="
rg -i "version|minver" --type xml | head -30Repository: mailtrap/mailtrap-dotnet
Length of output: 272
Ensure git tag v3.1.0 is created before or simultaneously with merging this README.
The project uses MinVer with MinVerTagPrefix set to v, which derives package versions from git tags. Since no tags currently exist in the repository, the version 3.1.0 will not be available on NuGet when users attempt to install via the commands documented in lines 41 and 47. The installation will fail with a "package not found" error unless the v3.1.0 tag exists at or before the time this README is published.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` at line 41, The README command referencing "dotnet add package
Mailtrap -v 3.1.0 -s github-mailtrap" will fail unless a git tag matching the
MinVerTagPrefix (v) exists; create and push the git tag v3.1.0 before or at
merge time, or update the README to use an existing published version (or remove
the explicit -v flag) so the "dotnet add package" command resolves; look for the
README line containing the dotnet command and the project MinVer configuration
(MinVerTagPrefix = v) to make the change.
| public string StartDate { get; set; } = string.Empty; | ||
|
|
||
| /// <summary> | ||
| /// Gets or sets the end date for the stats period. | ||
| /// </summary> | ||
| public string EndDate { get; set; } = string.Empty; |
There was a problem hiding this comment.
StatsFilter is constructible in an invalid state.
new StatsFilter() succeeds, but every stats call will fail later because BuildFilterUri() forwards StartDate and EndDate into AppendQueryParameter(), which rejects empty strings. Please make the date range required or validate it explicitly before URI construction.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Mailtrap.Abstractions/Stats/Models/StatsFilter.cs` around lines 12 - 17,
StatsFilter is constructible with empty StartDate/EndDate which later breaks
BuildFilterUri when it calls AppendQueryParameter; make the date range required
or validate it before building the URI. Update the StatsFilter class so
StartDate and EndDate cannot be empty (e.g., remove the string.Empty defaults
and force values via constructor or mark them required), or add explicit
validation in BuildFilterUri to throw a clear exception if StartDate/EndDate are
null/empty, and reference StartDate, EndDate, StatsFilter and
BuildFilterUri/AppendQueryParameter to locate where to enforce the check.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -e cs StatsFilter.cs --exec cat {}
echo "---"
fd -e cs StatsResource.cs --exec cat {}
Length of output: 8543
@mklocek, BuildFilterUri already has Ensure.NotNullOrEmpty guards for both fields, so at least a clear runtime exception is thrown. The remaining gap is that new StatsFilter() compiles without supplying either date. The cleanest fix is to mark them as required (C# 11+, already used elsewhere in the project via collection expressions):
src/Mailtrap.Abstractions/Stats/Models/StatsFilter.cs
public sealed record StatsFilter
{
/// <summary>
/// Gets or sets the start date for the stats period.
/// </summary>
public required string StartDate { get; set; }
/// <summary>
/// Gets or sets the end date for the stats period.
/// </summary>
public required string EndDate { get; set; }
// ... rest unchanged
}With required, any callsite that omits StartDate or EndDate in the object initializer gets a compile-time error instead of a runtime exception. The Ensure.NotNullOrEmpty checks in BuildFilterUri can stay as a defence-in-depth safety net.
The Stats.cs example already uses an object initialiser that provides both dates, so no breaking change there.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@mklocek tried to do it but:
The required keyword can't be used here because the library targets netstandard2.0, which doesn't support it (missing SetsRequiredMembersAttribute). The C# 11 required modifier needs at least .NET 7+ runtime types.
we are targeting v2
<TargetFrameworks>netstandard2.0</TargetFrameworks>
which seems old, or am I missing something?
| public IList<int> SendingDomainIds { get; } = []; | ||
|
|
There was a problem hiding this comment.
Use long for SendingDomainIds.
The rest of the SDK already models sending-domain identifiers as long (src/Mailtrap/Accounts/AccountResource.cs, SendingDomain(long domainId)), so this new filter narrows the contract and cannot represent valid IDs above Int32.MaxValue.
Suggested change
- public IList<int> SendingDomainIds { get; } = [];
+ public IList<long> SendingDomainIds { get; } = [];📝 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.
| public IList<int> SendingDomainIds { get; } = []; | |
| public IList<long> SendingDomainIds { get; } = []; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Mailtrap.Abstractions/Stats/Models/StatsFilter.cs` around lines 22 - 23,
The SendingDomainIds property in StatsFilter is typed as IList<int> which can't
represent IDs above Int32.MaxValue; change the property signature to use
IList<long> and initialize it with a long-capable collection (e.g., new
List<long>()) so it matches the rest of the SDK (see
AccountResource/SendingDomain(long)) and preserves compatibility with existing
long domain IDs.
| public IList<string> SendingStreams { get; } = []; | ||
|
|
||
| /// <summary> | ||
| /// Gets categories to filter by. | ||
| /// </summary> | ||
| public IList<string> Categories { get; } = []; | ||
|
|
||
| /// <summary> | ||
| /// Gets email service providers to filter by. | ||
| /// </summary> | ||
| public IList<string> EmailServiceProviders { get; } = []; |
There was a problem hiding this comment.
String filters need query-value escaping.
These collections accept arbitrary user input, but StatsResource.BuildFilterUri() currently passes each entry to UriExtensions.AppendQueryParameter(), which concatenates raw key=value pairs. Values containing spaces, +, &, or similar characters will alter or break the generated query string.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Mailtrap.Abstractions/Stats/Models/StatsFilter.cs` around lines 27 - 37,
The string filter collections (SendingStreams, Categories,
EmailServiceProviders) can contain characters that must be percent-encoded
before being appended to a query; update StatsResource.BuildFilterUri to
percent-encode each value passed to UriExtensions.AppendQueryParameter (e.g.,
via Uri.EscapeDataString or by calling an encoding helper) so spaces, +, &, etc.
are encoded, or alternatively update UriExtensions.AppendQueryParameter to
URL-encode the value it receives; locate usages in StatsResource.BuildFilterUri
and ensure every value from SendingStreams, Categories, and
EmailServiceProviders is encoded before appending.
| public string StartDate { get; set; } = string.Empty; | ||
|
|
||
| /// <summary> | ||
| /// Gets or sets the end date for the stats period. | ||
| /// </summary> | ||
| public string EndDate { get; set; } = string.Empty; |
| @@ -0,0 +1,17 @@ | |||
| [ | |||
| { | |||
| "category": "Transactional", | |||
There was a problem hiding this comment.
That's an unfortunate category name. Might be confused with sending stream
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/Mailtrap/Stats/StatsResource.cs (2)
88-90: Consider escapingStartDateandEndDatefor consistency.The string filter values are properly escaped with
Uri.EscapeDataString, butStartDateandEndDateare appended raw. While date strings typically don't contain special characters, escaping them would provide consistency and guard against unexpected formats.💡 Suggested change
var uri = baseUri - .AppendQueryParameter(StartDateParam, filter.StartDate) - .AppendQueryParameter(EndDateParam, filter.EndDate); + .AppendQueryParameter(StartDateParam, Uri.EscapeDataString(filter.StartDate)) + .AppendQueryParameter(EndDateParam, Uri.EscapeDataString(filter.EndDate));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Mailtrap/Stats/StatsResource.cs` around lines 88 - 90, The StartDate and EndDate values are appended unescaped; update the call that builds the query (the var uri = baseUri... block using AppendQueryParameter with StartDateParam and EndDateParam) to pass escaped values by wrapping filter.StartDate and filter.EndDate with Uri.EscapeDataString (i.e., use Uri.EscapeDataString(filter.StartDate) and Uri.EscapeDataString(filter.EndDate)) so the query parameters are consistently URL-encoded; keep AppendQueryParameter, StartDateParam, EndDateParam and baseUri unchanged otherwise.
67-80:GetPropertythrows if the grouping key is missing from the response.If the API response lacks the expected grouping key (e.g.,
"sending_domain_id"),item.GetProperty(groupKey)will throwKeyNotFoundException. Consider usingTryGetPropertyfor defensive handling:💡 Suggested defensive handling
return response.Select(item => { - var groupProp = item.GetProperty(groupKey); + if (!item.TryGetProperty(groupKey, out var groupProp)) + { + throw new InvalidOperationException($"Expected property '{groupKey}' not found in API response."); + } return new SendingStatGroup {This provides a clearer error message if the API contract changes or returns unexpected data.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Mailtrap/Stats/StatsResource.cs` around lines 67 - 80, The code calls item.GetProperty(groupKey) which throws when the response lacks that key; update the projection in StatsResource.cs to use item.TryGetProperty(groupKey, out var groupProp) instead of GetProperty, handle the missing-case by setting SendingStatGroup.Name/Value to an explicit fallback (e.g., empty string or a sentinel) or throw a clearer exception with context, and keep the existing logic for numeric/string conversion and the existing stats deserialization into SendingStats; reference the SendingStatGroup type, the groupKey variable, and the stats property handling so the change is localized to the Func used in the response.Select(...) projection.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/Mailtrap/Stats/StatsResource.cs`:
- Around line 88-90: The StartDate and EndDate values are appended unescaped;
update the call that builds the query (the var uri = baseUri... block using
AppendQueryParameter with StartDateParam and EndDateParam) to pass escaped
values by wrapping filter.StartDate and filter.EndDate with Uri.EscapeDataString
(i.e., use Uri.EscapeDataString(filter.StartDate) and
Uri.EscapeDataString(filter.EndDate)) so the query parameters are consistently
URL-encoded; keep AppendQueryParameter, StartDateParam, EndDateParam and baseUri
unchanged otherwise.
- Around line 67-80: The code calls item.GetProperty(groupKey) which throws when
the response lacks that key; update the projection in StatsResource.cs to use
item.TryGetProperty(groupKey, out var groupProp) instead of GetProperty, handle
the missing-case by setting SendingStatGroup.Name/Value to an explicit fallback
(e.g., empty string or a sentinel) or throw a clearer exception with context,
and keep the existing logic for numeric/string conversion and the existing stats
deserialization into SendingStats; reference the SendingStatGroup type, the
groupKey variable, and the stats property handling so the change is localized to
the Func used in the response.Select(...) projection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 646cf366-7785-4277-9d90-921e38b62c95
📒 Files selected for processing (29)
CHANGELOG.mdMailtrap.slnREADME.mdexamples/Mailtrap.Example.Stats/Mailtrap.Example.Stats.csprojexamples/Mailtrap.Example.Stats/Properties/launchSettings.jsonexamples/Mailtrap.Example.Stats/Stats.csexamples/Mailtrap.Example.Stats/appsettings.jsonsrc/Mailtrap.Abstractions/Accounts/IAccountResource.cssrc/Mailtrap.Abstractions/GlobalUsings.cssrc/Mailtrap.Abstractions/Stats/IStatsResource.cssrc/Mailtrap.Abstractions/Stats/Models/SendingStatGroup.cssrc/Mailtrap.Abstractions/Stats/Models/SendingStats.cssrc/Mailtrap.Abstractions/Stats/Models/StatsFilter.cssrc/Mailtrap/Accounts/AccountResource.cssrc/Mailtrap/Core/Constants/UrlSegments.cssrc/Mailtrap/GlobalUsings.cssrc/Mailtrap/Stats/StatsResource.cstests/Mailtrap.IntegrationTests/GlobalUsings.cstests/Mailtrap.IntegrationTests/Stats/ByCategories_Success.jsontests/Mailtrap.IntegrationTests/Stats/ByDate_Success.jsontests/Mailtrap.IntegrationTests/Stats/ByDomains_Success.jsontests/Mailtrap.IntegrationTests/Stats/ByEmailServiceProviders_Success.jsontests/Mailtrap.IntegrationTests/Stats/Get_Forbidden.jsontests/Mailtrap.IntegrationTests/Stats/Get_Success.jsontests/Mailtrap.IntegrationTests/Stats/StatsIntegrationTests.cstests/Mailtrap.IntegrationTests/TestConstants/UrlSegmentsTestConstants.cstests/Mailtrap.UnitTests/GlobalUsings.cstests/Mailtrap.UnitTests/Stats/StatsResourceTests.cstests/Mailtrap.UnitTests/TestConstants/UrlSegmentsTestConstants.cs
🚧 Files skipped from review as they are similar to previous changes (20)
- tests/Mailtrap.UnitTests/TestConstants/UrlSegmentsTestConstants.cs
- tests/Mailtrap.IntegrationTests/TestConstants/UrlSegmentsTestConstants.cs
- tests/Mailtrap.IntegrationTests/Stats/StatsIntegrationTests.cs
- src/Mailtrap/Core/Constants/UrlSegments.cs
- examples/Mailtrap.Example.Stats/Mailtrap.Example.Stats.csproj
- examples/Mailtrap.Example.Stats/appsettings.json
- tests/Mailtrap.IntegrationTests/GlobalUsings.cs
- tests/Mailtrap.UnitTests/GlobalUsings.cs
- README.md
- Mailtrap.sln
- src/Mailtrap.Abstractions/GlobalUsings.cs
- examples/Mailtrap.Example.Stats/Stats.cs
- src/Mailtrap.Abstractions/Stats/Models/SendingStats.cs
- tests/Mailtrap.IntegrationTests/Stats/ByDate_Success.json
- src/Mailtrap.Abstractions/Accounts/IAccountResource.cs
- tests/Mailtrap.IntegrationTests/Stats/ByDomains_Success.json
- tests/Mailtrap.IntegrationTests/Stats/Get_Success.json
- tests/Mailtrap.IntegrationTests/Stats/ByCategories_Success.json
- src/Mailtrap.Abstractions/Stats/Models/SendingStatGroup.cs
- src/Mailtrap/GlobalUsings.cs
3 participants
Motivation
/api/accounts/{account_id}/stats) to the .NET SDK, enabling users to retrieve aggregated email sending statistics.Changes
How to test
Examples
Summary by CodeRabbit
New Features
Documentation