Feature/125 contact imports #142
Conversation
- Added contact imports management interfaces, models, requests, and validation - Added example project to deminstrate Contact Imports API usage - Updated Contacts Interface to provide access to Contact Imports
- Added Contacts Import Unit tests - Added Contacts Import Integrational tests - Updates in Contacts Import and Contacts models for unification and to support serialization
- Corrected documentation for contact imports and requests
dr-3lo
requested review from
zhaparoff,
mklocek and
vittorius
as code owners
WalkthroughAdds Contact Imports: new abstractions (interfaces, models, requests, validators), client resources (create/get details), updates to contacts API surface, example project, serializers/constructors, Ensure.NotNullOrEmpty helper, global usings and suppressions, plus unit and integration tests with fixtures and helpers. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant ExampleApp as Example App
participant DI as DI Container
participant Client as IMailtrapClient
participant Contacts as Contacts Resource
participant Imports as Imports Collection
participant ImportRes as Import Resource
participant API as Mailtrap REST API
User->>ExampleApp: run example
ExampleApp->>DI: resolve IMailtrapClient
DI-->>ExampleApp: IMailtrapClient
ExampleApp->>Client: account(12345).Contacts
Client->>Contacts: get collection
Contacts->>Imports: Imports()
rect #dff3ff
ExampleApp->>Imports: Create(ContactsImportRequest)
Imports->>API: POST /accounts/{id}/contacts/imports
API-->>Imports: 201 Created (ContactsImport)
Imports-->>ExampleApp: ContactsImport
end
Contacts->>ImportRes: Import(importId)
rect #f0fff0
ExampleApp->>ImportRes: GetDetails()
ImportRes->>API: GET /accounts/{id}/contacts/imports/{importId}
API-->>ImportRes: 200 OK (ContactsImport)
ImportRes-->>ExampleApp: ContactsImport
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
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. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
✨ 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
🧹 Nitpick comments (36)
examples/Mailtrap.Example.ContactImports/appsettings.json (1)
14-16: Prefer env vars or user-secrets for ApiTokenKeep the placeholder in repo; load the token from environment (e.g., Mailtrap:ApiToken) or dotnet user-secrets to avoid accidental commits.
tests/Mailtrap.IntegrationTests/TestExtensions/FileHelpers.cs (2)
6-14: Return JSON content with explicit encoding and media typeMake StringContent explicit to avoid defaulting to text/plain and fix the param name typo.
Apply:
-internal static async Task<StringContent> LoadFileToStringContent( +internal static async Task<StringContent> LoadFileToStringContent( this string featureFolderName, string? fileName = null, - string filexExt = "json") + string fileExt = "json") { - var fileString = await LoadFileToString(featureFolderName, fileName, filexExt); - - return new StringContent(fileString); + var fileString = await LoadFileToString(featureFolderName, fileName, fileExt); + return new StringContent(fileString, System.Text.Encoding.UTF8, "application/json"); }
16-28: Consistent param naming (fileExt) and small cleanupAlign the second method’s param name and usage with the fix above.
Apply:
-internal static async Task<string> LoadFileToString( +internal static async Task<string> LoadFileToString( this string featureFolderName, - string? fileName, - string filexExt = "json") + string? fileName, + string fileExt = "json") { Ensure.NotNullOrEmpty(featureFolderName, nameof(featureFolderName)); - var name = $"{fileName ?? TestContext.CurrentContext.Test.MethodName ?? "Test"}.{filexExt}"; + var name = $"{fileName ?? TestContext.CurrentContext.Test.MethodName ?? "Test"}.{fileExt}"; var path = Path.Combine(featureFolderName, name); var fileString = await File.ReadAllTextAsync(path); return fileString; }src/Mailtrap.Abstractions/Core/Extensions/Ensure.cs (3)
107-115: Add an optional message to RangeCondition for better diagnosticsProviding an overload with a message improves exception clarity and keeps consistency with GreaterThanZero.
-internal static void RangeCondition(bool condition, string paramName) +internal static void RangeCondition(bool condition, string paramName, string? message = default) { if (condition) { return; } - throw new ArgumentOutOfRangeException(paramName); + if (message is null) + { + throw new ArgumentOutOfRangeException(paramName); + } + else + { + throw new ArgumentOutOfRangeException(paramName, message); + } }
5-5: Doc-comment spacing nitThe standalone triple-slash lines are harmless but inconsistent. Consider dropping them to match the style used elsewhere in this file.
Also applies to: 38-38, 86-86
59-81: Optimize NotNullOrEmpty to use O(1) Count checks
UseICollection<T>.CountandIReadOnlyCollection<T>.Countbefore falling back toAny(). Confirm thatSystem.Linqis available (no<ImplicitUsings>or globalusing System.Linq;was found) or addusing System.Linq;inEnsure.cs.tests/Mailtrap.IntegrationTests/Contacts/ContactsIntegrationTests.cs (1)
201-203: Clarify intent with a named argumentUse the named parameter to make it obvious that 10 is the total email length.
- var updatedEmail = TestContext.CurrentContext.Random.NextEmail(10); + var updatedEmail = TestContext.CurrentContext.Random.NextEmail(outputLength: 10);tests/Mailtrap.IntegrationTests/ContactImports/Create_Success_Request.json (1)
8-9: Zip/postal codes as strings to preserve leading zeros (if allowed by API)If the API accepts string field values, prefer quoting zip codes to avoid accidental loss of leading zeros in future examples.
- "zip_code": 11111 + "zip_code": "11111" ... - "zip_code": 22222 + "zip_code": "22222"Also applies to: 26-27
src/Mailtrap.Abstractions/Contacts/IContactCollectionResource.cs (2)
11-11: Trim empty XML doc linesRemove stray “///” blank lines to keep XML docs tidy.
- ///
28-28: Trim empty XML doc linesSame here; minor cleanup.
- ///src/Mailtrap.Abstractions/ContactImports/IContactsImportResource.cs (1)
3-5: Tighten XML docs.Minor polish: fix singular/plural and remove stray blank doc lines for consistency.
-/// Represents Contact import resource. +/// Represents Contacts import resource. @@ - /// + /// @@ - /// + ///Also applies to: 11-15
src/Mailtrap/ContactImports/ContactsImportResource.cs (1)
11-12: Consider dropping async/await to avoid the state machine.If
ConfigureAwait(false)isn’t mandated by repo guidelines, return the Task directly.- public async Task<ContactsImport> GetDetails(CancellationToken cancellationToken = default) - => await Get<ContactsImport>(cancellationToken).ConfigureAwait(false); + public Task<ContactsImport> GetDetails(CancellationToken cancellationToken = default) + => Get<ContactsImport>(cancellationToken);If
ConfigureAwait(false)is required, keep as-is for consistency.examples/Mailtrap.Example.ContactImports/Program.cs (5)
64-69: Don’t use Environment.FailFast in a sample; it bypasses disposal and makes the rethrow unreachable.Log and rethrow (or set exit code) instead.
- logger.LogError(ex, "An error occurred during API call."); - Environment.FailFast(ex.Message); - throw; + logger.LogError(ex, "An error occurred during API call."); + throw;Or, to exit gracefully:
- logger.LogError(ex, "An error occurred during API call."); - Environment.FailFast(ex.Message); - throw; + logger.LogError(ex, "An error occurred during API call."); + Environment.ExitCode = 1;
23-26: Plumb a CancellationToken from the host lifetime.Improves cancellation behavior for HTTP calls.
ILogger<Program> logger = host.Services.GetRequiredService<ILogger<Program>>(); IMailtrapClient mailtrapClient = host.Services.GetRequiredService<IMailtrapClient>(); +IHostApplicationLifetime lifetime = host.Services.GetRequiredService<IHostApplicationLifetime>(); +var cancellationToken = lifetime.ApplicationStopping;
49-50: Pass the CancellationToken to async API calls.Keeps the sample resilient to shutdowns/timeouts.
- ContactsImport importResponse = await contactsImportResource.Create(importRequest); + ContactsImport importResponse = await contactsImportResource.Create(importRequest, cancellationToken); @@ - ContactsImport contactImportDetails = await contactImportResource.GetDetails(); + ContactsImport contactImportDetails = await contactImportResource.GetDetails(cancellationToken);Also applies to: 56-57
28-29: Avoid hardcoding accountId; read from configuration.Makes the sample runnable without editing code.
- var accountId = 12345; + var accountId = config.GetValue<long>("AccountId");
35-35: Minor comment formatting.Add a space after
//for consistency.- //Get resource for contact imports collection + // Get resource for contact imports collectionsrc/Mailtrap.Abstractions/Contacts/Requests/UpdateContactRequest.cs (1)
15-16: Avoid emitting nulls and prefer init-only for request immutabilityConsider suppressing null emission and using init to keep requests immutable.
- [JsonPropertyName("unsubscribed")] - public bool? Unsubscribed { get; set; } + [JsonPropertyName("unsubscribed")] + [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] + public bool? Unsubscribed { get; init; }tests/Mailtrap.UnitTests/ContactImports/ContactsImportResourceTests.cs (2)
7-16: Minor: make factory a Mock to enable future verificationIf you later assert command dispatch, keep a Mock field and pass .Object.
- private readonly IRestResourceCommandFactory _commandFactoryMock = Mock.Of<IRestResourceCommandFactory>(); + private readonly Mock<IRestResourceCommandFactory> _commandFactoryMock = new(); ... - private ContactsImportResource CreateResource() => new(_commandFactoryMock, _resourceUri); + private ContactsImportResource CreateResource() => new(_commandFactoryMock.Object, _resourceUri);
39-47: Optional: add a GetDetails happy-path test stubA future test can verify that GetDetails issues a GET to _resourceUri and deserializes ContactsImport.
src/Mailtrap/ContactImports/ContactsImportCollectionResource.cs (1)
11-13: Null-guard the request at the boundaryDefensively check request before delegating.
- public async Task<ContactsImport> Create(ContactsImportRequest request, CancellationToken cancellationToken = default) - => await Create<ContactsImportRequest, ContactsImport>(request, cancellationToken).ConfigureAwait(false); + public async Task<ContactsImport> Create(ContactsImportRequest request, CancellationToken cancellationToken = default) + { + ArgumentNullException.ThrowIfNull(request); + return await Create<ContactsImportRequest, ContactsImport>(request, cancellationToken).ConfigureAwait(false); + }src/Mailtrap.Abstractions/ContactImports/IContactsImportCollectionResource.cs (2)
9-15: Tighten XML docs; fix confusing sentence and reference the limit constantReword the last sentence and cross-reference the validator constant to avoid drift.
- /// You can import up to 50,000 contacts per request. + /// You can import up to <see cref="Mailtrap.ContactImports.Validators.ContactsImportRequestValidator.MaxContactsPerRequest"/> contacts per request. @@ - /// List all contacts with details into <paramref name="request"/>. + /// Provide the contacts to import (with optional list assignments) in <paramref name="request"/>.
25-27: Return description should name the DTOName the returned type for clarity.
- /// Contact import id and status. + /// A <see cref="ContactsImport"/> containing the import ID and status.tests/Mailtrap.IntegrationTests/ContactImports/ContactImportsIntegrationTests.cs (2)
95-103: Use non-strict matcher when expecting zero callsYou expect validation to short-circuit, so prefer When(...) over Expect(...) to avoid lingering unmet expectations.
- var mockedRequest = mockHttp - .Expect(httpMethod, requestUri) + var mockedRequest = mockHttp + .When(httpMethod, requestUri) .WithHeaders("Authorization", $"Bearer {_clientConfig.ApiToken}") .WithHeaders("Accept", MimeTypes.Application.Json) .WithHeaders("User-Agent", HeaderValues.UserAgent.ToString()) .WithJsonContent(request, _jsonSerializerOptions) .Respond(HttpStatusCode.UnprocessableContent, responseContent);
165-171: Unnecessary stream position resetThe stream isn’t reused after deserialization; the reset is redundant.
- responseStream.Position = 0; // Reset stream positionsrc/Mailtrap.Abstractions/ContactImports/Validators/ContactsImportRequestValidator.cs (2)
4-7: Correct validator summaryThis validator targets the bulk import request and delegates per-item checks.
-/// Validator for Create/Update contact requests.<br /> -/// -/// Ensures contact's email is not empty and length is within the allowed range. +/// Validator for bulk contacts import requests.<br /> +/// Ensures the contacts collection size is within allowed bounds and each contact is valid.
22-28: Add an explicit message and drop redundant null checkNotEmpty already guards null; add a clear error message for size violations.
RuleFor(r => r.Contacts) .Cascade(CascadeMode.Stop) .NotEmpty() - .Must(list => list != null && list.Count is >= MinContactsPerRequest and <= MaxContactsPerRequest); + .Must(list => list.Count is >= MinContactsPerRequest and <= MaxContactsPerRequest) + .WithMessage($"Contacts collection size must be between {MinContactsPerRequest} and {MaxContactsPerRequest}.");src/Mailtrap.Abstractions/Contacts/Requests/ContactImportRequest.cs (1)
3-6: Fix summary: this is the per-contact import requestAvoid “update” wording to reduce confusion.
-/// Request object for updating a contact. +/// Per-contact request used in bulk contact imports.tests/Mailtrap.IntegrationTests/TestExtensions/EmailAddressHelpers.cs (2)
32-35: Handle full email strings in domain parameterIf a caller passes an email (e.g., user@example.com), trim everything up to the last '@' to avoid invalid domains.
- else - { - domainPart = domain.TrimStart('@'); - } + else + { + var cleaned = domain.Trim(); + var atIndex = cleaned.LastIndexOf('@'); + domainPart = atIndex >= 0 ? cleaned.Substring(atIndex + 1) : cleaned; + }
37-41: Clarify comment about minimum lengthDomain part already includes the dot and TLD.
- // Minimum email length: username (1) + '@' (1) + minimum domain (3) + '.' (1) + TLD (2) + // Minimum email length: username (1) + '@' (1) + domainPart.Length (domain + '.' + TLD)src/Mailtrap.Abstractions/ContactImports/Models/ContactsImport.cs (1)
3-6: Polish XML docs for grammar and consistencyMinor wording fixes to use singular “import” and clearer phrasing.
-/// Generic response object for contact imports operations. +/// Generic response object for contact import operations. @@ - /// Gets created contact imports identifier. + /// Gets the contact import identifier. @@ - /// Gets contact imports status. + /// Gets the contact import status. @@ - /// Gets count of created contacts. + /// Gets the count of created contacts. @@ - /// Gets count of updated contacts. + /// Gets the count of updated contacts. @@ - /// Gets count of contacts over limit. + /// Gets the count of contacts over the account limit.Also applies to: 8-18, 20-29, 31-41, 42-52, 53-63
tests/Mailtrap.UnitTests/ContactImports/Requests/ContactsImportRequestTests.cs (2)
23-34: Also assert defensive copy semanticsVerify the source list can change without affecting the request.
[Test] public void Constructor_ShouldDefensivelyCopyInputList() { var contacts = new List<ContactImportRequest> { RandomContactImportRequest() }; var request = new ContactsImportRequest(contacts); // Mutate the original list after construction contacts.Add(RandomContactImportRequest()); request.Contacts.Should().HaveCount(1, "the constructor should defensively copy the input list"); }
37-53: Add success-path boundary tests (min and max sizes)Ensure validator accepts the documented limits.
[Test] public void Validate_ShouldPass_WhenSizeIsAtMinimum() { var contacts = new List<ContactImportRequest> { RandomContactImportRequest() }; var request = new ContactsImportRequest(contacts); var result = request.Validate(); result.IsValid.Should().BeTrue(); } [Test] public void Validate_ShouldPass_WhenSizeIsAtMaximum() { var contacts = Enumerable.Range(0, 50_000) .Select(_ => RandomContactImportRequest()) .ToList(); var request = new ContactsImportRequest(contacts); var result = request.Validate(); result.IsValid.Should().BeTrue(); }src/Mailtrap.Abstractions/ContactImports/Requests/ContactsImportRequest.cs (3)
3-6: Fix XML summary: this type is for bulk imports, not generic CRUD.
The current summary is misleading.-/// Generic request object for contact CRUD operations. +/// Request payload for creating a Contacts Import (bulk upload).
37-45: Avoid double enumeration and reject null elements.
Ensure.NotNullOrEmpty enumerates once; then building the List enumerates again. Also, null items can slip through and later fail at serialization/validation.public ContactsImportRequest(IEnumerable<ContactImportRequest> contacts) { - Ensure.NotNullOrEmpty(contacts, nameof(contacts)); - - // Defensive copy to prevent post-ctor mutation. - Contacts = contacts is List<ContactImportRequest> list - ? new List<ContactImportRequest>(list) // defensive copy when already a List<T> - : new List<ContactImportRequest>(contacts); // otherwise enumerate once + Ensure.NotNull(contacts, nameof(contacts)); + // Materialize once + var materialized = contacts as List<ContactImportRequest> ?? contacts.ToList(); + if (materialized.Count == 0) + throw new ArgumentNullException(nameof(contacts)); + if (materialized.Any(c => c is null)) + throw new ArgumentNullException(nameof(contacts), "Contacts collection contains null item(s)."); + // Defensive copy to prevent post-ctor mutation of the input collection + Contacts = new List<ContactImportRequest>(materialized); }
15-18: Public mutability of Contacts may be unintended.
Exposing IList allows external mutation post-construction, undermining the defensive copy. If you want a stable request object, expose IReadOnlyList and keep a mutable backing list for serializers.I can provide a serializer-friendly pattern (backing List + IReadOnlyList facade with [JsonInclude]/[JsonIgnore]) if you want to tighten immutability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (39)
Mailtrap.sln(3 hunks)examples/Mailtrap.Example.ContactImports/Mailtrap.Example.ContactImports.csproj(1 hunks)examples/Mailtrap.Example.ContactImports/Program.cs(1 hunks)examples/Mailtrap.Example.ContactImports/Properties/launchSettings.json(1 hunks)examples/Mailtrap.Example.ContactImports/appsettings.json(1 hunks)src/Mailtrap.Abstractions/ContactImports/IContactsImportCollectionResource.cs(1 hunks)src/Mailtrap.Abstractions/ContactImports/IContactsImportResource.cs(1 hunks)src/Mailtrap.Abstractions/ContactImports/Models/ContactsImport.cs(1 hunks)src/Mailtrap.Abstractions/ContactImports/Models/ContactsImportStatus.cs(1 hunks)src/Mailtrap.Abstractions/ContactImports/Requests/ContactsImportRequest.cs(1 hunks)src/Mailtrap.Abstractions/ContactImports/Validators/ContactsImportRequestValidator.cs(1 hunks)src/Mailtrap.Abstractions/Contacts/IContactCollectionResource.cs(1 hunks)src/Mailtrap.Abstractions/Contacts/Requests/ContactImportRequest.cs(1 hunks)src/Mailtrap.Abstractions/Contacts/Requests/ContactRequest.cs(2 hunks)src/Mailtrap.Abstractions/Contacts/Requests/CreateContactRequest.cs(1 hunks)src/Mailtrap.Abstractions/Contacts/Requests/UpdateContactRequest.cs(2 hunks)src/Mailtrap.Abstractions/Core/Extensions/Ensure.cs(5 hunks)src/Mailtrap.Abstractions/GlobalSuppressions.cs(1 hunks)src/Mailtrap.Abstractions/GlobalUsings.cs(1 hunks)src/Mailtrap/ContactImports/ContactsImportCollectionResource.cs(1 hunks)src/Mailtrap/ContactImports/ContactsImportResource.cs(1 hunks)src/Mailtrap/Contacts/ContactCollectionResource.cs(1 hunks)src/Mailtrap/GlobalUsings.cs(1 hunks)tests/Mailtrap.IntegrationTests/ContactImports/ContactImportsIntegrationTests.cs(1 hunks)tests/Mailtrap.IntegrationTests/ContactImports/Create_ShouldFailValidation_WhenProvidedCollectionSizeIsInvalid.json(1 hunks)tests/Mailtrap.IntegrationTests/ContactImports/Create_Success_Request.json(1 hunks)tests/Mailtrap.IntegrationTests/ContactImports/Create_Success_Response.json(1 hunks)tests/Mailtrap.IntegrationTests/ContactImports/GetDetails_Success.json(1 hunks)tests/Mailtrap.IntegrationTests/Contacts/ContactsIntegrationTests.cs(2 hunks)tests/Mailtrap.IntegrationTests/GlobalUsings.cs(1 hunks)tests/Mailtrap.IntegrationTests/TestConstants/UrlSegmentsTestConstants.cs(1 hunks)tests/Mailtrap.IntegrationTests/TestExtensions/EmailAddressHelpers.cs(1 hunks)tests/Mailtrap.IntegrationTests/TestExtensions/FileHelpers.cs(1 hunks)tests/Mailtrap.UnitTests/ContactImports/ContactsImportCollectionResourceTests.cs(1 hunks)tests/Mailtrap.UnitTests/ContactImports/ContactsImportResourceTests.cs(1 hunks)tests/Mailtrap.UnitTests/ContactImports/Requests/ContactsImportRequestTests.cs(1 hunks)tests/Mailtrap.UnitTests/Contacts/ContactResourceTests.cs(1 hunks)tests/Mailtrap.UnitTests/GlobalUsings.cs(1 hunks)tests/Mailtrap.UnitTests/TestConstants/UrlSegmentsTestConstants.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-04T12:22:07.783Z
Learnt from: dr-3lo
PR: railsware/mailtrap-dotnet#139
File: src/Mailtrap/Contacts/Requests/ContactRequestExtensions.cs:6-9
Timestamp: 2025-09-04T12:22:07.783Z
Learning: In the Mailtrap .NET client codebase, internal extension methods with non-nullable reference types don't require explicit null guards as the compiler provides compile-time safety and call sites are controlled within the codebase.
Applied to files:
tests/Mailtrap.IntegrationTests/GlobalUsings.cstests/Mailtrap.UnitTests/GlobalUsings.cssrc/Mailtrap/GlobalUsings.cssrc/Mailtrap.Abstractions/GlobalUsings.csexamples/Mailtrap.Example.ContactImports/Program.cssrc/Mailtrap.Abstractions/Core/Extensions/Ensure.cstests/Mailtrap.IntegrationTests/TestExtensions/EmailAddressHelpers.cs
📚 Learning: 2025-09-04T08:53:06.292Z
Learnt from: vittorius
PR: railsware/mailtrap-dotnet#139
File: src/Mailtrap.Abstractions/Contacts/Models/Contact.cs:67-70
Timestamp: 2025-09-04T08:53:06.292Z
Learning: In the Mailtrap .NET client, DateTimeOffset properties should not use explicit JsonConverter attributes. All models in the codebase (TestingMessage, Inbox, EmailAttachment, etc.) handle DateTimeOffset properties using default JSON serialization without custom converters, and the Contact model should follow this same pattern for consistency.
Applied to files:
tests/Mailtrap.IntegrationTests/GlobalUsings.cstests/Mailtrap.UnitTests/GlobalUsings.cssrc/Mailtrap/GlobalUsings.cssrc/Mailtrap.Abstractions/GlobalUsings.cs
📚 Learning: 2025-09-04T12:23:59.276Z
Learnt from: dr-3lo
PR: railsware/mailtrap-dotnet#139
File: tests/Mailtrap.IntegrationTests/Contacts/Update_Success.json:16-17
Timestamp: 2025-09-04T12:23:59.276Z
Learning: Test fixtures in the Mailtrap .NET client should accurately represent the actual server response format and should not be modified to match client-side converters or serialization preferences.
Applied to files:
tests/Mailtrap.IntegrationTests/ContactImports/ContactImportsIntegrationTests.cstests/Mailtrap.IntegrationTests/Contacts/ContactsIntegrationTests.cs
🧬 Code graph analysis (19)
src/Mailtrap.Abstractions/ContactImports/IContactsImportResource.cs (3)
src/Mailtrap.Abstractions/Contacts/IContactCollectionResource.cs (3)
IContactsImportResource(32-32)Task(45-45)Task(62-62)src/Mailtrap/Contacts/ContactCollectionResource.cs (3)
IContactsImportResource(18-19)Task(21-22)Task(24-25)src/Mailtrap/ContactImports/ContactsImportResource.cs (1)
Task(11-12)
src/Mailtrap.Abstractions/ContactImports/IContactsImportCollectionResource.cs (5)
src/Mailtrap.Abstractions/Contacts/IContactCollectionResource.cs (3)
IContactsImportCollectionResource(15-15)Task(45-45)Task(62-62)src/Mailtrap/Contacts/ContactCollectionResource.cs (3)
IContactsImportCollectionResource(15-16)Task(21-22)Task(24-25)src/Mailtrap/ContactImports/ContactsImportCollectionResource.cs (1)
Task(11-12)src/Mailtrap.Abstractions/ContactImports/IContactsImportResource.cs (1)
Task(19-19)src/Mailtrap.Abstractions/ContactImports/Requests/ContactsImportRequest.cs (2)
ContactsImportRequest(37-45)ContactsImportRequest(50-50)
src/Mailtrap/ContactImports/ContactsImportResource.cs (3)
src/Mailtrap.Abstractions/Contacts/IContactCollectionResource.cs (3)
IContactsImportResource(32-32)Task(45-45)Task(62-62)src/Mailtrap/Contacts/ContactCollectionResource.cs (3)
IContactsImportResource(18-19)Task(21-22)Task(24-25)src/Mailtrap.Abstractions/ContactImports/IContactsImportResource.cs (1)
Task(19-19)
src/Mailtrap.Abstractions/Contacts/Requests/ContactImportRequest.cs (2)
tests/Mailtrap.UnitTests/ContactImports/Requests/ContactsImportRequestTests.cs (1)
ContactImportRequest(55-62)src/Mailtrap.Abstractions/Contacts/Requests/ContactRequest.cs (2)
ContactRequest(45-50)ContactRequest(55-55)
src/Mailtrap.Abstractions/ContactImports/Requests/ContactsImportRequest.cs (3)
src/Mailtrap.Abstractions/Contacts/Requests/ContactImportRequest.cs (2)
ContactImportRequest(31-31)ContactImportRequest(36-36)src/Mailtrap.Abstractions/Core/Extensions/Ensure.cs (3)
Ensure(9-116)NotNullOrEmpty(42-57)NotNullOrEmpty(66-81)src/Mailtrap.Abstractions/ContactImports/Validators/ContactsImportRequestValidator.cs (2)
ContactsImportRequestValidator(8-29)ContactsImportRequestValidator(20-28)
src/Mailtrap/Contacts/ContactCollectionResource.cs (4)
tests/Mailtrap.UnitTests/Contacts/ContactCollectionResourceTests.cs (1)
ContactCollectionResource(50-50)src/Mailtrap.Abstractions/Contacts/IContactCollectionResource.cs (2)
IContactsImportCollectionResource(15-15)IContactsImportResource(32-32)src/Mailtrap/ContactImports/ContactsImportCollectionResource.cs (2)
ContactsImportCollectionResource(6-13)ContactsImportCollectionResource(8-9)src/Mailtrap/ContactImports/ContactsImportResource.cs (2)
ContactsImportResource(6-13)ContactsImportResource(8-9)
tests/Mailtrap.UnitTests/ContactImports/ContactsImportCollectionResourceTests.cs (5)
tests/Mailtrap.UnitTests/ContactImports/ContactsImportResourceTests.cs (4)
TestFixture(4-52)Test(19-27)Test(29-37)Test(39-47)tests/Mailtrap.UnitTests/ContactImports/Requests/ContactsImportRequestTests.cs (4)
TestFixture(4-63)Test(7-13)Test(15-21)Test(23-34)tests/Mailtrap.UnitTests/Contacts/ContactResourceTests.cs (1)
TestFixture(4-51)tests/Mailtrap.UnitTests/TestConstants/UrlSegmentsTestConstants.cs (1)
UrlSegmentsTestConstants(4-19)src/Mailtrap/ContactImports/ContactsImportCollectionResource.cs (2)
ContactsImportCollectionResource(6-13)ContactsImportCollectionResource(8-9)
src/Mailtrap.Abstractions/Contacts/IContactCollectionResource.cs (1)
src/Mailtrap/Contacts/ContactCollectionResource.cs (2)
IContactsImportCollectionResource(15-16)IContactsImportResource(18-19)
tests/Mailtrap.IntegrationTests/ContactImports/ContactImportsIntegrationTests.cs (12)
tests/Mailtrap.IntegrationTests/Contacts/ContactsIntegrationTests.cs (8)
TestFixture(4-310)Test(33-71)Test(73-113)Test(115-152)Test(154-191)Test(193-234)Test(236-274)Test(276-309)tests/Mailtrap.UnitTests/ContactImports/ContactsImportCollectionResourceTests.cs (4)
TestFixture(4-52)Test(19-27)Test(29-37)Test(39-47)tests/Mailtrap.UnitTests/ContactImports/ContactsImportResourceTests.cs (4)
TestFixture(4-52)Test(19-27)Test(29-37)Test(39-47)tests/Mailtrap.UnitTests/ContactImports/Requests/ContactsImportRequestTests.cs (5)
TestFixture(4-63)Test(7-13)Test(15-21)Test(23-34)ContactImportRequest(55-62)src/Mailtrap/Configuration/MailtrapClientOptions.cs (1)
JsonSerializerOptions(148-154)tests/Mailtrap.IntegrationTests/TestConstants/UrlSegmentsTestConstants.cs (1)
UrlSegmentsTestConstants(4-22)src/Mailtrap.Abstractions/ContactImports/IContactsImportCollectionResource.cs (1)
Task(28-28)src/Mailtrap/ContactImports/ContactsImportCollectionResource.cs (1)
Task(11-12)tests/Mailtrap.IntegrationTests/TestExtensions/FileHelpers.cs (2)
Task(6-14)Task(16-28)src/Mailtrap.Abstractions/ContactImports/Requests/ContactsImportRequest.cs (2)
ContactsImportRequest(37-45)ContactsImportRequest(50-50)src/Mailtrap/Core/Constants/MimeTypes.cs (2)
MimeTypes(4-19)Application(6-11)src/Mailtrap/Core/Constants/HeaderValues.cs (1)
HeaderValues(7-12)
examples/Mailtrap.Example.ContactImports/Program.cs (4)
src/Mailtrap.Abstractions/Contacts/IContactCollectionResource.cs (2)
IContactsImportCollectionResource(15-15)IContactsImportResource(32-32)src/Mailtrap/Contacts/ContactCollectionResource.cs (2)
IContactsImportCollectionResource(15-16)IContactsImportResource(18-19)src/Mailtrap.Abstractions/Contacts/Requests/ContactImportRequest.cs (2)
ContactImportRequest(31-31)ContactImportRequest(36-36)src/Mailtrap.Abstractions/ContactImports/Requests/ContactsImportRequest.cs (2)
ContactsImportRequest(37-45)ContactsImportRequest(50-50)
tests/Mailtrap.IntegrationTests/Contacts/ContactsIntegrationTests.cs (1)
tests/Mailtrap.IntegrationTests/TestExtensions/EmailAddressHelpers.cs (1)
NextEmail(16-51)
src/Mailtrap.Abstractions/ContactImports/Models/ContactsImportStatus.cs (1)
src/Mailtrap.Abstractions/Core/Models/StringEnum.cs (1)
StringEnum(41-49)
src/Mailtrap.Abstractions/Contacts/Requests/UpdateContactRequest.cs (2)
src/Mailtrap.Abstractions/Contacts/Requests/ContactImportRequest.cs (2)
ContactImportRequest(31-31)ContactImportRequest(36-36)tests/Mailtrap.UnitTests/ContactImports/Requests/ContactsImportRequestTests.cs (1)
ContactImportRequest(55-62)
tests/Mailtrap.UnitTests/ContactImports/ContactsImportResourceTests.cs (4)
tests/Mailtrap.IntegrationTests/ContactImports/ContactImportsIntegrationTests.cs (4)
TestFixture(4-172)Test(33-77)Test(79-122)Test(124-163)tests/Mailtrap.UnitTests/ContactImports/ContactsImportCollectionResourceTests.cs (4)
TestFixture(4-52)Test(19-27)Test(29-37)Test(39-47)tests/Mailtrap.UnitTests/Contacts/ContactResourceTests.cs (1)
TestFixture(4-51)tests/Mailtrap.UnitTests/TestConstants/UrlSegmentsTestConstants.cs (1)
UrlSegmentsTestConstants(4-19)
src/Mailtrap.Abstractions/Core/Extensions/Ensure.cs (3)
src/Mailtrap.Abstractions/Core/Models/StringEnum.cs (2)
T(66-72)T(86-97)src/Mailtrap/Core/Converters/StringEnumJsonConverter.cs (1)
T(11-23)src/Mailtrap/Core/Extensions/HttpContentExtensions.cs (1)
T(28-35)
tests/Mailtrap.IntegrationTests/TestExtensions/FileHelpers.cs (3)
tests/Mailtrap.IntegrationTests/ContactImports/ContactImportsIntegrationTests.cs (4)
Task(165-171)Test(33-77)Test(79-122)Test(124-163)src/Mailtrap.Abstractions/Core/Extensions/Ensure.cs (1)
Ensure(9-116)tests/Mailtrap.IntegrationTests/Contacts/ContactsIntegrationTests.cs (7)
Test(33-71)Test(73-113)Test(115-152)Test(154-191)Test(193-234)Test(236-274)Test(276-309)
src/Mailtrap.Abstractions/ContactImports/Validators/ContactsImportRequestValidator.cs (1)
src/Mailtrap.Abstractions/ContactImports/Requests/ContactsImportRequest.cs (2)
ContactsImportRequest(37-45)ContactsImportRequest(50-50)
tests/Mailtrap.UnitTests/ContactImports/Requests/ContactsImportRequestTests.cs (3)
tests/Mailtrap.IntegrationTests/ContactImports/ContactImportsIntegrationTests.cs (3)
Test(33-77)Test(79-122)Test(124-163)src/Mailtrap.Abstractions/ContactImports/Requests/ContactsImportRequest.cs (2)
ContactsImportRequest(37-45)ContactsImportRequest(50-50)src/Mailtrap.Abstractions/Contacts/Requests/ContactImportRequest.cs (2)
ContactImportRequest(31-31)ContactImportRequest(36-36)
src/Mailtrap/ContactImports/ContactsImportCollectionResource.cs (4)
tests/Mailtrap.UnitTests/ContactImports/ContactsImportCollectionResourceTests.cs (1)
ContactsImportCollectionResource(51-51)src/Mailtrap/Contacts/ContactCollectionResource.cs (3)
IContactsImportCollectionResource(15-16)Task(21-22)Task(24-25)src/Mailtrap.Abstractions/ContactImports/IContactsImportCollectionResource.cs (1)
Task(28-28)src/Mailtrap.Abstractions/ContactImports/Requests/ContactsImportRequest.cs (2)
ContactsImportRequest(37-45)ContactsImportRequest(50-50)
🔇 Additional comments (28)
examples/Mailtrap.Example.ContactImports/Properties/launchSettings.json (1)
1-10: LGTMDev profile looks fine for a console sample.
tests/Mailtrap.UnitTests/TestConstants/UrlSegmentsTestConstants.cs (1)
18-18: LGTMImports segment constant is consistent with the API surface.
src/Mailtrap/GlobalUsings.cs (1)
35-37: LGTMScoped global usings for ContactImports are appropriate and align with existing patterns.
examples/Mailtrap.Example.ContactImports/Mailtrap.Example.ContactImports.csproj (2)
3-4: No changes needed: Central Package Management is enabled and VersionOverride references match the central versions.
1-11: No action needed – TargetFramework is inherited via Directory.Build.props
The example project auto-importsexamples/Directory.Build.props, which brings inbuild/mailtrap-global.props(TargetFrameworks=netstandard2.0) andbuild/mailtrap-example.props(TargetFrameworks=net9.0).src/Mailtrap.Abstractions/Contacts/Requests/CreateContactRequest.cs (1)
21-24: LGTMSerializer-friendly ctor is fine; base parameterless ctor sets Email to empty and validation will catch missing values.
src/Mailtrap.Abstractions/Contacts/Requests/ContactRequest.cs (2)
52-55: LGTMParameterless ctor set to Email = string.Empty is consistent with validation flow.
27-28: No action needed; STJ v9.0.7 is referenced centrally, so JsonObjectCreationHandling is supportedsrc/Mailtrap.Abstractions/ContactImports/Models/ContactsImportStatus.cs (1)
1-44: LGTM: StringEnum-backed status is well-defined and consistent with payloadsValues align with fixtures (“created/started/finished/failed”) and the existing StringEnum infrastructure.
Mailtrap.sln (1)
92-93: LGTM: Solution wiring for the new example project looks correctProject GUID/type matches other SDK-style examples, configs are added for Debug/Release, and nesting is under “examples”.
Also applies to: 172-175, 199-199
tests/Mailtrap.UnitTests/Contacts/ContactResourceTests.cs (1)
5-5: LGTM: Class rename fixes the typoName now matches the subject under test and file name convention.
tests/Mailtrap.IntegrationTests/GlobalUsings.cs (1)
17-18: LGTM: Global usings for ContactImports are correctly introducedKeeps test files concise and aligns with other namespaces.
tests/Mailtrap.IntegrationTests/ContactImports/GetDetails_Success.json (1)
1-7: LGTM: Fixture matches model expectationsSnake_case fields and “started” status align with the ContactsImport model and status enum.
tests/Mailtrap.UnitTests/GlobalUsings.cs (1)
28-29: LGTM: Unit test global usings extended for ContactImportsConsistent with integration tests and new API surface.
tests/Mailtrap.IntegrationTests/ContactImports/Create_Success_Response.json (1)
1-7: LGTM: Create response fixture is coherent with Finished statusFields and values match the expected schema.
tests/Mailtrap.IntegrationTests/Contacts/ContactsIntegrationTests.cs (1)
80-82: NextEmail() adoption reduces flaky email generation — LGTMGood move to use the helper; it enforces minimal email constraints and keeps tests robust.
tests/Mailtrap.IntegrationTests/TestConstants/UrlSegmentsTestConstants.cs (1)
21-22: ’imports’ segment is consistent across production code and tests; no action needed.tests/Mailtrap.IntegrationTests/ContactImports/Create_ShouldFailValidation_WhenProvidedCollectionSizeIsInvalid.json (1)
1-23: Confirm server-accurate error payloads and phrasingPer prior learning, fixtures must mirror real server responses. Please verify keys and exact messages (e.g., “cannot import more than 50000 contacts at once”, punctuation, casing).
src/Mailtrap.Abstractions/GlobalSuppressions.cs (1)
7-7: Targeted CA1716 suppression — LGTMScoped to the member and justified; keeps VB keyword clash noise down without broadening scope.
src/Mailtrap.Abstractions/Contacts/IContactCollectionResource.cs (1)
8-16: New Imports() accessor — LGTMInterface shape and docs look good.
src/Mailtrap.Abstractions/GlobalUsings.cs (1)
30-33: Global usings for ContactImports — LGTMAppropriate exposure for new namespaces.
src/Mailtrap.Abstractions/Contacts/Requests/UpdateContactRequest.cs (2)
6-6: Confirm base-type switch doesn’t alter the public JSON contract unexpectedlySwitching to ContactImportRequest likely reintroduces list include/exclude via inheritance. Please verify serialized shape for UpdateContactRequest remains compatible with existing API expectations.
18-24: Constructors look good for both ergonomic use and serializersRetaining email ctor and adding parameterless one is appropriate.
tests/Mailtrap.UnitTests/ContactImports/ContactsImportResourceTests.cs (1)
19-27: Constructor guard tests and ResourceUri assertion are solidNice coverage of null guards and URI initialization.
Also applies to: 29-37, 39-47
src/Mailtrap/ContactImports/ContactsImportCollectionResource.cs (1)
8-9: Ctor pass-through is fineStraight delegation to base RestResource is consistent with the SDK pattern.
tests/Mailtrap.IntegrationTests/ContactImports/ContactImportsIntegrationTests.cs (1)
33-77: LGTM: create flow and HTTP expectations are solidHeaders, body match, and deserialization checks look good.
tests/Mailtrap.UnitTests/ContactImports/ContactsImportCollectionResourceTests.cs (1)
17-49: LGTM: constructor guard and ResourceUri testsThe guard clauses and URI initialization checks align with the rest of the resources.
src/Mailtrap.Abstractions/ContactImports/Requests/ContactsImportRequest.cs (1)
15-17: Verify C# 12 and System.Text.Json v8+ support in imported props
The[]collection initializer andJsonObjectCreationHandlingattribute require LangVersion 12 and STJ v8+/net8; confirm thatmailtrap-nuget.props(imported via$(BuildConfigurationDirectory)) sets TargetFramework(s) andLangVersionaccordingly. Otherwise replace= []with= new List<ContactImportRequest>();.
src/Mailtrap.Abstractions/ContactImports/Requests/ContactsImportRequest.cs
Outdated
Show resolved
Hide resolved
src/Mailtrap.Abstractions/ContactImports/Requests/ContactsImportRequest.cs
Show resolved
Hide resolved
Enhance validation for ContactsImportRequest and improve documentation
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
src/Mailtrap.Abstractions/ContactImports/Validators/ContactsImportRequestValidator.cs (5)
4-7: Doc summary is for the wrong type/behavior.This validator targets ContactsImportRequest and enforces collection size + per-item validation, not create/update or email length.
Apply this diff:
/// <summary> -/// Validator for Create/Update contact requests.<br /> -/// Ensures contact's email is not empty and length is within the allowed range. +/// Validator for contacts import requests. +/// Ensures the 'contacts' collection is non-empty, within allowed bounds, and each contact is valid. /// </summary>
17-19: Minor: fix constructor doc.Not a primary constructor; update wording.
/// <summary> -/// Primary constructor. +/// Constructor. /// </summary>
10-11: Tighten visibility of constants (class is internal).public inside an internal type can confuse readers. Prefer internal (or private if not used externally).
- public const int MaxContactsPerRequest = 50_000; - public const int MinContactsPerRequest = 1; + internal const int MaxContactsPerRequest = 50_000; + internal const int MinContactsPerRequest = 1;
20-26: Simplify predicate and add explicit messages.
- Null check and lower-bound are redundant after NotEmpty + Cascade.
- Add WithMessage for actionable feedback.
- RuleFor(r => r.Contacts) - .Cascade(CascadeMode.Stop) - .NotEmpty() - .Must(list => list != null && list.Count is >= MinContactsPerRequest and <= MaxContactsPerRequest); + RuleFor(r => r.Contacts) + .Cascade(CascadeMode.Stop) + .NotEmpty().WithMessage($"contacts must contain at least {MinContactsPerRequest} item.") + .Must(list => list is { Count: <= MaxContactsPerRequest }) + .WithMessage($"contacts cannot exceed {MaxContactsPerRequest} items.");
27-30: Guard per-item validator and improve error messaging.Ensure the child validator runs only when the item is non-null; include indexed message.
- RuleForEach(r => r.Contacts) - .NotNull() - .SetValidator(ContactRequestValidator.Instance); + RuleForEach(r => r.Contacts) + .Cascade(CascadeMode.Stop) + .NotNull().WithMessage("contacts[{CollectionIndex}] must not be null.") + .SetValidator(ContactRequestValidator.Instance);tests/Mailtrap.IntegrationTests/TestExtensions/EmailAddressHelpers.cs (1)
23-24: Prefer const for the '@' token.Minor clarity/readability improvement.
- var atSymbol = "@"; + const string atSymbol = "@";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Mailtrap.Abstractions/ContactImports/Requests/ContactsImportRequest.cs(1 hunks)src/Mailtrap.Abstractions/ContactImports/Validators/ContactsImportRequestValidator.cs(1 hunks)src/Mailtrap.Abstractions/Core/Extensions/Ensure.cs(4 hunks)tests/Mailtrap.IntegrationTests/ContactImports/ContactImportsIntegrationTests.cs(1 hunks)tests/Mailtrap.IntegrationTests/TestExtensions/EmailAddressHelpers.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Mailtrap.Abstractions/Core/Extensions/Ensure.cs
- src/Mailtrap.Abstractions/ContactImports/Requests/ContactsImportRequest.cs
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: dr-3lo
PR: railsware/mailtrap-dotnet#142
File: src/Mailtrap/Contacts/ContactCollectionResource.cs:18-19
Timestamp: 2025-09-10T14:37:39.872Z
Learning: The Import method in ContactCollectionResource does not need explicit importId validation because this validation is already implemented in the URI model's Append method when handling long parameters.
📚 Learning: 2025-09-04T12:23:59.276Z
Learnt from: dr-3lo
PR: railsware/mailtrap-dotnet#139
File: tests/Mailtrap.IntegrationTests/Contacts/Update_Success.json:16-17
Timestamp: 2025-09-04T12:23:59.276Z
Learning: Test fixtures in the Mailtrap .NET client should accurately represent the actual server response format and should not be modified to match client-side converters or serialization preferences.
Applied to files:
tests/Mailtrap.IntegrationTests/ContactImports/ContactImportsIntegrationTests.cs
📚 Learning: 2025-09-04T12:22:07.783Z
Learnt from: dr-3lo
PR: railsware/mailtrap-dotnet#139
File: src/Mailtrap/Contacts/Requests/ContactRequestExtensions.cs:6-9
Timestamp: 2025-09-04T12:22:07.783Z
Learning: In the Mailtrap .NET client codebase, internal extension methods with non-nullable reference types don't require explicit null guards as the compiler provides compile-time safety and call sites are controlled within the codebase.
Applied to files:
tests/Mailtrap.IntegrationTests/TestExtensions/EmailAddressHelpers.cs
🧬 Code graph analysis (2)
tests/Mailtrap.IntegrationTests/ContactImports/ContactImportsIntegrationTests.cs (12)
tests/Mailtrap.UnitTests/ContactImports/Requests/ContactsImportRequestTests.cs (5)
Test(7-13)Test(15-21)Test(23-34)Test(37-53)ContactImportRequest(55-62)tests/Mailtrap.IntegrationTests/Contacts/ContactsIntegrationTests.cs (3)
Test(33-71)Test(73-113)Test(115-152)src/Mailtrap/Configuration/MailtrapClientOptions.cs (1)
JsonSerializerOptions(148-154)tests/Mailtrap.IntegrationTests/TestConstants/UrlSegmentsTestConstants.cs (1)
UrlSegmentsTestConstants(4-22)src/Mailtrap.Abstractions/ContactImports/IContactsImportCollectionResource.cs (1)
Task(28-28)src/Mailtrap/ContactImports/ContactsImportCollectionResource.cs (1)
Task(11-12)tests/Mailtrap.IntegrationTests/TestExtensions/FileHelpers.cs (2)
Task(6-14)Task(16-28)src/Mailtrap.Abstractions/ContactImports/Requests/ContactsImportRequest.cs (2)
ContactsImportRequest(39-47)ContactsImportRequest(52-52)src/Mailtrap/Core/Constants/MimeTypes.cs (2)
MimeTypes(4-19)Application(6-11)src/Mailtrap/Core/Constants/HeaderValues.cs (1)
HeaderValues(7-12)src/Mailtrap.Abstractions/Contacts/Requests/ContactImportRequest.cs (2)
ContactImportRequest(31-31)ContactImportRequest(36-36)tests/Mailtrap.IntegrationTests/TestExtensions/EmailAddressHelpers.cs (1)
NextEmail(16-51)
src/Mailtrap.Abstractions/ContactImports/Validators/ContactsImportRequestValidator.cs (2)
src/Mailtrap.Abstractions/ContactImports/Requests/ContactsImportRequest.cs (2)
ContactsImportRequest(39-47)ContactsImportRequest(52-52)src/Mailtrap.Abstractions/Core/Extensions/Ensure.cs (1)
NotNull(18-33)
🔇 Additional comments (10)
src/Mailtrap.Abstractions/ContactImports/Validators/ContactsImportRequestValidator.cs (1)
15-15: Singleton instance looks good.Thread-safe, allocation-free reuse path. Keep as-is.
tests/Mailtrap.IntegrationTests/TestExtensions/EmailAddressHelpers.cs (1)
16-16: Extension signature: good as-is (no null-guard needed).Matches our prior learning: internal extension methods with non-nullable refs don’t require explicit null checks.
tests/Mailtrap.IntegrationTests/ContactImports/ContactImportsIntegrationTests.cs (8)
7-12: LGTM!The test setup follows established patterns with proper field initialization and readonly enforcement for immutable test data.
15-31: LGTM!The constructor correctly builds the resource URI by appending the imports segment to the contacts endpoint, following the pattern established in other integration tests. The configuration setup with random data generation is consistent with existing test practices.
33-77: LGTM!The success test follows the established integration testing pattern with proper mock setup, request/response validation, and service container configuration. The test correctly validates headers, request content, and response deserialization.
79-122: LGTM!The validation test correctly verifies that oversized collections (50,001 items) trigger client-side validation without making HTTP requests. The test properly asserts that no HTTP request is sent when validation fails.
124-167: LGTM!The null validation test correctly verifies that collections containing null items trigger client-side validation. The test properly ensures no HTTP request is made when validation fails.
169-212: LGTM!The invalid email length validation test correctly tests boundary conditions (1 and 101 characters) and ensures client-side validation prevents invalid requests from being sent to the server.
214-253: LGTM!The GetDetails success test correctly validates the retrieval of import details by ID, following established patterns for GET operations with proper mock setup and response validation.
255-261: LGTM!The helper method correctly deserializes response content using the configured JSON options and properly resets the stream position for potential reuse, which is a good practice for test utilities.
src/Mailtrap.Abstractions/ContactImports/Validators/ContactsImportRequestValidator.cs
Show resolved
Hide resolved
- Enhance validation tests for ContactsImportRequest to include edge cases for collection size and entries format - Corrected ContactImportRequest documentation
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (12)
tests/Mailtrap.IntegrationTests/ContactImports/ContactImportsIntegrationTests.cs (7)
46-48: Return JSON content with correct media type in fixtures.StringContent defaults to text/plain; set application/json to better emulate the server and avoid future deserializer/content-type checks breaking.
Apply in tests/Mailtrap.IntegrationTests/TestExtensions/FileHelpers.cs:
@@ -internal static async Task<StringContent> LoadFileToStringContent( +internal static async Task<StringContent> LoadFileToStringContent( this string featureFolderName, string? fileName = null, string filexExt = "json") { - var fileString = await LoadFileToString(featureFolderName, fileName, filexExt); - - return new StringContent(fileString); + var fileString = await LoadFileToString(featureFolderName, fileName, filexExt); + return new StringContent(fileString, System.Text.Encoding.UTF8, MimeTypes.Application.Json); }
51-56: Also assert request Content-Type is JSON (robustly, ignore charset).This verifies the client sends JSON, complementing Accept and body checks.
mockHttp .Expect(httpMethod, requestUri) .WithHeaders("Authorization", $"Bearer {_clientConfig.ApiToken}") .WithHeaders("Accept", MimeTypes.Application.Json) .WithHeaders("User-Agent", HeaderValues.UserAgent.ToString()) + .With(req => req.Content?.Headers?.ContentType?.MediaType == MimeTypes.Application.Json) .WithJsonContent(request, _jsonSerializerOptions) .Respond(HttpStatusCode.Created, responseContent);
94-102: Avoid setting expectations for requests that must not be sent; use Fallback to fail-fast.These tests verify client-side validation prevents any HTTP call. Using Fallback is simpler and fails immediately if a request is made.
Example for all three tests:
-using var mockHttp = new MockHttpMessageHandler(); -var mockedRequest = mockHttp - .Expect(httpMethod, requestUri) - .WithHeaders("Authorization", $"Bearer {_clientConfig.ApiToken}") - .WithHeaders("Accept", MimeTypes.Application.Json) - .WithHeaders("User-Agent", HeaderValues.UserAgent.ToString()) - .WithJsonContent(request, _jsonSerializerOptions) - .Respond(HttpStatusCode.UnprocessableContent); +using var mockHttp = new MockHttpMessageHandler(); +mockHttp.Fallback.Respond(_ => throw new AssertionException("HTTP must not be called for invalid requests."));Then drop the GetMatchCount assertions (see next comment).
Also applies to: 139-147, 184-192 --- `121-122`: **Remove GetMatchCount checks when using Fallback.** With Fallback throwing, the absence of HTTP calls is guaranteed by the test passing. ```diff - mockHttp.GetMatchCount(mockedRequest).Should().Be(0);Also applies to: 166-167, 211-212
255-261: Simplify deserialization helper and avoid stream seeking.Read as string and deserialize; removes unnecessary Position reset and implicit stream lifetime concerns.
- private async Task<TValue?> DeserializeStringContentAsync<TValue>(StringContent responseContent) - { - var responseStream = await responseContent.ReadAsStreamAsync(); - var expectedResponse = await JsonSerializer.DeserializeAsync<TValue>(responseStream, _jsonSerializerOptions); - responseStream.Position = 0; // Reset stream position - return expectedResponse; - } + private async Task<TValue?> DeserializeStringContentAsync<TValue>(StringContent responseContent) + { + var json = await responseContent.ReadAsStringAsync(); + return JsonSerializer.Deserialize<TValue>(json, _jsonSerializerOptions); + }
19-19: Prefer positive IDs for readability.While not functionally required, using a positive accountId avoids surprising URIs with negative IDs.
- _accountId = random.NextLong(); + _accountId = Math.Abs(random.NextLong()) + 1;
71-73: Drop ConfigureAwait(false) in tests.Not needed in test code and adds noise.
- .Create(request) - .ConfigureAwait(false); + .Create(request);- .GetDetails() - .ConfigureAwait(false); + .GetDetails();Also applies to: 246-248
tests/Mailtrap.UnitTests/ContactImports/Requests/ContactsImportRequestTests.cs (5)
32-34: Also assert defensive copy, not just value equivalence.Validate that Contacts isn’t the same instance as the input list.
// Assert - request.Contacts.Should().BeEquivalentTo(contacts); + request.Contacts.Should().BeEquivalentTo(contacts); + request.Contacts.Should().NotBeSameAs(contacts);
76-93: Make the invalid record deterministic and fix the comment.Random.GetString(emailLength) can introduce non-deterministic characters; keep it stable and adjust the comment to not imply it’s purely a length failure.
- new(TestContext.CurrentContext.Random.GetString(emailLength)), // Invalid email length + new(new string('x', emailLength)), // Invalid email
95-102: Eliminate randomness from valid email generation to avoid flakiness.Randomized character sets can yield invalid emails (e.g., extra '@', spaces). Prefer a deterministic valid pattern.
- var email = TestContext.CurrentContext.Random.GetString(5) - + "@" - + TestContext.CurrentContext.Random.GetString(5) - + ".com"; - return new ContactImportRequest(email); + return new ContactImportRequest($"user{Guid.NewGuid():N}@example.com");
1-104: Add a positive-path validation test.Current tests only cover failures. Add one happy-path case to guard against accidental over-restriction in the validator.
[Test] public void Validate_ShouldPass_WhenProvidedCollectionIsWithinBoundsAndValid() { var contacts = new List<ContactImportRequest> { new("user1@example.com"), new("user2@example.com"), }; var result = new ContactsImportRequest(contacts).Validate(); result.IsValid.Should().BeTrue(); }
38-39: Use validator constants instead of magic numbersReplace the hard-coded
[Values(0, 50001)]with the public constants so tests auto-update if limits change:- public void Validate_ShouldFail_WhenProvidedCollectionSizeIsInvalid([Values(0, 50001)] int size) + public void Validate_ShouldFail_WhenProvidedCollectionSizeIsInvalid( + [Values( + Mailtrap.Abstractions.ContactImports.Validators.ContactsImportRequestValidator.MinContactsPerRequest - 1, + Mailtrap.Abstractions.ContactImports.Validators.ContactsImportRequestValidator.MaxContactsPerRequest + 1 + )] int size)
MinContactsPerRequestandMaxContactsPerRequestarepublic constinContactsImportRequestValidator.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Mailtrap.Abstractions/Contacts/Requests/ContactImportRequest.cs(1 hunks)tests/Mailtrap.IntegrationTests/ContactImports/ContactImportsIntegrationTests.cs(1 hunks)tests/Mailtrap.UnitTests/ContactImports/Requests/ContactsImportRequestTests.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Mailtrap.Abstractions/Contacts/Requests/ContactImportRequest.cs
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: dr-3lo
PR: railsware/mailtrap-dotnet#142
File: src/Mailtrap/Contacts/ContactCollectionResource.cs:18-19
Timestamp: 2025-09-10T14:37:39.872Z
Learning: The Import method in ContactCollectionResource does not need explicit importId validation because this validation is already implemented in the URI model's Append method when handling long parameters.
📚 Learning: 2025-09-10T17:01:49.247Z
Learnt from: dr-3lo
PR: railsware/mailtrap-dotnet#142
File: src/Mailtrap.Abstractions/ContactImports/Validators/ContactsImportRequestValidator.cs:27-30
Timestamp: 2025-09-10T17:01:49.247Z
Learning: In ContactsImportRequestValidator, ContactRequestValidator.Instance is used to validate ContactImportRequest items because ContactImportRequest inherits from ContactRequest and doesn't introduce additional validation rules that would require a separate validator.
Applied to files:
tests/Mailtrap.UnitTests/ContactImports/Requests/ContactsImportRequestTests.cs
📚 Learning: 2025-09-04T12:23:59.276Z
Learnt from: dr-3lo
PR: railsware/mailtrap-dotnet#139
File: tests/Mailtrap.IntegrationTests/Contacts/Update_Success.json:16-17
Timestamp: 2025-09-04T12:23:59.276Z
Learning: Test fixtures in the Mailtrap .NET client should accurately represent the actual server response format and should not be modified to match client-side converters or serialization preferences.
Applied to files:
tests/Mailtrap.IntegrationTests/ContactImports/ContactImportsIntegrationTests.cs
🧬 Code graph analysis (2)
tests/Mailtrap.UnitTests/ContactImports/Requests/ContactsImportRequestTests.cs (2)
src/Mailtrap.Abstractions/ContactImports/Requests/ContactsImportRequest.cs (2)
ContactsImportRequest(39-47)ContactsImportRequest(52-52)src/Mailtrap.Abstractions/Contacts/Requests/ContactImportRequest.cs (2)
ContactImportRequest(31-31)ContactImportRequest(36-36)
tests/Mailtrap.IntegrationTests/ContactImports/ContactImportsIntegrationTests.cs (10)
tests/Mailtrap.UnitTests/ContactImports/Requests/ContactsImportRequestTests.cs (8)
TestFixture(4-103)Test(7-13)Test(15-21)Test(23-34)Test(37-53)Test(55-73)Test(75-93)ContactImportRequest(95-102)src/Mailtrap/Configuration/MailtrapClientOptions.cs (1)
JsonSerializerOptions(148-154)tests/Mailtrap.IntegrationTests/TestConstants/UrlSegmentsTestConstants.cs (1)
UrlSegmentsTestConstants(4-22)src/Mailtrap.Abstractions/ContactImports/IContactsImportCollectionResource.cs (1)
Task(28-28)src/Mailtrap/ContactImports/ContactsImportCollectionResource.cs (1)
Task(11-12)tests/Mailtrap.IntegrationTests/TestExtensions/FileHelpers.cs (2)
Task(6-14)Task(16-28)src/Mailtrap.Abstractions/ContactImports/Requests/ContactsImportRequest.cs (2)
ContactsImportRequest(39-47)ContactsImportRequest(52-52)src/Mailtrap/Core/Constants/MimeTypes.cs (2)
MimeTypes(4-19)Application(6-11)src/Mailtrap/Core/Constants/HeaderValues.cs (1)
HeaderValues(7-12)tests/Mailtrap.IntegrationTests/TestExtensions/EmailAddressHelpers.cs (1)
NextEmail(16-51)
🔇 Additional comments (3)
tests/Mailtrap.IntegrationTests/ContactImports/ContactImportsIntegrationTests.cs (1)
19-31: URI building and client setup look solid (aligns with prior learning).Chained Append for accounts/contacts/imports is clear, and using MailtrapClientOptions.ToJsonSerializerOptions ensures parity with the client. No extra importId validation is introduced here, which matches the documented behavior that Append(long) validates.
tests/Mailtrap.UnitTests/ContactImports/Requests/ContactsImportRequestTests.cs (2)
56-73: Null items already validated – ContactsImportRequestValidator usesRuleForEach(r => r.Contacts).NotNull()beforeSetValidator, so null contacts are flagged as invalid.
16-21: Ignore exception type change—Ensure.NotNullOrEmpty(IEnumerable)throwsArgumentNullExceptionfor empty collections, so the test’s expectation is correct.Likely an incorrect or invalid review comment.
src/Mailtrap.Abstractions/ContactImports/Models/ContactsImport.cs
Outdated
Show resolved
Hide resolved
tests/Mailtrap.IntegrationTests/ContactImports/Create_Success_Response.json
Show resolved
Hide resolved
tests/Mailtrap.IntegrationTests/ContactImports/GetDetails_Success.json
Outdated
Show resolved
Hide resolved
- Corrected contact import variable names and - updated contact import status property - corrections in contact import integrational tests
Motivation
Contacts Import API support into .NET client
Changes
How to test
Summary by CodeRabbit
New Features
New Example
Tests
Refactor
Chores