Separate adapted libraries. - #30
Merged
Merged
Conversation
Contributor
Reviewer's GuideRefactors the LDAP adapter architecture by introducing a shared adapter contracts package, splitting Novell and mock adapters into dedicated adapter projects with new namespaces and documentation, and modernizing the root README and solution structure around these adapters. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- Exposing
ServerCertificateValidationByPass()on the coreILdapConnectionAdaptercontract forces all implementations (including mocks) to support a potentially unsafe operation; consider moving this to a provider-specific extension or separate interface so that the abstraction doesn’t encourage certificate bypass in general use. - Replacing the
.slnwith.slnxonly may break tooling for environments that don’t yet understand the new format; consider keeping the originalBitai.Ldap.Helper.slnalongside the new solution or clearly documenting the required tooling/SDK version for.slnxusage.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Exposing `ServerCertificateValidationByPass()` on the core `ILdapConnectionAdapter` contract forces all implementations (including mocks) to support a potentially unsafe operation; consider moving this to a provider-specific extension or separate interface so that the abstraction doesn’t encourage certificate bypass in general use.
- Replacing the `.sln` with `.slnx` only may break tooling for environments that don’t yet understand the new format; consider keeping the original `Bitai.Ldap.Helper.sln` alongside the new solution or clearly documenting the required tooling/SDK version for `.slnx` usage.
## Individual Comments
### Comment 1
<location path="src/Bitai.LDAPHelper/LdapAdapters/ILdapConnectionAdapter.cs" line_range="15-20" />
<code_context>
+
+ public interface ILdapConnectionAdapter : IDisposable
+ {
+ int ConnectionTimeout { get; set; }
+ bool SecureSocketLayer { get; set; }
+ bool IsBound { get; }
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Connection timeout units differ from IConnectionInfo, which may cause confusion and subtle bugs.
In `ILdapConnectionAdapter`, `ConnectionTimeout` is an `int` in milliseconds, while `IConnectionInfo` uses a `short` in seconds. This discrepancy can lead to incorrect conversions or loss of precision when mapping between them. Please either standardize the unit across both interfaces or make the unit explicit in the property names (e.g., `ConnectionTimeoutMs` / `ConnectionTimeoutSeconds`).
Suggested implementation:
```csharp
namespace Bitai.LDAPHelper.LdapAdapters;
/// <summary>
/// Defines the operations required to manage and use an LDAP connection.
/// </summary>
public interface ILdapConnectionAdapter : IDisposable
{
/// <summary>
/// Gets or sets the connection timeout in seconds.
/// This property uses the same unit and underlying type as <c>IConnectionInfo.ConnectionTimeout</c>
/// to avoid confusion and conversion issues.
/// </summary>
short ConnectionTimeoutSeconds { get; set; }
```
1. Update all implementations of `ILdapConnectionAdapter` to replace the old `ConnectionTimeout` property (milliseconds) with the new `short ConnectionTimeoutSeconds` property, including backing fields and any related logic.
2. Adjust any mapping code between `IConnectionInfo` and `ILdapConnectionAdapter` to use `ConnectionTimeoutSeconds` directly without converting units.
3. Fix any consumers that still expect a millisecond-based timeout; if millisecond precision is needed in some places, compute it locally via `TimeSpan.FromSeconds(ConnectionTimeoutSeconds)` or `ConnectionTimeoutSeconds * 1000`.
4. If the old `ConnectionTimeout` property was public and already in use, consider marking it as obsolete (with `[Obsolete]`) or removing it entirely, depending on your versioning and compatibility requirements.
</issue_to_address>
### Comment 2
<location path="src/Bitai.LDAPHelper/LdapAdapters/ILdapConnectionAdapter.cs" line_range="31" />
<code_context>
+ /// Bypasses server certificate validation.
+ /// </summary>
+ /// <remarks>Use only in controlled environments.</remarks>
+ void ServerCertificateValidationByPass();
+
+ /// <summary>
</code_context>
<issue_to_address>
**🚨 issue (security):** The `ServerCertificateValidationByPass` method name contains a typo and suggests potentially risky behavior.
Beyond the minor typo (`ByPass` → `Bypass`), exposing an API that disables certificate validation is inherently risky. To reduce misuse, consider renaming it to something clearly test-only (e.g., `DisableServerCertificateValidationForTesting`) and/or limiting it to internal or test-only usage so it’s not available in production contexts.
</issue_to_address>
### Comment 3
<location path="adapters/Bitai.LDAPHelper.LdapAdapters.Novell/README.md" line_range="156" />
<code_context>
+using Bitai.LDAPHelper.DTO;
+using Bitai.LDAPHelper.LdapAdapters.Novell;
+
+// 1. Instanciate the Novell connection factory
+var connectionFactory = new NovellLdapConnectionFactoryAdapter();
+
</code_context>
<issue_to_address>
**issue (typo):** Correct spelling of "Instanciate" to "Instantiate" in the quick start comment.
In the quick start comment, correct "Instanciate" to "Instantiate" to fix the typo.
Suggested implementation:
```
# 1. Instantiate the Novell connection factory
```
If this comment appears inside a C# code block in the README (e.g., between ```csharp and ```), keep it as a C# comment and only fix the spelling:
<<<<<<< SEARCH
// 1. Instanciate the Novell connection factory
=======
// 1. Instantiate the Novell connection factory
>>>>>>> REPLACE
Adjust the exact formatting (markdown heading vs C# comment, leading spaces) to match the surrounding README style you are using for the quick start section.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This was referenced Jul 21, 2026
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
1 participant
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.
Assign the adapted Novell LDAP and LDAPHelperMock libraries to their own source code folders.
Summary by Sourcery
Separate LDAP adapter abstractions and concrete implementations into dedicated libraries and update documentation and tooling to reflect the new adapter-based architecture.
New Features:
Bitai.LDAPHelper.LdapAdaptersabstraction package defining LDAP connection, search, entry, attribute, and modification interfaces and enums.Bitai.LDAPHelper.LdapAdapters.Novell) and in-memory mock LDAP (Bitai.LDAPHelper.LdapAdapters.LdapHelperMock) to be consumed by the core helper and demos.Enhancements:
Bitai.LDAPHelperREADME with detailed architecture, adapter infrastructure description, usage scenarios, and troubleshooting guidance..slnxformat and adjust project structure to place adapters under a dedicatedadaptersfolder.Build:
Documentation:
Bitai.LDAPHelper.LdapAdapterswith a focused README explaining their purpose and how to implement custom providers.Tests:
Bitai.LDAPHelper.Tests.Mocksproject in favor of the new adapter-based mock implementation and update existing tests to referenceBitai.LDAPHelper.LdapAdapters.LdapHelperMock.