Fixing ArgumentNullException on empty select/expand (#621)

Co-authored-by: Giuliano Barberi <gbarberi@microsoft.com>
OData · Jul 14, 2022 · f675f6c · f675f6c
1 parent ceac943
commit f675f6c
Show file tree

Hide file tree

Showing 9 changed files with 68 additions and 33 deletions.
diff --git a/src/Microsoft.AspNetCore.OData/Microsoft.AspNetCore.OData.csproj b/src/Microsoft.AspNetCore.OData/Microsoft.AspNetCore.OData.csproj
@@ -2,7 +2,7 @@
 
   <PropertyGroup>
     <TargetFrameworks>netcoreapp3.1;net5.0</TargetFrameworks>
-    <TargetFrameworks Condition="'$(VisualStudioVersion)' == '' or '$(VisualStudioVersion)' >= '17.0'">$(TargetFrameworks);net6.0</TargetFrameworks>
+    <TargetFrameworks Condition="'$(VisualStudioVersion)' == '' or '$(VisualStudioVersion)' &gt;= '17.0'">$(TargetFrameworks);net6.0</TargetFrameworks>
     <RootNamespace>Microsoft.AspNetCore.OData</RootNamespace>
     <DocumentationFile>$(OutputPath)$(AssemblyName).xml</DocumentationFile>
     <NoDefaultLaunchSettingsFile>true</NoDefaultLaunchSettingsFile>

diff --git a/src/Microsoft.AspNetCore.OData/Microsoft.AspNetCore.OData.xml b/src/Microsoft.AspNetCore.OData/Microsoft.AspNetCore.OData.xml
@@ -7234,6 +7234,11 @@
               Looks up a localized string similar to &apos;select&apos; and &apos;expand&apos; cannot be both null or empty..
             </summary>
         </member>
+        <member name="P:Microsoft.AspNetCore.OData.SRResources.SelectExpandEmptyOrWhitespace">
+            <summary>
+              Looks up a localized string similar to &apos;select&apos; and &apos;expand&apos; cannot be empty or whitespace. Omit the parameter from the query if it is not used..
+            </summary>
+        </member>
         <member name="P:Microsoft.AspNetCore.OData.SRResources.SelectionTypeNotSupported">
             <summary>
               Looks up a localized string similar to $select does not support selections of type &apos;{0}&apos;..

diff --git a/src/Microsoft.AspNetCore.OData/Properties/SRResources.Designer.cs b/src/Microsoft.AspNetCore.OData/Properties/SRResources.Designer.cs
diff --git a/src/Microsoft.AspNetCore.OData/Properties/SRResources.resx b/src/Microsoft.AspNetCore.OData/Properties/SRResources.resx
@@ -729,4 +729,7 @@
   <data name="MultipleActionImportFound" xml:space="preserve">
     <value>Found multiple action imports with same '{0}' name within an entity container..</value>
   </data>
+  <data name="SelectExpandEmptyOrWhitespace" xml:space="preserve">
+    <value>'select' and 'expand' cannot be empty or whitespace. Omit the parameter from the query if it is not used.</value>
+  </data>
 </root>
diff --git a/src/Microsoft.AspNetCore.OData/Query/ODataQueryOptions.cs b/src/Microsoft.AspNetCore.OData/Query/ODataQueryOptions.cs
@@ -1017,7 +1017,7 @@ private void BuildQueryOptions(IDictionary<string, string> queryParameters)
                 }
             }
 
