Fspw 777#11
Open
MichalFrends1 wants to merge 5 commits into
Open
Conversation
|
Warning Review limit reached
More reviews will be available in 51 minutes and 47 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughVersion 1.5.0 adds message-size enforcement, TCP keep-alive connection caching ( ChangesFrends.Mllp.Send v1.5.0 Feature Set
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Send
participant MessageLogger
participant SendWithRetry
participant SendWithWrapper
participant SendWithKeepAlive
participant MtlsMllpWrapper
participant AckParser
Caller->>Send: Send(input, options, token)
Send->>Send: compute byte size, check MaxMessageSize
alt size exceeded
Send->>MessageLogger: LogRejected(reason)
Send-->>Caller: throw ArgumentException
end
Send->>MessageLogger: initialize session log
Send->>SendWithRetry: loop up to RetryCount+1
loop attempt
alt KeepConnectionAlive=true
SendWithRetry->>SendWithKeepAlive: send via cached CachedConnection
SendWithKeepAlive->>MtlsMllpWrapper: SendAndReceiveAck / SendOnly
else
SendWithRetry->>SendWithWrapper: send via new wrapper
SendWithWrapper->>MtlsMllpWrapper: SendAndReceiveAck / SendOnly
end
MtlsMllpWrapper-->>SendWithWrapper: ack string
SendWithWrapper->>AckParser: Parse(ackString)
AckParser-->>SendWithWrapper: AckParseResult
SendWithWrapper->>AckParser: IsAcceptable(resultType, AcceptableAckCodes)
alt unacceptable ACK
SendWithWrapper-->>SendWithRetry: throw AckRejectedException
SendWithRetry-->>Send: rethrow (no retry)
else IO/socket error
SendWithRetry->>MessageLogger: LogRetry(attempt, reason)
SendWithRetry->>SendWithRetry: sleep RetryIntervalSeconds
else success
SendWithWrapper-->>SendWithRetry: SendOutcome
SendWithRetry-->>Send: SendOutcome
end
end
Send->>MessageLogger: LogSuccess / LogFailure
Send-->>Caller: Result (AckResultType, AckCodeValue, AckErrorDescription)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Frends.Mllp.Send/Frends.Mllp.Send.Tests/FunctionalTests.cs (1)
36-36:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLoad certificate password from environment/.env instead of hardcoding.
Hardcoding
_password = "password"in source violates the test-secret handling rule and makes secret rotation harder.As per coding guidelines, tests should “Load secrets via dotenv”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Frends.Mllp.Send/Frends.Mllp.Send.Tests/FunctionalTests.cs` at line 36, The `_password` field in the FunctionalTests class is hardcoded with the literal value "password", which violates security guidelines. Instead, load this password from an environment variable or .env file using dotenv as per the test-secret handling guidelines. Replace the hardcoded assignment with code that reads the password from the environment at test initialization time, ensuring the secret is not stored in source code.Source: Coding guidelines
🧹 Nitpick comments (1)
Frends.Mllp.Send/Frends.Mllp.Send.Tests/AckParserTests.cs (1)
11-14: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd coverage for
CA/CE/CRACK aliases.
AckParser.ClassifyAckCodemapsCA/CE/CRtoo, but those paths are currently untested. Adding them here will protect the ACK mapping contract from regressions.Suggested test-case additions
[TestCase("AA", AckResultType.Accept)] + [TestCase("CA", AckResultType.Accept)] [TestCase("AE", AckResultType.Error)] + [TestCase("CE", AckResultType.Error)] [TestCase("AR", AckResultType.Reject)] + [TestCase("CR", AckResultType.Reject)] [TestCase("XX", AckResultType.Invalid)]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Frends.Mllp.Send/Frends.Mllp.Send.Tests/AckParserTests.cs` around lines 11 - 14, The test cases for AckParser.ClassifyAckCode are missing coverage for the CA/CE/CR ACK aliases that the method handles. Add three additional [TestCase] attributes to the test method in AckParserTests.cs: one for CA mapping to AckResultType.Accept, one for CE mapping to AckResultType.Error, and one for CR mapping to AckResultType.Reject. These additions will complete the test coverage for all ACK code paths and prevent regressions in the ACK mapping contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Frends.Mllp.Send/Frends.Mllp.Send.Tests/FunctionalTests.cs`:
- Around line 871-877: The X509Certificate2 instance created in the serverCert
variable is not being disposed after use, which leaks unmanaged certificate
handles and is problematic during repeated test runs. Wrap the serverCert
certificate in a using statement to ensure it is properly disposed after the
SslStream authentication completes. Apply the same fix to the other TLS handler
location mentioned in the comment (around lines 940-947) where a similar
certificate is created and used in AuthenticateAsServerAsync or related TLS
operations.
- Around line 900-903: The bare catch blocks in the test helper methods (around
the catch block at lines 900-903 and the other instances at lines 925-927 and
984-987) are swallowing all exceptions indiscriminately, masking server-side
failures and creating false-positive tests. Replace these bare catch statements
with explicit exception handling that only catches expected exceptions like
OperationCanceledException, and allow all other exceptions to propagate through
_serverTask so they surface as test failures when the server encounters
unexpected errors.
- Around line 675-680: The test initializes and stops a stopwatch but never
asserts on the elapsed time, so it cannot verify that no retry delay occurred.
After the stopwatch.Stop() call, add an assertion that checks the elapsed time
is minimal (for example, using stopwatch.ElapsedMilliseconds to verify it
completes quickly without retry delays). This ensures the zero-retry test
properly validates that retry-delay logic does not regress.
- Line 510: The test is hanging or flapping because _serverTask is being awaited
without proper cancellation while keep-alive is enabled, causing the server
logic in SetupServerLogicMultiMessage to continue reading indefinitely on the
open socket. To fix this, ensure the server cancellation token is canceled
before or during the await of _serverTask in the keep-alive test, or add a
timeout to prevent the test from blocking indefinitely. This ensures that
SetupServerLogicMultiMessage stops reading when the test completes rather than
waiting for the socket to close naturally.
In `@Frends.Mllp.Send/Frends.Mllp.Send.Tests/Helpers.cs`:
- Around line 103-105: The calculation of targetBytes using int multiplication
sizeInMb * 1024 * 1024 can overflow for large inputs without clear error
reporting. Add parameter validation to ensure sizeInMb is within reasonable
bounds before the multiplication, then wrap the arithmetic in a checked block to
explicitly detect and handle overflow conditions. If overflow is detected or
sizeInMb exceeds the validated bounds, throw an appropriate exception with a
descriptive message to prevent silent failures and aid debugging.
In `@Frends.Mllp.Send/Frends.Mllp.Send/Definitions/Options.cs`:
- Around line 103-111: The documentation for the LogFilePath property states
that a default location is used when the path is unspecified, but the RequiredIf
validation attribute makes LogFilePath mandatory when EnableLogging is true.
Update the XML documentation summary for the LogFilePath property to clarify
that the path is required when logging is enabled, removing the reference to a
default location that does not actually exist when validation is enforced.
In `@Frends.Mllp.Send/Frends.Mllp.Send/Frends.Mllp.Send.cs`:
- Around line 295-299: The keep-alive wrapper cache key is not including
encoding information, which allows wrappers with different encodings to be
incorrectly reused for the same host/port combination. Include the encoding
value (obtained via GetEncoding(connection) or connection.EncodingInString) in
the cache key construction for the CreateWrapper method and any other related
caching logic (also at lines 352-353) to ensure that different encodings are
treated as separate cache entries and prevent incompatible wrapper reuse.
- Around line 283-287: The RemovedCallback lambda in the ConnectionCache is
skipping disposal of CachedConnection objects when RemovedReason equals Removed,
which causes resource leaks. In the RemovedCallback (around lines 283-287), the
condition `if (args.RemovedReason != CacheEntryRemovedReason.Removed)` prevents
disposal during explicit cache removal operations. Change the logic to dispose
CachedConnection when RemovedReason == Removed by inverting the condition or
removing the condition check entirely so that disposal happens for all cache
removal scenarios. Apply this same fix to the second occurrence at lines
333-346.
- Around line 138-140: The Thread.Sleep call in the retry logic blocks
uninterruptibly and does not respond to cancellation requests that arrive during
the wait period. Replace the
Thread.Sleep(TimeSpan.FromSeconds(options.RetryIntervalSeconds)) call with an
await-based cancellation-aware delay operation that passes the cancellation
token, allowing the wait to be interrupted immediately if cancellation is
requested rather than blocking until the sleep completes.
In `@Frends.Mllp.Send/Frends.Mllp.Send/Helpers/RequiredIfAttribute.cs`:
- Around line 18-21: The RequiredIfAttribute validation silently skips when the
dependentProperty name is invalid because GetProperty returns null and the
subsequent null-conditional operator hides this. Add a null check immediately
after the GetProperty call for the property variable in the RequiredIfAttribute
class and throw an appropriate exception if the property is not found, ensuring
that misspelled or invalid dependent property names fail fast and surface the
configuration error instead of silently bypassing validation.
---
Outside diff comments:
In `@Frends.Mllp.Send/Frends.Mllp.Send.Tests/FunctionalTests.cs`:
- Line 36: The `_password` field in the FunctionalTests class is hardcoded with
the literal value "password", which violates security guidelines. Instead, load
this password from an environment variable or .env file using dotenv as per the
test-secret handling guidelines. Replace the hardcoded assignment with code that
reads the password from the environment at test initialization time, ensuring
the secret is not stored in source code.
---
Nitpick comments:
In `@Frends.Mllp.Send/Frends.Mllp.Send.Tests/AckParserTests.cs`:
- Around line 11-14: The test cases for AckParser.ClassifyAckCode are missing
coverage for the CA/CE/CR ACK aliases that the method handles. Add three
additional [TestCase] attributes to the test method in AckParserTests.cs: one
for CA mapping to AckResultType.Accept, one for CE mapping to
AckResultType.Error, and one for CR mapping to AckResultType.Reject. These
additions will complete the test coverage for all ACK code paths and prevent
regressions in the ACK mapping contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e8fb4d7f-6d3b-464b-a9aa-af99406e93c2
📒 Files selected for processing (19)
Frends.Mllp.Send/CHANGELOG.mdFrends.Mllp.Send/Frends.Mllp.Send.Tests/AckParserTests.csFrends.Mllp.Send/Frends.Mllp.Send.Tests/FunctionalTests.csFrends.Mllp.Send/Frends.Mllp.Send.Tests/Helpers.csFrends.Mllp.Send/Frends.Mllp.Send/Definitions/Enums.csFrends.Mllp.Send/Frends.Mllp.Send/Definitions/Options.csFrends.Mllp.Send/Frends.Mllp.Send/Definitions/Result.csFrends.Mllp.Send/Frends.Mllp.Send/Frends.Mllp.Send.csFrends.Mllp.Send/Frends.Mllp.Send/Frends.Mllp.Send.csprojFrends.Mllp.Send/Frends.Mllp.Send/Helpers/AckParseResult.csFrends.Mllp.Send/Frends.Mllp.Send/Helpers/AckParser.csFrends.Mllp.Send/Frends.Mllp.Send/Helpers/AckRejectedException.csFrends.Mllp.Send/Frends.Mllp.Send/Helpers/CachedConnection.csFrends.Mllp.Send/Frends.Mllp.Send/Helpers/MessageLogger.csFrends.Mllp.Send/Frends.Mllp.Send/Helpers/RequiredIfAttribute.csFrends.Mllp.Send/Frends.Mllp.Send/Helpers/SendOutcome.csFrends.Mllp.Send/Frends.Mllp.Send/Helpers/ValidIpAddressAttribute.csFrends.Mllp.Send/Frends.Mllp.Send/Helpers/ValidationHandler.csFrends.Mllp.Send/Frends.Mllp.Send/MtlsMllpWrapper.cs
Comment on lines
+675
to
+680
| var stopwatch = Stopwatch.StartNew(); | ||
| var result = Mllp.Send(input, connection, options, CancellationToken.None); | ||
| stopwatch.Stop(); | ||
|
|
||
| Assert.That(result.Success, Is.False); | ||
| } |
There was a problem hiding this comment.
The zero-retry test doesn’t currently verify “no retry delay.”
A stopwatch is started/stopped, but elapsed time is never asserted. This means the test can still pass even if retry-delay logic regresses.
Suggested assertion
Assert.That(result.Success, Is.False);
+ Assert.That(stopwatch.Elapsed.TotalSeconds, Is.LessThan(4),
+ "RetryCount=0 should not incur RetryIntervalSeconds delay.");
📝 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.
Suggested change
| var stopwatch = Stopwatch.StartNew(); | |
| var result = Mllp.Send(input, connection, options, CancellationToken.None); | |
| stopwatch.Stop(); | |
| Assert.That(result.Success, Is.False); | |
| } | |
| var stopwatch = Stopwatch.StartNew(); | |
| var result = Mllp.Send(input, connection, options, CancellationToken.None); | |
| stopwatch.Stop(); | |
| Assert.That(result.Success, Is.False); | |
| Assert.That(stopwatch.Elapsed.TotalSeconds, Is.LessThan(4), | |
| "RetryCount=0 should not incur RetryIntervalSeconds delay."); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Frends.Mllp.Send/Frends.Mllp.Send.Tests/FunctionalTests.cs` around lines 675
- 680, The test initializes and stops a stopwatch but never asserts on the
elapsed time, so it cannot verify that no retry delay occurred. After the
stopwatch.Stop() call, add an assertion that checks the elapsed time is minimal
(for example, using stopwatch.ElapsedMilliseconds to verify it completes quickly
without retry delays). This ensures the zero-retry test properly validates that
retry-delay logic does not regress.
|
|
||
| namespace Frends.Mllp.Send.Helpers | ||
| { | ||
| internal sealed class ValidIpAddressAttribute : ValidationAttribute |
Contributor
There was a problem hiding this comment.
is it used anywhere?
|
|
||
| namespace Frends.Mllp.Send.Helpers | ||
| { | ||
| internal sealed class RequiredIfAttribute : ValidationAttribute |
Contributor
There was a problem hiding this comment.
Is it used anywhere?
| { | ||
| cancellationToken.ThrowIfCancellationRequested(); | ||
|
|
||
| ValidateParameters(input, connection); |
Contributor
There was a problem hiding this comment.
I dont see any validation attributes used - if none is used, remove this line and whole ValidationHandler, if its unused
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Please review my changes :)
Summary by CodeRabbit
New Features
Bug Fixes
Tests