Less memory more efficient#221
Conversation
Delete "lower" and add StringComparison.OrdinalIgnoreCase No duplicates, less memory and for speed
|
I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
WalkthroughThis PR refactors the LDAP result filtering logic in ChangesLDAP Filtering Optimization
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 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: 1
🤖 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 `@src/Producers/LdapProducer.cs`:
- Around line 150-152: The code uses distinguishedName.Contains(...,
StringComparison.OrdinalIgnoreCase) (and a StartsWith overload) which isn't
available on net472; replace the Contains call with a case-insensitive IndexOf
check (e.g., distinguishedName.IndexOf("cn=domainupdates,cn=system",
StringComparison.OrdinalIgnoreCase) >= 0) and keep or ensure the StartsWith uses
the StringComparison overload (distinguishedName.StartsWith("cn=user",
StringComparison.OrdinalIgnoreCase)) or, if you prefer maximum compatibility,
use a length check + string.Equals on the substring with
StringComparison.OrdinalIgnoreCase; update the checks around
distinguishedName.Contains and distinguishedName.StartsWith accordingly.
🪄 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: 8ef92340-ee0a-4d99-8911-91de6483511b
📒 Files selected for processing (1)
src/Producers/LdapProducer.cs
| if (distinguishedName.Contains("cn=domainupdates,cn=system", StringComparison.OrdinalIgnoreCase)) | ||
| continue; | ||
| if (lower.Contains("cn=policies,cn=system") && (lower.StartsWith("cn=user") || lower.StartsWith("cn=machine"))) | ||
| if (distinguishedName.Contains("cn=policies,cn=system", StringComparison.OrdinalIgnoreCase) && (distinguishedName.StartsWith("cn=user", StringComparison.OrdinalIgnoreCase) || distinguishedName.StartsWith("cn=machine", StringComparison.OrdinalIgnoreCase))) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify target framework supports Contains with StringComparison overload
# Check target framework in project file
echo "=== Target Framework ==="
fd -e csproj --exec cat {} \; | grep -i "TargetFramework"
# Search for other ToLower() patterns that could be optimized similarly
echo -e "\n=== Other ToLower() + comparison patterns ==="
rg -n -C2 '\.ToLower\(\)\s*\.\s*(Contains|StartsWith|EndsWith|Equals|IndexOf)' --type csRepository: SpecterOps/SharpHound
Length of output: 187
Fix: string.Contains(string, StringComparison) is unavailable on .NET Framework 4.7.2.
The code at lines 150–152 uses Contains() and StartsWith() with the StringComparison.OrdinalIgnoreCase parameter, which was introduced in .NET Core 2.1 and .NET Standard 2.1. The target framework is net472 (.NET Framework 4.7.2), which does not support this overload. This will cause a compilation error.
Options:
- Keep the original
ToLower()approach for framework compatibility. - Upgrade the target framework to .NET 5.0+ or .NET Standard 2.1+.
- Use a custom case-insensitive comparison method compatible with net472.
🤖 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 `@src/Producers/LdapProducer.cs` around lines 150 - 152, The code uses
distinguishedName.Contains(..., StringComparison.OrdinalIgnoreCase) (and a
StartsWith overload) which isn't available on net472; replace the Contains call
with a case-insensitive IndexOf check (e.g.,
distinguishedName.IndexOf("cn=domainupdates,cn=system",
StringComparison.OrdinalIgnoreCase) >= 0) and keep or ensure the StartsWith uses
the StringComparison overload (distinguishedName.StartsWith("cn=user",
StringComparison.OrdinalIgnoreCase)) or, if you prefer maximum compatibility,
use a length check + string.Equals on the substring with
StringComparison.OrdinalIgnoreCase; update the checks around
distinguishedName.Contains and distinguishedName.StartsWith accordingly.
1 participant
Delete "lower" and add StringComparison.OrdinalIgnoreCase No duplicates, less memory and for speed
Description
Motivation and Context
This PR addresses: [GitHub issue or Jira ticket number]
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist:
Summary by CodeRabbit