Add nullable annotations to System.DirectoryServices #48454

krwq · 2021-02-18T12:52:45Z

No description provided.

dotnet-issue-labeler · 2021-02-18T12:52:52Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

src/libraries/System.DirectoryServices/ref/System.DirectoryServices.cs

...raries/System.DirectoryServices/src/System/DirectoryServices/ActiveDirectory/ADAMInstance.cs

...m.DirectoryServices/src/System/DirectoryServices/ActiveDirectory/ActiveDirectoryPartition.cs

buyaa-n

Thanks @krwq i have 2 main comment:

Most of the comments related to PropertyManager.GetPropertyValue(...) which if look closely does not return null so better to remove ? which saving many 1 in many files
Most overrides of OnInsertComplete(int index, object? value), OnRemoveComplete(int index, object? value), OnSetComplete(int index, object? oldValue, object? newValue) are producing NRE if the value is null, i think we should suppress the warning instead making it nullable

Other than that overall LGTM.

buyaa-n · 2021-02-19T06:38:13Z

...raries/System.DirectoryServices/src/System/DirectoryServices/ActiveDirectory/ADAMInstance.cs

@@ -144,7 +144,7 @@ public static AdamInstance GetAdamInstance(DirectoryContext context)
                {
                    throw new ActiveDirectoryObjectNotFoundException(SR.Format(SR.AINotFound, context.Name), typeof(AdamInstance), context.Name);
                }
-                dnsHostName = (string)PropertyManager.GetPropertyValue(context, rootDSE, PropertyManager.DnsHostName);
+                dnsHostName = (string)PropertyManager.GetPropertyValue(context, rootDSE, PropertyManager.DnsHostName)!;


Here and everywhere else PropertyManager.GetPropertyValue(context, rootDSE, PropertyManager.DnsHostName) is not returning null, instead it throws

Suggested change

dnsHostName = (string)PropertyManager.GetPropertyValue(context, rootDSE, PropertyManager.DnsHostName)!;

dnsHostName = (string)PropertyManager.GetPropertyValue(context, rootDSE, PropertyManager.DnsHostName);

Note it will only throw if property value is not there at all but if null value is present it will not throw and return null instead. Also note that DirectoryEntry.Properties (which this helper accesses) is public and while there is a code path guarding against null in DirectoryEntry.Properties[anyName].Value (it's a no-op) then there is nothing guarding against DirectoryEntry.Properties[anyName].Add(null).

You are right, the code is not guaranteeing against null, but somehow PropertyValueCollection[index] and PropertyValueCollection.Add(object value) both documented to be non null (it says it throws ANE for null value). Literally for all references we are suppressing the return value from (string)PropertyManager.GetPropertyValue(context, rootDSE, PropertyManager.DnsHostName)! which also illustrates the intention was non-null, there is something wrong here or we are missing something.

With that being said I am resolving this and all comments related to this, please take a look at other unrelated comments

there is nothing guarding against DirectoryEntry.Properties[anyName].Add(null)

Even it is allowing null I think setting it into null should be an error, not sure who is the expert in this area, I am OK leaving this nullable for this PR but i think we should file an issue for this

...raries/System.DirectoryServices/src/System/DirectoryServices/ActiveDirectory/ADAMInstance.cs

...ryServices/src/System/DirectoryServices/ActiveDirectory/ActiveDirectoryInterSiteTransport.cs

src/libraries/System.DirectoryServices/src/System/DirectoryServices/PropertyCollection.cs

...DirectoryServices/src/System/DirectoryServices/ActiveDirectory/ReadOnlySiteLinkCollection.cs

src/libraries/System.DirectoryServices/src/System/DirectoryServices/DirectoryEntry.cs

buyaa-n · 2021-02-20T03:03:56Z

src/libraries/System.DirectoryServices/src/System/DirectoryServices/DirectorySearcher.cs

@@ -195,7 +196,7 @@ public TimeSpan ClientTimeout
        /// Gets or sets the Lightweight Directory Access Protocol (LDAP) filter string format.
        /// </devdoc>
        [DefaultValue(defaultFilter)]
-        public string Filter
+        public string? Filter


I think it shouldn't be nullable as it has defaulted to non-null and the setter is allowing null but also defaulting if the value was null, it will never be null unless set to null by constructor which might be mistake

Suggested change

public string? Filter

[AllowNull]

public string Filter

I've initially had it this way but this can currently be null (maybe due to bug, maybe intentional):

public DirectorySearcher(DirectoryEntry? searchRoot, string? filter, string[]? propertiesToLoad, SearchScope scope) is assigning filter directly and does not do any validation

in Utils.cs: internal static Hashtable GetValuesWithRangeRetrieval(DirectoryEntry searchRootEntry, string? filter, ArrayList propertiesWithRangeRetrieval, ArrayList propertiesWithoutRangeRetrieval, SearchScope searchScope) is being called with explicit null from DirectoryServer.cs GetParitions() (call goes through internal static Hashtable GetValuesWithRangeRetrieval(DirectoryEntry searchRootEntry, string? filter, ArrayList propertiesToLoad, SearchScope searchScope))

I'll leave this open and turn this into an issue

(likely constructor should assign to property rather than field)

buyaa-n · 2021-02-20T03:05:59Z

src/libraries/System.DirectoryServices/src/System/DirectoryServices/DirectorySearcher.cs

        {
        }

        /// <devdoc>
        /// Initializes a new instance of the <see cref='System.DirectoryServices.DirectorySearcher'/> class with the <see cref='System.DirectoryServices.DirectorySearcher.SearchRoot'/>, <see cref='System.DirectoryServices.DirectorySearcher.Filter'/>, <see cref='System.DirectoryServices.DirectorySearcher.PropertiesToLoad'/>, and <see cref='System.DirectoryServices.DirectorySearcher.SearchScope'/> properties set to the given
        /// values.
        /// </devdoc>
-        public DirectorySearcher(DirectoryEntry searchRoot, string filter, string[] propertiesToLoad, SearchScope scope)
+        public DirectorySearcher(DirectoryEntry? searchRoot, string? filter, string[]? propertiesToLoad, SearchScope scope)


Filter seems not expected to be nullable, it defaults with nonnull when not provided, the setter is allowing null but also defaulting if the value was null, it might be an error if it is set to null by the constructor

I'll add this info to the issue above once I create it but I'd say it should handle the null given this never threw anything and there is an obvious value which should be used in place of null

krwq · 2021-02-24T15:34:55Z

I've rebased since there was recently some change in DS

stephentoub

I only spot checked again, but what I saw LGTM.

krwq · 2021-03-04T09:17:25Z

(adjusted nullability of stuff deriving from CollectionBase based on offline conversation)

krwq · 2021-03-04T12:38:13Z

Given it's already signed off, CI is passing, all issues are resolved/turned into issues and only changes made after the sign-off are what @buyaa-n recommended in the comment and @stephentoub has also confirmed in the offline conversation as a right path I'm merging this.

krwq requested review from stephentoub, joperezr and buyaa-n February 18, 2021 12:52

dotnet-issue-labeler bot added area-System.DirectoryServices new-api-needs-documentation labels Feb 18, 2021