Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (3)
📝 WalkthroughWalkthroughUpdated the Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 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 |
![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
piobeny
force-pushed
the
stats-api
branch
3 times, most recently
from
a267e81 to
0c315b1
Compare
3 participants
Motivation
Changes
Summary by CodeRabbit