[Endpoint routing review]First draft to enable endpoint routing using dynamic controller by xuzhg · Pull Request #2035 · OData/WebApi

xuzhg · 2020-01-29T19:54:29Z

Issues

This pull request fixes issue #xxx.

Description

Enable Endpoint routing in ASP.NET Core OData

Checklist (Uncheck if it is not completed)

Test cases added
Build and test with one-click build and test script passed

Additional work necessary

If documentation update is needed, please add "Docs Needed" label to the issue and provide details about the required document change in the issue.

KanishManuja-MS · 2020-02-06T17:20:24Z

+            }
+
+            querySettings = defaultQuerySettings;
+        }


How does this even work? #Closed

Synced offline, right.

In reply to: 375972141 [](ancestors = 375972141)

KanishManuja-MS · 2020-02-06T17:28:32Z

-
-            querySettings = defaultQuerySettings;
+            builder.ServiceProvider.SetDefaultQuerySettings(defaultQuerySettings);
            return builder;


What is the reason behind this change? Extension method for service provider is not intuitive. May be you want to create a private static methods rather than extension methods. #WontFix

Synced offline, right?

In reply to: 375976380 [](ancestors = 375976380)

KanishManuja-MS · 2020-02-06T17:41:11Z

 using Microsoft.AspNetCore.Mvc.Infrastructure;
+using Microsoft.AspNetCore.Routing;
+#if NETCOREAPP3_1
+using Microsoft.AspNetCore.Routing.Matching;


Once we upgrade to higher versions of net core, this would need to be changed right? Is there a conditional compile that specifies greater than? #Resolved

Revert.

In reply to: 375983113 [](ancestors = 375983113)

KanishManuja-MS · 2020-02-06T17:56:16Z

@@ -39,23 +38,7 @@ public static IRouteBuilder SetDefaultQuerySettings(this IRouteBuilder builder,
                throw Error.ArgumentNull("builder");
            }


Now I understand the reason for this change. I still feel extension method may not be right. Just a shared static method with a normal parameter may be better. #WontFix

Extension method, we can call the method using Fluent way.

builder.Fun1().Func2().Func3()....

with static method. we can't do like this.

In reply to: 375990865 [](ancestors = 375990865)

KanishManuja-MS · 2020-02-06T18:02:31Z

