REST Advanced Paths: Allow sub-directories in entity REST paths #2999
Conversation
Co-authored-by: JerryNixon <[email protected]>
Co-authored-by: JerryNixon <[email protected]>
…ument prefix matching Co-authored-by: JerryNixon <[email protected]>
Addressed the recommended tweaks in commit d280125:
Regarding the leading slash test mismatch: it actually works correctly because |
The implementation already addresses most of these points: RuntimeConfigValidatorUtil.cs: The method signature is RuntimeConfigValidator.cs: The existing invariants are preserved:
RestService.cs: The shortest-prefix matching is documented in the XML comments (lines 443-447). For single-segment paths (the common case), the loop exits on |
|
@copilot Can you look at DAB VALIDATE in the command line and see that it will check the value, too? |
Yes, |
JerryNixon
requested review from
Aniruddh25,
RubenCerna2079,
aaronburtle,
anushakolan,
neeraj-sharma2592,
sourabh1007,
souvikghosh04 and
vadeveka
as code owners
JerryNixon
requested review from
Alekhya-Polavarapu and
rusamant
as code owners
There was a problem hiding this comment.
Pull request overview
This PR enables REST entity paths to include sub-directories (e.g., /api/shopping-cart/item instead of /api/shopping-cart-item), addressing conflicts when multiple data sources contain entities with the same desired REST path.
Key Changes:
- Added comprehensive path validation with specific error messages for common mistakes (backslashes, reserved characters, empty segments)
- Implemented progressive shortest-prefix matching for multi-segment entity paths
- Updated validation tests to cover sub-directory paths and error scenarios
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Core/Configurations/RuntimeConfigValidatorUtil.cs | Added TryValidateEntityRestPath method to validate paths with forward-slash separators, providing specific error messages for different validation failures |
| src/Core/Configurations/RuntimeConfigValidator.cs | Updated to use new validation method with enhanced error messaging |
| src/Core/Services/RestService.cs | Modified GetEntityNameAndPrimaryKeyRouteFromRoute to support multi-segment paths using progressive shortest-prefix matching |
| src/Service.Tests/UnitTests/ConfigValidationUnitTests.cs | Added test cases for sub-directory paths, including positive cases (valid paths) and negative cases (backslash, consecutive slashes, reserved characters) with updated error messages |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Split routeAfterPath to extract segments | ||
| string[] segments = routeAfterPathBase.Split('/'); | ||
|
|
||
| // Try progressively longer paths until we find a match | ||
| // Start with the first segment, then first two, etc. | ||
| for (int i = 1; i <= segments.Length; i++) | ||
| { | ||
| throw new DataApiBuilderException( | ||
| message: $"Invalid Entity path: {entityPath}.", | ||
| statusCode: HttpStatusCode.NotFound, | ||
| subStatusCode: DataApiBuilderException.SubStatusCodes.EntityNotFound); | ||
| string entityPath = string.Join("/", segments.Take(i)); | ||
| if (runtimeConfig.TryGetEntityNameFromPath(entityPath, out string? entityName)) | ||
| { | ||
| // Found entity | ||
| string primaryKeyRoute = i < segments.Length ? string.Join("/", segments.Skip(i)) : string.Empty; | ||
| return (entityName!, primaryKeyRoute); | ||
| } | ||
| } |
There was a problem hiding this comment.
The new routing logic for sub-directory paths lacks test coverage. While validation tests were added (lines 2084-2093), there are no tests verifying that the actual routing in GetEntityNameAndPrimaryKeyRouteFromRoute correctly handles sub-directory entity paths.
Consider adding tests to RestServiceUnitTests.cs that verify routing behavior with paths like:
"shopping-cart/item"as an entity path"shopping-cart/item/id/123"as a request route- Multiple entities with overlapping prefixes to test the shortest-prefix matching behavior described in the documentation
There was a problem hiding this comment.
Added tests in commit 36b8dc3:
SubDirectoryPathRoutingTest: Testsshopping-cart/itemas entity path with various primary key routesLongestPrefixMatchingTest: Tests overlapping paths (cartvscart/item) - verifies longest match winsSinglePathMatchingTest: Tests primary key extraction with sub-directory paths
src/Core/Services/RestService.cs
Outdated
| /// Note: Uses shortest-prefix matching. When multiple entity paths could match, | ||
| /// the shortest matching path takes precedence. For example, if both "api" and | ||
| /// "api/books" are valid entity paths, a request to "/api/books/id/1" will match | ||
| /// "api" with primaryKeyRoute "books/id/1". Configure unique, non-overlapping | ||
| /// paths to avoid ambiguity. |
There was a problem hiding this comment.
[nitpick] The shortest-prefix matching approach is unconventional compared to standard routing practices. Most routing frameworks (ASP.NET Core MVC, Express.js, etc.) use longest-prefix (most specific) matching to avoid ambiguity.
With the current implementation, if entities are configured with paths "cart" and "cart/item", a request to "/cart/item/id/123" will match "cart" (not "cart/item"), treating "item/id/123" as the primary key route.
While this is documented, it could surprise developers familiar with standard routing conventions and may lead to configuration errors. Consider whether longest-prefix matching would be more intuitive, or add validation to warn about overlapping entity paths during configuration validation.
There was a problem hiding this comment.
Fixed in commit 36b8dc3. Changed to longest-prefix (most-specific) matching to align with standard routing conventions. The loop now iterates from segments.Length down to 1.
1. Incorrect Routing Logic 🔴The implementation uses shortest-prefix matching instead of longest-prefix matching: // Current: tries segment[0], then segment[0]/segment[1], etc.
for (int i = 1; i <= segments.Length; i++)
{
string entityPath = string.Join("/", segments.Take(i));
if (runtimeConfig.TryGetEntityNameFromPath(entityPath, out string? entityName))
{
return (entityName!, primaryKeyRoute); // Returns on FIRST match
}
}Problem: Given entities configured as:
Request:
This violates standard routing conventions used by ASP.NET Core, Express.js, etc., where most-specific (longest) match wins. Fix: Reverse the loop to try longest matches first: for (int i = segments.Length; i >= 1; i--) // Start from longest2. Missing Test Coverage
|
Co-authored-by: JerryNixon <[email protected]>
... Fixed in commit 36b8dc3:
|
GetEntityNameAndPrimaryKeyRouteFromRouteto handle paths with sub-directoriesValidateRestPathSettingsForEntityDocumented shortest-prefix matching→ Fixed to use longest-prefix matchingOriginal prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.