Implemented Contact Imports API #33
Conversation
WalkthroughAdds contact imports feature: new API interface and implementation, request/response models, client wiring, example usage, and tests with fixtures. Also adjusts some test assertions order and adds a BaseTest field. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor DevApp as Example / Client Code
participant Client as MailtrapContactsApi
participant Imports as ContactImportsImpl
participant HTTP as Mailtrap HTTP API
DevApp->>Client: contactImports().importContacts(accountId, request)
Client->>Imports: importContacts(accountId, request)
Imports->>Imports: validate request
Imports->>HTTP: POST /api/accounts/{id}/contacts/imports\n(body: ImportContactsRequest)
HTTP-->>Imports: CreateContactsImportResponse (id, status)
Imports-->>Client: CreateContactsImportResponse
Client-->>DevApp: CreateContactsImportResponse
DevApp->>Client: contactImports().getContactImport(accountId, importId)
Client->>Imports: getContactImport(accountId, importId)
Imports->>HTTP: GET /api/accounts/{id}/contacts/imports/{importId}
HTTP-->>Imports: ContactsImportResponse (status, counts)
Imports-->>Client: ContactsImportResponse
Client-->>DevApp: ContactsImportResponse
sequenceDiagram
autonumber
actor DevApp
participant Imports as ContactImportsImpl
note over Imports: Validation path
DevApp->>Imports: importContacts(..., request with 50,001 contacts)
Imports->>Imports: validate size <= 50,000
Imports-->>DevApp: throws InvalidRequestBodyException
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (25)
src/test/java/io/mailtrap/testutils/BaseTest.java (1)
21-21: Nit: Prefer clearer naming and wrapper type for consistencyLine 21 introduces contactId as a primitive long. Most IDs in this class use the Long wrapper. Consider:
- Renaming to contactImportId to avoid confusion with an individual Contact’s ID (you already have contactUUID above).
- Using Long for consistency with other ID fields and typical assertEquals usage.
src/test/java/io/mailtrap/api/accountaccesses/AccountAccessesImplTest.java (1)
69-71: Good fix: assertEquals now uses (expected, actual)Switching to expected-first improves failure messages. Consider applying the same convention to nearby assertions for consistency (e.g., Line 51 getId(), Line 71 resourceId).
src/test/java/io/mailtrap/api/inboxes/InboxesImplTest.java (1)
73-73: LGTM: corrected expected-first ordering across string assertionsThese changes improve readability and diagnostics when tests fail. As a minor follow-up, several numeric ID assertions in this file still use actual-first (e.g., Lines 72, 82, 92, 102, 113, 123, 133, 143, 153, 164). Aligning those to (expected, actual) would make the file fully consistent.
Also applies to: 83-83, 93-93, 103-104, 114-114, 124-124, 134-134, 144-144, 154-154, 165-165
src/main/java/io/mailtrap/model/response/contactimports/ContactImportStatus.java (1)
23-32: Harden @JsonCreator to avoid ClassCastException and improve messagesfromValue(Object value) casts to String without type checks. If Jackson ever supplies a non-String token, this can throw a ClassCastException. Make the factory accept String directly, guard null/blank, and keep case-insensitivity.
Apply this diff:
- @JsonCreator - public static ContactImportStatus fromValue(Object value) { - for (ContactImportStatus level : ContactImportStatus.values()) { - if (level.value.equalsIgnoreCase((String) value)) { - return level; - } - } - - throw new IllegalArgumentException("Unknown value: " + value); - } + @JsonCreator(mode = JsonCreator.Mode.DELEGATING) + public static ContactImportStatus fromValue(String value) { + if (value == null || value.isBlank()) { + throw new IllegalArgumentException("ContactImportStatus value must not be null/blank"); + } + for (ContactImportStatus status : ContactImportStatus.values()) { + if (status.value.equalsIgnoreCase(value)) { + return status; + } + } + throw new IllegalArgumentException("Unknown ContactImportStatus: " + value); + }src/main/java/io/mailtrap/model/request/contactimports/Contact.java (3)
9-11: Consider omitting nulls from JSON payloads for forward compatibilityContacts often have optional attributes. Emitting nulls can bloat requests and cause stricter backends to reject payloads. Recommend adding @JsonInclude(Include.NON_NULL).
Apply this diff:
import com.fasterxml.jackson.annotation.JsonProperty; +import com.fasterxml.jackson.annotation.JsonInclude; import java.util.List; import java.util.Map; import lombok.AllArgsConstructor; import lombok.Getter; @Getter -@AllArgsConstructor +@AllArgsConstructor +@JsonInclude(JsonInclude.Include.NON_NULL) public class Contact {
13-16: Validate email early to prevent API roundtripsIf the API requires a valid, non-empty email, add bean validation to fail fast before HTTP calls.
Apply this diff (uses Jakarta Validation; switch to javax.validation.* if your project hasn’t migrated):
import com.fasterxml.jackson.annotation.JsonProperty; import com.fasterxml.jackson.annotation.JsonInclude; import java.util.List; import java.util.Map; import lombok.AllArgsConstructor; import lombok.Getter; +import jakarta.validation.constraints.Email; +import jakarta.validation.constraints.NotBlank; @Getter @AllArgsConstructor @JsonInclude(JsonInclude.Include.NON_NULL) public class Contact { - private String email; + @NotBlank + @Email + private String email; private Map<String, Object> fields;
< 8000 p dir="auto">17-21: JSON mapping looks correct; consider handling empty vs null listsSnake_case mappings for included/excluded list IDs are correct. If the server treats null and empty arrays differently, ensure callers pass empty lists when appropriate or normalize in a builder/factory to avoid nulls.
src/main/java/io/mailtrap/client/api/MailtrapContactsApi.java (1)
15-20: Constructor usage verified; consider null‐safety improvementsI’ve confirmed that the only direct instantiation of MailtrapContactsApi is in
MailtrapClientFactory.createContactsApi(and transitively via createMailtrapClient). No other rawnew MailtrapContactsApi(calls remain in the codebase—so consumers wired through the factory won’t break.As suggested, you can improve fail‐fast behavior by annotating the required fields with Lombok’s
@NonNull. This is an optional refactor but helps catch misconfiguration early:import lombok.RequiredArgsConstructor; +import lombok.NonNull; import lombok.experimental.Accessors; @Getter @Accessors(fluent = true) @RequiredArgsConstructor public class MailtrapContactsApi { - private final ContactLists contactLists; - private final Contacts contacts; - private final ContactImports contactImports; + private final @NonNull ContactLists contactLists; + private final @NonNull Contacts contacts; + private final @NonNull ContactImports contactImports; }No further constructor‐related fixes are required.
src/main/java/io/mailtrap/model/request/contactimports/ImportContactsRequest.java (2)
4-9: Validate nested items and guard against null/empty payloadsToday the list can be null or empty and still pass client-side validation; the server will likely reject it later. Also, nested Contact objects won’t be validated without @Valid.
Please confirm the API allows an empty contacts array. If not, consider the following changes:
import io.mailtrap.model.AbstractModel; import java.util.List; +import jakarta.validation.Valid; +import jakarta.validation.constraints.NotNull; import jakarta.validation.constraints.Size; import lombok.AllArgsConstructor; import lombok.Getter; @@ - @Size(max = 50_000, message = "Maximum 50000 contacts per request") - private List<Contact> contacts; + @NotNull(message = "Contacts list must be provided") + @Size(min = 1, max = 50_000, message = "Contacts must contain 1 to 50000 items") + @Valid + private List<Contact> contacts;Follow-up: The Contact model currently lacks field-level validation (e.g., @Email, @notblank on email). If the server requires these, add them in src/main/java/io/mailtrap/model/request/contactimports/Contact.java (Lines 8-21).
Also applies to: 14-15
4-9: Prevent accidental mutation by exposing an unmodifiable viewThe current Lombok-generated getter returns the mutable List. Downstream code can mutate the request after validation/serialization, making debugging harder.
Apply a custom getter and import Collections to return an unmodifiable view (null-safe):
import io.mailtrap.model.AbstractModel; import java.util.List; +import java.util.Collections; @@ -@Getter -@AllArgsConstructor +@Getter +@AllArgsConstructor public class ImportContactsRequest extends AbstractModel { @@ private List<Contact> contacts; + public List<Contact> getContacts() { + return contacts == null ? null : Collections.unmodifiableList(contacts); + }Alternative: switch to @builder + @Singular to construct an unmodifiable list at build time and drop @AllArgsConstructor.
Also applies to: 10-17
src/main/java/io/mailtrap/model/response/contactimports/ImportContactsResponse.java (1)
3-7: Add ignoreUnknown for forward compatibilityMailtrap responses may gain new fields over time. Ignoring unknowns avoids unexpected deserialization failures.
import com.fasterxml.jackson.annotation.JsonProperty; +import com.fasterxml.jackson.annotation.JsonIgnoreProperties; import lombok.Data; -@Data +@Data +@JsonIgnoreProperties(ignoreUnknown = true) public class ImportContactsResponse {examples/java/io/mailtrap/examples/contactimports/ContactImports.java (3)
7-10: Remove unused importsTidies the example and avoids IDE warnings.
-import io.mailtrap.model.request.contacts.UpdateContact; -import io.mailtrap.model.request.contacts.UpdateContactRequest; - -import java.util.Collections;
16-21: Prefer sourcing TOKEN and ACCOUNT_ID from env/system properties for runnable examplesKeeps secrets out of source and makes the example copy-paste runnable.
- private static final String TOKEN = "<YOUR MAILTRAP TOKEN>"; - private static final long ACCOUNT_ID = 1L; + private static final String TOKEN = System.getenv().getOrDefault("MAILTRAP_TOKEN", "<YOUR MAILTRAP TOKEN>"); + private static final long ACCOUNT_ID = Long.parseLong(System.getenv().getOrDefault("MAILTRAP_ACCOUNT_ID", "1"));
37-40: Rename variable to reflect operationThe second call fetches the created import; rename for clarity.
- var updateResponse = client.contactsApi().contactImports() - .getContactImport(ACCOUNT_ID, createResponse.getId()); + var getResponse = client.contactsApi().contactImports() + .getContactImport(ACCOUNT_ID, createResponse.getId()); - System.out.println(updateResponse); + System.out.println(getResponse);src/main/java/io/mailtrap/api/contactimports/ContactImports.java (2)
8-16: Tighten Javadoc wordingMinor clarity/punctuation: make the rate limit sentence complete and adjust the return type description.
/** * Import contacts in bulk with support for custom fields and list management. * Existing contacts with matching email addresses will be updated automatically. - * Up to 50,000 contacts per request + * Up to 50,000 contacts per request. * * @param accountId unique account ID * @param request request body - * @return contact data + * @return contact import metadata */
19-26: Update parameter name for clarity in interface and implementationTo align the Javadoc and symbol names with the fact that this ID refers to a contact import (not a contact), please optionally refactor the method signatures and Javadoc:
• In
src/main/java/io/mailtrap/api/contactimports/ContactImports.java
- Change the second parameter name from
contactIdtoimportId- Update the Javadoc tag and return description accordingly
/** * Get Contact Import * * @param accountId unique account ID -* @param contactId unique Contact Import ID -* @return contact data +* @param importId unique Contact Import ID +* @return contact import metadata */ -ImportContactsResponse getContactImport(long accountId, long contactId); +ImportContactsResponse getContactImport(long accountId, long importId);• In
src/main/java/io/mailtrap/api/contactimports/ContactImportsImpl.java
- Mirror the signature rename (
contactId→importId) and adjust any in-method references or JavadocCall sites (in tests and examples) invoke by position and continue to pass an ID value (
contactIdvariable orcreateResponse.getId()), so they won’t break. You may choose to rename local variables in tests/examples for consistency (e.g.contactId→importId), but that’s entirely optional.src/test/java/io/mailtrap/api/contactimports/ContactImportsImplTest.java (5)
45-56: Strengthen assertions for partial response shapeFor STARTED status, also assert other counts are null to lock expected contract.
assertSame(ContactImportStatus.STARTED, importContactsResponse.getStatus()); assertNull(importContactsResponse.getCreatedContactsCount()); + assertNull(importContactsResponse.getUpdatedContactsCount()); + assertNull(importContactsResponse.getContactsOverLimitCount());
58-63: Make validation message assertion resilient to localization/wording changesExact string equality is brittle. Assert key fragments instead.
- assertEquals("Invalid request body. Violations: contacts=Maximum 50000 contacts per request", exception.getMessage()); + assertTrue( + exception.getMessage().contains("Invalid request body.") && + exception.getMessage().contains("contacts=") && + exception.getMessage().contains("50000"), + "Unexpected validation message: " + exception.getMessage() + );
65-73: Avoid constructing 50k distinct Contact objects; use nCopies to cut test costYou only need to exceed the size limit; object identity/uniqueness doesn’t matter for @SiZe. This reduces memory and CPU.
- private List<Contact> generateContacts() { - List<Contact> contacts = new ArrayList<>(); - - for (int i = 0; i < 50001; i++) { - contacts.add(new Contact("stub_contact_%d@example.com".formatted(i), Map.of(), List.of(), List.of())); - } - - return contacts; - } + private List<Contact> generateContacts() { + Contact stub = new Contact("stub_contact@example.com", Map.of(), List.of(), List.of()); + // 50_001 > 50_000 to trigger @Size(max = 50_000) + return List.copyOf(java.util.Collections.nCopies(50_001, stub)); + }
53-54: Naming: contactId here represents an importIdThe path is /contacts/imports/{id}; this ID is for the import job, not a contact. Consider renaming usages to importId for clarity (and update BaseTest accordingly).
I can prep a follow-up PR renaming the field in BaseTest and its usages in these tests if you want.
Also applies to: 79-84
75-84: Boundary/null cases worth covering
- Exactly 50,000 contacts should succeed validation.
- Null request should fail fast with InvalidRequestBodyException (after implementing the null check suggested in the impl).
Sample tests you can add:
@Test void test_importContacts_exactly_50000_should_pass_validation() { var c = new Contact("ok@example.com", Map.of(), List.of(), List.of()); var request = new ImportContactsRequest(java.util.Collections.nCopies(50_000, c)); // If you want to avoid hitting HTTP, you can just call the validator through the API and expect no exception. assertDoesNotThrow(() -> api.importContacts(accountId, request)); } @Test void test_importContacts_null_request_should_fail_fast() { var ex = assertThrows(InvalidRequestBodyException.class, () -> api.importContacts(accountId, null)); assertTrue(ex.getMessage().contains("Invalid request body")); }src/main/java/io/mailtrap/api/contactimports/ContactImportsImpl.java (4)
19-22: Add a null request guard for consistent error semanticsBean Validation may not reliably handle null here; fail fast with InvalidRequestBodyException to keep error type/message consistent with other validation failures.
@Override public ImportContactsResponse importContacts(long accountId, ImportContactsRequest request) { - - validateRequestBodyAndThrowException(request); + if (request == null) { + throw new io.mailtrap.exception.InvalidRequestBodyException( + "Invalid request body. Violations: request=must not be null" + ); + } + validateRequestBodyAndThrowException(request);Note: add the import (see next comment) or fully qualify the class as above.
3-10: Import required if you prefer not to fully-qualify the exception classAdd this import to support the null-guard change cleanly.
import io.mailtrap.CustomValidator; import io.mailtrap.api.apiresource.ApiResourceWithValidation; import io.mailtrap.config.MailtrapConfig; import io.mailtrap.http.RequestData; +import io.mailtrap.exception.InvalidRequestBodyException;
23-28: Avoid mixing concatenation with String.format and use numeric specifiersSlightly cleaner and avoids accidental format surprises.
- return httpClient.post( - String.format(apiHost + "/api/accounts/%s/contacts/imports", accountId), - request, - new RequestData(), - ImportContactsResponse.class - ); + return httpClient.post( + String.format("%s/api/accounts/%d/contacts/imports", apiHost, accountId), + request, + new RequestData(), + ImportContactsResponse.class + );
31-38: RenamecontactIdtoimportIdand update format string ingetContactImportTo improve clarity—since the second parameter is the import-job ID—please update the method signature, its implementation, and all call sites/tests:
• src/main/java/io/mailtrap/api/contactimports/ContactImports.java
@@ -26,1 +26,1 @@ - ImportContactsResponse getContactImport(long accountId, long contactId); 8000 + ImportContactsResponse getContactImport(long accountId, long importId);• src/main/java/io/mailtrap/api/contactimports/ContactImportsImpl.java
@@ -32,1 +32,1 @@ - public ImportContactsResponse getContactImport(long accountId, long contactId) { + public ImportContactsResponse getContactImport(long accountId, long importId) { return httpClient.get( - String.format(apiHost + "/api/accounts/%s/contacts/imports/%s", accountId, contactId), + String.format("%s/api/accounts/%d/contacts/imports/%d", apiHost, accountId, importId), new RequestData(), ImportContactsResponse.class );• src/test/java/io/mailtrap/api/contactimports/ContactImportsImplTest.java
@@ -75,4 +75,6 @@ @Test void test_getContactImport() { - ImportContactsResponse contactImport = api.getContactImport(accountId, contactId); - assertEquals(contactId, contactImport.getId()); + // rename contactId → importId for clarity + long importId = contactId; + ImportContactsResponse contactImport = api.getContactImport(accountId, importId); + assertEquals(importId, contactImport.getId()); }These changes are confined to a new interface and its tests/examples, so they won’t break existing consumers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (17)
examples/java/io/mailtrap/examples/contactimports/ContactImports.java(1 hunks)src/main/java/io/mailtrap/api/contactimports/ContactImports.java(1 hunks)src/main/java/io/mailtrap/api/contactimports/ContactImportsImpl.java(1 hunks)src/main/java/io/mailtrap/client/api/MailtrapContactsApi.java(2 hunks)src/main/java/io/mailtrap/factory/MailtrapClientFactory.java(2 hunks)src/main/java/io/mailtrap/model/request/contactimports/Contact.java(1 hunks)src/main/java/io/mailtrap/model/request/contactimports/ImportContactsRequest.java(1 hunks)src/main/java/io/mailtrap/model/response/contactimports/ContactImportStatus.java(1 hunks)src/main/java/io/mailtrap/model/response/contactimports/ImportContactsResponse.java(1 hunks)src/test/java/io/mailtrap/api/accountaccesses/AccountAccessesImplTest.java(1 hunks)src/test/java/io/mailtrap/api/billing/BillingImplTest.java(1 hunks)src/test/java/io/mailtrap/api/contactimports/ContactImportsImplTest.java(1 hunks)src/test/java/io/mailtrap/api/inboxes/InboxesImplTest.java(10 hunks)src/test/java/io/mailtrap/testutils/BaseTest.java(1 hunks)src/test/resources/api/contactimports/createContactsImportRequest.json(1 hunks)src/test/resources/api/contactimports/createContactsImportResponse.json(1 hunks)src/test/resources/api/contactimports/getContactsImportResponse.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
src/main/java/io/mailtrap/api/contactimports/ContactImports.java (1)
examples/java/io/mailtrap/examples/contactimports/ContactImports.java (1)
ContactImports(14-42)
examples/java/io/mailtrap/examples/contactimports/ContactImports.java (1)
src/main/java/io/mailtrap/factory/MailtrapClientFactory.java (1)
MailtrapClientFactory(29-112)
src/test/java/io/mailtrap/api/contactimports/ContactImportsImplTest.java (5)
src/main/java/io/mailtrap/Constants.java (1)
Constants(6-15)src/main/java/io/mailtrap/exception/InvalidRequestBodyException.java (1)
InvalidRequestBodyException(6-12)src/main/java/io/mailtrap/factory/MailtrapClientFactory.java (1)
MailtrapClientFactory(29-112)src/test/java/io/mailtrap/testutils/BaseTest.java (1)
BaseTest(7-22)src/test/java/io/mailtrap/testutils/TestHttpClient.java (1)
TestHttpClient(19-160)
src/main/java/io/mailtrap/model/request/contactimports/ImportContactsRequest.java (2)
src/main/java/io/mailtrap/model/AbstractModel.java (1)
AbstractModel(10-29)src/main/java/io/mailtrap/model/request/contactimports/Contact.java (1)
Getter(9-22)
src/main/java/io/mailtrap/api/contactimports/ContactImportsImpl.java (3)
src/main/java/io/mailtrap/Constants.java (1)
Constants(6-15)src/main/java/io/mailtrap/CustomValidator.java (1)
CustomValidator(12-56)src/main/java/io/mailtrap/api/apiresource/ApiResourceWithValidation.java (1)
ApiResourceWithValidation(7-26)
src/main/java/io/mailtrap/model/request/contactimports/Contact.java (1)
src/main/java/io/mailtrap/model/request/contactimports/ImportContactsRequest.java (1)
Getter(10-17)
src/main/java/io/mailtrap/client/api/MailtrapContactsApi.java (1)
examples/java/io/mailtrap/examples/contactimports/ContactImports.java (1)
ContactImports(14-42)
src/main/java/io/mailtrap/factory/MailtrapClientFactory.java (1)
src/main/java/io/mailtrap/api/contactimports/ContactImportsImpl.java (1)
ContactImportsImpl(11-39)
🔇 Additional comments (9)
src/test/java/io/mailtrap/api/billing/BillingImplTest.java (1)
41-42: LGTM: flipped to (expected, actual) for clearer failuresBoth assertions now read naturally and produce better messages on failure.
src/test/resources/api/contactimports/createContactsImportResponse.json (1)
1-4: Fixture LGTMShape matches a typical create/import response (id + status). No issues.
src/test/resources/api/contactimports/getContactsImportResponse.json (1)
1-7: ContactImportStatus enum covers all API statusesReviewed the
ContactImportStatusenum and confirmed it defines all statuses returned by the API—CREATED,STARTED,FINISHED, andFAILED—so the fixture’s"finished"value is fully supported. No further action needed.src/test/resources/api/contactimports/createContactsImportRequest.json (1)
1-28: Request fixture matches model mappingFields map cleanly to Contact.java (email, fields, list_ids_included/excluded). This will exercise Map<String,Object> as intended.
src/main/java/io/mailtrap/client/api/MailtrapContactsApi.java (1)
3-3: New dependency wiring looks correctImporting ContactImports integrates the new API surface into the Contacts API. No concerns here.
src/main/java/io/mailtrap/factory/MailtrapClientFactory.java (2)
47-52: Wiring looks correct; nice addition of validation into contacts APIPassing CustomValidator into createContactsApi keeps validation consistent with other APIs. The overall factory composition remains cohesive.
54-60: No changes needed: constructor already uses the interfaceThe
MailtrapContactsApiclass is annotated with Lombok’s@RequiredArgsConstructor, and its third field is declared as theContactImportsinterface, not the implementation. The factory passing aContactImportsImplinstance still injects via the interface, so no update is required.Likely an incorrect or invalid review comment.
src/test/java/io/mailtrap/api/contactimports/ContactImportsImplTest.java (1)
27-43: Good use of factory + mock HTTP client to exercise validation and wiringSetup mirrors production wiring (validator included) and validates request/response via fixtures. Solid baseline.
src/main/java/io/mailtrap/api/contactimports/ContactImportsImpl.java (1)
13-16: Host selection is correctContact imports belong to the GENERAL host; setting apiHost = Constants.GENERAL_HOST is consistent with other general endpoints.
examples/java/io/mailtrap/examples/contactimports/ContactImports.java
Outdated
Show resolved
Hide resolved
src/main/java/io/mailtrap/model/response/contactimports/ContactImportStatus.java
Outdated
Show resolved
Hide resolved
src/main/java/io/mailtrap/model/response/contactimports/ImportContactsResponse.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Let's just fix naming so it's consistent with the API docs.
examples/java/io/mailtrap/examples/contactimports/ContactImports.java
Outdated
Show resolved
Hide resolved
src/main/java/io/mailtrap/api/contactimports/ContactImports.java
Outdated
Show resolved
Hide resolved
src/main/java/io/mailtrap/api/contactimports/ContactImportsImpl.java
Outdated
Show resolved
Hide resolved
src/main/java/io/mailtrap/model/response/contactimports/ImportContactsResponse.java
Outdated
Show resolved
Hide resolved
src/test/java/io/mailtrap/api/contactimports/ContactImportsImplTest.java
Outdated
Show resolved
Hide resolved
src/test/java/io/mailtrap/api/contactimports/ContactImportsImplTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/main/java/io/mailtrap/api/contactimports/ContactImports.java (1)
7-27: Confirm naming intent: ImportContactResponse vs ContactImportResponse.
Two similarly named types (create vs get) may confuse users; if aligned with API docs, fine—otherwise consider a clearer pair (e.g., CreateContactImportResponse vs ContactImportResponse).Would you like me to propose a rename across the API surface if we decide to adjust?
🧹 Nitpick comments (10)
src/test/java/io/mailtrap/testutils/BaseTest.java (1)
20-20: Optional: align type with other ID fieldsMost IDs here use Long; consider matching for consistency unless primitives are intentional.
Apply if desired:
-protected final long importId = 1L; +protected final Long importId = 1L;src/main/java/io/mailtrap/model/response/contactimports/ImportContactResponse.java (1)
3-6: Be defensive to future API fields.
Recommend ignoring unknown JSON to avoid breakage if the API adds properties.Apply:
package io.mailtrap.model.response.contactimports; +import com.fasterxml.jackson.annotation.JsonIgnoreProperties; import lombok.Data; -@Data +@Data +@JsonIgnoreProperties(ignoreUnknown = true) public class ImportContactResponse {src/main/java/io/mailtrap/model/response/contactimports/ContactImportResponse.java (1)
3-7: Harden JSON parsing for forward compatibility.
Same suggestion as for the create response: ignore unknowns.package io.mailtrap.model.response.contactimports; import com.fasterxml.jackson.annotation.JsonProperty; +import com.fasterxml.jackson.annotation.JsonIgnoreProperties; import lombok.Data; -@Data +@Data +@JsonIgnoreProperties(ignoreUnknown = true) public class ContactImportResponse {src/main/java/io/mailtrap/api/contactimports/ContactImports.java (2)
9-18: Clarify Javadoc wording.
Tighten wording and punctuation; specify that return is the created import metadata./** * Import contacts in bulk with support for custom fields and list management. * Existing contacts with matching email addresses will be updated automatically. - * Up to 50,000 contacts per request + * Up to 50,000 contacts per request. * * @param accountId unique account ID * @param request request body - * @return contact data + * @return created contact import metadata */ ImportContactResponse importContacts(long accountId, ImportContactsRequest request);
20-28: Use precise return description for GET.
Minor Javadoc fix to reflect returned type./** * Get Contact Import * * @param accountId unique account ID * @param importId unique Contact Import ID - * @return contact data + * @return contact import data */ ContactImportResponse getContactImport(long accountId, long importId);src/main/java/io/mailtrap/api/contactimports/ContactImportsImpl.java (5)
19-30: Null-safety before validation and cleaner URL formatting.
- Guard against null request to avoid Validator throwing an IllegalArgumentException.
- Prefer %d for numeric IDs and avoid string concatenation around apiHost for readability.
@Override public ImportContactResponse importContacts(long accountId, ImportContactsRequest request) { - validateRequestBodyAndThrowException(request); + java.util.Objects.requireNonNull(request, "request must not be null"); + validateRequestBodyAndThrowException(request); - return httpClient.post( - String.format(apiHost + "/api/accounts/%s/contacts/imports", accountId), + return httpClient.post( + String.format("%s/api/accounts/%d/contacts/imports", apiHost, accountId), request, new RequestData(), ImportContactResponse.class ); }
3-11: Import Objects explicitly (style).
Optional: add the import instead of FQN.package io.mailtrap.api.contactimports; import io.mailtrap.Constants; import io.mailtrap.CustomValidator; import io.mailtrap.api.apiresource.ApiResourceWithValidation; import io.mailtrap.config.MailtrapConfig; import io.mailtrap.http.RequestData; import io.mailtrap.model.request.contactimports.ImportContactsRequest; import io.mailtrap.model.response.contactimports.ContactImportResponse; import io.mailtrap.model.response.conta 8000 ctimports.ImportContactResponse; +import java.util.Objects;
32-39: Consistent URL formatting for GET.
Match the POST style with %d placeholders and no concatenation.@Override public ContactImportResponse getContactImport(long accountId, long importId) { return httpClient.get( - String.format(apiHost + "/api/accounts/%s/contacts/imports/%s", accountId, importId), + String.format("%s/api/accounts/%d/contacts/imports/%d", apiHost, accountId, importId), new RequestData(), ContactImportResponse.class ); }
19-39: Optional: extract a reusable path template.
Reduce duplication and risk of typos by centralizing the path.public class ContactImportsImpl extends ApiResourceWithValidation implements ContactImports { + private static final String CONTACT_IMPORTS_BASE = "%s/api/accounts/%d/contacts/imports"; + public ContactImportsImpl(final MailtrapConfig config, final CustomValidator validator) { super(config, validator); this.apiHost = Constants.GENERAL_HOST; } @@ public ImportContactResponse importContacts(long accountId, ImportContactsRequest request) { - Objects.requireNonNull(request, "request must not be null"); - validateRequestBodyAndThrowException(request); - - return httpClient.post( - String.format("%s/api/accounts/%d/contacts/imports", apiHost, accountId), + Objects.requireNonNull(request, "request must not be null"); + validateRequestBodyAndThrowException(request); + + return httpClient.post( + String.format(CONTACT_IMPORTS_BASE, apiHost, accountId), request, new RequestData(), ImportContactResponse.class ); } @@ public ContactImportResponse getContactImport(long accountId, long importId) { - return httpClient.get( - String.format("%s/api/accounts/%d/contacts/imports/%d", apiHost, accountId, importId), + return httpClient.get( + String.format(CONTACT_IMPORTS_BASE + "/%d", apiHost, accountId, importId), new RequestData(), ContactImportResponse.class ); }
19-39: Consider validating IDs are positive.
If upstream doesn’t guarantee it, add lightweight checks to fail fast on invalid IDs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
examples/java/io/mailtrap/examples/contactimports/ContactImports.java(1 hunks)src/main/java/io/mailtrap/api/contactimports/ContactImports.java(1 hunks)src/main/java/io/mailtrap/api/contactimports/ContactImportsImpl.java(1 hunks)src/main/java/io/mailtrap/model/response/contactimports/ContactImportResponse.java(1 hunks)src/main/java/io/mailtrap/model/response/contactimports/ContactImportStatus.java(1 hunks)src/main/java/io/mailtrap/model/response/contactimports/ImportContactResponse.java(1 hunks)src/test/java/io/mailtrap/api/contactimports/ContactImportsImplTest.java(1 hunks)src/test/java/io/mailtrap/testutils/BaseTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/test/java/io/mailtrap/api/contactimports/ContactImportsImplTest.java
- examples/java/io/mailtrap/examples/contactimports/ContactImports.java
- src/main/java/io/mailtrap/model/response/contactimports/ContactImportStatus.java
🧰 Additional context used
🧬 Code graph analysis (4)
src/main/java/io/mailtrap/model/response/contactimports/ImportContactResponse.java (1)
src/main/java/io/mailtrap/model/response/contactimports/ContactImportResponse.java (1)
Data(6-22)
src/main/java/io/mailtrap/model/response/contactimports/ContactImportResponse.java (1)
src/main/java/io/mailtrap/model/response/contactimports/ImportContactResponse.java (1)
Data(5-12)
src/main/java/io/mailtrap/api/contactimports/ContactImports.java (1)
examples/java/io/mailtrap/examples/contactimports/ContactImports.java (1)
ContactImports(14-42)
src/main/java/io/mailtrap/api/contactimports/ContactImportsImpl.java (3)
src/main/java/io/mailtrap/Constants.java (1)
Constants(6-15)src/main/java/io/mailtrap/CustomValidator.java (1)
CustomValidator(12-56)src/main/java/io/mailtrap/api/apiresource/ApiResourceWithValidation.java (1)
ApiResourceWithValidation(7-26)
🔇 Additional comments (4)
src/test/java/io/mailtrap/testutils/BaseTest.java (1)
20-20: LGTM: sensible shared test constant for Contact ImportsMatches fixtures using id 1 and keeps tests tidy.
src/main/java/io/mailtrap/model/response/contactimports/ImportContactResponse.java (1)
5-12: LGTM: minimal DTO fits the create-import response shape.
Fields and naming align with GET model and enum.src/main/java/io/mailtrap/model/response/contactimports/ContactImportResponse.java (1)
6-22: LGTM: response model matches fixture (snake_case counts).
Wrapper Longs for counts are appropriate since they can be absent until completion.src/main/java/io/mailtrap/api/contactimports/ContactImportsImpl.java (1)
12-17: Host selection looks correct.
Using GENERAL_HOST matches the public API base used elsewhere.
There was a problem hiding this comment.
Just one more rename and we're good, thanks!
src/main/java/io/mailtrap/api/contactimports/ContactImports.java
Outdated
Show resolved
Hide resol
8000
ved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/main/java/io/mailtrap/model/response/contactimports/ImportContactsResponse.java (1)
10-10: Ensure enum is resilient to unknown statusesThe enum should not throw on new API values; fallback to UNRECOGNIZED/UNKNOWN. This mirrors an existing comment.
#!/bin/bash set -euo pipefail # Check ContactImportStatus fallback presence and creator behavior status_file="$(fd ContactImportStatus.java src/main/java || true)" echo "Status file: $status_file" [ -n "$status_file" ] || { echo "ContactImportStatus.java not found"; exit 1; } rg -nP 'enum\s+ContactImportStatus' "$status_file" rg -n 'UNRECOGNIZED|UNKNOWN' "$status_file" || echo "MISSING_FALLBACK_ENUM" rg -n '@JsonCreator' "$status_file" >/dev/null && rg -nP 'return\s+(UNRECOGNIZED|UNKNOWN)\b' "$status_file" || echo "MISSING_FALLBACK_RETURN" # Ensure response models ignore unknown fields rg -n '@JsonIgnoreProperties\(ignoreUnknown\s*=\s*true\)' src/main/java/io/mailtrap/model/response/contactimports || echo "MISSING @JsonIgnoreProperties on response models"
🧹 Nitpick comments (7)
src/main/java/io/mailtrap/model/response/contactimports/ImportContactsResponse.java (2)
3-6: Harden model against additive API fieldsAdd JsonIgnoreProperties to avoid breakage when the API adds new fields.
package io.mailtrap.model.response.contactimports; +import com.fasterxml.jackson.annotation.JsonIgnoreProperties; import lombok.Data; -@Data +@Data +@JsonIgnoreProperties(ignoreUnknown = true) public class ImportContactsResponse {
8-8: Prefer wrapper for nullable IDsUse Long in case the field is absent during partial responses.
- private long id; + private Long id;src/main/java/io/mailtrap/api/contactimports/ContactImports.java (2)
16-17: Clarify return description for create-importDocument that the POST returns import metadata, not contact data.
- * @return contact data + * @return import metadata (id and status)
25-26: Clarify return description for get-importAlign wording with the model name.
- * @return contact data + * @return contact import datasrc/test/java/io/mailtrap/api/contactimports/ContactImportsImplTest.java (3)
62-62: Make assertion resilient to message formattingAssert on the salient part to avoid brittle failures.
- assertEquals("Invalid request body. Violations: contacts=Maximum 50000 contacts per request", exception.getMessage()); + assertTrue(exception.getMessage().contains("Maximum 50000 contacts per request"));
66-66: Pre-size the list to avoid reallocationsTiny perf/readability win in test helper.
- List<Contact> contacts = new ArrayList<>(); + List<Contact> contacts = new ArrayList<>(50001);
75-84: Add a test for unknown status fallback (once enum is updated)After adding UNRECOGNIZED/UNKNOWN, include a fixture with an unknown status and assert mapping to the fallback.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
src/main/java/io/mailtrap/api/contactimports/ContactImports.java(1 hunks)src/main/java/io/mailtrap/api/contactimports/ContactImportsImpl.java(1 hunks)src/main/java/io/mailtrap/model/response/contactimports/ImportContactsResponse.java(1 hunks)src/test/java/io/mailtrap/api/contactimports/ContactImportsImplTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/io/mailtrap/api/contactimports/ContactImportsImpl.java
🧰 Additional context used
🧬 Code graph analysis (2)
src/main/java/io/mailtrap/api/contactimports/ContactImports.java (1)
examples/java/io/mailtrap/examples/contactimports/ContactImports.java (1)
ContactImports(14-42)
src/test/java/io/mailtrap/api/contactimports/ContactImportsImplTest.java (4)
src/main/java/io/mailtrap/Constants.java (1)
Constants(6-15)src/main/java/io/mailtrap/factory/MailtrapClientFactory.java (1)
MailtrapClientFactory(29-112)src/test/java/io/mailtrap/testutils/BaseTest.java (1)
BaseTest(6-21)src/test/java/io/mailtrap/testutils/TestHttpClient.java (1)
TestHttpClient(19-160)
🔇 Additional comments (3)
src/main/java/io/mailtrap/api/contactimports/ContactImports.java (1)
18-27: Interface shape LGTMMethod names, signatures, and model pairing (create → ImportContactsResponse, get → ContactImportResponse) are consistent with the docs.
src/test/java/io/mailtrap/api/contactimports/ContactImportsImplTest.java (2)
30-44: Mocks and wiring LGTMEndpoints and factory wiring look correct for POST/GET flows.
54-56: Correct use of assertSame for enumsUsing assertSame with ContactImportStatus is appropriate.
Also applies to: 79-84
src/main/java/io/mailtrap/model/response/contactimports/ImportContactsResponse.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/main/java/io/mailtrap/model/response/contactimports/CreateContactsImportResponse.java (1)
3-6: Add @JsonIgnoreProperties to future-proof deserializationAPIs evolve; ignoring unknown fields at the DTO level reduces brittleness regardless of global ObjectMapper settings.
Apply:
+import com.fasterxml.jackson.annotation.JsonIgnoreProperties; import lombok.Data; -@Data +@Data +@JsonIgnoreProperties(ignoreUnknown = true) public class CreateContactsImportResponse {src/main/java/io/mailtrap/model/response/contactimports/ContactsImportResponse.java (1)
3-7: Ignore unknown fields for resilient parsingSame rationale as for the create DTO; safer against backend additions.
Apply:
import com.fasterxml.jackson.annotation.JsonProperty; +import com.fasterxml.jackson.annotation.JsonIgnoreProperties; import lombok.Data; -@Data +@Data +@JsonIgnoreProperties(ignoreUnknown = true) public class ContactsImportResponse {src/test/java/io/mailtrap/api/contactimports/ContactImportsImplTest.java (2)
58-63: Relax brittle assertion on exception messageExact message matching is fragile. Assert on the salient part to reduce false positives across locales/minor phrasing changes.
Apply:
- assertEquals("Invalid request body. Violations: contacts=Maximum 50000 contacts per request", exception.getMessage()); + String msg = exception.getMessage(); + assertTrue(msg.contains("Maximum 50000 contacts per request"), "Unexpected error: " + msg);
65-73: Use String.format for wider JDK compatibility
String#formattedrequires newer JDKs;String.formatworks on 8/11+.Apply:
- contacts.add(new Contact("stub_contact_%d@example.com".formatted(i), Map.of(), List.of(), List.of())); + contacts.add(new Contact(String.format("stub_contact_%d@example.com", i), Map.of(), List.of(), List.of()));If the project targets JDK 17+, feel free to keep
formatted; otherwise this change avoids build issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
src/main/java/io/mailtrap/api/contactimports/ContactImports.java(1 hunks)src/main/java/io/mailtrap/api/contactimports/ContactImportsImpl.java(1 hunks)src/main/java/io/mailtrap/model/response/contactimports/ContactsImportResponse.java(1 hunks)src/main/java/io/mailtrap/model/response/contactimports/CreateContactsImportResponse.java(1 hunks)src/test/java/io/mailtrap/api/contactimports/ContactImportsImplTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/io/mailtrap/api/contactimports/ContactImportsImpl.java
- src/main/java/io/mailtrap/api/contactimports/ContactImports.java
🧰 Additional context used
🧬 Code graph analysis (3)
src/main/java/io/mailtrap/model/response/contactimports/CreateContactsImportResponse.java (1)
src/main/java/io/mailtrap/model/response/contactimports/ContactsImportResponse.java (1)
Data(6-22)
src/main/java/io/mailtrap/model/response/contactimports/ContactsImportResponse.java (1)
src/main/java/io/mailtrap/model/response/contactimports/CreateContactsImportResponse.java (1)
Data(5-12)
src/test/java/io/mailtrap/api/contactimports/ContactImportsImplTest.java (5)
src/main/java/io/mailtrap/Constants.java (1)
Constants(6-15)src/main/java/io/mailtrap/exception/InvalidRequestBodyException.java (1)
InvalidRequestBodyException(6-12)src/main/java/io/mailtrap/factory/MailtrapClientFactory.java (1)
MailtrapClientFactory(29-112)src/test/java/io/mailtrap/testutils/BaseTest.java (1)
BaseTest(6-21)src/test/java/io/mailtrap/testutils/TestHttpClient.java (1)
TestHttpClient(19-160)
🔇 Additional comments (5)
src/main/java/io/mailtrap/model/response/contactimports/CreateContactsImportResponse.java (1)
8-10: LGTM: minimal, correct shape for creation response
idas primitive andstatusenum are sufficient for the create response.src/main/java/io/mailtrap/model/response/contactimports/ContactsImportResponse.java (1)
13-20: LGTM: optional counts correctly modeled as boxed LongUsing
Long(vs.long) handles absent values when import hasn’t finished yet.src/test/java/io/mailtrap/api/contactimports/ContactImportsImplTest.java (3)
31-36: Mocks wired to the correct base host and endpointsURLs align with Constants.GENERAL_HOST and expected paths for POST/GET. Good.
46-56: LGTM: happy-path create testEnum compared with
assertSameis appropriate; payload and assertions look good.
75-84: LGTM: retrieval test covers status and countersValidates id, terminal status, and all three counters.
Motivation
Next steps implementing Mailtrap API - Contact Imports
Changes
Summary by CodeRabbit
New Features
Documentation
Tests