Conversation
📝 WalkthroughWalkthroughAdds a new Stats feature: interface, filter DTO, implementation, response models, client wiring, example usage, and tests with JSON fixtures to fetch sending metrics overall or grouped by domain, category, ESP, or date. Changes
Sequence DiagramsequenceDiagram
actor User
participant Client as MailtrapClient
participant General as MailtrapGeneralApi/Stats
participant HTTP as HttpClient
participant Remote as Remote API
User->>Client: createMailtrapClient(config)
User->>Client: call stats method (getStats / byDomain / ...)
Client->>General: invoke StatsImpl.method(accountId, filter)
General->>General: buildUrl & query params from StatsFilter
General->>HTTP: GET /stats[...query...]
HTTP->>Remote: send HTTP request
Remote-->>HTTP: return JSON response
HTTP-->>General: deserialize to SendingStatsResponse / SendingStatGroupResponse
General-->>Client: return DTO(s)
Client-->>User: deliver results
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)
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 |
piobeny
requested review from
VladimirTaytor
and removed request for
VladimirTaytor
![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
🧹 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
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.
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