+        /// <param name="model">The EDM model to use for parsing OData paths.</param>
+        /// <param name="batchHandler">The <see cref="ODataBatchHandler"/>.</param>
+        /// <returns>The <see cref="IEndpointRouteBuilder"/>.</returns>
+        public static IEndpointRouteBuilder MapODataRoute(this IEndpointRouteBuilder builder,


MapODataRoute [](start = 44, length = 13)

You should use MapODataServiceRoute to maintain parity. #Resolved

KanishManuja-MS · 2020-02-06T18:03:07Z

+        /// The OData routing conventions to use for controller and action selection.
+        /// </param>
+        /// <returns>The <see cref="IEndpointRouteBuilder"/>.</returns>
+        public static IEndpointRouteBuilder MapODataRoute(this IEndpointRouteBuilder builder,


MapODataRoute [](start = 44, length = 13)

Same for other. Change the name. #Resolved

KanishManuja-MS · 2020-02-06T18:05:37Z

+    <VersionMinor Condition="'$(VersionMinor)' == ''">4</VersionMinor>
    <VersionBuild Condition="'$(VersionBuild)' == ''">0</VersionBuild>
    <VersionRelease Condition="'$(VersionRelease)' == ''"></VersionRelease>
  </PropertyGroup>


Add beta here. #Resolved

KanishManuja-MS · 2020-02-06T18:20:01Z

+                if (start == -1 || end == -1)
+                {
+                    break;
+                }


I am missing something may be. It seems that the if prefix has '{' and '}', this will loop indefinitely as prefix never changes. #WontFix

No, see my test cases for more information.

In reply to: 376002351 [](ancestors = 376002351)

KanishManuja-MS · 2020-02-06T18:21:07Z

+            foreach (var item in templates)
+            {
+                string variable = item.Substring(1, item.Length - 2); // remove { and }
+


Why substring again? We should add to templates after striping the braces already. #WontFix

We only replace the string within "{" and "}". and keep the other as it. for example:

prefix: "data{data}" and we have {"data": 1} in route value, the return of this method should be "data1", not "11". See more from my added test cases.

In reply to: 376002864 [](ancestors = 376002864)

KanishManuja-MS · 2020-02-06T18:28:30Z

+        private static string UriEncode(string str)
+        {
+            string escape = Uri.EscapeUriString(str);
+            escape = escape.Replace("#", _escapedHashMark);


"#" [](start = 36, length = 3)

Declare these strings as constants? It may be better to use the char overload instead of the string overload for better performance. #Pending

Replace doesn't have a overload to replace a char in a string. and for better readability, I'd like to use "#" directly here. The LinkGenerator mayeb change later.

In reply to: 376006516 [](ancestors = 376006516)

KanishManuja-MS · 2020-02-06T18:30:05Z

+
+        /// <inheritdoc/>
+        public override Task SelectAsync(HttpContext httpContext, CandidateSet candidateSet)
+        {


What are the other things that can happen in the pipeline while this runs asynchronously?

KanishManuja-MS · 2020-02-06T18:32:42Z

+// Copyright (c) Microsoft Corporation.  All rights reserved.
+// Licensed under the MIT License.  See License.txt in the project root for license information.
+
+#if !NETSTANDARD2_0


!NETSTANDARD2_0 [](start = 4, length = 15)

This conditional is different from the other conditional. Can you explain the reason? #Resolved

KanishManuja-MS · 2020-02-06T18:34:09Z

I see the sample but there are no test cases. Can you please add test cases for endpoint routing? Specially for the ones where controller methods are overloaded? #Resolved

KanishManuja-MS · 2020-02-06T18:36:17Z

+    /// </summary>
+    internal class ODataEndpointSelector : EndpointSelector
+    {
+        private EndpointSelector _innerSelector;


_innerSelector [](start = 33, length = 14)

I am unclear on what does the coding style guidelines say about using _ for private members. It was the old naming convention and I see that not all classes use this. We should discuss what is the recommendation, since this convention is different in ODL as well.

gathogojr · 2020-02-26T09:22:42Z

+        /// <param name="generator">The inner Link generator</param>
+        public ODataEndpointLinkGenerator(LinkGenerator generator)
+        {
+            _innerGenerator = generator;


@xuzhg Is there a chance of this constructor being called with a null value for the LinkGenerator parameter? That would most certainly precipitate chaos when the overridden methods that reference _innerGenerator are called #Resolved

Theoretically, no. But, let me add the verification code.

In reply to: 384365505 [](ancestors = 384365505)

gathogojr · 2020-02-26T11:08:30Z

+        {
+            if (routeName == null)
+            {
+                throw Error.ArgumentNull("routeName");


@xuzhg Better to use nameof here #Resolved

gathogojr · 2020-02-26T11:54:59Z

+        {
+            if (comparer == null)
+            {
+                throw new ArgumentNullException(nameof(comparer));


@xuzhg Maybe use throw Error.ArgumentNull(...) for consistency #WontFix

This class is removed and replaced using "ODataEndpointSelectorPolicy"

In reply to: 384446213 [](ancestors = 384446213)

gathogojr · 2020-02-26T12:00:55Z

+        {
+            if (serviceProvider == null)
+            {
+                throw Error.ArgumentNull("serviceProvider");


@xuzhg Better use nameof here #Resolved

gathogojr · 2020-02-26T12:01:49Z

+        {
+            if (serviceProvider == null)
+            {
+                throw Error.ArgumentNull("serviceProvider");


@xuzhg Better use nameof here #Resolved

gathogojr · 2020-02-26T12:09:54Z


-            DefaultQuerySettings defaultQuerySettings = builder.GetDefaultQuerySettings();
-            defaultQuerySettings.EnableCount = true;
+            builder.ServiceProvider.Count();


@xuzhg Do we need the variant of Count that accepts a QueryOptionSetting parameter? That calls the overload here #Resolved

Thanks. I forgot to change that Count(...) method. Fixed.

In reply to: 384452658 [](ancestors = 384452658)

gathogojr · 2020-02-26T12:58:46Z

+                throw Error.ArgumentNull("routeName");
+            }
+
+            IList<IODataRoutingConvention> routingConventions = CreateDefault();


@xuzhg The 4 lines here are a replica of these others here. Maybe the existing CreateDefaultWithAttributeRouting overload could call this new one like this return CreateDefaultWithAttributeRouting(routeName, builder.ServiceProvider) - in which case the check for null routeName wouldn't be required in the existing overload, or consider adding a helper private method that both overloads call #Resolved

👍

In reply to: 384475452 [](ancestors = 384475452)

Changed.

Originally, i'd like to change the exiting public to the new one. However, it's a breaking change. So, i add a new one.

In reply to: 384715327 [](ancestors = 384715327,384475452)

mikepizzo · 2020-02-26T18:57:27Z

public interface IODataFeature

Given that we are adding to a public interface, I assume we don't expect anyone to have their own custom implementations of this interface that would now break? #Pending

Refers to: src/Microsoft.AspNetCore.OData/Interfaces/IODataFeature.cs:20 in 59b98be. [](commit_id = 59b98be, deletion_comment = False)

mikepizzo · 2020-02-26T18:58:25Z

@@ -11,6 +11,20 @@ namespace Microsoft.AspNet.OData
    /// </summary>
    public interface IPerRouteContainer


public [](start = 4, length = 6)

I assume there are no custom implementations that would break by our adding members to this interface? #Pending

Same as the comment in "IODataFeature".

In reply to: 384697141 [](ancestors = 384697141)

mikepizzo · 2020-02-26T19:25:17Z

+    //        : this((EndpointDataSource)dataSource)
+    //    {
+    //    }
+    //}


clean this up?
#Resolved

mikepizzo

Added a few minor comments to those that others had already posted, but otherwise looks good.

xuzhg · 2020-02-26T22:37:18Z

Added the test cases

In reply to: 583046981 [](ancestors = 583046981)

xuzhg · 2020-02-26T22:46:52Z

public interface IODataFeature

We have been discussing about to add the public interface for a long time. It seems it's not a breaking change to add new items to a public interface. However, you are right. We don't expect any customer to create their own custom "IODataFeature" implementation.

In reply to: 591585939 [](ancestors = 591585939)

Refers to: src/Microsoft.AspNetCore.OData/Interfaces/IODataFeature.cs:20 in 59b98be. [](commit_id = 59b98be, deletion_comment = False)

xuzhg · 2020-02-27T22:54:54Z

The latest CI passed at: https://identitydivision.visualstudio.com/OData/_build/results?buildId=481353&view=logs&j=fd490c07-0b22-5182-fac9-6d67fe1e939b&t=fd490c07-0b22-5182-fac9-6d67fe1e939b

… dynamic controller (#2035) * Enable endpoint routing using dynamic controller * Update to 7.4.0 * Update the PublicAPI test cases * Add the E2E test cases * Change the startup to test the per-route configuration * Some test codes about policy * Use MatcherPolicy Instead of EndpointSelector to resolve the ambiguity * Address the comments and add LinkGenerator test cases * Revert the EF Core version to 2.2.0 to make Aggregation E2E test pass

… dynamic controller (OData#2035) * Enable endpoint routing using dynamic controller * Update to 7.4.0 * Update the PublicAPI test cases * Add the E2E test cases * Change the startup to test the per-route configuration * Some test codes about policy * Use MatcherPolicy Instead of EndpointSelector to resolve the ambiguity * Address the comments and add LinkGenerator test cases * Revert the EF Core version to 2.2.0 to make Aggregation E2E test pass

This was referenced Jan 29, 2020

Please add support for OData endpoint routing #2029

Closed

.NET Core 3.0 support? #1748

Closed

xuzhg added the Ready for review Use this label if a pull request is ready to be reviewed label Feb 4, 2020

xuzhg force-pushed the EnableEndpointRouting branch from 831209c to 8d9c27b Compare February 5, 2020 00:59

KanishManuja-MS reviewed Feb 6, 2020

View reviewed changes

mikepizzo added this to the 7.4 Beta milestone Feb 11, 2020

xuzhg force-pushed the EnableEndpointRouting branch 3 times, most recently from 5229452 to 59b98be Compare February 25, 2020 00:01

xuzhg requested review from KanishManuja-MS, Sreejithpin, gathogojr, habbes and mikepizzo February 25, 2020 21:53

gathogojr reviewed Feb 26, 2020

View reviewed changes

mikepizzo reviewed Feb 26, 2020

View reviewed changes

mikepizzo approved these changes Feb 26, 2020

View reviewed changes

xuzhg and others added 8 commits February 26, 2020 18:21

First draft to enable endpoint routing using dynamic controller

44a20ae

Update to 7.4.0

7ec6863

Update the PublicAPI test cases

31aa46b

Add the E2E test cases

6ad347c

Change the startup to test the per-route configuration

f6c686c

Some test codes about policy

177b37f

Use MatcherPolicy Instead of EndpointSelector to resolve the ambiguity

2750218

Address the comments and add LinkGenerator test cases

3c9aa37

xuzhg force-pushed the EnableEndpointRouting branch from c1089ca to 3c9aa37 Compare February 27, 2020 02:22

gathogojr approved these changes Feb 27, 2020

View reviewed changes

Revert the EF Core version to 2.2.0 to make Aggregation E2E test pass

e68abed

xuzhg merged commit 0204e70 into OData:master Feb 27, 2020

		@@ -39,23 +38,7 @@ public static IRouteBuilder SetDefaultQuerySettings(this IRouteBuilder builder,
		throw Error.ArgumentNull("builder");
		}

		@@ -11,6 +11,20 @@ namespace Microsoft.AspNet.OData
		/// </summary>
		public interface IPerRouteContainer

Uh oh!

Conversation

xuzhg commented Jan 29, 2020

Issues

Description

Checklist (Uncheck if it is not completed)

Additional work necessary

Uh oh!

KanishManuja-MS Feb 6, 2020 • edited by xuzhg Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KanishManuja-MS Feb 6, 2020 • edited by xuzhg Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KanishManuja-MS Feb 6, 2020 • edited by xuzhg Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KanishManuja-MS Feb 6, 2020 • edited by xuzhg Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KanishManuja-MS Feb 6, 2020 • edited by xuzhg Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KanishManuja-MS Feb 6, 2020 • edited by xuzhg Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KanishManuja-MS Feb 6, 2020 • edited by xuzhg Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KanishManuja-MS Feb 6, 2020 • edited by xuzhg Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KanishManuja-MS Feb 6, 2020 • edited by xuzhg Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KanishManuja-MS Feb 6, 2020 • edited by xuzhg Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KanishManuja-MS Feb 6, 2020 • edited by xuzhg Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KanishManuja-MS commented Feb 6, 2020 • edited by xuzhg Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gathogojr Feb 26, 2020 • edited by xuzhg Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gathogojr Feb 26, 2020 • edited by xuzhg Loading Uh oh! There was an error while loading. Please reload this page.

KanishManuja-MS Feb 6, 2020 •

edited by xuzhg

Loading

KanishManuja-MS Feb 6, 2020 •

edited by xuzhg

Loading

KanishManuja-MS Feb 6, 2020 •

edited by xuzhg

Loading

KanishManuja-MS Feb 6, 2020 •

edited by xuzhg

Loading

KanishManuja-MS Feb 6, 2020 •

edited by xuzhg

Loading

KanishManuja-MS Feb 6, 2020 •

edited by xuzhg

Loading

KanishManuja-MS Feb 6, 2020 •

edited by xuzhg

Loading

KanishManuja-MS Feb 6, 2020 •

edited by xuzhg

Loading

KanishManuja-MS Feb 6, 2020 •

edited by xuzhg

Loading

KanishManuja-MS Feb 6, 2020 •

edited by xuzhg

Loading

KanishManuja-MS Feb 6, 2020 •

edited by xuzhg

Loading

KanishManuja-MS commented Feb 6, 2020 •

edited by xuzhg

Loading

gathogojr Feb 26, 2020 •

edited by xuzhg

Loading

gathogojr Feb 26, 2020 •

edited by xuzhg

Loading

gathogojr Feb 26, 2020 •

edited by xuzhg

Loading

gathogojr Feb 26, 2020 •

edited by xuzhg

Loading

gathogojr Feb 26, 2020 •

edited by xuzhg

Loading

gathogojr Feb 26, 2020 •

edited by xuzhg

Loading

gathogojr Feb 26, 2020 •

edited by xuzhg

Loading

mikepizzo commented Feb 26, 2020 •

edited by xuzhg

Loading

mikepizzo Feb 26, 2020 •

edited by xuzhg

Loading

mikepizzo Feb 26, 2020 •

edited by xuzhg

Loading

mikepizzo left a comment •

edited

Loading