Trim forward slashes from route prefix - #395
Conversation
| throw Error.ArgumentNull(nameof(routePrefix)); | ||
| } | ||
|
|
||
| string sanitizedRoutePrefix = routePrefix.Trim('/'); |
There was a problem hiding this comment.
I believe Trim does both a TrimStart and a TrimeEnd. Are we okay with that?
There was a problem hiding this comment.
Yeah. If you don't trim the end, an exception is thrown by AspNetCore as mentioned in the description.
| if (!routePrefix.StartsWith('/') && !routePrefix.EndsWith('/')) | ||
| { | ||
| return routePrefix; | ||
| } |
There was a problem hiding this comment.
I add this check to avoid allocating a new string in case the string contains no slashes. But I confirmed that the implementation of string.Trim returns the original string if no actual trimming was performed. But I kept this code here for clarity and also cause it seems to be doing less work (haven't confirmed with actual benchmakrs though) compared to string.Trim when there's nothing to trim.
There was a problem hiding this comment.
If string.Trim actually does that then I'm inclined to think that this method is an overkill 'coz I'd expect that string.Trim does it almost same way that you're doing. Another thing... The above logic would throw an exception if routePrefix was null, though I assume that the null check is done elsewhere - you could just add a Debug.Assert. If you choose to retain this, maybe this would give us better performance:
if (routePrefix.Length > 0 && routePrefix[0] != '/' && routePrefix[routePrefix.Length - 1] != '/')There was a problem hiding this comment.
I do check that the prefix is not null on the public methods that call SanitizeRoutePrefix.
I've added the Debug.Assert and used your suggested if statement. I assume the string.StartsWith and string.EndsWith would have been inlined eitherway?
| } | ||
|
|
||
| if (RouteComponents.ContainsKey(routePrefix)) | ||
| if (routePrefix == null) |
There was a problem hiding this comment.
Don't we have cases where the prefix is null?
There was a problem hiding this comment.
We have a case where the prefix is empty (the default case). Not sure about null though.
In any case, you can't add a null key to a dictionary. So I think throwing on null here is the right thing to do.
KenitoInc
left a comment
KenitoInc
left a comment
There was a problem hiding this comment.
Consider adding tests for these edge cases:
- Single slash as the route prefix
- Double slash as the route prefix
What should be the expected behaviour?
Good point. A single slash route prefix would be handled as an empty string. I think that is the correct behaviour. A double slash will also be handled as a an empty string, but I think in this case that is invalid and ought to be an error. But I don't know if we should go the extra-length of validating such route prefixes. I personally am not sure it's worth the effort of checking the string contents. |
5 participants
Fix #382
This PR strips leading and trailing forward slashes from the OData path route prefix before registering route components service provider. This avoids the issues mentioned in #382 which result in the route prefix with leading slashes not being routed to OData endpoints. Route prefix with trailing slashes were causing an exception to be thrown because it would be combined with the endpoint path leading to 2 consecutive forward slashes.