Improve GetNavigationSource performance
#1158
Conversation
habbes
requested review from
xuzhg,
wachugamaina,
KenitoInc,
mikepizzo and
gathogojr
| continue; | ||
| } | ||
|
|
||
| return null; |
There was a problem hiding this comment.
Should all other Segments i.e
DynamicPathSegment
PathTemplateSegment
ValueSegment
CountSegment
BatchSegment
MetadataSegment
ODataPathSegment
Return null?
Was this the pre-existing behavour ?
There was a problem hiding this comment.
Yes, they should return null. That's the existing behaviour. Also handled in tests.
| @@ -77,9 +77,59 @@ public static IEdmNavigationSource GetNavigationSource(this ODataPath path) | |||
| throw Error.ArgumentNull(nameof(path)); | |||
| } | |||
|
|
|||
| ODataPathNavigationSourceHandler handler = new ODataPathNavigationSourceHandler(); | |||
There was a problem hiding this comment.
With this type of of refactors, I think it would be wise to add the comprehensive tests first and have that go in, then come to the refractor. because when the tests come in with the change, it is quite hard to tell if indeed the tests represent the pre-existing behavior.
There was a problem hiding this comment.
Alright, let me create a separate PR to add the tests.
There was a problem hiding this comment.
I've created the tests PR: #1159
| [InlineData("Customers(1)/Account/Amount/$value", null, EdmNavigationSourceKind.None)] | ||
| [InlineData("$batch", null, EdmNavigationSourceKind.None)] | ||
| [InlineData("$metadata", null, EdmNavigationSourceKind.None)] | ||
| public void GetNavigationSource_ReturnsCorrectNavigationSource(string path, string expectedNavigationSource, EdmNavigationSourceKind navigationSourceKind) |
There was a problem hiding this comment.
I dont belive that all the handler cases are covered in these tests, for example I cannot see
operationImportSegment
operationSegment
TypeSegment
DynamicPathSegment
PathTemplateSegment
ValueSegment
CountSegment
BatchSegment
MetadataSegment
ODataPathSegment
There was a problem hiding this comment.
All those scenarios are covered:
OperationImportSegment: lines 78 and 79OperationSegment: lines 8-83, 87TypeSegment: 88. 89DynamicPathSegment: 77ValueSegment: 91CountSegment: 90MetadataSegment: 93BatchSegment: 92
PathTemplateSegment is covered separately in the test GetNavigationSource_WhenPathTemplateSegment_ReturnsNull on line 116
ODataPathSegment is the abstract base class. I tested this by creating a new UnknownTestODataPathSegment and verifying that it returns null in GetNavigationSource_WhenUnkownPathSegment_ReturnsNull on line 130.
|
Closing this PR since I'm splitting the test and refactoring in separate PRs. I will re-create the PR once the tests have been merged: #1159 |
|
Replaced with: #1161 |
Fix #1118
Replaces the use of
ODataPathNavigationSourceHandlerinODataPath.GetNavigationSourceextension method with a simpler reverse loop of the path.With
ODataPathNavigationSourceHandlerList<string>is created and populated with the string representation of each path segment. This list is used to generate a string representation of the path via aPathproperty. But this property is not needed to determine the navigation source (it's also not used by existing code).The new implementation scans the path segments from the end and stops as soon as it finds the navigation source because only the last navigation source in the path matters.
Also, the
ODataPath.GetNavigationSource()method didn't have good test coverage, so I added tests to verify all the scenarios the original implementation handled.The
ODataPathNavigationSourceHandleris public so I kept it to avoid breaking changes.Before
After