Throw on query filters with unsupported method overloads (#47184)

christothes · web-flow · commit 9853adde6b84 · 2024-11-15T16:08:50.000-06:00
diff --git a/sdk/tables/Azure.Data.Tables/CHANGELOG.md b/sdk/tables/Azure.Data.Tables/CHANGELOG.md
@@ -6,6 +6,8 @@
 
 ### Breaking Changes
 
+- Calling `TableClient.Query`, `TableClient.QueryAsync`, or `TableClient.CreateQueryFilter` with a filter expression that uses `string.Equals` or `string.Compare` with a `StringComparison` parameter will now throw an exception. This is because the Azure Table service does not support these methods in query filters. Previously the `StringComparison` argument was silently ignored, which can lead to subtle bugs in client code.
+
 ### Bugs Fixed
 
 ### Other Changes
diff --git a/sdk/tables/Azure.Data.Tables/src/Queryable/ExpressionNormalizer.cs b/sdk/tables/Azure.Data.Tables/src/Queryable/ExpressionNormalizer.cs
@@ -162,11 +162,19 @@ internal Expression VisitMethodCallNoRewrite(MethodCallExpression call)
 
             if (visited.Method.IsStatic && visited.Method.Name == "Equals" && visited.Arguments.Count > 1)
             {
+                if (visited.Arguments.Count > 2)
+                {
+                    throw new NotSupportedException("string.Equals method with more than two arguments is not supported.");
+                }
                 return Expression.Equal(visited.Arguments[0], visited.Arguments[1], false, visited.Method);
             }
 
             if (!visited.Method.IsStatic && visited.Method.Name == "Equals" && visited.Arguments.Count > 0)
             {
+                if (visited.Arguments.Count > 1)
+                {
+                    throw new NotSupportedException("Equals method with more than two arguments is not supported.");
+                }
                 return CreateRelationalOperator(ExpressionType.Equal, visited.Object, visited.Arguments[0]);
             }
 
@@ -182,6 +190,10 @@ internal Expression VisitMethodCallNoRewrite(MethodCallExpression call)
 
             if (visited.Method.IsStatic && visited.Method.Name == "Compare" && visited.Arguments.Count > 1 && visited.Method.ReturnType == typeof(int))
             {
+                if (visited.Arguments.Count > 2)
+                {
+                    throw new NotSupportedException("string.Compare method with more than two arguments is not supported.");
+                }
                 return CreateCompareExpression(visited.Arguments[0], visited.Arguments[1]);
             }
 
diff --git a/sdk/tables/Azure.Data.Tables/src/TableClient.cs b/sdk/tables/Azure.Data.Tables/src/TableClient.cs
@@ -1041,6 +1041,15 @@ public virtual Response UpdateEntity<T>(T entity, ETag ifMatch, TableUpdateMode
         /// <param name="filter">
         /// Returns only entities that satisfy the specified filter expression.
         /// For example, the following expression would filter entities with a PartitionKey of 'foo': <c>e => e.PartitionKey == "foo"</c>.
+        /// <para>
+        /// The following string comparison methods are supported as part of a filter expression:
+        /// <list type="bullet">
+        /// <item><description><see cref="string.Equals(string)"/></description></item>
+        /// <item><description><see cref="string.Equals(string, string)"/></description></item>
+        /// <item><description><see cref="string.CompareTo(string)"/></description></item>
+        /// <item><description><see cref="string.Compare(string, string)"/></description></item>
+        /// </list>
+        /// </para>
         /// </param>
         /// <param name="maxPerPage">
         /// The maximum number of entities that will be returned per page. If unspecified, the default value is 1000 for storage accounts and is not limited for Cosmos DB table API.
@@ -1079,6 +1088,15 @@ public virtual AsyncPageable<T> QueryAsync<T>(
         /// <param name="filter">
         /// Returns only entities that satisfy the specified filter expression.
         /// For example, the following expression would filter entities with a PartitionKey of 'foo': <c>e => e.PartitionKey == "foo"</c>.
+        /// <para>
+        /// The following string comparison methods are supported as part of a filter expression:
+        /// <list type="bullet">
+        /// <item><description><see cref="string.Equals(string)"/></description></item>
+        /// <item><description><see cref="string.Equals(string, string)"/></description></item>
+        /// <item><description><see cref="string.CompareTo(string)"/></description></item>
+        /// <item><description><see cref="string.Compare(string, string)"/></description></item>
+        /// </list>
+        /// </para>
         /// </param>
         /// <param name="maxPerPage">
         /// The maximum number of entities that will be returned per page. If unspecified, the default value is 1000 for storage accounts and is not limited for Cosmos DB table API.
@@ -1494,7 +1512,18 @@ public virtual Response SetAccessPolicy(IEnumerable<TableSignedIdentifier> table
         /// </summary>
         /// <typeparam name="T">The type of the entity being queried. Typically this will be derived from <see cref="ITableEntity"/>
         /// or <see cref="Dictionary{String, Object}"/>.</typeparam>
-        /// <param name="filter">A filter expression.</param>
+        /// <param name="filter">A filter expression.
+        /// for example: <c>e => e.PartitionKey == "foo"</c>.
+        /// <para>
+        /// The following string comparison methods are supported as part of a filter expression:
+        /// <list type="bullet">
+        /// <item><description><see cref="string.Equals(string)"/></description></item>
+        /// <item><description><see cref="string.Equals(string, string)"/></description></item>
+        /// <item><description><see cref="string.CompareTo(string)"/></description></item>
+        /// <item><description><see cref="string.Compare(string, string)"/></description></item>
+        /// </list>
+        /// </para>
+        /// </param>
         /// <returns>The string representation of the filter expression.</returns>
         public static string CreateQueryFilter<T>(Expression<Func<T, bool>> filter) => Bind(filter);
 
diff --git a/sdk/tables/Azure.Data.Tables/tests/TableClientQueryExpressionTests.cs b/sdk/tables/Azure.Data.Tables/tests/TableClientQueryExpressionTests.cs
@@ -23,6 +23,7 @@ public class TableClientQueryExpressionTests
         private const string TableName = "someTableName";
         private const string TableName2 = "otherTableName";
         private const string Partition = "partition";
+        private const string MixedCasePK = "PartitionKey";
         private const string Row = "row";
         private const int SomeInt = 10;
         private const double SomeDouble = 10.10;
@@ -94,6 +95,13 @@ public class TableClientQueryExpressionTests
         private static readonly Expression<Func<TableEntity, bool>> s_tableEntExpImplicitBooleanCmpCasted = ent => (bool)ent.GetBoolean("Bool");
         private static readonly Expression<Func<TableEntity, bool>> s_tableEntExpImplicitBooleanCmpOr = ent => ent.GetBoolean("Bool").Value || (bool)ent.GetBoolean("Bool");
         private static readonly Expression<Func<TableEntity, bool>> s_tableEntExpImplicitBooleanCmpNot = ent => !ent.GetBoolean("Bool").Value;
+        private static readonly Expression<Func<TableEntity, bool>> s_tableEntExpEquals = ent => ent.PartitionKey.Equals(Partition);
+        private static readonly Expression<Func<ComplexEntity, bool>> s_CEtableEntExpEquals = ent => ent.String.Equals(Partition);
+        private static readonly Expression<Func<ComplexEntity, bool>> s_CEtableEntExpStaticEquals = ent => string.Equals(ent.String, Partition);
+        private static readonly Expression<Func<TableEntity, bool>> s_TEequalsUnsupported = ent => ent.PartitionKey.Equals(Partition, StringComparison.OrdinalIgnoreCase);
+        private static readonly Expression<Func<TableEntity, bool>> s_TEequalsToLowerUnsupported = ent => ent.PartitionKey.Equals(Partition.ToLower(), StringComparison.OrdinalIgnoreCase);
+        private static readonly Expression<Func<TableEntity, bool>> s_TEequalsStaticUnsupported = ent => string.Equals(ent.PartitionKey, Partition, StringComparison.OrdinalIgnoreCase);
+        private static readonly Expression<Func<TableEntity, bool>> s_TECompareStaticUnsupported = ent => string.Compare(ent.PartitionKey, Partition, StringComparison.OrdinalIgnoreCase) > 0;
 
         public static object[] TableEntityExpressionTestCases =
         {
@@ -125,7 +133,9 @@ public class TableClientQueryExpressionTests
             new object[] { $"not (Bool eq true)", s_complexExpImplicitBooleanCmpNot },
             new object[] { $"BoolN eq true", s_complexExpImplicitCastedNullableBooleanCmp },
             new object[] { $"(Bool eq true) and (BoolN eq true)", s_complexExpImplicitBooleanCmpAnd },
-            new object[] { $"(Bool eq true) or (BoolN eq true)", s_complexExpImplicitCastedBooleanCmpOr }
+            new object[] { $"(Bool eq true) or (BoolN eq true)", s_complexExpImplicitCastedBooleanCmpOr  },
+            new object[] { $"String eq '{Partition}'", s_CEtableEntExpEquals },
+            new object[] { $"String eq '{Partition}'", s_CEtableEntExpStaticEquals },
         };
 
         public static object[] TableItemExpressionTestCases =
@@ -162,7 +172,16 @@ public class TableClientQueryExpressionTests
             new object[] { $"Bool eq true", s_tableEntExpImplicitBooleanCmp },
             new object[] { $"Bool eq true", s_tableEntExpImplicitBooleanCmpCasted },
             new object[] { $"(Bool eq true) or (Bool eq true)", s_tableEntExpImplicitBooleanCmpOr },
-            new object[] { $"not (Bool eq true)", s_tableEntExpImplicitBooleanCmpNot }
+            new object[] { $"not (Bool eq true)", s_tableEntExpImplicitBooleanCmpNot },
+            new object[] { $"PartitionKey eq '{Partition}'", s_tableEntExpEquals },
+        };
+
+        public static object[] UnSupportedTableItemExpressionTestCases =
+        {
+            new object[] { s_TEequalsUnsupported },
+            new object[] { s_TEequalsStaticUnsupported },
+            new object[] { s_TEequalsToLowerUnsupported },
+            new object[] { s_TECompareStaticUnsupported },
         };
 
         [TestCaseSource(nameof(TableItemExpressionTestCases))]
@@ -191,5 +210,12 @@ public void TestDictionaryTableEntityFilterExpressions(string expectedFilter, Ex
 
             Assert.That(filter, Is.EqualTo(expectedFilter));
         }
+
+        [TestCaseSource(nameof(UnSupportedTableItemExpressionTestCases))]
+        [Test]
+        public void TestTableItemFilterExpressionsUnsupported(Expression<Func<TableEntity, bool>> expression)
+        {
+            Assert.Throws<NotSupportedException>(() => TableClient.CreateQueryFilter(expression));
+        }
     }
 }
diff --git a/sdk/tables/Azure.Data.Tables/tests/samples/Sample0_Auth.cs b/sdk/tables/Azure.Data.Tables/tests/samples/Sample0_Auth.cs
@@ -108,10 +108,7 @@ public async Task TokenCredentialAuth()
 #if SNIPPET
                 new DefaultAzureCredential());
 #else
