Nitpick SDK updates #39
Conversation
WalkthroughBroad refactor replaces CustomValidator with MailtrapValidator across APIs, tightens parameter immutability, standardizes numeric URL placeholders, and updates HTTP client default headers. EmailTemplates interface gains full CRUD. Validation is strengthened in send flows and UpdateInboxRequest. Examples are renamed to *Example and adjusted. Minor mapper and serializer tweaks applied. Tests updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant F as MailtrapClientFactory
participant C as MailtrapClient
participant SA as Sending API (Impl)
participant V as MailtrapValidator
participant HC as HttpClient
Dev->>F: createMailtrapClient(config)
F->>F: build VALIDATOR (MailtrapValidator)
F->>C: wire APIs with VALIDATOR
Dev->>C: send(mail)
C->>SA: validateMailPayload(mail)
SA->>V: validate(mail)
V-->>SA: violations (if any)
SA->>SA: validateContentRules(mail)
alt valid
SA->>HC: POST /api/send ...
HC-->>SA: SendResponse
SA-->>C: SendResponse
C-->>Dev: SendResponse
else invalid
SA-->>Dev: throw InvalidRequestBodyException
end
sequenceDiagram
autonumber
actor Dev as Developer
participant ET as EmailTemplates API
participant HC as HttpClient
Dev->>ET: createEmailTemplate(accountId, request)
ET->>HC: POST /api/accounts/%d/email/templates
HC-->>ET: EmailTemplateResponse
ET-->>Dev: EmailTemplateResponse
Dev->>ET: getEmailTemplate(accountId, templateId)
ET->>HC: GET /api/accounts/%d/email/templates/%d
HC-->>ET: EmailTemplateResponse
ET-->>Dev: EmailTemplateResponse
Dev->>ET: updateEmailTemplate(accountId, templateId, request)
ET->>HC: PUT /api/accounts/%d/email/templates/%d
HC-->>ET: EmailTemplateResponse
ET-->>Dev: EmailTemplateResponse
Dev->>ET: deleteEmailTemplate(accountId, templateId)
ET->>HC: DELETE /api/accounts/%d/email/templates/%d
HC-->>ET: 204/void
ET-->>Dev: void
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
examples/java/io/mailtrap/examples/sending/EverythingExample.java (1)
74-76: Fix stale class reference in EverythingExample (compile error).
readAndEncodeAttachment still references Everything.class — replace with EverythingExample.class.
Location: examples/java/io/mailtrap/examples/sending/EverythingExample.java:75- try (InputStream inputStream = Everything.class.getClassLoader().getResourceAsStream(filename)) { + try (InputStream inputStream = EverythingExample.class.getClassLoader().getResourceAsStream(filename)) {src/main/java/io/mailtrap/http/impl/DefaultMailtrapHttpClient.java (1)
141-160: Broaden success handling to all 2xx; fix 5xx threshold; handle 204/empty bodies gracefully.Currently only 200 is treated as success; 201/202/204 are common and should succeed. Also 500 should be considered a server error (not only >500).
Apply this diff:
- final int statusCode = response.statusCode(); - if (statusCode == 200) { + final int statusCode = response.statusCode(); + if (statusCode >= 200 && statusCode < 300) { + // No content: return null (useful for DELETE/HEAD or Void responses) + if (response.body().isBlank()) { + return null; + } if (responseClassType != null) { if (String.class.equals(responseClassType)) { return responseClassType.cast(response.body()); } return Mapper.get().readValue(response.body(), responseClassType); } else if (responseJavaType != null) { return Mapper.get().readValue(response.body(), responseJavaType); } else { throw new IllegalArgumentException("Both responseType and typeReference are null"); } - } else if (statusCode >= 400 && statusCode < 500) { + } else if (statusCode >= 400 && statusCode < 500) { final ErrorResponse errorResponse = Mapper.get().readValue(response.body(), ErrorResponse.class); throw new HttpClientException(String.join(", ", errorResponse.getErrors()), statusCode); - } else if (statusCode > 500) { + } else if (statusCode >= 500) { throw new HttpServerException(String.format("Internal Server Error. HTTP response code (%d) received from the API server. Retry later or contact support.", statusCode), statusCode); }
🧹 Nitpick comments (79)
examples/java/io/mailtrap/examples/suppressions/SuppressionsExample.java (2)
19-20: Make locals final for consistency with PR goalMark
searchResponseanddeletedSuppressionasfinalto keep single-assignment style.- var searchResponse = client.sendingApi().suppressions() + final var searchResponse = client.sendingApi().suppressions() .search(ACCOUNT_ID, EMAIL); ... - var deletedSuppression = client.sendingApi().suppressions() + final var deletedSuppression = client.sendingApi().suppressions() .deleteSuppression(ACCOUNT_ID, searchResponse.get(0).getId());Also applies to: 25-26
19-20: Optional: de-duplicate chained callsHoist
client.sendingApi().suppressions()to a local to reduce repetition.- final var searchResponse = client.sendingApi().suppressions() - .search(ACCOUNT_ID, EMAIL); + final var suppressionsApi = client.sendingApi().suppressions(); + final var searchResponse = suppressionsApi.search(ACCOUNT_ID, EMAIL); ... - final var deletedSuppression = client.sendingApi().suppressions() - .deleteSuppression(ACCOUNT_ID, searchResponse.get(0).getId()); + final var deletedSuppression = + suppressionsApi.deleteSuppression(ACCOUNT_ID, searchResponse.get(0).getId());Also applies to: 25-26
src/test/java/io/mailtrap/testutils/BaseSendTest.java (3)
24-24: Nit: make helper static.getAddress has no instance dependency; prefer static for clarity.
Apply:
- private Address getAddress(final String email, final String name) { + private static Address getAddress(final String email, final String name) {
111-111: Fix typo in subject string."invalidvalid" → "invalid".
Apply:
- .subject("Sample invalidvalid mail subject") + .subject("Sample invalid mail subject")
13-23: Optional: mark constants static.These read-only constants can be static final to avoid per-instance fields and signal compile-time constants.
Apply:
- protected final long INBOX_ID = 4308; - protected final String BULK_AND_SANDBOX_TRUE_IN_SENDING_CONFIG = "Bulk mode is not applicable for Testing API"; - protected final String INBOX_ID_REQUIRED = "Testing API requires inbox ID"; - protected final String INVALID_REQUEST_EMPTY_BODY_FROM_EMAIL = "Invalid request body. Violations: from.email=must not be empty"; - protected final String TEMPLATE_UUID_IS_USED_SUBJECT_AND_TEXT_AND_HTML_SHOULD_BE_EMPTY = "When templateUuid is used, subject, text, and html must not be used"; - protected final String TEMPLATE_VARIABLES_SHOULD_BE_USED_WITH_TEMPLATE_UUID = "Mail templateVariables must only be used with templateUuid"; - protected final String MAIL_MUST_HAVE_SUBJECT_AND_EITHER_TEXT_OR_HTML = "Mail must have subject and either text or html when templateUuid is not provided"; - protected final String MAIL_MUST_NOT_BE_NULL = "Mail must not be null"; - protected final String BATCH_MAIL_MUST_NOT_BE_NULL = "BatchMail must not be null"; - protected final String SUBJECT_MUST_NOT_BE_NULL = "Subject must not be null or empty"; + protected static final long INBOX_ID = 4308; + protected static final String BULK_AND_SANDBOX_TRUE_IN_SENDING_CONFIG = "Bulk mode is not applicable for Testing API"; + protected static final String INBOX_ID_REQUIRED = "Testing API requires inbox ID"; + protected static final String INVALID_REQUEST_EMPTY_BODY_FROM_EMAIL = "Invalid request body. Violations: from.email=must not be empty"; + protected static final String TEMPLATE_UUID_IS_USED_SUBJECT_AND_TEXT_AND_HTML_SHOULD_BE_EMPTY = "When templateUuid is used, subject, text, and html must not be used"; + protected static final String TEMPLATE_VARIABLES_SHOULD_BE_USED_WITH_TEMPLATE_UUID = "Mail templateVariables must only be used with templateUuid"; + protected static final String MAIL_MUST_HAVE_SUBJECT_AND_EITHER_TEXT_OR_HTML = "Mail must have subject and either text or html when templateUuid is not provided"; + protected static final String MAIL_MUST_NOT_BE_NULL = "Mail must not be null"; + protected static final String BATCH_MAIL_MUST_NOT_BE_NULL = "BatchMail must not be null"; + protected static final String SUBJECT_MUST_NOT_BE_NULL = "Subject must not be null or empty";src/main/java/io/mailtrap/serialization/BlacklistsReportInfoDeserializer.java (1)
17-25: Avoid ObjectMapper cast; use ObjectCodec consistently (and drop redundant exception).Casting the codec to ObjectMapper is unnecessary; ObjectCodec already supports treeToValue. Also JacksonException extends IOException, so listing both is redundant.
Apply:
- public BlacklistsReportInfoWrapper deserialize(final JsonParser jsonParser, final DeserializationContext deserializationContext) throws IOException, JacksonException { - final JsonNode node = jsonParser.getCodec().readTree(jsonParser); + public BlacklistsReportInfoWrapper deserialize(final JsonParser jsonParser, final DeserializationContext deserializationContext) throws IOException { + final var codec = jsonParser.getCodec(); + final JsonNode node = codec.readTree(jsonParser); @@ - } else { - final ObjectMapper mapper = (ObjectMapper) jsonParser.getCodec(); - return new BlacklistsReportInfoWrapper(mapper.convertValue(node, BlacklistsReportInfo.class)); - } + } else { + return new BlacklistsReportInfoWrapper(codec.treeToValue(node, BlacklistsReportInfo.class)); + }src/test/java/io/mailtrap/serialization/BlacklistsReportInfoDeserializerTest.java (1)
37-66: Add a few field-level asserts to strengthen the object-path test.Validate mapped scalar fields and first report entry for completeness.
Apply:
final BlacklistsReportInfo blacklistsReportInfo = message.getBlacklistsReportInfoWrapper().getObjectValue(); assertEquals(BlacklistReportInfoResult.SUCCESS, blacklistsReportInfo.getResult()); assertEquals(1, blacklistsReportInfo.getReport().size()); + assertEquals("mailtrap.io", blacklistsReportInfo.getDomain()); + assertEquals("192.168.1.10", blacklistsReportInfo.getIp()); + assertEquals("SpamCheck", blacklistsReportInfo.getReport().get(0).getName()); + assertEquals("https://blacklist.example.com/report/spamcheck", blacklistsReportInfo.getReport().get(0).getUrl()); + assertFalse(blacklistsReportInfo.getReport().get(0).isInBlackList());src/main/java/io/mailtrap/serialization/AccountAccessResponseDeserializer.java (2)
41-75: Guard against missing/invalid fields to avoid NPEs and clearer errors.Current reads (e.g., rootNode.get("id").asLong()) can NPE if fields are absent; fail fast with explicit validation.
Apply:
- final ObjectMapper mapper = (ObjectMapper) p.getCodec(); - final JsonNode rootNode = mapper.readTree(p); + final ObjectMapper mapper = (ObjectMapper) p.getCodec(); + final JsonNode rootNode = mapper.readTree(p); + + if (!rootNode.hasNonNull("id")) { + throw new IllegalArgumentException("Missing required field: id"); + } + if (!rootNode.hasNonNull("specifier_type")) { + throw new IllegalArgumentException("Missing required field: specifier_type"); + } + if (!rootNode.hasNonNull("specifier")) { + throw new IllegalArgumentException("Missing required field: specifier"); + } @@ - final long id = rootNode.get("id").asLong(); - final JsonNode resourcesNode = rootNode.get("resources"); - final JsonNode permissionsNode = rootNode.get("permissions"); + final long id = rootNode.get("id").asLong(); + final JsonNode resourcesNode = rootNode.get("resources"); + final JsonNode permissionsNode = rootNode.get("permissions"); + if (resourcesNode != null && !resourcesNode.isArray()) { + throw new IllegalArgumentException("'resources' must be an array when present"); + } + if (permissionsNode != null && !permissionsNode.isObject()) { + throw new IllegalArgumentException("'permissions' must be an object when present"); + } @@ - final var resources = mapper.convertValue(resourcesNode, new TypeReference<List<Resource>>() { - }); - final var permissions = mapper.treeToValue(permissionsNode, Permission.class); + final var resources = resourcesNode == null + ? List.<Resource>of() + : mapper.convertValue(resourcesNode, new TypeReference<List<Resource>>() {}); + final var permission = permissionsNode == null + ? null + : mapper.treeToValue(permissionsNode, Permission.class); @@ - response.setPermission(permissions); + response.setPermission(permission);
52-75: Nit: singular naming for Permission improves clarity.Local var is a single Permission, but named plural.
Rename local variable to permission (as shown in the diff above).
src/test/java/io/mailtrap/serialization/AccountAccessResponseDeserializerTest.java (3)
21-50: Assert newly populated fields on response.Since the deserializer now sets specifierType/resources/permission, add lightweight checks.
Apply:
import io.mailtrap.model.response.accountaccesses.AccountAccessResponse; import io.mailtrap.model.response.accountaccesses.ApiTokenSpecifier; +import io.mailtrap.model.response.accountaccesses.SpecifierType; @@ final AccountAccessResponse response = mapper.readValue(json, AccountAccessResponse.class); assertInstanceOf(ApiTokenSpecifier.class, response.getSpecifier()); + assertEquals(SpecifierType.API_TOKEN, response.getSpecifierType()); + assertNotNull(response.getResources()); + assertEquals(0, response.getResources().size()); + assertNotNull(response.getPermission());
54-80: Mirror the extra assertions for Invite case.Apply:
final AccountAccessResponse response = mapper.readValue(json, AccountAccessResponse.class); assertInstanceOf(InviteSpecifier.class, response.getSpecifier()); + assertEquals(SpecifierType.INVITE, response.getSpecifierType()); + assertNotNull(response.getResources()); + assertEquals(0, response.getResources().size()); + assertNotNull(response.getPermission());
83-112: Mirror the extra assertions for User case.Apply:
final AccountAccessResponse response = mapper.readValue(json, AccountAccessResponse.class); assertInstanceOf(UserSpecifier.class, response.getSpecifier()); + assertEquals(SpecifierType.USER, response.getSpecifierType()); + assertNotNull(response.getResources()); + assertEquals(0, response.getResources().size()); + assertNotNull(response.getPermission());src/main/java/io/mailtrap/serialization/ErrorResponseDeserializer.java (1)
38-66: Small robustness tweaks (optional).Prefer hasNonNull to skip nulls; minor micro-cleanup on iteration.
Apply:
- if (node.has("errors")) { - final JsonNode errorsNode = node.get("errors"); + if (node.hasNonNull("errors")) { + final JsonNode errorsNode = node.get("errors"); if (errorsNode.isArray()) { - final Iterator<JsonNode> elements = errorsNode.elements(); - while (elements.hasNext()) { - errors.add(elements.next().asText()); - } + for (final JsonNode e : errorsNode) { + errors.add(e.asText()); + } } else if (errorsNode.isTextual()) { errors.add(errorsNode.asText()); } else if (errorsNode.isObject()) { final Iterator<Map.Entry<String, JsonNode>> fields = errorsNode.fields(); while (fields.hasNext()) { final Map.Entry<String, JsonNode> field = fields.next(); final JsonNode fieldValue = field.getValue(); if (fieldValue.isArray()) { - final Iterator<JsonNode> fieldElements = fieldValue.elements(); - while (fieldElements.hasNext()) { - errors.add(fieldElements.next().asText()); - } + for (final JsonNode fe : fieldValue) { + errors.add(fe.asText()); + } } else if (fieldValue.isTextual()) { errors.add(fieldValue.asText()); } } } - } else if (node.has("error") && node.get("error").isTextual()) { + } else if (node.hasNonNull("error") && node.get("error").isTextual()) { errors.add(node.get("error").asText()); }src/main/java/io/mailtrap/model/request/inboxes/UpdateInboxRequest.java (1)
15-18: Add a clear validation message and avoid sending explicit nulls on partial updates.
- Give a user-friendly message for the null case.
- Consider omitting null fields in InboxUpdateData to prevent unintended field clearing after the recent global ObjectMapper inclusion change.
Apply within the selected lines:
- @NotNull + @NotNull(message = "inbox must not be null")Outside the selected lines (imports and nested class annotation):
// add import import com.fasterxml.jackson.annotation.JsonInclude; // annotate the nested DTO @JsonInclude(JsonInclude.Include.NON_NULL) public static class InboxUpdateData { ... }Please confirm the API treats missing properties as “no change” and null as “clear,” so NON_NULL is desirable here.
examples/java/io/mailtrap/examples/testing/InboxesExample.java (1)
20-53: Make locals final for single-assignment consistency.
Matches the PR objective; none of these are reassigned.- var projects = testingClient.projects().getProjects(ACCOUNT_ID); + final var projects = testingClient.projects().getProjects(ACCOUNT_ID); @@ - var firstProject = projects.get(0); + final var firstProject = projects.get(0); @@ - var createdInbox = testingClient.inboxes().createInbox(ACCOUNT_ID, firstProject.getId(), new CreateInboxRequest(new CreateInboxRequest.InboxCreateData("test-inbox"))); + final var createdInbox = testingClient.inboxes().createInbox(ACCOUNT_ID, firstProject.getId(), new CreateInboxRequest(new CreateInboxRequest.InboxCreateData("test-inbox"))); @@ - var inboxes = testingClient.inboxes().getInboxes(ACCOUNT_ID); + final var inboxes = testingClient.inboxes().getInboxes(ACCOUNT_ID); @@ - long firstInboxId = inboxes.get(0).getId(); + final long firstInboxId = inboxes.get(0).getId(); @@ - var inboxAttributes = testingClient.inboxes().getInboxAttributes(ACCOUNT_ID, firstInboxId); + final var inboxAttributes = testingClient.inboxes().getInboxAttributes(ACCOUNT_ID, firstInboxId); @@ - var inboxUpdate = testingClient.inboxes().updateInbox(ACCOUNT_ID, firstInboxId, new UpdateInboxRequest(new UpdateInboxRequest.InboxUpdateData("updated-test-inbox", "mock"))); + final var inboxUpdate = testingClient.inboxes().updateInbox(ACCOUNT_ID, firstInboxId, new UpdateInboxRequest(new UpdateInboxRequest.InboxUpdateData("updated-test-inbox", "mock"))); @@ - var markAsRead = testingClient.inboxes().markAsRead(ACCOUNT_ID, firstInboxId); + final var markAsRead = testingClient.inboxes().markAsRead(ACCOUNT_ID, firstInboxId); @@ - var cleanInbox = testingClient.inboxes().cleanInbox(ACCOUNT_ID, firstInboxId); + final var cleanInbox = testingClient.inboxes().cleanInbox(ACCOUNT_ID, firstInboxId); @@ - var resetCredentials = testingClient.inboxes().resetCredentials(ACCOUNT_ID, firstInboxId); + final var resetCredentials = testingClient.inboxes().resetCredentials(ACCOUNT_ID, firstInboxId); @@ - var enableEmailAddress = testingClient.inboxes().enableEmailAddress(ACCOUNT_ID, firstInboxId); + final var enableEmailAddress = testingClient.inboxes().enableEmailAddress(ACCOUNT_ID, firstInboxId); @@ - var resetEmailAddresses = testingClient.inboxes().resetEmailAddresses(ACCOUNT_ID, firstInboxId); + final var resetEmailAddresses = testingClient.inboxes().resetEmailAddresses(ACCOUNT_ID, firstInboxId);examples/java/io/mailtrap/examples/testing/MessagesExample.java (1)
22-64: Prefer final for locals not reassigned.
Keeps examples consistent with the “final” policy.- var inboxes = testingClient.inboxes().getInboxes(ACCOUNT_ID); + final var inboxes = testingClient.inboxes().getInboxes(ACCOUNT_ID); @@ - long firstInboxId = inboxes.get(0).getId(); + final long firstInboxId = inboxes.get(0).getId(); @@ - var messages = testingClient.messages().getMessages(ACCOUNT_ID, firstInboxId, ListMessagesQueryParams.empty()); + final var messages = testingClient.messages().getMessages(ACCOUNT_ID, firstInboxId, ListMessagesQueryParams.empty()); @@ - long firstMessageId = messages.get(0).getId(); + final long firstMessageId = messages.get(0).getId(); @@ - var getMessage = testingClient.messages().getMessage(ACCOUNT_ID, firstInboxId, firstMessageId); + final var getMessage = testingClient.messages().getMessage(ACCOUNT_ID, firstInboxId, firstMessageId); @@ - var htmlAnalysis = testingClient.messages().getMessageHtmlAnalysis(ACCOUNT_ID, firstInboxId, firstMessageId); + final var htmlAnalysis = testingClient.messages().getMessageHtmlAnalysis(ACCOUNT_ID, firstInboxId, firstMessageId); @@ - String htmlMessage = testingClient.messages().getHtmlMessage(ACCOUNT_ID, firstInboxId, firstMessageId); + final String htmlMessage = testingClient.messages().getHtmlMessage(ACCOUNT_ID, firstInboxId, firstMessageId); @@ - String textMessage = testingClient.messages().getTextMessage(ACCOUNT_ID, firstInboxId, firstMessageId); + final String textMessage = testingClient.messages().getTextMessage(ACCOUNT_ID, firstInboxId, firstMessageId); @@ - String rawMessage = testingClient.messages().getRawMessage(ACCOUNT_ID, firstInboxId, firstMessageId); + final String rawMessage = testingClient.messages().getRawMessage(ACCOUNT_ID, firstInboxId, firstMessageId); @@ - String messageAsEml = testingClient.messages().getMessageAsEml(ACCOUNT_ID, firstInboxId, firstMessageId); + final String messageAsEml = testingClient.messages().getMessageAsEml(ACCOUNT_ID, firstInboxId, firstMessageId); @@ - String messageSource = testingClient.messages().getMessageSource(ACCOUNT_ID, firstInboxId, firstMessageId); + final String messageSource = testingClient.messages().getMessageSource(ACCOUNT_ID, firstInboxId, firstMessageId); @@ - var mailHeaders = testingClient.messages().getMailHeaders(ACCOUNT_ID, firstInboxId, firstMessageId); + final var mailHeaders = testingClient.messages().getMailHeaders(ACCOUNT_ID, firstInboxId, firstMessageId); @@ - var spamScore = testingClient.messages().getSpamScore(ACCOUNT_ID, firstInboxId, firstMessageId); + final var spamScore = testingClient.messages().getSpamScore(ACCOUNT_ID, firstInboxId, firstMessageId); @@ - var updateMessage = testingClient.messages().updateMessage(ACCOUNT_ID, firstInboxId, firstMessageId, new UpdateMessageRequest(new UpdateMessageRequest.MessageUpdateData("true"))); + final var updateMessage = testingClient.messages().updateMessage(ACCOUNT_ID, firstInboxId, firstMessageId, new UpdateMessageRequest(new UpdateMessageRequest.MessageUpdateData("true"))); @@ - var forwardMessage = testingClient.messages().forwardMessage(ACCOUNT_ID, firstInboxId, firstMessageId, new ForwardMessageRequest(FORWARD_EMAIL_TO)); + final var forwardMessage = testingClient.messages().forwardMessage(ACCOUNT_ID, firstInboxId, firstMessageId, new ForwardMessageRequest(FORWARD_EMAIL_TO));examples/java/io/mailtrap/examples/general/PermissionsExample.java (1)
28-41: Guard against empty resources to avoid IndexOutOfBounds in example.
Small robustness tweak; also align locals with final style.- var firstResource = client.generalApi().permissions().getResources(ACCOUNT_ID).get(0); - System.out.println(firstResource); + final var resources = client.generalApi().permissions().getResources(ACCOUNT_ID); + if (resources.isEmpty()) { + System.out.println("No resources found"); + return; + } + final var firstResource = resources.get(0); + System.out.println(firstResource); @@ - var request = new ManagePermissionsRequest(List.of( + final var request = new ManagePermissionsRequest(List.of(examples/java/io/mailtrap/examples/testing/ProjectsExample.java (1)
19-30: Align locals with “final everywhere” convention.
Make single-assignment locals final for consistency.Apply:
- var projects = testingClient.projects().getProjects(ACCOUNT_ID); + final var projects = testingClient.projects().getProjects(ACCOUNT_ID); @@ - long firstProjectId = projects.get(0).getId(); + final long firstProjectId = projects.get(0).getId(); @@ - var updatedProject = + final var updatedProject = @@ - var deletedProject = testingClient.projects().deleteProject(ACCOUNT_ID, firstProjectId); + final var deletedProject = testingClient.projects().deleteProject(ACCOUNT_ID, firstProjectId);examples/java/io/mailtrap/examples/sending/TemplateExample.java (1)
13-16: Extract template UUID into a constant (avoids magic string).public class TemplateExample { private static final String TOKEN = "<YOUR MAILTRAP TOKEN>"; private static final String SENDER_EMAIL = "[email protected]"; private static final String RECIPIENT_EMAIL = "[email protected]"; + private static final String TEMPLATE_UUID = "813e39db-c74a-4830-b037-0e6ba8b1fe88"; @@ - .templateUuid("813e39db-c74a-4830-b037-0e6ba8b1fe88") + .templateUuid(TEMPLATE_UUID)Also applies to: 27-27
examples/java/io/mailtrap/examples/contactlists/ContactListsExample.java (1)
18-21: Minor: make local final for consistency.- var contacts = client.contactsApi().contactLists().findAll(ACCOUNT_ID); + final var contacts = client.contactsApi().contactLists().findAll(ACCOUNT_ID);examples/java/io/mailtrap/examples/sending/ErrorsExample.java (1)
40-43: Make clientWithInvalidToken final for consistency.- var clientWithInvalidToken = MailtrapClientFactory.createMailtrapClient(new MailtrapConfig.Builder() + final var clientWithInvalidToken = MailtrapClientFactory.createMailtrapClient(new MailtrapConfig.Builder() .token("invalid token") .build());examples/java/io/mailtrap/examples/sending/MinimalExample.java (1)
10-10: Minimal → MinimalExample: looks good.Consider marking locals as final (final var ...) for consistency with the PR’s style.
examples/java/io/mailtrap/examples/general/AccountAccessExample.java (1)
9-9: AccountAccess → AccountAccessExample: OK. Add a small guard in the example.In main(), responses.get(0) may throw if the list is empty; gate the removal call behind an isEmpty() check.
examples/java/io/mailtrap/examples/bulk/BatchExample.java (1)
13-13: Batch → BatchExample: OK.Nit: java.util.Map import appears unused; remove to keep examples lean.
src/main/java/io/mailtrap/exception/http/HttpServerException.java (1)
7-7: final parameters in constructor: LGTM.Optional: declare an explicit serialVersionUID to silence serialization warnings in some setups.
examples/java/io/mailtrap/examples/contacts/ContactsExample.java (1)
14-14: Contacts → ContactsExample: LGTM.Minor: consider List.of() instead of Collections.emptyList() for symmetry with nearby List.of(...) usage; also consider final var for locals.
examples/java/io/mailtrap/examples/contactfields/ContactFieldsExample.java (1)
13-13: Rename looks good; make locals final for consistency with PR goal.Align remaining locals with the “final where possible” convention.
Apply this diff:
- var createResponse = client.contactsApi().contactFields() + final var createResponse = client.contactsApi().contactFields() .createContactField(ACCOUNT_ID, new CreateContactFieldRequest("Contact name", ContactFieldDataType.TEXT, "merge-tag")); @@ - var updateResponse = client.contactsApi().contactFields() + final var updateResponse = client.contactsApi().contactFields() .updateContactField(ACCOUNT_ID, createResponse.getId(), new UpdateContactFieldRequest("Updated name", "updated-merge-tag")); @@ - var allContactFields = client.contactsApi().contactFields() + final var allContactFields = client.contactsApi().contactFields() .getAllContactFields(ACCOUNT_ID); @@ - var contactField = client.contactsApi().contactFields() + final var contactField = client.contactsApi().contactFields() .getContactField(ACCOUNT_ID, createResponse.getId());src/main/java/io/mailtrap/model/mailvalidation/MailContentView.java (1)
33-39: Null-safety: guard constructor/input.Defensive null-checks avoid later NPEs when accessing mail fields.
Apply this diff:
+import java.util.Objects; @@ - public static ContentView of(final MailtrapMail content) { - return new MailContentView(content); + public static ContentView of(final MailtrapMail content) { + return new MailContentView(Objects.requireNonNull(content, "content")); } @@ - private MailContentView(final MailtrapMail content) { - this.mail = content; + private MailContentView(final MailtrapMail content) { + this.mail = Objects.requireNonNull(content, "content"); }src/main/java/io/mailtrap/http/RequestData.java (2)
43-49: Consider deterministic iteration order or immutability for built params.
If downstream code ever serializes query params to a string, LinkedHashMap preserves insertion order; alternatively return an unmodifiable map to prevent accidental mutation.Apply if desired:
- public static <T extends Optional<?>> Map<String, T> buildQueryParams(final Map.Entry<String, T>... entries) { - Map<String, T> params = new HashMap<>(); + public static <T extends Optional<?>> Map<String, T> buildQueryParams(final Map.Entry<String, T>... entries) { + final Map<String, T> params = new java.util.LinkedHashMap<>(); for (Map.Entry<String, T> entry : entries) { params.put(entry.getKey(), entry.getValue()); } - return params; + return java.util.Collections.unmodifiableMap(params); }
58-60: Use JDK 9+ Map.entry for brevity.
Equivalent and avoids AbstractMap reference.- public static <T extends Optional<?>> Map.Entry<String, T> entry(final String key, final T value) { - return new AbstractMap.SimpleEntry<>(key, value); - } + public static <T extends Optional<?>> Map.Entry<String, T> entry(final String key, final T value) { + return Map.entry(key, value); + }src/main/java/io/mailtrap/api/testingemails/TestingEmailsImpl.java (1)
26-41: Minor consistency nit: make local requestData final.
Matches project style and intent of immutability.- RequestData requestData = new RequestData(); + final RequestData requestData = new RequestData();src/test/java/io/mailtrap/testutils/TestHttpClient.java (3)
103-112: Close streams and fix charset determinism.
Use try-with-resources and UTF-8 to avoid resource leaks and default-charset variance.+import java.nio.charset.StandardCharsets; +import java.io.ByteArrayOutputStream; @@ - final InputStream requestInputStream = this.getClass().getClassLoader().getResourceAsStream(dataMock.getRequestFile()); + try (final InputStream requestInputStream = + this.getClass().getClassLoader().getResourceAsStream(dataMock.getRequestFile())) { @@ - final String requestPayloadMock = new BufferedReader(new InputStreamReader(requestInputStream)).lines().collect(Collectors.joining("\n")); + final String requestPayloadMock = + new String(requestInputStream.readAllBytes(), StandardCharsets.UTF_8); + - final boolean sameRequests = Mapper.get().readTree(requestPayloadMock).equals(Mapper.get().readTree(requestBody.toJson())); + final boolean sameRequests = + Mapper.get().readTree(requestPayloadMock).equals(Mapper.get().readTree(requestBody.toJson())); + } @@ - final InputStream responseInputStream = this.getClass().getClassLoader().getResourceAsStream(dataMock.getResponseFile()); + try (final InputStream responseInputStream = + this.getClass().getClassLoader().getResourceAsStream(dataMock.getResponseFile())) { @@ - final String responsePayloadMock = new BufferedReader(new InputStreamReader(responseInputStream)).lines().collect(Collectors.joining("\n")); + final String responsePayloadMock = + new String(responseInputStream.readAllBytes(), StandardCharsets.UTF_8); + }Also applies to: 133-138
111-121: Remove redundant second payload-mismatch check.
The later if (!sameRequests) is unreachable due to the earlier throw/continue.- if (!sameRequests) { - throw new AssertionError("No match for request payload: " + requestIdentifier); - }
135-136: Fix missing space in error message.
Minor string formatting nit.- throw new AssertionError("Failed to load response mock payload " + dataMock.getResponseFile() + " for request" + requestIdentifier); + throw new AssertionError("Failed to load response mock payload " + dataMock.getResponseFile() + " for request " + requestIdentifier);src/main/java/io/mailtrap/CustomValidator.java (1)
9-12: Javadoc: javax → jakarta.
Class uses jakarta.validation; update comment to match.src/test/java/io/mailtrap/CustomValidatorTest.java (1)
19-24: Make local errors final for consistency.
Tiny style nit to match the rest of this PR.- Map<String, String> errors = customValidator.validate(validAddress); + final Map<String, String> errors = customValidator.validate(validAddress);src/test/java/io/mailtrap/api/projects/ProjectsImplTest.java (1)
93-96: Swap expected/actual order in assertEquals for clarity.JUnit reports read better with expected first.
- assertEquals(deletedProject.getId(), projectId); + assertEquals(projectId, deletedProject.getId());src/test/java/io/mailtrap/api/accountaccesses/AccountAccessesImplTest.java (3)
47-58: Flip expected/actual in assertions for readability.Keeps test output consistent.
- assertEquals(accountAccessResponse.getId(), accountAccessId); + assertEquals(accountAccessId, accountAccessResponse.getId());
62-76: Flip expected/actual in assertions for readability.Same rationale as above.
- assertEquals(accountAccessResponse.getResources().get(0).getResourceId(), projectId); + assertEquals(projectId, accountAccessResponse.getResources().get(0).getResourceId());
80-84: Flip expected/actual in assertions for readability.Consistent assertion ordering.
- assertEquals(removeAccountAccessResponse.getId(), accountAccessId); + assertEquals(accountAccessId, removeAccountAccessResponse.getId());src/main/java/io/mailtrap/api/contactimports/ContactImports.java (1)
9-18: Tiny doc polish: end sentence with a period.Purely cosmetic, optional.
- * Up to 50,000 contacts per request + * Up to 50,000 contacts per request.src/test/java/io/mailtrap/api/sendingdomains/SendingDomainsImplTest.java (1)
25-47: Immutability tweaks look good.Local finals improve clarity and prevent accidental reassignment. Consider grouping repeated assertions with assertAll to keep failures compact and readable.
Also applies to: 48-51, 58-58, 67-67, 78-78
src/test/java/io/mailtrap/api/contactfields/ContactFieldsImplTest.java (1)
26-41: LGTM on final locals.Consistent immutability in tests. Optionally, use assertAll for grouped field checks to reduce noise.
Also applies to: 43-46, 53-53, 62-62, 71-71, 81-81
src/main/java/io/mailtrap/api/accounts/AccountsImpl.java (1)
21-24: Avoid unnecessary String.format with a constant string.No placeholders are used; simplify the call.
Apply this diff:
- return httpClient.getList( - String.format(apiHost + "/api/accounts"), - new RequestData(), - AccountsResponse.class - ); + return httpClient.getList(apiHost + "/api/accounts", new RequestData(), AccountsResponse.class);src/test/java/io/mailtrap/api/inboxes/InboxesImplTest.java (1)
69-166: Use expected, actual order in assertions.Several assertEquals calls pass actual first (e.g., assertEquals(createdInbox.getProjectId(), projectId)). Prefer assertEquals(expected, actual) for clearer diffs and failure messages.
src/test/java/io/mailtrap/api/permissions/PermissionsImplTest.java (1)
28-34: Final locals — consistent and clear.Minor: for symmetry, consider making response final as well in managePermissions().
Also applies to: 36-39, 46-46, 58-58
src/test/java/io/mailtrap/api/testingemails/TestingEmailsImplTest.java (1)
28-63: Good: finals and thorough negative-case coverage.Optional: parameterize the exception-path tests to reduce duplication; group multi-assert sections with assertAll for tighter failure output.
Also applies to: 69-69, 71-71, 79-79, 81-81, 89-89, 91-91, 99-99, 101-101, 109-109, 111-111, 119-119, 121-121, 128-129, 135-136, 138-139, 148-149, 151-152, 161-162, 164-165, 174-175, 177-178, 187-190, 192-193, 202-205, 207-208, 217-220, 222-223, 229-230, 236-237, 240-241
src/test/java/io/mailtrap/api/contacts/ContactsImplTest.java (1)
31-54: Final locals — consistent with the PR goals.Minor naming mismatch: method test_createContact_byId actually exercises updateContact; consider renaming for clarity.
Apply this diff to align the test name:
- void test_createContact_byId() { + void test_updateContact_byId() {Also applies to: 58-61, 78-85, 89-96
src/main/java/io/mailtrap/api/attachments/Attachments.java (1)
27-30: Good addition ofattachmentTypedocs; consider clarifying accepted values.If the API supports a constrained set (e.g., "image", "text", etc.), document it or reference the enum/source to reduce ambiguity. Also note explicitly that
nullis accepted (matches@Nullable).src/main/java/io/mailtrap/api/inboxes/InboxesImpl.java (1)
37-40: Reduce duplicated path formatting with a small helper.Repeated
String.format(apiHost + "...", accountId, inboxId)sequences are error‑prone to keep in sync. Extract a formatter to centralize:+private String accountPath(long accountId, String suffix) { + return String.format(apiHost + "/api/accounts/%d%s", accountId, suffix); +} + // examples -String.format(apiHost + "/api/accounts/%d/inboxes/%d", accountId, inboxId) +accountPath(accountId, String.format("/inboxes/%d", inboxId))Also applies to: 46-49, 58-62, 68-72, 78-82, 88-92, 98-102, 108-112, 118-121
src/test/java/io/mailtrap/api/attachments/AttachmentsImplTest.java (1)
41-46: Add a null check before using the list.Call
assertNotNull(attachments)beforeattachments.size()to avoid NPE masking the real failure signal.-assertEquals(1, attachments.size()); +assertNotNull(attachments); +assertEquals(1, attachments.size());src/main/java/io/mailtrap/model/mailvalidation/ResolvedMailView.java (1)
24-27: Constructor paramsfinal— LGTM; consider clarifying nullability contracts on getters.Several getters may return null if both item and base lack a value. Consider annotating them for clarity.
Example change (outside this hunk):
+import org.jetbrains.annotations.Nullable; ... - public String getSubject() { + @Nullable + public String getSubject() { String subject = item.getSubject(); if (StringUtils.isBlank(subject) && base != null) { subject = base.getSubject(); } return subject; }Apply similarly to
getText,getHtml,getTemplateUuid, andgetTemplateVariables(or document non‑null guarantees if enforced elsewhere).src/main/java/io/mailtrap/model/mailvalidation/ResolvedMailContentView.java (1)
32-34: Finalize + add null‑safety in factory/ctor.Guard against accidental nulls to fail fast with a clear message.
+import java.util.Objects; ... - public static ContentView of(final ResolvedMailView mailView) { - return new ResolvedMailContentView(mailView); + public static ContentView of(final ResolvedMailView mailView) { + return new ResolvedMailContentView(Objects.requireNonNull(mailView, "mailView must not be null")); } ... - private ResolvedMailContentView(final ResolvedMailView mailView) { - this.mailView = mailView; + private ResolvedMailContentView(final ResolvedMailView mailView) { + this.mailView = Objects.requireNonNull(mailView, "mailView must not be null"); }Also applies to: 36-38
src/main/java/io/mailtrap/api/billing/BillingImpl.java (1)
19-21: Minor: avoid pre-concatenation inside String.format.Slight readability win by formatting both host and id in one shot.
Apply this diff:
- String.format(apiHost + "/api/accounts/%d/billing/usage", accountId), + String.format("%s/api/accounts/%d/billing/usage", apiHost, accountId),src/test/java/io/mailtrap/api/bulkemails/BulkEmailsImplTest.java (1)
133-141: Good additions: assert messageIds[0] in success paths.These assertions improve verification beyond isSuccess. Consider also asserting list sizes to catch partial responses.
Also applies to: 146-154, 159-167, 172-180, 185-195, 200-210
src/main/java/io/mailtrap/api/apiresource/ApiResource.java (1)
25-30: Optional: fail fast on null config/token.Guarding early gives clearer errors and avoids NPEs later. This is behavior‑affecting; consider introducing under a minor release flag if external users sometimes omit token in tests.
Apply this diff and import:
+import java.util.Objects; @@ - protected ApiResource(final MailtrapConfig config) { - this.httpClient = config.getHttpClient() != null + protected ApiResource(final MailtrapConfig config) { + Objects.requireNonNull(config, "config must not be null"); + this.httpClient = config.getHttpClient() != null ? config.getHttpClient() : new DefaultMailtrapHttpClient(config); - this.token = config.getToken(); + this.token = Objects.requireNonNull(config.getToken(), "token must not be null"); }src/test/java/io/mailtrap/client/MailtrapClientTest.java (1)
99-109: Nice: batch assertions check size and first id.Solid coverage. You might add an isSuccess assertion alongside id checks for symmetry with single-send tests.
Also applies to: 119-124, 129-137
src/main/java/io/mailtrap/api/permissions/PermissionsImpl.java (1)
21-27: Optional: validate request is non-null (consistency with other APIs).Other write endpoints validate payloads. Either switch to ApiResourceWithValidation or add a lightweight null check to keep message format consistent.
Apply this diff (keeping current class hierarchy):
public ManagePermissionsResponse managePermissions(final long accountAccessId, final long accountId, final ManagePermissionsRequest request) { + if (request == null) { + throw new InvalidRequestBodyException("Invalid request body. Violations: request must not be null"); + } return httpClient.put( String.format(apiHost + "/api/accounts/%d/account_accesses/%d/permissions/bulk", accountId, accountAccessId), request, new RequestData(), ManagePermissionsResponse.class ); }And minor formatting nit:
- String.format(apiHost + "/api/accounts/%d/account_accesses/%d/permissions/bulk", accountId, accountAccessId), + String.format("%s/api/accounts/%d/account_accesses/%d/permissions/bulk", apiHost, accountId, accountAccessId),src/test/java/io/mailtrap/api/contactimports/ContactImportsImplTest.java (2)
60-63: Make assertion resilient to future validator copy changes.Exact message equality is brittle. Prefer contains/startsWith to lock only on the key violation.
Apply this diff:
- assertEquals("Invalid request body. Violations: contacts=Maximum 50000 contacts per request", exception.getMessage()); + assertTrue(exception.getMessage().contains("contacts=Maximum 50000 contacts per request"));
66-73: Reduce test data construction overhead.Generating 50,001 distinct Contact objects is heavy and unnecessary if only the list size matters. Use nCopies with a single stub instance.
Apply this diff and add import for java.util.Collections:
- final List<Contact> contacts = new ArrayList<>(); - - for (int i = 0; i < 50001; i++) { - contacts.add(new Contact(String.format("stub_contact_%[email protected]", i), Map.of(), List.of(), List.of())); - } - - return contacts; + final Contact stub = new Contact("[email protected]", Map.of(), List.of(), List.of()); + return new ArrayList<>(java.util.Collections.nCopies(50001, stub));src/main/java/io/mailtrap/api/emailtemplates/EmailTemplates.java (1)
11-18: Naming consistency (nit): consider “listEmailTemplates” instead of “getAllTemplates”.Aligns with list* naming across other APIs.
src/main/java/io/mailtrap/api/contacts/ContactsImpl.java (1)
24-29: Use %d for numeric IDs for consistency with the rest of the PR.Currently using %s for accountId here while other methods use %d.
Apply this diff:
- String.format(apiHost + "/api/accounts/%s/contacts", accountId), + String.format(apiHost + "/api/accounts/%d/contacts", accountId),src/main/java/io/mailtrap/http/impl/DefaultMailtrapHttpClient.java (4)
107-115: Preserve thread interrupt status when catching InterruptedException.Combine-catch swallows interrupts; reset it before rethrowing.
Apply this diff:
- try { - final var send = httpClient.send(request, HttpResponse.BodyHandlers.ofString()); - - return handleResponse(send, responseType); - } catch (final InterruptedException | IOException e) { - throw new BaseMailtrapException("An error has occurred while sending request", e); - } + try { + final var send = httpClient.send(request, HttpResponse.BodyHandlers.ofString()); + return handleResponse(send, responseType); + } catch (final InterruptedException e) { + Thread.currentThread().interrupt(); + throw new BaseMailtrapException("An error has occurred while sending request", e); + } catch (final IOException e) { + throw new BaseMailtrapException("An error has occurred while sending request", e); + }
117-125: Apply the same interrupt handling for JavaType requests.Apply this diff:
- try { - final var send = httpClient.send(request, HttpResponse.BodyHandlers.ofString()); - - return handleResponse(send, responseType); - } catch (final InterruptedException | IOException e) { - throw new BaseMailtrapException("An error has occurred while sending request", e); - } + try { + final var send = httpClient.send(request, HttpResponse.BodyHandlers.ofString()); + return handleResponse(send, responseType); + } catch (final InterruptedException e) { + Thread.currentThread().interrupt(); + throw new BaseMailtrapException("An error has occurred while sending request", e); + } catch (final IOException e) { + throw new BaseMailtrapException("An error has occurred while sending request", e); + }
165-176: Avoid duplicate headers; allow safe overrides of defaults.Using header() appends values; if users set Accept/Content-Type, duplicates may be sent. Prefer setHeader() to replace; optionally block Authorization/User-Agent overrides.
Apply this minimal diff:
- final Map<String, Object> headers = new HashMap<>(requestData.getHeaders()); - for (Map.Entry<String, ?> entry : headers.entrySet()) { - requestBuilder = requestBuilder.header(entry.getKey(), entry.getValue().toString()); - } + final Map<String, Object> headers = new HashMap<>(requestData.getHeaders()); + for (Map.Entry<String, ?> entry : headers.entrySet()) { + requestBuilder = requestBuilder.setHeader(entry.getKey(), String.valueOf(entry.getValue())); + }Optional hardening (follow-up): skip overrides of Authorization/User-Agent to prevent header injection.
169-171: Optional: set Content-Type only for requests with a body.It’s harmless on GET/DELETE/HEAD but unnecessary; consider moving Content-Type to post/put/patch call sites.
src/main/java/io/mailtrap/api/accountaccesses/AccountAccessesImpl.java (1)
39-44: Parameter order is atypical; interface and callers are consistent.AccountAccesses.removeAccountAccess(long accountAccessId, long accountId) is declared at src/main/java/io/mailtrap/api/accountaccesses/AccountAccesses.java:34, implemented at src/main/java/io/mailtrap/api/accountaccesses/AccountAccessesImpl.java:39, and exercised by tests/examples (src/test/java/io/mailtrap/api/accountaccesses/AccountAccessesImplTest.java:80, examples/java/io/mailtrap/examples/general/AccountAccessExample.java:29). If you want to match the common (accountId, resourceId) ordering, change the public interface and update callers or add a clearly-named wrapper to avoid silent argument inversion.
src/test/java/io/mailtrap/api/emailtemplates/EmailTemplatesImplTest.java (4)
54-59: Avoid order-sensitive list assertions unless ordering is guaranteed.If the API doesn’t guarantee stable ordering, prefer asserting that any element matches the expected ID/category.
Do responses guarantee ordering? If not, consider:
-assertEquals(emailTemplateId, allTemplates.get(0).getId()); -assertEquals("Promotion", allTemplates.get(0).getCategory()); +assertTrue(allTemplates.stream().anyMatch(t -> t.getId() == emailTemplateId)); +assertTrue(allTemplates.stream().anyMatch(t -> "Promotion".equals(t.getCategory())));
80-92: Make validation-message checks resilient to provider/localization changes.Relying on exact Bean Validation default text is brittle. Match by path and a regex for the range.
Apply:
- final InvalidRequestBodyException exception = assertThrows(InvalidRequestBodyException.class, () -> api.createEmailTemplate(accountId, request)); - - assertTrue(exception.getMessage().contains("emailTemplate.name")); - assertTrue(exception.getMessage().contains("size must be between 1 and 255")); + final InvalidRequestBodyException exception = assertThrows(InvalidRequestBodyException.class, () -> api.createEmailTemplate(accountId, request)); + final var msg = exception.getMessage(); + assertTrue(msg.contains("emailTemplate.name")); + assertTrue(msg.matches("(?s).*size must be between \\d+ and \\d+.*"));
95-101: Reduce coupling to exception prefix text.Assert on the violation itself, not the “Violations:” wrapper.
Apply:
- assertTrue(exception.getMessage().contains("Violations: emailTemplate=must not be null")); + assertTrue(exception.getMessage().contains("emailTemplate=must not be null"));
131-143: Mirror the resilient assertion approach for subject-size validation.Apply:
- final InvalidRequestBodyException exception = assertThrows(InvalidRequestBodyException.class, () -> api.updateEmailTemplate(accountId, emailTemplateId, request)); - - assertTrue(exception.getMessage().contains("emailTemplate.subject")); - assertTrue(exception.getMessage().contains("size must be between 1 and 255")); + final InvalidRequestBodyException exception = assertThrows(InvalidRequestBodyException.class, () -> api.updateEmailTemplate(accountId, emailTemplateId, request)); + final var msg = exception.getMessage(); + assertTrue(msg.contains("emailTemplate.subject")); + assertTrue(msg.matches("(?s).*size must be between \\d+ and \\d+.*"));src/main/java/io/mailtrap/factory/MailtrapClientFactory.java (1)
106-113: Context holder now includes inboxId.Looks fine assuming builder permits null. If not guaranteed, guard or document.
src/main/java/io/mailtrap/api/apiresource/SendApiResource.java (1)
88-114: Remove duplicated subject/text/html check.You already gate on subjectTextHtmlEmpty; the later redundant check adds noise.
Apply:
if (templateUuidBlank) { if (subjectTextHtmlEmpty) { throw new InvalidRequestBodyException("Mail must have subject and either text or html when templateUuid is not provided"); } if (MapUtils.isNotEmpty(v.getTemplateVariables())) { throw new InvalidRequestBodyException("Mail templateVariables must only be used with templateUuid"); } - if (StringUtils.isBlank(v.getSubject())) { - throw new InvalidRequestBodyException("Subject must not be null or empty"); - } - if (StringUtils.isBlank(v.getText()) && StringUtils.isBlank(v.getHtml())) { - throw new InvalidRequestBodyException("Mail must have subject and either text or html when templateUuid is not provided"); - } + if (StringUtils.isBlank(v.getSubject())) { + throw new InvalidRequestBodyException("Subject must not be null or empty"); + }src/main/java/io/mailtrap/api/contactfields/ContactFieldsImpl.java (2)
32-42: Make request parameter final for consistency.Aligns with repo-wide immutability goal.
Apply:
- public ContactFieldResponse createContactField(final long accountId, CreateContactFieldRequest request) { + public ContactFieldResponse createContactField(final long accountId, final CreateContactFieldRequest request) {
54-64: Make update request parameter final for consistency.Apply:
- public ContactFieldResponse updateContactField(final long accountId, final long fieldId, UpdateContactFieldRequest request) { + public ContactFieldResponse updateContactField(final long accountId, final long fieldId, final UpdateContactFieldRequest request) {src/main/java/io/mailtrap/api/messages/MessagesImpl.java (4)
35-41: Add @nonnull to request parameter.
Fail-fast on null payloads; Lombok is already in use.Apply this diff:
- public MessageResponse updateMessage(final long accountId, final long inboxId, final long messageId, final UpdateMessageRequest request) { + public MessageResponse updateMessage(final long accountId, final long inboxId, final long messageId, @NonNull final UpdateMessageRequest request) {
54-58: Optional: validate/normalize query params.
- Consider rejecting page < 1 and last_id < 1 to avoid 4xx from the API.
- Optionally trim search to avoid sending whitespace-only queries.
68-74: Add @nonnull to request parameter.
Same rationale as updateMessage(..).Apply this diff:
- public ForwardMessageResponse forwardMessage(final long accountId, final long inboxId, final long messageId, final ForwardMessageRequest request) { + public ForwardMessageResponse forwardMessage(final long accountId, final long inboxId, final long messageId, @NonNull final ForwardMessageRequest request) {
26-31: Use "%s" for apiHost and correct numeric formatters (%d); fix remaining occurrences.
- MessagesImpl.java — replace concatenation in String.format:
- String.format(apiHost + "/api/accounts/%d/inboxes/%d/messages/%d", accountId, inboxId, messageId), + String.format("%s/api/accounts/%d/inboxes/%d/messages/%d", apiHost, accountId, inboxId, messageId),
- src/main/java/io/mailtrap/api/contacts/ContactsImpl.java:25 — replace concatenation and use %d for numeric ID:
- String.format(apiHost + "/api/accounts/%s/contacts", accountId), + String.format("%s/api/accounts/%d/contacts", apiHost, accountId),
examples/java/io/mailtrap/examples/contactimports/ContactImportsExample.java
Show resolved
Hide resolved
src/main/java/io/mailtrap/api/apiresource/ApiResourceWithValidation.java
Show resolved
Hide resolved
| public class AccountAccessesImpl extends ApiResource implements AccountAccesses { | ||
|
|
||
| public AccountAccessesImpl(MailtrapConfig config) { | ||
| public AccountAccessesImpl(final MailtrapConfig config) { |
There was a problem hiding this comment.
Wow didn't know that final is available for method arguments. Java is getting truly modern 🙂
There was a problem hiding this comment.
Yeah, we can make almost everything final - even method params in an interface 😎
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
examples/java/io/mailtrap/examples/contactimports/ContactImportsExample.java (1)
11-11: Class/file name alignment with Example suffix: LGTM.Matches package and file; resolves the earlier mismatch. Imports are lean now.
src/main/java/io/mailtrap/factory/MailtrapClientFactory.java (1)
115-127: Resolved: don’t close ValidatorFactory before using Validator.The previous issue is addressed; no try-with-resources. Good.
🧹 Nitpick comments (11)
examples/java/io/mailtrap/examples/contactimports/ContactImportsExample.java (2)
29-31: Optional: wrap API calls with minimal error handling.Keeps the example terse while surfacing failures clearly.
- final var createResponse = client.contactsApi().contactImports() - .importContacts(ACCOUNT_ID, importRequest); - System.out.println(createResponse); + try { + final var createResponse = client.contactsApi().contactImports() + .importContacts(ACCOUNT_ID, importRequest); + System.out.println(createResponse); + // ... + } catch (Exception e) { + System.err.println("Contact import failed: " + e.getMessage()); + e.printStackTrace(System.err); + return; + }
34-36: Optional: note async completion and poll in real usage.If imports complete asynchronously, consider a short poll with backoff before fetching by ID to avoid transient 404/empty states in non-demo code.
examples/java/io/mailtrap/examples/testing/ProjectsExample.java (2)
22-22: Make firstProjectId final to match single-assignment style.Apply this diff:
- long firstProjectId = projects.get(0).getId(); + final long firstProjectId = projects.get(0).getId();
24-26: Extract request into a named variable for readability.
Reduces nesting and improves scanability.- final var updatedProject = - testingClient.projects().updateProject(ACCOUNT_ID, firstProjectId, new CreateUpdateProjectRequest(new CreateUpdateProjectRequest.ProjectData("mock project"))); + final var request = + new CreateUpdateProjectRequest(new CreateUpdateProjectRequest.ProjectData("mock project")); + final var updatedProject = + testingClient.projects().updateProject(ACCOUNT_ID, firstProjectId, request);examples/java/io/mailtrap/examples/testing/AttachmentsExample.java (1)
29-29: Avoid passing null for optional query params.For consistency with
ListMessagesQueryParams.empty(), prefer a typed empty/sentinel value or an overload instead ofnull—if available in the API. Please confirmnullis explicitly supported here.src/main/java/io/mailtrap/api/contacts/ContactsImpl.java (1)
33-37: Avoid URLEncoder for path segments; use path-segment encoding.URLEncoder is for form/query encoding and replaces spaces with '+', which is not ideal in path segments. Emails rarely have spaces, but safer to encode as %20.
Apply:
- String.format(apiHost + "/api/accounts/%d/contacts/%s", accountId, - URLEncoder.encode(idOrEmail, StandardCharsets.UTF_8)), + String.format(apiHost + "/api/accounts/%d/contacts/%s", accountId, + encodePathSegment(idOrEmail)),Add helper (outside the shown range):
private static String encodePathSegment(final String segment) { // Convert '+' (produced by URLEncoder) to '%20' for path safety return URLEncoder.encode(segment, StandardCharsets.UTF_8).replace("+", "%20"); }Also applies to: 43-47
src/main/java/io/mailtrap/factory/MailtrapClientFactory.java (1)
115-127: Cache the Validator/Factory to avoid repeated allocation and potential leaks.
Validation.buildDefaultValidatorFactory()can be expensive; building per client risks resource churn. Prefer a single cached instance.Apply within this method:
- private static CustomValidator createValidator() { - return new CustomValidator(Validation.buildDefaultValidatorFactory().getValidator()); - } + private static CustomValidator createValidator() { + return CACHED_VALIDATOR; + }Add once at class level (outside the shown range):
private static final jakarta.validation.ValidatorFactory VALIDATOR_FACTORY = Validation.buildDefaultValidatorFactory(); private static final CustomValidator CACHED_VALIDATOR = new CustomValidator(VALIDATOR_FACTORY.getValidator()); // Optionally: // static { Runtime.getRuntime().addShutdownHook(new Thread(VALIDATOR_FACTORY::close)); }examples/java/io/mailtrap/examples/testing/InboxesExample.java (4)
10-12: Avoid hardcoded secrets/IDs; prefer environment variables.Prevents accidental commits of real tokens and eases local setup.
Apply this diff:
- private static final String TOKEN = "<YOUR MAILTRAP TOKEN>"; - private static final long ACCOUNT_ID = 1L; + // Prefer env vars to avoid committing secrets and hardcoded IDs. + private static final String TOKEN = System.getenv("MAILTRAP_TOKEN"); + private static final long ACCOUNT_ID = Long.parseLong(System.getenv().getOrDefault("MAILTRAP_ACCOUNT_ID", "0"));Optionally guard at startup:
if (TOKEN == null || TOKEN.isBlank()) { System.err.println("Set MAILTRAP_TOKEN env var before running this example."); return; }
20-24: Handle the “no projects” branch with a friendly message.Improves UX when an empty account is used in the example.
Add:
else { System.out.printf("No projects found for accountId=%d%n", ACCOUNT_ID); }
28-31: Prefer operating on the created inbox ID; reduce an extra round-trip.Using the freshly created inbox avoids acting on an unrelated “first” inbox and saves one API call.
Example (adjust accessor to actual return type):
final var firstInboxId = createdInbox.getId();If you keep the current approach, at least make the variable final for consistency:
- long firstInboxId = inboxes.get(0).getId(); + final var firstInboxId = inboxes.get(0).getId();
39-53: Add a caution note: following calls are destructive.Mark-as-read, clean, reset-credentials, enable/reset addresses change state; warn users running the example.
Add a comment before these calls:
// CAUTION: The following operations mutate inbox state in the testing environment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
examples/java/io/mailtrap/examples/contactfields/ContactFieldsExample.java(2 hunks)examples/java/io/mailtrap/examples/contactimports/ContactImportsExample.java(2 hunks)examples/java/io/mailtrap/examples/contactlists/ContactListsExample.java(2 hunks)examples/java/io/mailtrap/examples/contacts/ContactsExample.java(2 hunks)examples/java/io/mailtrap/examples/general/AccountAccessExample.java(2 hunks)examples/java/io/mailtrap/examples/general/PermissionsExample.java(2 hunks)examples/java/io/mailtrap/examples/sending/BatchExample.java(1 hunks)examples/java/io/mailtrap/examples/sending/ErrorsExample.java(3 hunks)examples/java/io/mailtrap/examples/sending/EverythingExample.java(2 hunks)examples/java/io/mailtrap/examples/suppressions/SuppressionsExample.java(2 hunks)examples/java/io/mailtrap/examples/testing/AttachmentsExample.java(2 hunks)examples/java/io/mailtrap/examples/testing/InboxesExample.java(1 hunks)examples/java/io/mailtrap/examples/testing/MessagesExample.java(3 hunks)examples/java/io/mailtrap/examples/testing/ProjectsExample.java(2 hunks)src/main/java/io/mailtrap/api/contacts/ContactsImpl.java(1 hunks)src/main/java/io/mailtrap/factory/MailtrapClientFactory.java(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- examples/java/io/mailtrap/examples/contacts/ContactsExample.java
- examples/java/io/mailtrap/examples/contactlists/ContactListsExample.java
- examples/java/io/mailtrap/examples/general/PermissionsExample.java
- examples/java/io/mailtrap/examples/contactfields/ContactFieldsExample.java
- examples/java/io/mailtrap/examples/sending/ErrorsExample.java
🧰 Additional context used
🧬 Code graph analysis (3)
src/main/java/io/mailtrap/api/contacts/ContactsImpl.java (1)
src/main/java/io/mailtrap/Constants.java (1)
Constants(6-15)
examples/java/io/mailtrap/examples/testing/InboxesExample.java (1)
src/main/java/io/mailtrap/factory/MailtrapClientFactory.java (1)
MailtrapClientFactory(32-128)
examples/java/io/mailtrap/examples/sending/BatchExample.java (2)
examples/java/io/mailtrap/examples/bulk/BatchExample.java (1)
BatchExample(13-43)examples/java/io/mailtrap/examples/testing/BatchExample.java (1)
BatchExample(13-48)
🔇 Additional comments (16)
examples/java/io/mailtrap/examples/sending/BatchExample.java (1)
13-13: Rename to BatchExample is correct and consistent — LGTM.
Search of examples/ and README.md found no leftover Batch.java files or references.examples/java/io/mailtrap/examples/contactimports/ContactImportsExample.java (1)
26-27: Immutability on request construction: LGTM.
final var+ compact builders read well for an example.examples/java/io/mailtrap/examples/testing/ProjectsExample.java (3)
7-7: Rename aligns with example naming convention — LGTM.
Consistent with the repo-wide "Example" suffix.
19-21: Confirm SDK nullability for getProjects.
If getProjects can return null, add a null guard; otherwise this is fine.
28-29: LGTM on delete call.
Clear and consistent with the rest of the example.examples/java/io/mailtrap/examples/testing/AttachmentsExample.java (2)
7-7: Rename to AttachmentsExample is correct.Public class name matches the file and avoids confusion with API interfaces.
19-19: final var usage — approved (project targets Java 17)
pom.xml sets maven.compiler.source/target = 17 and .github/workflows use java-version: 17, so local-variable type inference is supported.examples/java/io/mailtrap/examples/general/AccountAccessExample.java (1)
9-9: Rename + immutability LGTM.Class suffix alignment and final locals improve clarity and safety.
Also applies to: 22-22, 25-25
examples/java/io/mailtrap/examples/suppressions/SuppressionsExample.java (1)
6-6: Rename + final locals LGTM.Consistent naming and immutability; behavior unchanged.
Also applies to: 19-21, 25-26
examples/java/io/mailtrap/examples/testing/MessagesExample.java (1)
9-9: Renaming and immutability updates look good.Final locals reduce accidental reassignment; no behavior change.
Also applies to: 22-22, 27-27, 32-36, 53-57, 59-63
src/main/java/io/mailtrap/api/contacts/ContactsImpl.java (1)
17-20: Constructor wiring and path formatting LGTM.
- Uses StandardCharsets.UTF_8.
- Switch to %d for accountId is correct for long.
- Final params are good API hygiene.
Also applies to: 22-31
src/main/java/io/mailtrap/factory/MailtrapClientFactory.java (3)
43-56: Factory assembly LGTM.Clear wiring, final parameter, and single return point.
84-92: Testing API wiring LGTM.ProjectsImpl and MessagesImpl inclusion looks correct.
106-113: SendingContext now includes inboxId — good.Improves downstream routing without changing defaults.
examples/java/io/mailtrap/examples/testing/InboxesExample.java (2)
8-9: LGTM on example structure and naming.Clear, linear flow; aligns with the “Example” suffix convention in this PR.
18-18: MailtrapClient is not AutoCloseable — try-with-resources not applicable.Checked src/main/java/io/mailtrap/client/MailtrapClient.java: the class is declared as public class MailtrapClient and does not implement AutoCloseable nor expose a close() method; no lifecycle change required.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
examples/java/io/mailtrap/examples/sending/EverythingExample.java (1)
75-79: Resource loader now uses the correct class (fix confirmed).
Switching toEverythingExample.class.getClassLoader()resolves the compile/runtime issue flagged earlier.src/main/java/io/mailtrap/api/apiresource/ApiResourceWithValidation.java (1)
19-28: Preserve domain-specific null messages (avoid breaking tests); add overload to accept custom null message.Current generic message (“request must not be null”) may conflict with tests expecting endpoint‑specific messages. Add an overload and route callers through it when a specific message is needed.
Apply:
- protected <T> void validateRequestBodyAndThrowException(final T object) { - if (object == null) { - throw new InvalidRequestBodyException("Invalid request body. Violations: request must not be null"); - } - - final String violations = mailtrapValidator.validateAndGetViolationsAsString(object); - - if (!violations.isEmpty()) { - throw new InvalidRequestBodyException("Invalid request body. Violations: " + violations); - } - } + protected <T> void validateRequestBodyAndThrowException(final T object) { + validateRequestBodyAndThrowException(object, "Invalid request body. Violations: request must not be null"); + } + + protected <T> void validateRequestBodyAndThrowException(final T object, final String nullMessage) { + if (object == null) { + throw new InvalidRequestBodyException(nullMessage); + } + final String violations = mailtrapValidator.validateAndGetViolationsAsString(object); + if (!violations.isEmpty()) { + throw new InvalidRequestBodyException("Invalid request body. Violations: " + violations); + } + }Please confirm which endpoints require specific messages so we can wire them accordingly.
src/main/java/io/mailtrap/factory/MailtrapClientFactory.java (1)
34-49: Good fix: keep ValidatorFactory open; reuse a single Validator instance.This resolves the earlier lifecycle issue and is thread‑safe per Jakarta validation contracts.
src/main/java/io/mailtrap/api/apiresource/SendApiResource.java (1)
88-114: Unify null/error messages with domain‑specific variants.To keep tests stable and messages consistent (“MAIL_MUST_NOT_BE_NULL”, “BATCH_MAIL_MUST_NOT_BE_NULL”, etc.), route null cases through the new overload with endpoint‑specific messages and consider extracting message constants.
Example:
- if (mail == null) { - throw new InvalidRequestBodyException("Mail must not be null"); - } + if (mail == null) { + throw new InvalidRequestBodyException(MESSAGES.MAIL_MUST_NOT_BE_NULL); + } ... - validateRequestBodyAndThrowException(mailView); + validateRequestBodyAndThrowException( + mailView, + "requests[" + i + "]: " + MESSAGES.BATCH_MAIL_MUST_NOT_BE_NULL + );
🧹 Nitpick comments (9)
examples/java/io/mailtrap/examples/sending/EverythingExample.java (1)
80-82: Narrow the catch to IOException and avoid fully swallowing errors.
CatchingExceptionhides non-I/O bugs. PreferIOExceptionand emit a brief diagnostic.- } catch (final Exception e) { - return ""; + } catch (final java.io.IOException e) { + System.err.println("Failed to read attachment '" + filename + "': " + e.getMessage()); + return ""; }If you prefer an import instead of FQN:
import java.io.IOException;src/test/java/io/mailtrap/api/contacts/ContactsImplTest.java (1)
67-74: Add assertDoesNotThrow around delete calls.These tests currently pass implicitly. Wrap calls to make intent explicit and avoid false positives.
Apply:
- void test_deleteContact_byEmail() { - api.deleteContact(accountId, email); - } + void test_deleteContact_byEmail() { + org.junit.jupiter.api.Assertions.assertDoesNotThrow(() -> api.deleteContact(accountId, email)); + } - void test_deleteContact_byId() { - api.deleteContact(accountId, contactUUID); - } + void test_deleteContact_byId() { + org.junit.jupiter.api.Assertions.assertDoesNotThrow(() -> api.deleteContact(accountId, contactUUID)); + }src/main/java/io/mailtrap/MailtrapValidator.java (2)
9-12: Javadoc: update javax → jakarta.The comment references javax.validation but the code uses jakarta.validation.
- * Wrapper for validating objects using a javax.validation.Validator. + * Wrapper for validating objects using a jakarta.validation.Validator.
29-39: Null-safety: fail fast on null input.Validator.validate(null) typically throws IllegalArgumentException. Guard with requireNonNull to produce a clearer message and keep behavior consistent.
+import java.util.Objects; @@ - public <T> Map<String, String> validate(final T objectToValidate) { - final var violations = validator.validate(objectToValidate); + public <T> Map<String, String> validate(final T objectToValidate) { + Objects.requireNonNull(objectToValidate, "objectToValidate must not be null"); + final var violations = validator.validate(objectToValidate);src/test/java/io/mailtrap/MailtrapValidatorTest.java (1)
19-24: Consistency: make local variable final.Aligns with the PR goal of single-assignment locals.
- Map<String, String> errors = mailtrapValidator.validate(validAddress); + final Map<String, String> errors = mailtrapValidator.validate(validAddress);src/main/java/io/mailtrap/api/testingemails/TestingEmailsImpl.java (1)
26-40: Tiny consistency nit: make RequestData final.Other implementations in this PR use final for the local RequestData.
- RequestData requestData = new RequestData(); + final RequestData requestData = new RequestData();src/main/java/io/mailtrap/factory/MailtrapClientFactory.java (1)
34-49: Option: allow custom Validator for tests/extensibility.If you foresee custom constraints or alternative providers in tests, consider package‑private overloads that accept a MailtrapValidator and thread it through create*Api methods (optional).
src/main/java/io/mailtrap/api/projects/ProjectsImpl.java (1)
35-41: Minor: guard path params (optional).If upstream can pass non‑positive IDs, consider fast‑failing with a clear message before hitting HTTP.
Also applies to: 44-50, 53-63, 66-72
src/test/java/io/mailtrap/api/accountaccesses/AccountAccessesImplTest.java (1)
60-76: Swap expected/actual in one assertion.Line 71 passes actual first; prefer expected-first for consistency and clearer failures.
- assertEquals(accountAccessResponse.getId(), accountAccessId); + assertEquals(accountAccessId, accountAccessResponse.getId());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
examples/java/io/mailtrap/examples/sending/EverythingExample.java(2 hunks)src/main/java/io/mailtrap/MailtrapValidator.java(3 hunks)src/main/java/io/mailtrap/api/accountaccesses/AccountAccesses.java(1 hunks)src/main/java/io/mailtrap/api/accountaccesses/AccountAccessesImpl.java(1 hunks)src/main/java/io/mailtrap/api/apiresource/ApiResourceWithValidation.java(2 hunks)src/main/java/io/mailtrap/api/apiresource/SendApiResource.java(6 hunks)src/main/java/io/mailtrap/api/bulkemails/BulkEmailsImpl.java(2 hunks)src/main/java/io/mailtrap/api/contactfields/ContactFieldsImpl.java(2 hunks)src/main/java/io/mailtrap/api/contactimports/ContactImportsImpl.java(2 hunks)src/main/java/io/mailtrap/api/emailtemplates/EmailTemplatesImpl.java(2 hunks)src/main/java/io/mailtrap/api/inboxes/InboxesImpl.java(2 hunks)src/main/java/io/mailtrap/api/projects/ProjectsImpl.java(2 hunks)src/main/java/io/mailtrap/api/sendingemails/SendingEmailsImpl.java(2 hunks)src/main/java/io/mailtrap/api/testingemails/TestingEmailsImpl.java(2 hunks)src/main/java/io/mailtrap/factory/MailtrapClientFactory.java(4 hunks)src/main/java/io/mailtrap/http/impl/DefaultMailtrapHttpClient.java(4 hunks)src/test/java/io/mailtrap/MailtrapValidatorTest.java(1 hunks)src/test/java/io/mailtrap/api/accountaccesses/AccountAccessesImplTest.java(4 hunks)src/test/java/io/mailtrap/api/contactimports/ContactImportsImplTest.java(1 hunks)src/test/java/io/mailtrap/api/contacts/ContactsImplTest.java(4 hunks)src/test/java/io/mailtrap/api/permissions/PermissionsImplTest.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/test/java/io/mailtrap/api/permissions/PermissionsImplTest.java
- src/main/java/io/mailtrap/api/contactfields/ContactFieldsImpl.java
- src/main/java/io/mailtrap/http/impl/DefaultMailtrapHttpClient.java
- src/main/java/io/mailtrap/api/accountaccesses/AccountAccessesImpl.java
- src/test/java/io/mailtrap/api/contactimports/ContactImportsImplTest.java
🧰 Additional context used
🧬 Code graph analysis (10)
src/main/java/io/mailtrap/api/contactimports/ContactImportsImpl.java (2)
src/main/java/io/mailtrap/MailtrapValidator.java (1)
MailtrapValidator(12-56)src/main/java/io/mailtrap/Constants.java (1)
Constants(6-15)
src/main/java/io/mailtrap/api/apiresource/ApiResourceWithValidation.java (1)
src/main/java/io/mailtrap/MailtrapValidator.java (1)
MailtrapValidator(12-56)
src/main/java/io/mailtrap/api/testingemails/TestingEmailsImpl.java (2)
src/main/java/io/mailtrap/MailtrapValidator.java (1)
MailtrapValidator(12-56)src/main/java/io/mailtrap/Constants.java (1)
Constants(6-15)
src/main/java/io/mailtrap/api/projects/ProjectsImpl.java (2)
src/main/java/io/mailtrap/MailtrapValidator.java (1)
MailtrapValidator(12-56)src/main/java/io/mailtrap/Constants.java (1)
Constants(6-15)
src/main/java/io/mailtrap/api/apiresource/SendApiResource.java (1)
src/main/java/io/mailtrap/MailtrapValidator.java (1)
MailtrapValidator(12-56)
src/main/java/io/mailtrap/api/emailtemplates/EmailTemplatesImpl.java (2)
src/main/java/io/mailtrap/MailtrapValidator.java (1)
MailtrapValidator(12-56)src/main/java/io/mailtrap/Constants.java (1)
Constants(6-15)
src/main/java/io/mailtrap/api/bulkemails/BulkEmailsImpl.java (2)
src/main/java/io/mailtrap/MailtrapValidator.java (1)
MailtrapValidator(12-56)src/main/java/io/mailtrap/Constants.java (1)
Constants(6-15)
src/main/java/io/mailtrap/factory/MailtrapClientFactory.java (1)
src/main/java/io/mailtrap/MailtrapValidator.java (1)
MailtrapValidator(12-56)
src/main/java/io/mailtrap/api/inboxes/InboxesImpl.java (2)
src/main/java/io/mailtrap/MailtrapValidator.java (1)
MailtrapValidator(12-56)src/main/java/io/mailtrap/Constants.java (1)
Constants(6-15)
src/main/java/io/mailtrap/api/sendingemails/SendingEmailsImpl.java (2)
src/main/java/io/mailtrap/MailtrapValidator.java (1)
MailtrapValidator(12-56)src/main/java/io/mailtrap/Constants.java (1)
Constants(6-15)
🔇 Additional comments (30)
examples/java/io/mailtrap/examples/sending/EverythingExample.java (1)
14-14: Re-run verification — PCRE error made the first search inconclusive
- The first rg failed with "PCRE2: error compiling pattern..." so the "No stale references" echo is unreliable.
- Resource-loader check found no matches.
- README references EverythingExample.java only (README.md:123).
Run these corrected checks (remove -P / use whole-word match):
rg -n -C2 '\bEverything\b' || echo "No stale references to 'Everything' found." rg -n -C2 'Everything\.class\.getClassLoader\(\)\.getResourceAsStream' || echo "No old resource loader usages found." rg -n 'EverythingExample\.java|Everything\.java' -S README.md || truesrc/test/java/io/mailtrap/api/contacts/ContactsImplTest.java (3)
31-54: Immutable test setup — good cleanup.Using final for httpClient and MailtrapConfig improves readability and prevents accidental reassignment.
58-64: Create contact test reads well.Inputs are explicit; assertions target meaningful fields.
88-96: Minor: ensure payload fields match update mode.When updating by contact UUID, confirm UpdateContact’s semantics don’t require omitting/changing email; current request sets email while identifying by UUID. If API ignores it, fine.
src/test/java/io/mailtrap/MailtrapValidatorTest.java (1)
12-16: Test rename + wiring look good.src/main/java/io/mailtrap/api/testingemails/TestingEmailsImpl.java (2)
20-23: Constructor update to MailtrapValidator — looks correct.
43-51: Throws declaration style check.batchSend declares checked exceptions while send does not. If HttpException/InvalidRequestBodyException are runtime, consider omitting throws for consistency across impls; if checked, align interfaces accordingly.
src/main/java/io/mailtrap/api/inboxes/InboxesImpl.java (3)
16-19: Validator migration + host setup LGTM.
22-31: Type-safe path formatting — good.Switched to %d for numeric IDs; matches method signatures.
35-121: Do not reuse a shared RequestData instance — it's mutableRequestData declares a mutable queryParams map (src/main/java/io/mailtrap/http/RequestData.java), so sharing a single instance would be unsafe. Keep per-call new RequestData() or introduce an explicit immutable EMPTY_REQUEST / make RequestData immutable before reusing.
Likely an incorrect or invalid review comment.
src/main/java/io/mailtrap/api/bulkemails/BulkEmailsImpl.java (3)
20-23: Constructor migration & host — good.
26-40: Final locals + clear POST — good.
43-52: Exception surface: confirm consistency with other modules.batchSend declares throws; send does not. Ensure this mirrors the interface and other impls’ choices.
src/main/java/io/mailtrap/api/contactimports/ContactImportsImpl.java (3)
14-17: Validator migration + host — good.
19-30: Numeric placeholders + validation call — solid.
32-39: LGTM on GET path formatting.src/main/java/io/mailtrap/api/sendingemails/SendingEmailsImpl.java (4)
20-23: Constructor migration & host — correct.
26-41: Final RequestData and header pass-through — good.
44-53: Batch send path + exceptions — consistent with intent.
20-23: Verified: no CustomValidator usages; %s in API paths are intentional for string IDsrg found no usages of CustomValidator. The String.format calls use %d for numeric accountId and %s for string identifiers (suppressionId, idOrEmail) — formatting is correct; no changes required.
src/main/java/io/mailtrap/factory/MailtrapClientFactory.java (2)
59-69: Consistent validator injection across APIs looks solid.Centralized VALIDATOR and wiring across create*Api methods is clean and reduces duplication.
Also applies to: 72-118
120-127: Nice: SendingContext now includes inboxId.This matches typical resolution rules downstream.
src/main/java/io/mailtrap/api/projects/ProjectsImpl.java (1)
16-19: LGTM: validator migration and numeric path formatting.Constructor update and %d usage are correct; request validation is invoked before POST.
Also applies to: 21-32
src/main/java/io/mailtrap/api/emailtemplates/EmailTemplatesImpl.java (1)
16-19: CRUD surface and validator migration look good.Endpoints, %d formatting, and request validation are consistent with other modules; delete→Void is appropriate.
Also applies to: 21-29, 31-42, 44-52, 54-65, 67-75
src/main/java/io/mailtrap/api/apiresource/SendApiResource.java (3)
33-43: Good: combine bean validation with content rules for single‑mail flow.Null precheck plus validateRequestBodyAndThrowException(...) then content rules is the right order.
75-86: Batch envelope checks are solid.Null/empty list and null item guards will save you from NPEs and produce clearer errors.
51-73: Resolved — ResolvedMailView is annotated for bean validation.
ResolvedMailView has @NotNull/@Valid on getFrom, and MailtrapMail.from, Address.email, BatchEmailBase and MailtrapBatchMail.requests are annotated as well, so validating ResolvedMailView will trigger the expected bean constraints.src/test/java/io/mailtrap/api/accountaccesses/AccountAccessesImplTest.java (3)
25-43: Final locals — LGTM.Using final for httpClient/testConfig is consistent and clear.
47-58: Assertions cleanup — LGTM.Expected-first assert on Line 53 is correct; rest reads well.
80-84: Test reflects new param order; will need update if API is reverted/renamed.If you take Option A (revert) or Option B (rename) in the interface, update this call accordingly. Otherwise, add a clear migration note in CHANGELOG.
src/main/java/io/mailtrap/api/accountaccesses/AccountAccesses.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/main/java/io/mailtrap/api/accountaccesses/AccountAccessesImpl.java (3)
19-22: Add @nonnull to config for consistency and early null‑safetyConstructor already sets
apiHost; mirroring the method param style keeps things uniform and defensive.- public AccountAccessesImpl(final MailtrapConfig config) { + public AccountAccessesImpl(@NonNull final MailtrapConfig config) {
31-35: Avoid concatenation inside String.format; passapiHostas an argumentChange keeps the nice
%dfor IDs, avoids an extra intermediate string, and matches the format‑string style used elsewhere.- return httpClient.getList( - String.format(apiHost + "/api/accounts/%d/account_accesses", accountId), + return httpClient.getList( + String.format("%s/api/accounts/%d/account_accesses", apiHost, accountId), new RequestData(queryParams), AccountAccessResponse.class );
39-44: Same formatting tweak here; minor note on argument order
- Use a single
String.formatwith%s/%d/%d.- Optional: the method signature is
(accountAccessId, accountId)while the path is.../accounts/{accountId}/account_accesses/{accountAccessId}. Not proposing a breaking change, but a brief Javadoc on param order would reduce caller confusion.- return httpClient.delete( - String.format(apiHost + "/api/accounts/%d/account_accesses/%d", accountId, accountAccessId), + return httpClient.delete( + String.format("%s/api/accounts/%d/account_accesses/%d", apiHost, accountId, accountAccessId), new RequestData(), RemoveAccountAccessResponse.class );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/io/mailtrap/api/accountaccesses/AccountAccessesImpl.java(1 hunks)src/test/java/io/mailtrap/api/accountaccesses/AccountAccessesImplTest.java(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/java/io/mailtrap/api/accountaccesses/AccountAccessesImplTest.java
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/io/mailtrap/api/accountaccesses/AccountAccessesImpl.java (1)
src/main/java/io/mailtrap/Constants.java (1)
Constants(6-15)
Motivation
Keep SDK code clean and modern
Changes
final- more of a codestyle/single-assignment vibes, used quite often in modern Java appsExamplesuffix to all example classes to prevent confusion with API interfaces%swith%dfor Long params inString.formatserializationInclusionin global ObjectMapperSummary by CodeRabbit