Nitpick SDK updates #39
Conversation
WalkthroughRefactors example class names to include “Example”, updates README references, and applies widespread parameter immutability (final) and numeric URL formatting (%d). Adds stricter validation in send flows, default HTTP headers, EmailTemplates CRUD interface methods, serialization tweaks, mapper inclusion change, and minor validations/encoding updates. Tests adjusted for immutability and additional assertions. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Application
participant Client as MailtrapClient
participant Factory as MailtrapClientFactory
participant SendAPI as Sending/Bulk/Testing API
participant Validator as SendApiResource (validation)
participant HTTP as DefaultMailtrapHttpClient
App->>Factory: createMailtrapClient(config)
Factory->>Factory: Wire APIs (Contacts, Templates, Testing, etc.)
Factory->>Client: MailtrapClient instance
App->>Client: send(mail) / batchSend(batch)
alt bulk or sandbox
Client->>SendAPI: send/batchSend(...)
else default
Client->>SendAPI: send/batchSend(...)
end
SendAPI->>Validator: validateMailPayload / validateBatchPayload
note right of Validator: Null checks, content rules,<br/>required from in batch,<br/>bean validation
Validator-->>SendAPI: ok or throw InvalidRequestBodyException
SendAPI->>HTTP: POST /api/... (JSON)
note right of HTTP: Adds headers:<br/>Accept, Content-Type UTF-8,<br/>User-Agent
HTTP-->>SendAPI: Response
SendAPI-->>Client: SendResponse / BatchSendResponse
Client-->>App: Result
sequenceDiagram
autonumber
participant App as Application
participant Templates as EmailTemplatesImpl
participant HTTP as DefaultMailtrapHttpClient
App->>Templates: create/get/update/delete(accountId, ...)
Templates->>HTTP: POST/GET/PUT/DELETE /api/accounts/%d/email_templates[%d]
HTTP-->>Templates: EmailTemplateResponse or void
Templates-->>App: Result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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),
| @@ -11,7 +11,7 @@ | |||
| import java.util.List; | |||
| import java.util.Map; | |||
|
|
|||
| public class ContactImports { | |||
| public class ContactImportsExample { | |||
There was a problem hiding this comment.
💡 Verification agent
💡 Verification agent
🧩 Analysis chain
Rename to ContactImportsExample: LGTM. Also prune unused imports and scan for stale references.
- Unused here: UpdateContact, UpdateContactRequest, and Collections.
- Optional: make locals final (final var ...) to match the PR’s immutability goal.
Run to catch any leftover references to old example class names:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --glob 'examples/java/**' -P '\b(ContactImports|Minimal|AccountAccess|Batch|Attachments|Contacts|BulkSend)(?!Example)\b' || trueLength of output: 83
🏁 Script executed:
#!/bin/bash
set -euo pipefail
f="examples/java/io/mailtrap/examples/contactimports/ContactImportsExample.java"
if [ ! -f "$f" ]; then
echo "MISSING: $f"
exit 0
fi
echo "=== File head (1-200) ==="
sed -n '1,200p' "$f"
echo
echo "=== Import lines ==="
rg -n '^import ' "$f" || true
echo
echo "=== Symbols in file: UpdateContact, UpdateContactRequest, Collections ==="
rg -n 'UpdateContact|UpdateContactRequest|Collections' "$f" || true
echo
echo "=== Project search for base example names (word match) ==="
rg -n --glob 'examples/java/**' -w -e 'ContactImports|Minimal|AccountAccess|Batch|Attachments|Contacts|BulkSend' || trueLength of output: 2342
Remove unused imports and fix example class name mismatch.
- Remove unused imports UpdateContact, UpdateContactRequest, and Collections from examples/java/io/mailtrap/examples/contactimports/ContactImportsExample.java (lines 7–8, 10).
- Fix class/file name mismatch: examples/java/io/mailtrap/examples/sending/BatchExample.java declares
public class Batch(line 13) — rename the class to BatchExample or rename the file to Batch.java. - Optional: make local vars final (importRequest, createResponse, contactImportResponse) to match the PR’s immutability goal.
🤖 Prompt for AI Agents
In examples/java/io/mailtrap/examples/contactimports/ContactImportsExample.java
around lines 7–10 and 14, remove the unused imports UpdateContact,
UpdateContactRequest, and Collections and ensure the public class name matches
the file name (ContactImportsExample) if it does not; additionally, in
examples/java/io/mailtrap/examples/sending/BatchExample.java at line 13 change
the declared class name from Batch to BatchExample (or rename the file to
Batch.java) so class and filename match, and optionally mark local variables
importRequest, createResponse, and contactImportResponse as final to follow
immutability conventions.
| 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 = customValidator.validateAndGetViolationsAsString(object); | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Behavior change: new null-check message may conflict with existing tests.
You now throw “Invalid request body. Violations: request must not be null”. Some tests elsewhere assert domain‑specific messages like MAIL_MUST_NOT_BE_NULL/BATCH_MAIL_MUST_NOT_BE_NULL. Ensure upstream validators still preempt this generic check where needed.
Run to find potentially conflicting assertions:
If conflicts exist, either:
- keep domain-specific null checks before calling validateRequestBodyAndThrowException, or
- align tests to the new generic message (prefer contains over equals).
🏁 Script executed:
#!/bin/bash
# Look for strict "must not be null" expectations and new generic message
rg -n --type=java -C2 'MAIL_MUST_NOT_BE_NULL|BATCH_MAIL_MUST_NOT_BE_NULL|request must not be null|Invalid request body\. Violations: request must not be null|must not be null' src/testLength of output: 5277
Critical: preserve domain-specific null messages — tests assert exact strings
validateRequestBodyAndThrowException now throws "Invalid request body. Violations: request must not be null" but multiple tests expect MAIL_MUST_NOT_BE_NULL / BATCH_MAIL_MUST_NOT_BE_NULL and will fail.
Affected locations:
- src/main/java/io/mailtrap/api/apiresource/ApiResourceWithValidation.java (validateRequestBodyAndThrowException)
- src/test/java/io/mailtrap/testutils/BaseSendTest.java:20-21
- src/test/java/io/mailtrap/api/testingemails/TestingEmailsImplTest.java:129,230
- src/test/java/io/mailtrap/api/sendingemails/SendingEmailsImplTest.java:127,228
- src/test/java/io/mailtrap/api/bulkemails/BulkEmailsImplTest.java:127,228
Actions:
- Recommended: restore per-endpoint null checks (throw MAIL_MUST_NOT_BE_NULL / BATCH_MAIL_MUST_NOT_BE_NULL) before calling validateRequestBodyAndThrowException, or add an overload/parameter to pass a domain-specific message to that method.
- Alternative: if the change is intentional, update tests to assert contains(...) rather than equals(...) for the exception message.
🤖 Prompt for AI Agents
In src/main/java/io/mailtrap/api/apiresource/ApiResourceWithValidation.java
around lines 19-25, the method validateRequestBodyAndThrowException currently
throws a generic "request must not be null" message which breaks tests that
expect domain-specific constants (MAIL_MUST_NOT_BE_NULL /
BATCH_MAIL_MUST_NOT_BE_NULL); restore per-endpoint null checks so each endpoint
throws its specific constant before calling
validateRequestBodyAndThrowException, or modify
validateRequestBodyAndThrowException to accept an optional domain-specific
message/enum parameter and use that when object is null, updating callers
accordingly to pass the correct domain-specific message.
| /** | ||
| * Create a new email template | ||
| * | ||
| * @param accountId unique account ID | ||
| * @param request email template create payload | ||
| * @return created email template | ||
| */ | ||
| EmailTemplateResponse createEmailTemplate(long accountId, CreateEmailTemplateRequest request); | ||
|
|
||
| /** | ||
| * Get an email template by ID | ||
| * | ||
| * @param accountId unique account ID | ||
| * @param emailTemplateId unique email template ID | ||
| * @return email template attributes | ||
| */ | ||
| EmailTemplateResponse getEmailTemplate(long accountId, long emailTemplateId); | ||
|
|
||
| /** | ||
| * Update an email template | ||
| * | ||
| * @param accountId unique account ID | ||
| * @param emailTemplateId unique email template ID | ||
| * @param request email template update payload | ||
| * @return updated email template | ||
| */ | ||
| EmailTemplateResponse updateEmailTemplate(long accountId, long emailTemplateId, UpdateEmailTemplateRequest request); | ||
|
|
||
| /** | ||
| * Delete an email template | ||
| * | ||
| * @param accountId unique account ID | ||
| * @param emailTemplateId unique email template ID | ||
| */ | ||
| void deleteEmailTemplate(long accountId, long emailTemplateId); |
There was a problem hiding this comment.
Public interface expansion is a breaking change; prefer default methods to preserve binary compatibility.
Adding non-default methods to a published interface breaks third‑party implementors at compile time. Make new CRUD methods default (throwing UnsupportedOperationException by default) to avoid breakage.
Apply this diff:
- EmailTemplateResponse createEmailTemplate(long accountId, CreateEmailTemplateRequest request);
+ default EmailTemplateResponse createEmailTemplate(long accountId, CreateEmailTemplateRequest request) {
+ throw new UnsupportedOperationException("createEmailTemplate is not implemented");
+ }
- EmailTemplateResponse getEmailTemplate(long accountId, long emailTemplateId);
+ default EmailTemplateResponse getEmailTemplate(long accountId, long emailTemplateId) {
+ throw new UnsupportedOperationException("getEmailTemplate is not implemented");
+ }
- EmailTemplateResponse updateEmailTemplate(long accountId, long emailTemplateId, UpdateEmailTemplateRequest request);
+ default EmailTemplateResponse updateEmailTemplate(long accountId, long emailTemplateId, UpdateEmailTemplateRequest request) {
+ throw new UnsupportedOperationException("updateEmailTemplate is not implemented");
+ }
- void deleteEmailTemplate(long accountId, long emailTemplateId);
+ default void deleteEmailTemplate(long accountId, long emailTemplateId) {
+ throw new UnsupportedOperationException("deleteEmailTemplate is not implemented");
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Create a new email template | |
| * | |
| * @param accountId unique account ID | |
| * @param request email template create payload | |
| * @return created email template | |
| */ | |
| EmailTemplateResponse createEmailTemplate(long accountId, CreateEmailTemplateRequest request); | |
| /** | |
| * Get an email template by ID | |
| * | |
| * @param accountId unique account ID | |
| * @param emailTemplateId unique email template ID | |
| * @return email template attributes | |
| */ | |
| EmailTemplateResponse getEmailTemplate(long accountId, long emailTemplateId); | |
| /** | |
| * Update an email template | |
| * | |
| * @param accountId unique account ID | |
| * @param emailTemplateId unique email template ID | |
| * @param request email template update payload | |
| * @return updated email template | |
| */ | |
| EmailTemplateResponse updateEmailTemplate(long accountId, long emailTemplateId, UpdateEmailTemplateRequest request); | |
| /** | |
| * Delete an email template | |
| * | |
| * @param accountId unique account ID | |
| * @param emailTemplateId unique email template ID | |
| */ | |
| void deleteEmailTemplate(long accountId, long emailTemplateId); | |
| /** | |
| * Create a new email template | |
| * | |
| * @param accountId unique account ID | |
| * @param request email template create payload | |
| * @return created email template | |
| */ | |
| default EmailTemplateResponse createEmailTemplate(long accountId, CreateEmailTemplateRequest request) { | |
| throw new UnsupportedOperationException("createEmailTemplate is not implemented"); | |
| } | |
| /** | |
| * Get an email template by ID | |
| * | |
| * @param accountId unique account ID | |
| * @param emailTemplateId unique email template ID | |
| * @return email template attributes | |
| */ | |
| default EmailTemplateResponse getEmailTemplate(long accountId, long emailTemplateId) { | |
| throw new UnsupportedOperationException("getEmailTemplate is not implemented"); | |
| } | |
| /** | |
| * Update an email template | |
| * | |
| * @param accountId unique account ID | |
| * @param emailTemplateId unique email template ID | |
| * @param request email template update payload | |
| * @return updated email template | |
| */ | |
| default EmailTemplateResponse updateEmailTemplate(long accountId, long emailTemplateId, UpdateEmailTemplateRequest request) { | |
| throw new UnsupportedOperationException("updateEmailTemplate is not implemented"); | |
| } | |
| /** | |
| * Delete an email template | |
| * | |
| * @param accountId unique account ID | |
| * @param emailTemplateId unique email template ID | |
| */ | |
| default void deleteEmailTemplate(long accountId, long emailTemplateId) { | |
| throw new UnsupportedOperationException("deleteEmailTemplate is not implemented"); | |
| } |
🤖 Prompt for AI Agents
In src/main/java/io/mailtrap/api/emailtemplates/EmailTemplates.java around lines
19 to 53, the new non-default CRUD methods on a public interface are a
binary-breaking change; convert each added method into a default method that
throws UnsupportedOperationException to preserve compatibility for existing
implementors. For each of the methods createEmailTemplate, getEmailTemplate,
updateEmailTemplate and deleteEmailTemplate change the signature to a default
method and implement it to immediately throw new
UnsupportedOperationException("Not implemented"); leaving Javadoc intact so
implementors can override with actual implementations. Ensure imports remain
unchanged and compile.
| String.format(apiHost + "/api/accounts/%d/projects/%d/inboxes", accountId, projectId), | ||
| request, | ||
| new RequestData(), | ||
| InboxResponse.class | ||
| ); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Switching URL formatting to %d for long IDs — correct.
Type‑safe for long args; removes accidental string coercion.
To ensure consistency repo‑wide, scan for leftover numeric IDs still formatted with %s:
🏁 Script executed:
#!/bin/bash
# Verify no numeric ID placeholders use %s in main sources
rg -nP --type=java -C1 '%s\)' src/main/java | rg -nP '/api/.*%s'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Repo-wide search for "%s" usages in API URL formatting (Java)
set -euo pipefail
echo "1) String literals containing /api/ and %s (Java files under src/)..."
rg -nP --type=java -C2 '"[^"]*/api/[^"]*%s[^"]*"' src || true
echo "2) String.format(...) calls that include %s (Java files under src/)..."
rg -nP --type=java -C2 'String\.format\([^)]*%s' src || true
echo "3) Any literal '%s' in Java sources under src/..."
rg -n --type=java -n -C1 -F '%s' src || true
echo "4) Any literal '%s' in repository (excluding .git and target)..."
rg -n -F '%s' --hidden -g '!.git' -g '!target' || trueLength of output: 7651
Replace remaining "%s" with "%d" for accountId in ContactsImpl.
- src/main/java/io/mailtrap/api/contacts/ContactsImpl.java:25 —
String.format(apiHost + "/api/accounts/%s/contacts", accountId)→ use%dto match other endpoints and keep type-safety. - src/main/java/io/mailtrap/api/suppressions/SuppressionsImpl.java uses
%dfor accountId and%sfor suppressionId (correct); test-format strings using%sare acceptable.
🤖 Prompt for AI Agents
In src/main/java/io/mailtrap/api/contacts/ContactsImpl.java around line 25, the
String.format call uses "%s" for accountId but other endpoints use "%d" and
accountId is a numeric type; change the format placeholder from "%s" to "%d" in
the format string (e.g. "/api/accounts/%d/contacts"), ensure the argument
remains accountId (numeric) and recompile/tests to verify no formatting/type
errors.
| try (final ValidatorFactory factory = Validation.buildDefaultValidatorFactory()) { | ||
| return new CustomValidator(factory.getValidator()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Don’t close ValidatorFactory before using the Validator.
Closing the factory may invalidate dependent resources; spec doesn’t guarantee Validator usability post-close.
Apply:
- try (final ValidatorFactory factory = Validation.buildDefaultValidatorFactory()) {
- return new CustomValidator(factory.getValidator());
- }
+ // Keep factory alive; caller manages lifecycle via JVM shutdown.
+ final ValidatorFactory factory = Validation.buildDefaultValidatorFactory();
+ return new CustomValidator(factory.getValidator());Optionally cache a single ValidatorFactory per JVM for efficiency.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try (final ValidatorFactory factory = Validation.buildDefaultValidatorFactory()) { | |
| return new CustomValidator(factory.getValidator()); | |
| } | |
| } | |
| // Keep factory alive; caller manages lifecycle via JVM shutdown. | |
| final ValidatorFactory factory = Validation.buildDefaultValidatorFactory(); | |
| return new CustomValidator(factory.getValidator()); | |
| } |
🤖 Prompt for AI Agents
In src/main/java/io/mailtrap/factory/MailtrapClientFactory.java around lines 120
to 123, the code currently closes the ValidatorFactory immediately which can
invalidate the returned Validator; remove the try-with-resources so the
ValidatorFactory is not closed before the Validator is used, instead create and
hold the ValidatorFactory for the lifetime needed (e.g., as a static final
cached ValidatorFactory in the class or a singleton per JVM), then obtain and
return a Validator (or a CustomValidator wrapping it) without closing the
factory; ensure thread-safety for the cached factory and document
lifecycle/cleanup if applicable.
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