diff --git a/src/Core/Services/MetadataProviders/SqlMetadataProvider.cs b/src/Core/Services/MetadataProviders/SqlMetadataProvider.cs index 8def098a9c..900812c9bb 100644 --- a/src/Core/Services/MetadataProviders/SqlMetadataProvider.cs +++ b/src/Core/Services/MetadataProviders/SqlMetadataProvider.cs @@ -1241,7 +1241,14 @@ private async Task GetTableWithSchemaFromDataSetAsync( 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,35 +1368,31 @@ private async Task FillSchemaForTableAsync( 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) + /// + /// 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. + /// + /// Name of schema the table belongs within. + /// Name of the table. + /// Properly formatted table name with schema prefix if it exists. + 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); @@ -1397,7 +1400,8 @@ internal string GetTablePrefix(string databaseName, string schemaName) tablePrefix.Append(schemaName); } - return tablePrefix.ToString(); + string queryPrefix = string.IsNullOrEmpty(tablePrefix.ToString()) ? string.Empty : $"{tablePrefix}."; + return $"{queryPrefix}{SqlQueryBuilder.QuoteIdentifier(tableName)}"; } /// diff --git a/src/Service.Tests/Configuration/ConfigurationTests.cs b/src/Service.Tests/Configuration/ConfigurationTests.cs index fc895e5cca..9356c37202 100644 --- a/src/Service.Tests/Configuration/ConfigurationTests.cs +++ b/src/Service.Tests/Configuration/ConfigurationTests.cs @@ -1155,7 +1155,7 @@ public async Task TestSqlMetadataForInvalidConfigEntities() Assert.AreEqual(2, configValidator.ConfigValidationExceptions.Count); List 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); } diff --git a/src/Service.Tests/DatabaseSchema-DwSql.sql b/src/Service.Tests/DatabaseSchema-DwSql.sql index d5dd1cde76..ca2608ff14 100644 --- a/src/Service.Tests/DatabaseSchema-DwSql.sql +++ b/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); diff --git a/src/Service.Tests/DatabaseSchema-MsSql.sql b/src/Service.Tests/DatabaseSchema-MsSql.sql index f7032931d4..0c8d9c4a5c 100644 --- a/src/Service.Tests/DatabaseSchema-MsSql.sql +++ b/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 diff --git a/src/Service.Tests/DatabaseSchema-PostgreSql.sql b/src/Service.Tests/DatabaseSchema-PostgreSql.sql index 535c40d7d7..7a77cd79f2 100644 --- a/src/Service.Tests/DatabaseSchema-PostgreSql.sql +++ b/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'); diff --git a/src/Service.Tests/SqlTests/RestApiTests/Find/DwSqlFindApiTests.cs b/src/Service.Tests/SqlTests/RestApiTests/Find/DwSqlFindApiTests.cs index e7323a547c..f5d84fc1fe 100644 --- a/src/Service.Tests/SqlTests/RestApiTests/Find/DwSqlFindApiTests.cs +++ b/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 [始計], " + diff --git a/src/Service.Tests/SqlTests/RestApiTests/Find/FindApiTestBase.cs b/src/Service.Tests/SqlTests/RestApiTests/Find/FindApiTestBase.cs index 8c3111d8bc..67df09c205 100644 --- a/src/Service.Tests/SqlTests/RestApiTests/Find/FindApiTestBase.cs +++ b/src/Service.Tests/SqlTests/RestApiTests/Find/FindApiTestBase.cs @@ -127,6 +127,23 @@ await SetupAndRunRestApiTest( sqlQuery: GetQuery("FindOnTableWithUniqueCharacters")); } + /// + /// 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". + /// + [TestMethod] + public virtual async Task FindOnTableWithNamingCollision() + { + await SetupAndRunRestApiTest( + primaryKeyRoute: string.Empty, + queryString: string.Empty, + entityNameOrPath: _collisionEntity, + sqlQuery: GetQuery("FindOnTableWithNamingCollision")); + } + /// /// 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. diff --git a/src/Service.Tests/SqlTests/RestApiTests/Find/MsSqlFindApiTests.cs b/src/Service.Tests/SqlTests/RestApiTests/Find/MsSqlFindApiTests.cs index d3bbf024c0..f06e8e99cc 100644 --- a/src/Service.Tests/SqlTests/RestApiTests/Find/MsSqlFindApiTests.cs +++ b/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 } " + diff --git a/src/Service.Tests/SqlTests/RestApiTests/Find/MySqlFindApiTests.cs b/src/Service.Tests/SqlTests/RestApiTests/Find/MySqlFindApiTests.cs index 9e627a13a1..55b9320833 100644 --- a/src/Service.Tests/SqlTests/RestApiTests/Find/MySqlFindApiTests.cs +++ b/src/Service.Tests/SqlTests/RestApiTests/Find/MySqlFindApiTests.cs @@ -1008,11 +1008,11 @@ public override string GetDefaultSchema() } /// - /// 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. /// - /// + /// public override string GetDefaultSchemaForEdmModel() { return string.Empty; @@ -1023,6 +1023,19 @@ public override string GetQuery(string key) return _queryMap[key]; } + /// + /// 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. + /// + /// + [TestMethod] + [Ignore] + public override Task FindOnTableWithNamingCollision() + { + throw new NotImplementedException(); + } + // Pending Stored Procedure Support [TestMethod] [Ignore] diff --git a/src/Service.Tests/SqlTests/RestApiTests/Find/PostgreSqlFindApiTests.cs b/src/Service.Tests/SqlTests/RestApiTests/Find/PostgreSqlFindApiTests.cs index 0382409f44..776bafbbee 100644 --- a/src/Service.Tests/SqlTests/RestApiTests/Find/PostgreSqlFindApiTests.cs +++ b/src/Service.Tests/SqlTests/RestApiTests/Find/PostgreSqlFindApiTests.cs @@ -170,6 +170,16 @@ SELECT to_jsonb(subq) AS data 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", @" diff --git a/src/Service.Tests/SqlTests/RestApiTests/RestApiTestBase.cs b/src/Service.Tests/SqlTests/RestApiTests/RestApiTestBase.cs index edc65851c7..f0e33209cf 100644 --- a/src/Service.Tests/SqlTests/RestApiTests/RestApiTestBase.cs +++ b/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"; diff --git a/src/Service.Tests/SqlTests/SqlTestBase.cs b/src/Service.Tests/SqlTests/SqlTestBase.cs index 6a73853680..b3fdd6f6d9 100644 --- a/src/Service.Tests/SqlTests/SqlTestBase.cs +++ b/src/Service.Tests/SqlTests/SqlTestBase.cs @@ -96,9 +96,31 @@ protected async static Task InitializeTestFixture( // 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. diff --git a/src/Service.Tests/Unittests/SqlMetadataProviderUnitTests.cs b/src/Service.Tests/Unittests/SqlMetadataProviderUnitTests.cs index f4e0131965..1cd114da7f 100644 --- a/src/Service.Tests/Unittests/SqlMetadataProviderUnitTests.cs +++ b/src/Service.Tests/Unittests/SqlMetadataProviderUnitTests.cs @@ -97,20 +97,24 @@ public async Task CheckExceptionForBadConnectionStringForMsSql(string connection } /// - /// Do: Tests with different combinations of connection string to ensure - /// tableprefix generated is correctly. - /// Check: Making sure table prefix matches expected prefix. + /// Do: Tests with different combinations of schema and table names + /// to validate that the correct full table name with schema as prefix is generated. For example if + /// schemaName = model, and tableName = TrainedModel, then correct would mean + /// [model].[TrainedModel], and any other form would be incorrect. + /// Check: Making sure table name with prefix matches expected name with prefix. /// [DataTestMethod] - [DataRow("", "", "")] - [DataRow("", "model", "[model]")] - [DataRow("TestDB", "", "[TestDB]")] - [DataRow("TestDB", "model", "[TestDB].[model]")] - public void CheckTablePrefix(string databaseName, string schemaName, string expectedPrefix) + [DataRow("", "", "[]")] + [DataRow("model", "TrainedModel", "[model].[TrainedModel]")] + [DataRow("", "TestTable", "[TestTable]")] + [DataRow("model", "TrainedModel", "[model].[TrainedModel]")] + public void CheckTablePrefix(string schemaName, string tableName, string expectedTableNameWithPrefix) { TestHelper.SetupDatabaseEnvironment(TestCategory.MSSQL); RuntimeConfig baseConfigFromDisk = SqlTestHelper.SetupRuntimeConfig(); RuntimeConfigProvider runtimeConfigProvider = TestHelper.GenerateInMemoryRuntimeConfigProvider(baseConfigFromDisk); + RuntimeConfig runtimeConfig = runtimeConfigProvider.GetConfig(); + string dataSourceName = runtimeConfig.GetDefaultDataSourceName(); ILogger sqlMetadataLogger = new Mock>().Object; Mock queryExecutor = new(); @@ -120,9 +124,13 @@ public void CheckTablePrefix(string databaseName, string schemaName, string expe queryManagerFactory.Setup(x => x.GetQueryBuilder(It.IsAny())).Returns(queryBuilder); queryManagerFactory.Setup(x => x.GetQueryExecutor(It.IsAny())).Returns(queryExecutor.Object); - SqlMetadataProvider provider = new MsSqlMetadataProvider(runtimeConfigProvider, queryManagerFactory.Object, sqlMetadataLogger, runtimeConfigProvider.GetConfig().GetDefaultDataSourceName()); - string tableprefix = provider.GetTablePrefix(databaseName, schemaName); - Assert.AreEqual(tableprefix, expectedPrefix); + SqlMetadataProvider provider = new MsSqlMetadataProvider( + runtimeConfigProvider, + queryManagerFactory.Object, + sqlMetadataLogger, + dataSourceName); + string tableNameWithPrefix = provider.GetTableNameWithSchemaPrefix(schemaName, tableName); + Assert.AreEqual(expectedTableNameWithPrefix, tableNameWithPrefix); } /// @@ -177,6 +185,13 @@ private static async Task CheckExceptionForBadConnectionStringHelperAsync(string RuntimeConfigProvider runtimeConfigProvider = TestHelper.GenerateInMemoryRuntimeConfigProvider(runtimeConfig); ILogger sqlMetadataLogger = new Mock>().Object; + // MySQL test will not error out before calling the query builder's format function and + // therefore can not be null + if (string.Equals(databaseType, TestCategory.MYSQL)) + { + _queryBuilder = new MySqlQueryBuilder(); + } + try { string dataSourceName = runtimeConfigProvider.GetConfig().GetDefaultDataSourceName();