[Cherry pick] Table name collision bug fix (#1990) (#2065)

Cherry picks to `release/0.11` from `main`

## Why make this change?

Closes #1908

## What is this change?

We were previously filling the schema information for the entity data
set but only using the table name. We add in the schema name to
deconflict between two tables with the same name in different schemas.
This means that we retrieve information from the entity data set by
using schema and tablename when schema is available, and just the table
name otherwise.

The reason that we do not need to worry about collisions at the schema
and table name level (eg. where we have the same schema and table names
within different databases) is because this entity data set is a part of
the SqlMetadataProvider, and we have a SqlMetadataProvider for each
database (not database type but actual DBs).

## How was this tested?

A regression test with a collision between table names in different
schemas was added. We also update the unit test for SqlMetadatProvider.

Co-authored-by: aaronburtle <93220300+aaronburtle@users.noreply.github.com>

src/Core/Services/MetadataProviders/SqlMetadataProvider.cs

@@ -1241,7 +1241,14 @@ public static bool IsGraphQLReservedName(Entity entity, string databaseColumnNam
             string schemaName,
             string tableName)
         {
-            DataTable? dataTable = EntitiesDataSet.Tables[tableName];
+            // Because we have an instance of SqlMetadataProvider for each individual database
+            // (note: this means each actual database not each database type), we do not
+            // need to worry about collisions beyond that schema, hence no database name is needed.
+            string tableNameWithSchemaPrefix = GetTableNameWithSchemaPrefix(
+                schemaName: schemaName,
+                tableName: tableName);
+
+            DataTable? dataTable = EntitiesDataSet.Tables[tableNameWithSchemaPrefix];
             if (dataTable is null)
             {
                 try
@@ -1361,43 +1368,40 @@ private async Task ValidateDatabaseConnection()
                 Connection = conn
             };
 
-            string tablePrefix = GetTablePrefix(conn.Database, schemaName);
-            string queryPrefix = string.IsNullOrEmpty(tablePrefix) ? string.Empty : $"{tablePrefix}.";
+            string tableNameWithSchemaPrefix = GetTableNameWithSchemaPrefix(schemaName, tableName);
             selectCommand.CommandText
-                = $"SELECT * FROM {queryPrefix}{SqlQueryBuilder.QuoteIdentifier(tableName)}";
+                = $"SELECT * FROM {tableNameWithSchemaPrefix}";
             adapterForTable.SelectCommand = selectCommand;
 
-            DataTable[] dataTable = adapterForTable.FillSchema(EntitiesDataSet, SchemaType.Source, tableName);
+            DataTable[] dataTable = adapterForTable.FillSchema(EntitiesDataSet, SchemaType.Source, tableNameWithSchemaPrefix);
             return dataTable[0];
         }
 
-        internal string GetTablePrefix(string databaseName, string schemaName)
+        /// <summary>
+        /// Gets the correctly formatted table name with schema as prefix, if one exists.
+        /// A schema prefix is simply the correctly formatted and prefixed schema name that
+        /// is provided, separated from the table name by a ".". The formatting for both the
+        /// schema and table name is based on database type and may or may not include
+        /// [] quotes depending how the particular database type handles said format.
+        /// </summary>
+        /// <param name="schemaName">Name of schema the table belongs within.</param>
+        /// <param name="tableName">Name of the table.</param>
+        /// <returns>Properly formatted table name with schema prefix if it exists.</returns>
+        internal string GetTableNameWithSchemaPrefix(string schemaName, string tableName)
         {
             IQueryBuilder queryBuilder = GetQueryBuilder();
             StringBuilder tablePrefix = new();
 
-            if (!string.IsNullOrEmpty(databaseName))
-            {
-                // Determine databaseName for prefix.
-                databaseName = queryBuilder.QuoteIdentifier(databaseName);
-                tablePrefix.Append(databaseName);
-
-                if (!string.IsNullOrEmpty(schemaName))
-                {
-                    // Determine schemaName for prefix.
-                    schemaName = queryBuilder.QuoteIdentifier(schemaName);
-                    tablePrefix.Append($".{schemaName}");
-                }
-            }
-            else if (!string.IsNullOrEmpty(schemaName))
+            if (!string.IsNullOrEmpty(schemaName))
             {
                 // Determine schemaName for prefix.
                 schemaName = queryBuilder.QuoteIdentifier(schemaName);
                 // Database name is empty we just need the schema name.
                 tablePrefix.Append(schemaName);
             }
 
-            return tablePrefix.ToString();
+            string queryPrefix = string.IsNullOrEmpty(tablePrefix.ToString()) ? string.Empty : $"{tablePrefix}.";
+            return $"{queryPrefix}{SqlQueryBuilder.QuoteIdentifier(tableName)}";
         }
 
         /// <summary>

src/Service.Tests/Configuration/ConfigurationTests.cs

@@ -1155,7 +1155,7 @@ public async Task TestSqlMetadataForInvalidConfigEntities()
             Assert.AreEqual(2, configValidator.ConfigValidationExceptions.Count);
             List<Exception> exceptionsList = configValidator.ConfigValidationExceptions;
             Assert.AreEqual("Cannot obtain Schema for entity Book with underlying database "
-                + "object source: dbo.bokos due to: Invalid object name 'master.dbo.bokos'.", exceptionsList[0].Message);
+                + "object source: dbo.bokos due to: Invalid object name 'dbo.bokos'.", exceptionsList[0].Message);
             Assert.AreEqual("No stored procedure definition found for the given database object publishers", exceptionsList[1].Message);
         }
 

src/Service.Tests/DatabaseSchema-DwSql.sql

@@ -13,6 +13,7 @@ DROP TABLE IF EXISTS book_website_placements;
 DROP TABLE IF EXISTS website_users;
 DROP TABLE IF EXISTS books;
 DROP TABLE IF EXISTS [foo].[magazines];
+DROP TABLE IF EXISTS [bar].[magazines];
 DROP TABLE IF EXISTS stocks_price;
 DROP TABLE IF EXISTS stocks;
 DROP TABLE IF EXISTS comics;
@@ -41,6 +42,7 @@ DROP PROCEDURE IF EXISTS delete_last_inserted_book;
 DROP PROCEDURE IF EXISTS update_book_title;
 DROP PROCEDURE IF EXISTS insert_and_display_all_books_for_given_publisher;
 DROP SCHEMA IF EXISTS [foo];
+DROP SCHEMA IF EXISTS [bar];
 COMMIT;
 
 CREATE TABLE books(
@@ -85,6 +87,14 @@ CREATE TABLE [foo].[magazines](
     issue_number int NULL
 );
 
+EXEC('CREATE SCHEMA [bar]');
+
+CREATE TABLE [bar].[magazines](
+    upc int NOT NULL,
+    comic_name varchar(2048) NOT NULL,
+    issue int NULL
+);
+
 CREATE TABLE comics(
     id int,
     title varchar(2048) NOT NULL,
@@ -312,6 +322,7 @@ VALUES (1, 'Star Trek', 'SciFi', NULL), (2, 'Cinderella', 'Tales', 3001),(3,'Ún
 (5, 'Snow White', 'AnotherTales', 3001);
 
 INSERT INTO [foo].[magazines](id, title, issue_number) VALUES (1, 'Vogue', 1234), (11, 'Sports Illustrated', NULL), (3, 'Fitness', NULL);
+INSERT INTO [bar].[magazines](upc, comic_name, issue) VALUES (0, 'NotVogue', 0);
 INSERT INTO brokers([ID Number], [First Name], [Last Name]) VALUES (1, 'Michael', 'Burry'), (2, 'Jordan', 'Belfort');
 INSERT INTO publishers(id, name) VALUES (1234, 'Big Company'), (2345, 'Small Town Publisher'), (2323, 'TBD Publishing One'), (2324, 'TBD Publishing Two Ltd'), (1940, 'Policy Publisher 01'), (1941, 'Policy Publisher 02'), (1156, 'The First Publisher');
 INSERT INTO book_author_link(book_id, author_id) VALUES (1, 123), (2, 124), (3, 123), (3, 124), (4, 123), (4, 124), (5, 126);

src/Service.Tests/DatabaseSchema-MsSql.sql

@@ -28,6 +28,7 @@ DROP TABLE IF EXISTS players;
 DROP TABLE IF EXISTS clubs;
 DROP TABLE IF EXISTS publishers;
 DROP TABLE IF EXISTS [foo].[magazines];
+DROP TABLE IF EXISTS [bar].[magazines];
 DROP TABLE IF EXISTS stocks_price;
 DROP TABLE IF EXISTS stocks;
 DROP TABLE IF EXISTS comics;
@@ -52,6 +53,7 @@ DROP TABLE IF EXISTS intern_data;
 DROP TABLE IF EXISTS books_sold;
 DROP TABLE IF EXISTS default_with_function_table;
 DROP SCHEMA IF EXISTS [foo];
+DROP SCHEMA IF EXISTS [bar];
 COMMIT;
 
 --Autogenerated id seed are set at 5001 for consistency with Postgres
@@ -118,6 +120,14 @@ CREATE TABLE [foo].[magazines](
     issue_number int NULL
 );
 
+EXEC('CREATE SCHEMA [bar]');
+
+CREATE TABLE [bar].[magazines](
+    upc int PRIMARY KEY,
+    comic_name varchar(max) NOT NULL,
+    issue int NULL
+);
+
 CREATE TABLE comics(
     id int PRIMARY KEY,
     title varchar(max) NOT NULL,
@@ -472,6 +482,7 @@ INSERT INTO journals(id, journalname, color, ownername) VALUES (1, 'Journal1', '
 
 INSERT INTO website_users(id, username) VALUES (1, 'George'), (2, NULL), (3, ''), (4, 'book_lover_95'), (5, 'null');
 INSERT INTO [foo].[magazines](id, title, issue_number) VALUES (1, 'Vogue', 1234), (11, 'Sports Illustrated', NULL), (3, 'Fitness', NULL);
+INSERT INTO [bar].[magazines](upc, comic_name, issue) VALUES (0, 'NotVogue', 0);
 INSERT INTO brokers([ID Number], [First Name], [Last Name]) VALUES (1, 'Michael', 'Burry'), (2, 'Jordan', 'Belfort');
 
 SET IDENTITY_INSERT series ON

src/Service.Tests/DatabaseSchema-PostgreSql.sql

@@ -16,6 +16,7 @@ DROP TABLE IF EXISTS players;
 DROP TABLE IF EXISTS clubs;
 DROP TABLE IF EXISTS publishers;
 DROP TABLE IF EXISTS foo.magazines;
+DROP TABLE IF EXISTS bar.magazines;
 DROP TABLE IF EXISTS stocks_price;
 DROP TABLE IF EXISTS stocks;
 DROP TABLE IF EXISTS comics;
@@ -38,8 +39,10 @@ DROP TABLE IF EXISTS default_with_function_table;
 DROP FUNCTION IF EXISTS insertCompositeView;
 
 DROP SCHEMA IF EXISTS foo;
+DROP SCHEMA IF EXISTS bar;
 
 CREATE SCHEMA foo;
+CREATE SCHEMA bar;
 
 CREATE TABLE publishers(
     id int GENERATED BY DEFAULT AS IDENTITY PRIMARY KEY,
@@ -100,6 +103,12 @@ CREATE TABLE foo.magazines(
     issue_number int NULL
 );
 
+CREATE TABLE bar.magazines(
+    upc int PRIMARY KEY,
+    comic_name text NOT NULL,
+    issue int NULL
+);
+
 CREATE TABLE comics(
     id int PRIMARY KEY,
     title text NOT NULL,
@@ -341,6 +350,7 @@ INSERT INTO website_users(id, username) VALUES (1, 'George'), (2, NULL), (3, '')
 INSERT INTO book_author_link(book_id, author_id) VALUES (1, 123), (2, 124), (3, 123), (3, 124), (4, 123), (4, 124), (5, 126);;
 INSERT INTO reviews(id, book_id, content) VALUES (567, 1, 'Indeed a great book'), (568, 1, 'I loved it'), (569, 1, 'best book I read in years');
 INSERT INTO foo.magazines(id, title, issue_number) VALUES (1, 'Vogue', 1234), (11, 'Sports Illustrated', NULL), (3, 'Fitness', NULL);
+INSERT INTO bar.magazines(upc, comic_name, issue) VALUES (0, 'NotVogue', 0);
 INSERT INTO series(id, name) VALUES (3001, 'Foundation'), (3002, 'Hyperion Cantos');
 INSERT INTO comics(id, title, "categoryName", series_id)
 VALUES (1, 'Star Trek', 'SciFi', NULL), (2, 'Cinderella', 'Tales', 3001),(3,'Únknown','', 3002), (4, 'Alexander the Great', 'Historical', NULL);INSERT INTO stocks(categoryid, pieceid, "categoryName") VALUES (1, 1, 'SciFi'), (2, 1, 'Tales'),(0,1,''),(100, 99, 'Historical');

src/Service.Tests/SqlTests/RestApiTests/Find/DwSqlFindApiTests.cs

@@ -37,6 +37,11 @@ public class DwSqlFindApiTests : FindApiTestBase
                 $"SELECT * FROM { _integrationTableName } " +
                 $"WHERE 1 != 1 FOR JSON PATH, INCLUDE_NULL_VALUES"
             },
+            {
+                "FindOnTableWithNamingCollision",
+                $"SELECT upc, comic_name, issue FROM { _collisionTable } " +
+                $"WHERE 1 = 1 FOR JSON PATH, INCLUDE_NULL_VALUES"
+            },
             {
                 "FindOnTableWithUniqueCharacters",
                 $"SELECT [NoteNum] AS [┬─┬ノ( º _ ºノ)], [DetailAssessmentAndPlanning] AS [始計], " +

src/Service.Tests/SqlTests/RestApiTests/Find/FindApiTestBase.cs

@@ -127,6 +127,23 @@ public virtual async Task FindOnTableWithUniqueCharacters()
                 sqlQuery: GetQuery("FindOnTableWithUniqueCharacters"));
         }
 
+        /// <summary>
+        /// Tests the Rest Api to validate that queries work
+        /// when there is the same table name in two different
+        /// schemas. In this test we have two tables both
+        /// named magazines but with one in the schema "foo" and
+        /// the other in the schema "bar".
+        /// </summary>
+        [TestMethod]
+        public virtual async Task FindOnTableWithNamingCollision()
+        {
+            await SetupAndRunRestApiTest(
+                primaryKeyRoute: string.Empty,
+                queryString: string.Empty,
+                entityNameOrPath: _collisionEntity,
+                sqlQuery: GetQuery("FindOnTableWithNamingCollision"));
+        }
+
         /// <summary>
         /// Validates that a Find request on a single item with $select with only non-PK fields
         /// returns only the selected fields and does not contain PK fields.

src/Service.Tests/SqlTests/RestApiTests/Find/MsSqlFindApiTests.cs

@@ -50,6 +50,12 @@ public class MsSqlFindApiTests : FindApiTestBase
                 $"[WagingWar] AS [作戰], [StrategicAttack] AS [謀攻] FROM { _integrationUniqueCharactersTable } " +
                 $"FOR JSON PATH, INCLUDE_NULL_VALUES"
             },
+            {
+                "FindOnTableWithNamingCollision",
+                $"SELECT [upc], [comic_name], [issue] " +
+                $"FROM { _collisionTable } AS { _collisionEntity } " +
+                $"WHERE 1 = 1 ORDER BY [upc] ASC FOR JSON PATH"
+            },
             {
                 "FindViewAll",
                 $"SELECT * FROM { _simple_all_books } " +

src/Service.Tests/SqlTests/RestApiTests/Find/MySqlFindApiTests.cs

@@ -1008,11 +1008,11 @@ public override string GetDefaultSchema()
         }
 
         /// <summary>
-        /// MySql does not a schema so it lacks
+        /// MySql does not have a schema so it lacks
         /// the '.' between schema and table, we
         /// return empty string here for this reason.
         /// </summary>
-        /// <returns></returns>
+        /// <exception cref="NotImplementedException"></exception>
         public override string GetDefaultSchemaForEdmModel()
         {
             return string.Empty;
@@ -1023,6 +1023,19 @@ public override string GetQuery(string key)
             return _queryMap[key];
         }
 
+        /// <summary>
+        /// MySql does not have a schema and so this test
+        /// which validates we de-conflict the same table
+        /// name in different schemas is not valid.
+        /// </summary>
+        /// <exception cref="NotImplementedException"></exception>
+        [TestMethod]
+        [Ignore]
+        public override Task FindOnTableWithNamingCollision()
+        {
+            throw new NotImplementedException();
+        }
+
         // Pending Stored Procedure Support
         [TestMethod]
         [Ignore]

src/Service.Tests/SqlTests/RestApiTests/Find/PostgreSqlFindApiTests.cs

@@ -170,6 +170,16 @@ LIMIT 1
                         WHERE 1 <> 1
                     ) AS subq"
             },
+            {
+                "FindOnTableWithNamingCollision",
+                @"
+                    SELECT to_jsonb(subq) AS data
+                    FROM (
+                        SELECT upc, comic_name, issue
+                        FROM " + _collisionTable + @"
+                        WHERE 1 = 1
+                    ) AS subq"
+            },
             {
                 "FindTestWithQueryStringOneField",
                 @"

src/Service.Tests/SqlTests/RestApiTests/RestApiTestBase.cs

@@ -24,6 +24,8 @@ public abstract class RestApiTestBase : SqlTestBase
         protected const int STARTING_ID_FOR_TEST_INSERTS = 5001;
         protected static readonly string _integration_NonAutoGenPK_EntityName = "magazine";
         protected static readonly string _integration_NonAutoGenPK_TableName = "magazines";
+        protected static readonly string _collisionEntity = "bar_magazine";
+        protected static readonly string _collisionTable = "bar.magazines";
         protected static readonly string _integration_AutoGenNonPK_EntityName = "Comic";
         protected static readonly string _integration_AutoGenNonPK_TableName = "comics";
         protected static readonly string _Composite_NonAutoGenPK_TableName = "stocks";

src/Service.Tests/SqlTests/SqlTestBase.cs

@@ -96,9 +96,31 @@ public abstract class SqlTestBase
             // Add magazines entity to the config
             runtimeConfig = DatabaseEngine switch
             {
-                TestCategory.MYSQL => TestHelper.AddMissingEntitiesToConfig(runtimeConfig, "magazine", "magazines"),
-                TestCategory.DWSQL => TestHelper.AddMissingEntitiesToConfig(runtimeConfig, "magazine", "foo.magazines", new string[] { "id" }),
-                _ => TestHelper.AddMissingEntitiesToConfig(runtimeConfig, "magazine", "foo.magazines"),
+                TestCategory.MYSQL => TestHelper.AddMissingEntitiesToConfig(
+                    config: runtimeConfig,
+                    entityKey: "magazine",
+                    entityName: "magazines"),
+                TestCategory.DWSQL => TestHelper.AddMissingEntitiesToConfig(
+                    config: runtimeConfig,
+                    entityKey: "magazine",
+                    entityName: "foo.magazines",
+                    keyfields: new string[] { "id" }),
+                _ => TestHelper.AddMissingEntitiesToConfig(
+                    config: runtimeConfig,
+                    entityKey: "magazine",
+                    entityName: "foo.magazines"),
+            };
+
+            // Add table name collision testing entity to the config
+            runtimeConfig = DatabaseEngine switch
+            {
+                // MySql does not handle schema the same as other DB, so this testing entity is not needed
+                TestCategory.MYSQL => runtimeConfig,
+                _ => TestHelper.AddMissingEntitiesToConfig(
+                    config: runtimeConfig,
+                    entityKey: "bar_magazine",
+                    entityName: "bar.magazines",
+                    keyfields: new string[] { "upc" })
             };
 
             // Add custom entities for the test, if any.