-                new ClientSecretCredential(
-                    GetVariable("TENANT_ID"),
-                    GetVariable("CLIENT_ID"),
-                    GetVariable("CLIENT_SECRET")));
+                Credential);
 #endif
 
             // Create the table if it doesn't already exist to verify we've successfully authenticated.
diff --git a/sdk/tables/Azure.Data.Tables/tests/samples/Sample8_CustomizingSerialization.cs b/sdk/tables/Azure.Data.Tables/tests/samples/Sample8_CustomizingSerialization.cs
@@ -28,10 +28,7 @@ public async Task CustomizeSerialization()
 #if SNIPPET
                 new DefaultAzureCredential());
 #else
-                new ClientSecretCredential(
-                    GetVariable("TENANT_ID"),
-                    GetVariable("CLIENT_ID"),
-                    GetVariable("CLIENT_SECRET")));
+                Credential);
 #endif
 
             // Create the table if it doesn't already exist.
diff --git a/sdk/tables/test-resources.json b/sdk/tables/test-resources.json
@@ -1,5 +1,5 @@
 {
-    "$schema": "https://schema.management.azure.com/schemas/2015-01-01/deploymentTemplate.json#",
+    "$schema": "https://schema.management.azure.com/schemas/2019-04-01/deploymentTemplate.json#",
     "contentVersion": "1.0.0.0",
     "parameters": {
         "baseName": {
@@ -12,18 +12,18 @@
             }
         },
         "cosmosEndpointSuffix": {
-          "type": "string",
-          "defaultValue": "cosmos.azure.com",
-          "metadata": {
-              "description": "The url suffix to use when accessing the cosmos data plane."
-          }
+            "type": "string",
+            "defaultValue": "cosmos.azure.com",
+            "metadata": {
+                "description": "The url suffix to use when accessing the cosmos data plane."
+            }
         },
         "storageEndpointSuffix": {
-          "type": "string",
-          "defaultValue": "core.windows.net",
-          "metadata": {
-              "description": "The url suffix to use when accessing the storage data plane."
-          }
+            "type": "string",
+            "defaultValue": "core.windows.net",
+            "metadata": {
+                "description": "The url suffix to use when accessing the storage data plane."
+            }
         }
     },
     "variables": {
@@ -48,7 +48,16 @@
             "virtualNetworkRules": [],
             "ipRules": [],
             "defaultAction": "Allow"
-        }
+        },
+        "customCosmosRoleName": "Azure Cosmos DB SDK role for Table Data Plane",
+        "customCosmosRoleDescription": "Azure Cosmos DB SDK role for Table Data Plane",
+        "customCosmosRoleActions": [
+            "Microsoft.DocumentDB/databaseAccounts/readMetadata",
+            "Microsoft.DocumentDB/databaseAccounts/tables/*",
+            "Microsoft.DocumentDB/databaseAccounts/tables/containers/*",
+            "Microsoft.DocumentDB/databaseAccounts/tables/containers/entities/*",
+            "Microsoft.DocumentDB/databaseAccounts/throughputSettings/read"
+        ]
     },
     "resources": [
         {
@@ -118,6 +127,39 @@
                 ],
                 "ipRules": []
             }
