Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR releases version 1.2.0 of the Mailtrap Java library, introducing a new Stats API module. It adds a Stats interface with methods to fetch aggregated and grouped sending statistics, implements the interface with HTTP client integration, and adds supporting filter and response model classes. The changes integrate the new module into the client factory and update documentation and tests accordingly. Changes
Sequence DiagramsequenceDiagram
participant Client as Client Code
participant Factory as MailtrapClientFactory
participant StatsImpl as StatsImpl
participant HTTPClient as HTTP Client
participant Response as Statistics Response
Client->>Factory: Create MailtrapClient with config
Factory->>Factory: Create StatsImpl with config
Factory->>Factory: Create MailtrapGeneralApi with StatsImpl
Client->>Client: Build StatsFilter (dates, categories)
Client->>StatsImpl: Call getStats(accountId, filter)
StatsImpl->>StatsImpl: Build URL: /accounts/{accountId}/stats
StatsImpl->>StatsImpl: Build query params from filter
StatsImpl->>HTTPClient: GET with query params
HTTPClient->>Response: Fetch from API
Response-->>HTTPClient: SendingStatsResponse JSON
HTTPClient-->>StatsImpl: Parsed SendingStatsResponse
StatsImpl-->>Client: Return SendingStatsResponse
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
mklocek
approved these changes
Mar 9, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
15-23:⚠️ Potential issue | 🔴 CriticalThe breaking constructor change remains unresolved; suggested fix was not applied.
The code still uses
@RequiredArgsConstructoron line 17 without a backward-compatible 4-arg constructor overload. Adding thestatsfield to this annotation changes the generated public constructor from 4 args to 5 args. External consumers upgrading to 1.2.0 and callingnew MailtrapGeneralApi(accountAccesses, accounts, billing, permissions)will encounter a compilation error orNoSuchMethodError, making this a breaking change for a minor version.To preserve API compatibility, either remove
@RequiredArgsConstructorand provide explicit constructors (including a 4-arg overload that delegates to the 5-arg version), or bump to version 2.0.0.🤖 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 15 - 23, The class MailtrapGeneralApi currently uses `@RequiredArgsConstructor` which generates a 5-arg public constructor after adding the stats field and thereby breaks the existing 4-arg API; restore backward compatibility by removing `@RequiredArgsConstructor` and adding explicit constructors: a public four-argument constructor MailtrapGeneralApi(AccountAccesses, Accounts, Billing, Permissions) that delegates to a new five-argument constructor, and a public five-argument constructor MailtrapGeneralApi(AccountAccesses, Accounts, Billing, Permissions, Stats) that initializes all fields (or keep the five-arg and have the four-arg call it with a default/null stats), ensuring both constructors exist to avoid breaking callers of MailtrapGeneralApi(accountAccesses, accounts, billing, permissions).
🧹 Nitpick comments (3)
src/main/java/io/mailtrap/api/stats/StatsFilter.java (1)
8-16: PreferLocalDatefor the date filters.
startDateandendDateas rawStrings create a type-safety gap; the builder currently passes them directly to query parameters without format validation. ExposingLocalDatehere aligns with the pattern used elsewhere in the codebase for temporal data and ensures date-range bounds are type-checked at compile time.♻️ Proposed refactor
-import java.util.List; +import java.time.LocalDate; +import java.util.List; @@ - private String startDate; - private String endDate; + private LocalDate startDate; + private LocalDate endDate;🤖 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/StatsFilter.java` around lines 8 - 16, Change StatsFilter.startDate and endDate from String to java.time.LocalDate to gain type safety: update the class fields in StatsFilter, replace the imports (add java.time.LocalDate), keep Lombok annotations (`@Data`, `@Builder`, etc.), and adjust any call sites or builders that pass String values so they parse/format using LocalDate.parse(...) or accept LocalDate directly; also update any serialization or query-parameter construction logic that previously relied on raw strings to format the LocalDate (e.g., .toString() or a DateTimeFormatter) before sending to downstream queries.src/main/java/io/mailtrap/model/response/stats/SendingStatGroupResponse.java (1)
9-18:nameis the group field key, not the grouped value.Line 16 stores keys like
"sending_domain_id"/"category"inname, while the actual grouped value ends up invalue. Since this is a brand-new public model, I’d make that explicit now with something likegroupField/groupValuebefore 1.2.0 locks the API shape.🤖 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 9 - 18, The current SendingStatGroupResponse stores the group key in name and the grouped value in value, which is confusing for a public model; rename the fields name -> groupField and value -> groupValue (update their declarations and usages) and adjust the JsonAnySetter method setDynamicField(String key, Object value) to set groupField/groupValue (keeping stats as-is), and update any getters/setters/serializers referencing name or value to the new identifiers so the public API clearly reflects "group field" vs "group value".src/test/java/io/mailtrap/api/stats/StatsImplTest.java (1)
28-40: Add one test that exercises the list-based filters.This suite only validates
start_date/end_date.StatsImplalso serializessending_domain_ids[],sending_streams[],categories[], andemail_service_providers[], andTestHttpClientrequires an exact query-param match. A regression in any of those keys would still pass here, even though list-filter serialization is part of the new API surface.Also applies to: 51-144
🤖 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 28 - 40, The tests only cover the start_date/end_date query params but miss verifying list-based filters; add a new unit test in StatsImplTest that calls the StatsImpl method (via the same setup using TestHttpClient) with non-empty lists for sending_domain_ids, sending_streams, categories, and email_service_providers so the client serializes keys like sending_domain_ids[], sending_streams[], categories[], and email_service_providers[]; register a matching DataMock in the TestHttpClient (use the same baseUrl endpoints as in init) that includes the exact list query params and assert the request was matched, ensuring StatsImpl’s serialization logic for those parameters is exercised (refer to StatsImpl, StatsImplTest, TestHttpClient, and DataMock to locate relevant code).
🤖 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 15-23: The class MailtrapGeneralApi currently uses
`@RequiredArgsConstructor` which generates a 5-arg public constructor after adding
the stats field and thereby breaks the existing 4-arg API; restore backward
compatibility by removing `@RequiredArgsConstructor` and adding explicit
constructors: a public four-argument constructor
MailtrapGeneralApi(AccountAccesses, Accounts, Billing, Permissions) that
delegates to a new five-argument constructor, and a public five-argument
constructor MailtrapGeneralApi(AccountAccesses, Accounts, Billing, Permissions,
Stats) that initializes all fields (or keep the five-arg and have the four-arg
call it with a default/null stats), ensuring both constructors exist to avoid
breaking callers of MailtrapGeneralApi(accountAccesses, accounts, billing,
permissions).
---
Nitpick comments:
In `@src/main/java/io/mailtrap/api/stats/StatsFilter.java`:
- Around line 8-16: Change StatsFilter.startDate and endDate from String to
java.time.LocalDate to gain type safety: update the class fields in StatsFilter,
replace the imports (add java.time.LocalDate), keep Lombok annotations (`@Data`,
`@Builder`, etc.), and adjust any call sites or builders that pass String values
so they parse/format using LocalDate.parse(...) or accept LocalDate directly;
also update any serialization or query-parameter construction logic that
previously relied on raw strings to format the LocalDate (e.g., .toString() or a
DateTimeFormatter) before sending to downstream queries.
In
`@src/main/java/io/mailtrap/model/response/stats/SendingStatGroupResponse.java`:
- Around line 9-18: The current SendingStatGroupResponse stores the group key in
name and the grouped value in value, which is confusing for a public model;
rename the fields name -> groupField and value -> groupValue (update their
declarations and usages) and adjust the JsonAnySetter method
setDynamicField(String key, Object value) to set groupField/groupValue (keeping
stats as-is), and update any getters/setters/serializers referencing name or
value to the new identifiers so the public API clearly reflects "group field" vs
"group value".
In `@src/test/java/io/mailtrap/api/stats/StatsImplTest.java`:
- Around line 28-40: The tests only cover the start_date/end_date query params
but miss verifying list-based filters; add a new unit test in StatsImplTest that
calls the StatsImpl method (via the same setup using TestHttpClient) with
non-empty lists for sending_domain_ids, sending_streams, categories, and
email_service_providers so the client serializes keys like sending_domain_ids[],
sending_streams[], categories[], and email_service_providers[]; register a
matching DataMock in the TestHttpClient (use the same baseUrl endpoints as in
init) that includes the exact list query params and assert the request was
matched, ensuring StatsImpl’s serialization logic for those parameters is
exercised (refer to StatsImpl, StatsImplTest, TestHttpClient, and DataMock to
locate relevant code).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8b5fd282-a6b4-4f26-aa55-7d2d6c2444e1
📒 Files selected for processing (17)
README.mddocs/getting-started.mdexamples/java/io/mailtrap/examples/general/StatsExample.javapom.xmlsrc/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
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
2 participants
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
Changes
How to test
N/A
Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests