-
Notifications
You must be signed in to change notification settings - Fork 345
Sanitize autoentity-generated names to prevent invalid REST paths and GraphQL singular/plural tables for SQL objects with spaces #3609
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
ab89cb7
4aff5f4
4dbae6e
f322f1f
9d24e8f
652215c
4701eab
da92c03
29f0e79
615e982
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -733,6 +733,33 @@ private void RemoveGeneratedAutoentities() | |
| _runtimeConfigProvider.RemoveGeneratedAutoentitiesFromConfig(); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Removes whitespace from the generated entity name and capitalizes the character | ||
| /// immediately following each removed whitespace (camelCase join). | ||
| /// For example, "Order Items" becomes "OrderItems" and "dbo_Order Items" becomes "dbo_OrderItems". | ||
| /// </summary> | ||
| /// <param name="name">The entity name to process.</param> | ||
| /// <returns>The entity name with whitespace removed and following characters capitalized.</returns> | ||
| protected static string RemoveWhitespaceAndCamelCase(string name) | ||
| { | ||
| StringBuilder result = new(name.Length); | ||
| bool capitalizeNext = false; | ||
|
|
||
| foreach (char character in name) | ||
| { | ||
| if (char.IsWhiteSpace(character)) | ||
|
Comment on lines
+736
to
+750
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we sanitize other valid characters like
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use a more specific name (e.g.,
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Renamed to
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done — |
||
| { | ||
| capitalizeNext = true; | ||
| continue; | ||
| } | ||
|
|
||
| result.Append(capitalizeNext ? char.ToUpperInvariant(character) : character); | ||
| capitalizeNext = false; | ||
| } | ||
|
|
||
| return result.ToString(); | ||
| } | ||
|
|
||
| protected void PopulateDatabaseObjectForEntity( | ||
| Entity entity, | ||
| string entityName, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5759,16 +5759,16 @@ public async Task TestAutoentitiesWithSameObjectDifferentSchemas() | |
| } | ||
|
|
||
| /// <summary> | ||
| /// Ensures that autoentities are properly generated into in-memory entities when entities have non-default schemas. | ||
| /// Ensures that autoentities are properly generated into in-memory entities when entities have unusual elements such as non-default schemas. | ||
| /// </summary> | ||
| /// <param name="includePattern">The pattern to include for autoentities</param> | ||
| /// <param name="isPatternFoo">Boolean that indicates if the pattern is for the foo schema</param> | ||
| /// <param name="patternType">Integer that indicates which input pattern is being used</param> | ||
| /// <returns></returns> | ||
|
RubenCerna2079 marked this conversation as resolved.
|
||
| [TestCategory(TestCategory.MSSQL)] | ||
| [DataTestMethod] | ||
| [DataRow("foo.%", true, DisplayName = "Test Autoentities with foo schema")] | ||
| [DataRow("bar.%", false, DisplayName = "Test Autoentities with bar schema")] | ||
| public async Task TestAutoentitiesGeneratedWithDifferentSchemas(string includePattern, bool isPatternFoo) | ||
| [DataRow("foo.%", 0, DisplayName = "Test Autoentities with foo schema")] | ||
| [DataRow("bar.%", 1, DisplayName = "Test Autoentities with bar schema")] | ||
| public async Task TestAutoentitiesGeneratedWithUnusualElements(string includePattern, int patternType) | ||
|
RubenCerna2079 marked this conversation as resolved.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The tests validate that REST and GraphQL access work end-to-end, which is great. One improvement would be to explicitly assert the generated/sanitized entity name (e.g., "dbo_OrderItems") in the test. That would make the behavior more explicit and protect against future regressions in naming logic.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The new |
||
| { | ||
| // Arrange | ||
| Dictionary<string, Autoentity> autoentityMap = new() | ||
|
|
@@ -5826,11 +5826,28 @@ public async Task TestAutoentitiesGeneratedWithDifferentSchemas(string includePa | |
| using (HttpClient client = server.CreateClient()) | ||
| { | ||
| // Act | ||
| string path = isPatternFoo ? "foo_magazines" : "bar_magazines"; | ||
| string path; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The current test combines multiple scenarios (schemas + whitespace handling) using a It may be cleaner to split whitespace-specific validation into a separate test method for better readability and maintainability.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Extracted into a dedicated |
||
| string item; | ||
| string expectedResponseFragment; | ||
| switch (patternType) | ||
| { | ||
| case 0: | ||
| path = "foo_magazines"; | ||
| item = "title"; | ||
| expectedResponseFragment = @"""title"":""Vogue"""; | ||
| break; | ||
| case 1: | ||
| path = "bar_magazines"; | ||
| item = "comic_name"; | ||
| expectedResponseFragment = @"""comic_name"":""NotVogue"""; | ||
| break; | ||
| default: | ||
| throw new ArgumentException("Invalid pattern type"); | ||
| } | ||
|
|
||
| using HttpRequestMessage restRequest = new(HttpMethod.Get, $"/api/{path}"); | ||
| using HttpResponseMessage restResponse = await client.SendAsync(restRequest); | ||
|
|
||
| string item = isPatternFoo ? "title" : "comic_name"; | ||
| string graphqlQuery = $@" | ||
| {{ | ||
| {path} {{ | ||
|
|
@@ -5848,8 +5865,6 @@ public async Task TestAutoentitiesGeneratedWithDifferentSchemas(string includePa | |
| HttpResponseMessage graphqlResponse = await client.SendAsync(graphqlRequest); | ||
|
|
||
| // Assert | ||
| string expectedResponseFragment = isPatternFoo ? @"""title"":""Vogue""" : @"""comic_name"":""NotVogue"""; | ||
|
|
||
| // Verify REST response | ||
| Assert.AreEqual(HttpStatusCode.OK, restResponse.StatusCode, "REST request to auto-generated entity should succeed"); | ||
|
|
||
|
|
@@ -5867,6 +5882,116 @@ public async Task TestAutoentitiesGeneratedWithDifferentSchemas(string includePa | |
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Ensures that autoentities are generated with valid names when the SQL object name contains spaces. | ||
| /// Whitespace is removed and the following character is capitalized (camelCase join), so that the | ||
| /// resulting entity name is a valid REST path segment and GraphQL type name. | ||
| /// For example, "dbo.[Order Items]" with the default pattern "{schema}_{object}" produces the | ||
| /// entity name "dbo_OrderItems" — not "dbo_Order Items". | ||
| /// </summary> | ||
| [TestCategory(TestCategory.MSSQL)] | ||
| [TestMethod] | ||
| public async Task TestAutoentitiesGeneratedWithSpacesInObjectName() | ||
| { | ||
| // Arrange | ||
| const string EXPECTED_ENTITY_NAME = "dbo_OrderItems"; | ||
| const string EXPECTED_ITEM_FIELD = "productname"; | ||
| const string EXPECTED_RESPONSE_FRAGMENT = @"""productname"":""Sample Product"""; | ||
|
|
||
| Dictionary<string, Autoentity> autoentityMap = new() | ||
| { | ||
| { | ||
| "SpacedObjectAutoEntity", new Autoentity( | ||
| Patterns: new AutoentityPatterns( | ||
| Include: new[] { "dbo.Order Items" }, | ||
| Exclude: null, | ||
| Name: null | ||
| ), | ||
| Template: new AutoentityTemplate( | ||
| Rest: new EntityRestOptions(Enabled: true), | ||
| GraphQL: new EntityGraphQLOptions( | ||
| Singular: string.Empty, | ||
| Plural: string.Empty, | ||
| Enabled: true | ||
| ), | ||
| Health: null, | ||
| Cache: null | ||
| ), | ||
| Permissions: new[] { GetMinimalPermissionConfig(AuthorizationResolver.ROLE_ANONYMOUS) } | ||
| ) | ||
| } | ||
| }; | ||
|
|
||
| DataSource dataSource = new(DatabaseType.MSSQL, | ||
| GetConnectionStringFromEnvironmentConfig(environment: TestCategory.MSSQL), Options: null); | ||
|
|
||
| RuntimeConfig configuration = new( | ||
| Schema: "TestAutoentitiesSpacesSchema", | ||
| DataSource: dataSource, | ||
| Runtime: new( | ||
| Rest: new(Enabled: true), | ||
| GraphQL: new(Enabled: true), | ||
| Mcp: new(Enabled: false), | ||
| Host: new( | ||
| Cors: null, | ||
| Authentication: new Config.ObjectModel.AuthenticationOptions( | ||
| Provider: nameof(EasyAuthType.StaticWebApps), | ||
| Jwt: null | ||
| ) | ||
| ) | ||
| ), | ||
| Entities: new(new Dictionary<string, Entity>()), | ||
| Autoentities: new RuntimeAutoentities(autoentityMap) | ||
| ); | ||
|
|
||
| File.WriteAllText(CUSTOM_CONFIG_FILENAME, configuration.ToJson()); | ||
|
|
||
| string[] args = new[] { $"--ConfigFileName={CUSTOM_CONFIG_FILENAME}" }; | ||
| using (TestServer server = new(Program.CreateWebHostBuilder(args))) | ||
| using (HttpClient client = server.CreateClient()) | ||
| { | ||
| // Assert that the sanitized entity name "dbo_OrderItems" is reachable via REST, | ||
| // explicitly confirming the generated name is EXPECTED_ENTITY_NAME and not "dbo_Order Items". | ||
| using HttpRequestMessage restRequest = new(HttpMethod.Get, $"/api/{EXPECTED_ENTITY_NAME}"); | ||
| using HttpResponseMessage restResponse = await client.SendAsync(restRequest); | ||
| Assert.AreEqual( | ||
| HttpStatusCode.OK, | ||
| restResponse.StatusCode, | ||
| $"REST path '/api/{EXPECTED_ENTITY_NAME}' should exist; the entity name must be sanitized from 'dbo_Order Items' to '{EXPECTED_ENTITY_NAME}'."); | ||
|
|
||
| string restResponseBody = await restResponse.Content.ReadAsStringAsync(); | ||
| Assert.IsTrue(!string.IsNullOrEmpty(restResponseBody), "REST response should contain data"); | ||
| Assert.IsTrue(restResponseBody.Contains(EXPECTED_RESPONSE_FRAGMENT)); | ||
|
|
||
| // Also verify via GraphQL using the sanitized name as the query root field. | ||
| string graphqlQuery = $@" | ||
| {{ | ||
| {EXPECTED_ENTITY_NAME} {{ | ||
| items {{ | ||
| {EXPECTED_ITEM_FIELD} | ||
| }} | ||
| }} | ||
| }}"; | ||
|
|
||
| object graphqlPayload = new { query = graphqlQuery }; | ||
| HttpRequestMessage graphqlRequest = new(HttpMethod.Post, "/graphql") | ||
| { | ||
| Content = JsonContent.Create(graphqlPayload) | ||
| }; | ||
| HttpResponseMessage graphqlResponse = await client.SendAsync(graphqlRequest); | ||
|
|
||
| Assert.AreEqual( | ||
| HttpStatusCode.OK, | ||
| graphqlResponse.StatusCode, | ||
| $"GraphQL query for '{EXPECTED_ENTITY_NAME}' should succeed with the sanitized entity name."); | ||
|
|
||
| string graphqlResponseBody = await graphqlResponse.Content.ReadAsStringAsync(); | ||
| Assert.IsTrue(!string.IsNullOrEmpty(graphqlResponseBody), "GraphQL response should contain data"); | ||
| Assert.IsFalse(graphqlResponseBody.Contains("errors"), "GraphQL response should not contain errors"); | ||
| Assert.IsTrue(graphqlResponseBody.Contains(EXPECTED_RESPONSE_FRAGMENT)); | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Tests that DAB fails if the entities generated from autoentities property | ||
| /// do not contain unique parameters such as rest path, graphql singular/plural names, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sanitization via whitespace removal can introduce collisions. For example:
Both would map to the same entity name after sanitization.
Do we currently detect or handle such collisions? It might be useful to either log or explicitly guard against these cases to avoid silent overrides or ambiguous schema behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Collisions are now surfaced explicitly. The original (pre-sanitization) name is stored before
RemoveWhitespaceAndCamelCaseis called. Whenentities.TryAddfails (covering the"Order Item"/"OrderItem"→"OrderItem"collision case), the exception message includes both the sanitized name and the original name along with the schema, e.g.:This makes it clear to the user that the collision may be a result of whitespace removal rather than an exact name match.