-            if (RawValues.Select != null || RawValues.Expand != null)
+            if (!string.IsNullOrWhiteSpace(RawValues.Select) || !string.IsNullOrWhiteSpace(RawValues.Expand))
             {
                 SelectExpand = new SelectExpandQueryOption(RawValues.Select, RawValues.Expand,
                     Context, _queryOptionParser);

diff --git a/src/Microsoft.AspNetCore.OData/Query/Query/SelectExpandQueryOption.cs b/src/Microsoft.AspNetCore.OData/Query/Query/SelectExpandQueryOption.cs
@@ -48,7 +48,7 @@ public SelectExpandQueryOption(string select, string expand, ODataQueryContext c
                 throw Error.ArgumentNull(nameof(context));
             }
 
-            if (string.IsNullOrEmpty(select) && string.IsNullOrEmpty(expand))
+            if (string.IsNullOrWhiteSpace(select) && string.IsNullOrWhiteSpace(expand))
             {
                 throw Error.Argument(SRResources.SelectExpandEmptyOrNull);
             }
@@ -60,7 +60,7 @@ public SelectExpandQueryOption(string select, string expand, ODataQueryContext c
 
             if (!(context.ElementType is IEdmStructuredType))
             {
-                throw Error.Argument(SRResources.SelectNonStructured, context.ElementType);
+                throw Error.Argument("context", SRResources.SelectNonStructured, context.ElementType.ToTraceString());
             }
 
             Context = context;
@@ -82,33 +82,19 @@ internal SelectExpandQueryOption(
 
         // This constructor is intended for unit testing only.
         internal SelectExpandQueryOption(string select, string expand, ODataQueryContext context)
+            : this(select, expand, context, queryOptionParser: context != null
+                    ? new ODataQueryOptionParser(
+                        context.Model,
+                        context.ElementType,
+                        context.NavigationSource,
+                        new Dictionary<string, string>
+                        {
+                            { "$select", select },
+                            { "$expand", expand }
+                        },
+                        context.RequestContainer)
+                    : null)
         {
-            if (context == null)
-            {
-                throw Error.ArgumentNull(nameof(context));
-            }
-
-            if (string.IsNullOrEmpty(select) && string.IsNullOrEmpty(expand))
-            {
-                throw Error.Argument(SRResources.SelectExpandEmptyOrNull);
-            }
-
-            if (!(context.ElementType is IEdmStructuredType))
-            {
-                throw Error.Argument("context", SRResources.SelectNonStructured, context.ElementType.ToTraceString());
-            }
-
-            Context = context;
-            RawSelect = select;
-            RawExpand = expand;
-            Validator = SelectExpandQueryValidator.GetSelectExpandQueryValidator(context);
-
-            _queryOptionParser = new ODataQueryOptionParser(
-                context.Model,
-                context.ElementType,
-                context.NavigationSource,
-                new Dictionary<string, string> { { "$select", select }, { "$expand", expand } },
-                context.RequestContainer);
         }
 
         /// <summary>

diff --git a/src/Microsoft.AspNetCore.OData/Query/Validator/ODataQueryValidator.cs b/src/Microsoft.AspNetCore.OData/Query/Validator/ODataQueryValidator.cs
@@ -97,11 +97,13 @@ public virtual void Validate(ODataQueryOptions options, ODataValidationSettings
 
             if (options.RawValues.Expand != null)
             {
+                ValidateNotEmptyOrWhitespace(options.RawValues.Expand);
                 ValidateQueryOptionAllowed(AllowedQueryOptions.Expand, validationSettings.AllowedQueryOptions);
             }
 
             if (options.RawValues.Select != null)
             {
+                ValidateNotEmptyOrWhitespace(options.RawValues.Select);
                 ValidateQueryOptionAllowed(AllowedQueryOptions.Select, validationSettings.AllowedQueryOptions);
             }
 
@@ -139,5 +141,13 @@ private static void ValidateQueryOptionAllowed(AllowedQueryOptions queryOption,
                 throw new ODataException(Error.Format(SRResources.NotAllowedQueryOption, queryOption, "AllowedQueryOptions"));
             }
         }
+
+        private static void ValidateNotEmptyOrWhitespace(string rawValue)
+        {
+            if (rawValue != null && string.IsNullOrWhiteSpace(rawValue))
+            {
+                throw new ODataException(SRResources.SelectExpandEmptyOrWhitespace);
+            }
+        }
     }
 }
diff --git a/test/Microsoft.AspNetCore.OData.Tests/Query/Query/SelectExpandQueryOptionTest.cs b/test/Microsoft.AspNetCore.OData.Tests/Query/Query/SelectExpandQueryOptionTest.cs
@@ -37,16 +37,19 @@ public void Ctor_ThrowsArgumentNull_Context()
                 "context");
         }
 
-        [Fact]
-        public void Ctor_ThrowsArgument_IfBothSelectAndExpandAreNull()
+        [Theory]
+        [InlineData(null, null)]
+        [InlineData("", "")]
+        [InlineData("  ", "  ")]
+        public void Ctor_ThrowsArgument_IfBothSelectAndExpandAreNullOrWhitespace(string select, string expand)
         {
             // Arrange
             _model.Model.SetAnnotationValue<ClrTypeAnnotation>(_model.Customer, new ClrTypeAnnotation(typeof(Customer)));
             ODataQueryContext context = new ODataQueryContext(_model.Model, typeof(Customer));
 
             // Act & Assert
             ExceptionAssert.Throws<ArgumentException>(
-                () => new SelectExpandQueryOption(select: null, expand: null, context: context),
+                () => new SelectExpandQueryOption(select, expand, context: context),
                 "'select' and 'expand' cannot be both null or empty.");
         }
 

diff --git a/test/Microsoft.AspNetCore.OData.Tests/Query/Validator/ODataQueryValidatorTest.cs b/test/Microsoft.AspNetCore.OData.Tests/Query/Validator/ODataQueryValidatorTest.cs
@@ -269,5 +269,24 @@ public void Validate_ValidatesSelectExpandQueryOption_IfItIsNotNull()
             // Assert
             selectExpandValidator.Verify(v => v.Validate(option.SelectExpand, settings), Times.Once());
         }
+
+        [Theory]
+        [InlineData("$select=")]
+        [InlineData("$select=  ")]
+        [InlineData("$expand=")]
+        [InlineData("$expand=  ")]
+        [InlineData("$select=   &$expand=  &")]
+        public void Validate_ValidatesNotEmptyOrWhitespaceSelectExpandQueryOption_IfEmptyOrWhitespace(string query)
+        {
+            var expectedMessage = "'select' and 'expand' cannot be empty or whitespace. Omit the parameter from the query if it is not used.";
+
+            // Arrange
+            var message = RequestFactory.Create("Get", $"http://localhost/?{query}", setupAction: null);
+            ODataQueryOptions option = new ODataQueryOptions(_context, message);
+            ODataValidationSettings settings = new ODataValidationSettings();
+
+            // Act & Assert
+            ExceptionAssert.Throws<ODataException>(() => _validator.Validate(option, settings), expectedMessage);
+        }
     }
 }