Conversation
📝 WalkthroughWalkthroughThis pull request adds a complete Stats API feature to the Mailtrap Java client library, including a new Stats interface and StatsImpl implementation for retrieving sending statistics grouped by domain, category, email service provider, or date. Supporting changes include two response DTOs, a StatsFilter data class, factory integration to wire Stats into the client, comprehensive tests with JSON fixtures, an example demonstrating usage, and relocation of the SendingStream enum to a common package. Changes
Sequence DiagramsequenceDiagram
participant User
participant StatsAPI as StatsImpl
participant HttpClient
participant Server
User->>StatsAPI: getStats(accountId, filter)
StatsAPI->>StatsAPI: buildUrl("/stats")
StatsAPI->>StatsAPI: buildStatsQueryParams(filter)
StatsAPI->>HttpClient: GET /accounts/{id}/stats?startDate=...&endDate=...
HttpClient->>Server: HTTP GET request
Server-->>HttpClient: SendingStatsResponse JSON
HttpClient-->>StatsAPI: Response
StatsAPI-->>User: SendingStatsResponse
User->>StatsAPI: byDomain(accountId, filter)
StatsAPI->>StatsAPI: buildUrl("/domains")
StatsAPI->>HttpClient: GET /accounts/{id}/stats/domains?...
HttpClient->>Server: HTTP GET request
Server-->>HttpClient: List<SendingStatGroupResponse> JSON
HttpClient-->>StatsAPI: Response
StatsAPI-->>User: List<SendingStatGroupResponse>
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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)
📝 Coding Plan
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/java/io/mailtrap/model/response/stats/SendingStatGroupResponse.java (1)
13-18: Guard against silently overwriting extra dynamic fields.Every non-
statsproperty replacesnameandvalue. If this payload ever gains another top-level field, the first group key is lost with no signal. Consider rejecting a second dynamic key or storing extras explicitly instead of overwriting them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/io/mailtrap/model/response/stats/SendingStatGroupResponse.java` around lines 13 - 18, The current JsonAnySetter in SendingStatGroupResponse (setDynamicField) blindly overwrites name/value for every non-"stats" key, losing the first key; change setDynamicField to preserve the first encountered dynamic key by: add a new Map<String,Object> extras field on SendingStatGroupResponse, set name and value only if name is null (or blank) on first non-"stats" key, and for any subsequent non-"stats" keys put them into extras instead of overwriting; alternatively (if you prefer strictness) throw an IllegalArgumentException when a second dynamic key is encountered—implement one of these behaviors and update setDynamicField to check existing name before mutating it and to populate extras (or throw) accordingly.src/test/java/io/mailtrap/api/stats/StatsImplTest.java (1)
30-40: Add coverage for the list-filter query params.These fixtures only assert
start_date/end_date, butsrc/main/java/io/mailtrap/api/stats/StatsImpl.java:72-80also serializessending_domain_ids[],sending_streams[],categories[], andemail_service_providers[]. If one of those keys is misspelled or emitted incorrectly, this suite still stays green.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/io/mailtrap/api/stats/StatsImplTest.java` around lines 30 - 40, The tests only assert start_date/end_date; update the test to include the list-filter query params that StatsImpl.serializeFilters emits (sending_domain_ids[], sending_streams[], categories[], email_service_providers[]) so the suite will catch misspellings: add those keys with representative values to the defaultQueryParams Map used by TestHttpClient (the variable currently named defaultQueryParams) and ensure each DataMock.build entry in the TestHttpClient expectations includes those same keys (matching the bracketed parameter names) so requests created by StatsImpl (methods in StatsImpl that serialize filters around lines 72-80) must include the list params to satisfy the mocks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/io/mailtrap/api/stats/StatsImpl.java`:
- Around line 72-80: Validate the incoming StatsFilter and its start/end dates
at the start of buildStatsQueryParams: check that filter is not null and that
filter.getStartDate() and filter.getEndDate() are non-null (or non-empty, per
domain rules) and throw a clear IllegalArgumentException if any are missing;
only after these checks keep the existing RequestData.buildQueryParams(...) with
Optional.ofNullable(...) calls. Reference: method buildStatsQueryParams and
class StatsFilter — add
Objects.requireNonNull/filter.getStartDate()/filter.getEndDate() checks or
explicit if-statements that throw with a descriptive message so callers fail
fast instead of producing an NPE or invalid request.
---
Nitpick comments:
In
`@src/main/java/io/mailtrap/model/response/stats/SendingStatGroupResponse.java`:
- Around line 13-18: The current JsonAnySetter in SendingStatGroupResponse
(setDynamicField) blindly overwrites name/value for every non-"stats" key,
losing the first key; change setDynamicField to preserve the first encountered
dynamic key by: add a new Map<String,Object> extras field on
SendingStatGroupResponse, set name and value only if name is null (or blank) on
first non-"stats" key, and for any subsequent non-"stats" keys put them into
extras instead of overwriting; alternatively (if you prefer strictness) throw an
IllegalArgumentException when a second dynamic key is encountered—implement one
of these behaviors and update setDynamicField to check existing name before
mutating it and to populate extras (or throw) accordingly.
In `@src/test/java/io/mailtrap/api/stats/StatsImplTest.java`:
- Around line 30-40: The tests only assert start_date/end_date; update the test
to include the list-filter query params that StatsImpl.serializeFilters emits
(sending_domain_ids[], sending_streams[], categories[],
email_service_providers[]) so the suite will catch misspellings: add those keys
with representative values to the defaultQueryParams Map used by TestHttpClient
(the variable currently named defaultQueryParams) and ensure each DataMock.build
entry in the TestHttpClient expectations includes those same keys (matching the
bracketed parameter names) so requests created by StatsImpl (methods in
StatsImpl that serialize filters around lines 72-80) must include the list
params to satisfy the mocks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5a7ec49e-7893-457d-8a99-5d5d6a14e086
📒 Files selected for processing (14)
examples/java/io/mailtrap/examples/general/StatsExample.javasrc/main/java/io/mailtrap/api/stats/Stats.javasrc/main/java/io/mailtrap/api/stats/StatsFilter.javasrc/main/java/io/mailtrap/api/stats/StatsImpl.javasrc/main/java/io/mailtrap/client/api/MailtrapGeneralApi.javasrc/main/java/io/mailtrap/factory/MailtrapClientFactory.javasrc/main/java/io/mailtrap/model/response/stats/SendingStatGroupResponse.javasrc/main/java/io/mailtrap/model/response/stats/SendingStatsResponse.javasrc/test/java/io/mailtrap/api/stats/StatsImplTest.javasrc/test/resources/api/stats/byCategory.jsonsrc/test/resources/api/stats/byDate.jsonsrc/test/resources/api/stats/byDomain.jsonsrc/test/resources/api/stats/byEmailServiceProvider.jsonsrc/test/resources/api/stats/getStats.json
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/main/java/io/mailtrap/api/stats/StatsImpl.java (1)
76-80:⚠️ Potential issue | 🟡 MinorReject blank date values too.
Whitespace-only
startDate/endDatestill pass these checks and get sent as invalid query params. Trim or blank-check them here so callers fail fast.Suggested fix
- if (filter.getStartDate() == null || filter.getStartDate().isEmpty()) { + if (filter.getStartDate() == null || filter.getStartDate().trim().isEmpty()) { throw new IllegalArgumentException("startDate must not be null or empty"); } - if (filter.getEndDate() == null || filter.getEndDate().isEmpty()) { + if (filter.getEndDate() == null || filter.getEndDate().trim().isEmpty()) { throw new IllegalArgumentException("endDate must not be null or empty"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/io/mailtrap/api/stats/StatsImpl.java` around lines 76 - 80, The current validation in StatsImpl (checks on filter.getStartDate() and filter.getEndDate()) allows whitespace-only strings; update the validation to reject blank values by trimming before emptiness check (e.g., if (filter.getStartDate() == null || filter.getStartDate().trim().isEmpty()) and similarly for getEndDate()) so callers fail fast and no whitespace-only dates are sent as query params; keep throwing IllegalArgumentException with the same messages.
🧹 Nitpick comments (1)
examples/java/io/mailtrap/examples/general/StatsExample.java (1)
19-22: Use placeholder dates in the sample.A fixed January 2026 window will go stale and can easily return empty results for readers. Placeholder values or named constants keep the example evergreen.
Suggested tweak
- final var filter = StatsFilter.builder() - .startDate("2026-01-01") - .endDate("2026-01-31") + final var filter = StatsFilter.builder() + .startDate("<START_DATE: YYYY-MM-DD>") + .endDate("<END_DATE: YYYY-MM-DD>") .build();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/java/io/mailtrap/examples/general/StatsExample.java` around lines 19 - 22, The sample uses fixed dates in StatsFilter.builder() which will become stale; replace the hard-coded "2026-01-01"/"2026-01-31" values in the StatsExample's filter variable with placeholder constants or descriptive variables (e.g., START_DATE/END_DATE or "YYYY-MM-DD" strings) and/or add a short comment explaining they should be set by the user; update the StatsFilter.builder().startDate(...) and .endDate(...) calls to use those placeholders so the example remains evergreen.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/main/java/io/mailtrap/api/stats/StatsImpl.java`:
- Around line 76-80: The current validation in StatsImpl (checks on
filter.getStartDate() and filter.getEndDate()) allows whitespace-only strings;
update the validation to reject blank values by trimming before emptiness check
(e.g., if (filter.getStartDate() == null ||
filter.getStartDate().trim().isEmpty()) and similarly for getEndDate()) so
callers fail fast and no whitespace-only dates are sent as query params; keep
throwing IllegalArgumentException with the same messages.
---
Nitpick comments:
In `@examples/java/io/mailtrap/examples/general/StatsExample.java`:
- Around line 19-22: The sample uses fixed dates in StatsFilter.builder() which
will become stale; replace the hard-coded "2026-01-01"/"2026-01-31" values in
the StatsExample's filter variable with placeholder constants or descriptive
variables (e.g., START_DATE/END_DATE or "YYYY-MM-DD" strings) and/or add a short
comment explaining they should be set by the user; update the
StatsFilter.builder().startDate(...) and .endDate(...) calls to use those
placeholders so the example remains evergreen.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 57307b4d-7843-4f9f-9cb8-b5f5cef595ec
📒 Files selected for processing (14)
examples/java/io/mailtrap/examples/general/StatsExample.javasrc/main/java/io/mailtrap/api/stats/Stats.javasrc/main/java/io/mailtrap/api/stats/StatsFilter.javasrc/main/java/io/mailtrap/api/stats/StatsImpl.javasrc/main/java/io/mailtrap/client/api/MailtrapGeneralApi.javasrc/main/java/io/mailtrap/factory/MailtrapClientFactory.javasrc/main/java/io/mailtrap/model/response/stats/SendingStatGroupResponse.javasrc/main/java/io/mailtrap/model/response/stats/SendingStatsResponse.javasrc/test/java/io/mailtrap/api/stats/StatsImplTest.javasrc/test/resources/api/stats/byCategory.jsonsrc/test/resources/api/stats/byDate.jsonsrc/test/resources/api/stats/byDomain.jsonsrc/test/resources/api/stats/byEmailServiceProvider.jsonsrc/test/resources/api/stats/getStats.json
🚧 Files skipped from review as they are similar to previous changes (10)
- src/main/java/io/mailtrap/client/api/MailtrapGeneralApi.java
- src/test/java/io/mailtrap/api/stats/StatsImplTest.java
- src/main/java/io/mailtrap/api/stats/StatsFilter.java
- src/test/resources/api/stats/getStats.json
- src/main/java/io/mailtrap/model/response/stats/SendingStatsResponse.java
- src/test/resources/api/stats/byDomain.json
- src/test/resources/api/stats/byEmailServiceProvider.json
- src/main/java/io/mailtrap/api/stats/Stats.java
- src/test/resources/api/stats/byDate.json
- src/test/resources/api/stats/byCategory.json
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/io/mailtrap/client/api/MailtrapGeneralApi.java (1)
17-23:⚠️ Potential issue | 🟠 MajorPreserve the existing
MailtrapGeneralApiconstructor signature.Adding a new
finalfield under@RequiredArgsConstructorchanges the public constructor from four args to five. That is a breaking SDK change for anyone instantiatingMailtrapGeneralApidirectly, even though this PR only adds a feature. Please keep the old overload around until the next major release, or switch this type away from Lombok-generated constructors so the public surface stays stable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/io/mailtrap/client/api/MailtrapGeneralApi.java` around lines 17 - 23, The change added a new final field (stats) under `@RequiredArgsConstructor` which changes MailtrapGeneralApi's public constructor signature and breaks compatibility; fix by removing the Lombok-generated constructor and adding explicit constructors: implement the original four-argument constructor MailtrapGeneralApi(AccountAccesses accountAccesses, Accounts accounts, Billing billing, Permissions permissions) that assigns those fields and sets stats to null (preserving the old API), and add an additional five-argument constructor MailtrapGeneralApi(AccountAccesses, Accounts, Billing, Permissions, Stats) that assigns all fields; ensure the class-level `@RequiredArgsConstructor` is removed so the explicit constructors are the only public ones.
🧹 Nitpick comments (1)
src/test/java/io/mailtrap/api/stats/StatsImplTest.java (1)
31-41: Add coverage for list-filter query serialization.These mocks only verify
start_dateandend_date. The new behavior in this PR is the optional list filters with[]query keys, but none of the tests exercisesending_domain_ids[],sending_streams[],categories[], oremail_service_providers[]. Please add at least one positive-path case that asserts those parameters are serialized correctly, since that is the easiest part of this feature to break.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/io/mailtrap/api/stats/StatsImplTest.java` around lines 31 - 41, The current TestHttpClient mocks only assert start_date/end_date; add a positive-path mock that verifies the new list-filter query serialization (keys with []), e.g. create an expectedQueryParams map including "sending_domain_ids[]", "sending_streams[]", "categories[]", and "email_service_providers[]" mapped to their expected list values alongside start_date/end_date, and add a DataMock.build entry that uses that map (similar to existing builds) so the StatsImplTest (TestHttpClient/DataMock.build) asserts those array-style query keys are sent correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/main/java/io/mailtrap/client/api/MailtrapGeneralApi.java`:
- Around line 17-23: The change added a new final field (stats) under
`@RequiredArgsConstructor` which changes MailtrapGeneralApi's public constructor
signature and breaks compatibility; fix by removing the Lombok-generated
constructor and adding explicit constructors: implement the original
four-argument constructor MailtrapGeneralApi(AccountAccesses accountAccesses,
Accounts accounts, Billing billing, Permissions permissions) that assigns those
fields and sets stats to null (preserving the old API), and add an additional
five-argument constructor MailtrapGeneralApi(AccountAccesses, Accounts, Billing,
Permissions, Stats) that assigns all fields; ensure the class-level
`@RequiredArgsConstructor` is removed so the explicit constructors are the only
public ones.
---
Nitpick comments:
In `@src/test/java/io/mailtrap/api/stats/StatsImplTest.java`:
- Around line 31-41: The current TestHttpClient mocks only assert
start_date/end_date; add a positive-path mock that verifies the new list-filter
query serialization (keys with []), e.g. create an expectedQueryParams map
including "sending_domain_ids[]", "sending_streams[]", "categories[]", and
"email_service_providers[]" mapped to their expected list values alongside
start_date/end_date, and add a DataMock.build entry that uses that map (similar
to existing builds) so the StatsImplTest (TestHttpClient/DataMock.build) asserts
those array-style query keys are sent correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: abcddd79-cf5b-4bef-9f3e-edbb40ab278c
📒 Files selected for processing (14)
examples/java/io/mailtrap/examples/general/StatsExample.javasrc/main/java/io/mailtrap/api/stats/Stats.javasrc/main/java/io/mailtrap/api/stats/StatsFilter.javasrc/main/java/io/mailtrap/api/stats/StatsImpl.javasrc/main/java/io/mailtrap/client/api/MailtrapGeneralApi.javasrc/main/java/io/mailtrap/factory/MailtrapClientFactory.javasrc/main/java/io/mailtrap/model/response/stats/SendingStatGroupResponse.javasrc/main/java/io/mailtrap/model/response/stats/SendingStatsResponse.javasrc/test/java/io/mailtrap/api/stats/StatsImplTest.javasrc/test/resources/api/stats/byCategory.jsonsrc/test/resources/api/stats/byDate.jsonsrc/test/resources/api/stats/byDomain.jsonsrc/test/resources/api/stats/byEmailServiceProvider.jsonsrc/test/resources/api/stats/getStats.json
🚧 Files skipped from review as they are similar to previous changes (9)
- src/test/resources/api/stats/byDomain.json
- src/main/java/io/mailtrap/model/response/stats/SendingStatGroupResponse.java
- src/main/java/io/mailtrap/api/stats/StatsFilter.java
- src/main/java/io/mailtrap/api/stats/StatsImpl.java
- src/main/java/io/mailtrap/model/response/stats/SendingStatsResponse.java
- src/main/java/io/mailtrap/api/stats/Stats.java
- src/test/resources/api/stats/byDate.json
- src/test/resources/api/stats/byCategory.json
- src/test/resources/api/stats/byEmailServiceProvider.json
| final var stats = new StatsImpl(config); | ||
|
|
||
| return new MailtrapGeneralApi(accountAccess, accounts, billing, permissions); | ||
| return new MailtrapGeneralApi(accountAccess, accounts, billing, permissions, stats); |
There was a problem hiding this comment.
I'd make it part of sendingApi (similar like Suppressions and Sending Domains)
EDIT: And add a note about it to README
| private String startDate; | ||
| private String endDate; | ||
| private List<Long> sendingDomainIds; | ||
| private List<String> sendingStreams; |
There was a problem hiding this comment.
We could use an enum for sendingStreams filter values
There was a problem hiding this comment.
🧹 Nitpick comments (1)
examples/java/io/mailtrap/examples/general/StatsExample.java (1)
1-43: LGTM!The example clearly demonstrates usage of all Stats API methods with proper fluent API chaining. The placeholder values and date format comment provide good guidance for users.
Consider adding an example using optional filter parameters (e.g.,
sendingDomainIds,sendingStreams) to showcase the full filtering capability:final var filterWithOptions = StatsFilter.builder() .startDate(START_DATE) .endDate(END_DATE) .sendingDomainIds(List.of(1L, 2L)) .sendingStreams(List.of(SendingStream.TRANSACTIONAL)) .build();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/java/io/mailtrap/examples/general/StatsExample.java` around lines 1 - 43, Add a second example showing optional StatsFilter parameters: create a new variable (e.g., filterWithOptions) using StatsFilter.builder() and set startDate/endDate plus sendingDomainIds (List.of(...)) and sendingStreams (e.g., SendingStream.TRANSACTIONAL) to demonstrate advanced filtering; call the same client.sendingApi().stats() methods (getStats, byDomain, byCategory, byEmailServiceProvider, byDate) with ACCOUNT_ID and filterWithOptions and add necessary imports (java.util.List and the SendingStream enum) at the top of StatsExample.java.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@examples/java/io/mailtrap/examples/general/StatsExample.java`:
- Around line 1-43: Add a second example showing optional StatsFilter
parameters: create a new variable (e.g., filterWithOptions) using
StatsFilter.builder() and set startDate/endDate plus sendingDomainIds
(List.of(...)) and sendingStreams (e.g., SendingStream.TRANSACTIONAL) to
demonstrate advanced filtering; call the same client.sendingApi().stats()
methods (getStats, byDomain, byCategory, byEmailServiceProvider, byDate) with
ACCOUNT_ID and filterWithOptions and add necessary imports (java.util.List and
the SendingStream enum) at the top of StatsExample.java.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3eeebb62-bd8f-48a4-b2e9-2a1cbea3e332
📒 Files selected for processing (17)
examples/java/io/mailtrap/examples/general/StatsExample.javasrc/main/java/io/mailtrap/api/stats/Stats.javasrc/main/java/io/mailtrap/api/stats/StatsFilter.javasrc/main/java/io/mailtrap/api/stats/StatsImpl.javasrc/main/java/io/mailtrap/client/api/MailtrapEmailSendingApi.javasrc/main/java/io/mailtrap/factory/MailtrapClientFactory.javasrc/main/java/io/mailtrap/model/SendingStream.javasrc/main/java/io/mailtrap/model/response/stats/SendingStatGroupResponse.javasrc/main/java/io/mailtrap/model/response/stats/SendingStatsResponse.javasrc/main/java/io/mailtrap/model/response/suppressions/SuppressionsResponse.javasrc/test/java/io/mailtrap/api/stats/StatsImplTest.javasrc/test/java/io/mailtrap/api/suppressions/SuppressionsImplTest.javasrc/test/resources/api/stats/byCategory.jsonsrc/test/resources/api/stats/byDate.jsonsrc/test/resources/api/stats/byDomain.jsonsrc/test/resources/api/stats/byEmailServiceProvider.jsonsrc/test/resources/api/stats/getStats.json
✅ Files skipped from review due to trivial changes (2)
- src/main/java/io/mailtrap/model/response/suppressions/SuppressionsResponse.java
- src/test/java/io/mailtrap/api/suppressions/SuppressionsImplTest.java
🚧 Files skipped from review as they are similar to previous changes (7)
- src/test/resources/api/stats/byCategory.json
- src/test/resources/api/stats/byEmailServiceProvider.json
- src/main/java/io/mailtrap/model/response/stats/SendingStatsResponse.java
- src/test/resources/api/stats/byDate.json
- src/main/java/io/mailtrap/api/stats/StatsFilter.java
- src/test/java/io/mailtrap/api/stats/StatsImplTest.java
- src/test/resources/api/stats/getStats.json
Motivation
/api/accounts/{account_id}/stats) to the Java SDK, enabling users to retrieve aggregated email sending statistics.Changes
How to test
Examples
Summary by CodeRabbit
New Features
Tests