Fix break in DirectoryServices SearchResultCollection #113775
Conversation
Added enumeration handling when the internal ArrayList has already been loaded but an enumeration follows. This fixes the issue with 9.x Linq statements against the collection caused by the access of the .Count Fixes dotnet#113265
dotnet-policy-service
bot
added
the
community-contribution
|
I think the code is the simplest way to fix this issue without making tons more changes. I'm not sure my test class is useful/valid because I'm not on a LDAP domain-joined machine so it simply bypasses for me in local runs. :pointup: @BillArmstrong @stephentoub @tarekgh |
src/libraries/System.DirectoryServices/src/System/DirectoryServices/SearchResultCollection.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
LGTM, thanks @IDisposable
@stephentoub @steveharter could you please have a quick look? Note, this is a candidate for 9.0 service porting .
|
Pushed PR review updates and improved the unit tests. Can anyone do the |
Switched enumerator to struct Improved testing actual scenario of the bug.
IDisposable
force-pushed
the
search-result-eager
branch
from
9b4af68 to
d05ec50
Compare
src/libraries/System.DirectoryServices/src/System/DirectoryServices/SearchResultCollection.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.DirectoryServices/tests/System.DirectoryServices.Tests.csproj
Show resolved
Hide resolved
src/libraries/System.DirectoryServices/src/System/DirectoryServices/SearchResultCollection.cs
Show resolved
Hide resolved
src/libraries/System.DirectoryServices/tests/System/DirectoryServices/DirectorySearcherTests.cs
Outdated
Show resolved
Hide resolved
| private const int ADS_SYSTEMFLAG_CR_NTDS_DOMAIN = 0x2; | ||
|
|
||
| [ConditionalFact(nameof(IsLdapConfigurationExist))] | ||
| public void DirectorySearch_IteratesCorrectly_SimpleEnumeration() |
There was a problem hiding this comment.
Did all of these new tests fail prior?
There was a problem hiding this comment.
There were never any tests against this class as far as I could find, so no :)
The two tests come from from @stephentoub's minimal reproduction in this comment
There was a problem hiding this comment.
Sorry, that might have been unclear... the first test would have always succeeded (and I put this test in to ensure I didn't break anything). The second test would have failed without the changes in this PR.
src/libraries/System.DirectoryServices/tests/System/DirectoryServices/DirectorySearcherTests.cs
Outdated
Show resolved
Hide resolved
Switched the enumerator doc comments to XML docs. Resorted the test class in the csproj Strong type the SearchResultCollection in the test class.
| _innerList.Add(enumerator.Current); | ||
| eagerList.Add(enumerator.Current); | ||
|
|
||
| _innerList = eagerList; |
There was a problem hiding this comment.
The change to build the inner list in it's entirety and then set the member is to ensure that if an exception in thrown while populating this inner list we do NOT memoize the incomplete list because then we would (if enumerated again) not throw again and would instead silently only yield the entries we previously gathered. Of course anyone attempting _re_enumeration of a list that yielded an error the first time is probably wrong, but why add to the problem?
Made the unit test helper use actual variable types (not var)
|
FYI link to coding guidelines: https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/coding-style.md which do discuss the use of "var". |
src/libraries/System.DirectoryServices/src/System/DirectoryServices/SearchResultCollection.cs
Show resolved
Hide resolved
Per [review feedback from @stephentoub](dotnet#113775 (comment)) post-merge
Added enumeration handling when the internal ArrayList has already been loaded but an enumeration follows. This fixes the issue with 9.x Linq statements against the collection caused by the access of the .Count
Fixes #113265
I am not entirely sure the best way to test this, as we never had tests for this class until now so I created a new DirectorySearcherTests.cs (conditionally run when LDAP is available) using the setup shown in the bug report.