+        },
+        {
+          "dependsOn": [
+            "[resourceId('Microsoft.DocumentDB/databaseAccounts', variables('primaryAccountName'))]"
+          ],
+          "type": "Microsoft.DocumentDB/databaseAccounts/tableRoleDefinitions",
+          "apiVersion": "2024-05-15",
+          "name": "[concat(variables('primaryAccountName'), '/', guid(variables('customCosmosRoleName')))]",
+          "properties": {
+            "roleName": "[variables('customCosmosRoleName')]",
+            "description": "[variables('customCosmosRoleDescription')]",
+            "permissions": [
+              {
+                "dataActions": "[variables('customCosmosRoleActions')]"
+              }
+            ],
+            "assignableScopes": [
+              "[concat('/subscriptions/', subscription().subscriptionId, '/resourceGroups/', resourceGroup().name, '/providers/Microsoft.DocumentDB/databaseAccounts/', variables('primaryAccountName'))]"
+            ]
+          }
+        },
+        {
+          "dependsOn": [
+            "[resourceId('Microsoft.DocumentDB/databaseAccounts/tableRoleDefinitions', variables('primaryAccountName'), guid(variables('customCosmosRoleName')))]"
+          ],
+          "type": "Microsoft.DocumentDB/databaseAccounts/tableRoleAssignments",
+          "apiVersion": "2024-05-15",
+          "name": "[concat(variables('primaryAccountName'), '/', guid(variables('customCosmosRoleName')))]",
+          "properties": {
+            "scope": "[concat('/subscriptions/', subscription().subscriptionId, '/resourceGroups/', resourceGroup().name, '/providers/Microsoft.DocumentDB/databaseAccounts/', variables('primaryAccountName'))]",
+            "roleDefinitionId": "[resourceId('Microsoft.DocumentDB/databaseAccounts/tableRoleDefinitions', variables('primaryAccountName'), guid(variables('customCosmosRoleName')))]",
+            "principalId": "[parameters('testApplicationOid')]"
+          }
         }
     ],
     "outputs": {

Original file line number	Diff line number	Diff line change
`@@ -162,11 +162,19 @@ internal Expression VisitMethodCallNoRewrite(MethodCallExpression call)`
`162`	`162`
`163`	`163`	`if (visited.Method.IsStatic && visited.Method.Name == "Equals" && visited.Arguments.Count > 1)`
`164`	`164`	`{`
	`165`	`+ if (visited.Arguments.Count > 2)`
	`166`	`+ {`
	`167`	`+ throw new NotSupportedException("string.Equals method with more than two arguments is not supported.");`
	`168`	`+ }`
`165`	`169`	`return Expression.Equal(visited.Arguments[0], visited.Arguments[1], false, visited.Method);`
`166`	`170`	`}`
`167`	`171`
`168`	`172`	`if (!visited.Method.IsStatic && visited.Method.Name == "Equals" && visited.Arguments.Count > 0)`
`169`	`173`	`{`
	`174`	`+ if (visited.Arguments.Count > 1)`
	`175`	`+ {`
	`176`	`+ throw new NotSupportedException("Equals method with more than two arguments is not supported.");`
	`177`	`+ }`
`170`	`178`	`return CreateRelationalOperator(ExpressionType.Equal, visited.Object, visited.Arguments[0]);`
`171`	`179`	`}`
`172`	`180`
`@@ -182,6 +190,10 @@ internal Expression VisitMethodCallNoRewrite(MethodCallExpression call)`
`182`	`190`
`183`	`191`	`if (visited.Method.IsStatic && visited.Method.Name == "Compare" && visited.Arguments.Count > 1 && visited.Method.ReturnType == typeof(int))`
`184`	`192`	`{`
	`193`	`+ if (visited.Arguments.Count > 2)`
	`194`	`+ {`
	`195`	`+ throw new NotSupportedException("string.Compare method with more than two arguments is not supported.");`
	`196`	`+ }`
`185`	`197`	`return CreateCompareExpression(visited.Arguments[0], visited.Arguments[1]);`
`186`	`198`	`}`
`187`	`199`