Adding support for custom http routes #743

mathewc · 2016-10-05T17:02:08Z

Adds the following features:

ability to specify custom routes for functions, with arbitrarily complex route constraints (e.g. "route": "products/{Category:alpha}/{Id:int}"). Documentation on supported constraint formats can be found here. Basically this is just regular Asp.Net WebApi routing.
integrates captured route parameters with our binding data pipeline
allow for the default "api" route prefix to be customized or completely removed via host.json
changes/simplifications to the way we were populating binding data + POCOs from request data. The way it works now:
- if the function binds to a poco, we initialize it from the WebHook or from the request body
- we create an aggregated bag of binding data from that poco (if it exists) and from route parameters, query string parameters, and request body
- finally we apply that binding data to the poco, which enables scenarios where the poco binds to data from the body as well as other sources

The initial PR for this work was here: #490. It has been completely reworked to use Asp.Net core types for route templating, to avoid pulling in a bunch of unecessary Asp.Net Core packages. I've also reworked this on top of our new HttpTrigger binding, which didn't exist when Connor did his initial work. Thanks @ConnorMcMahon for driving these improvements :)

I'm still working on unit tests, but I have e2e test coverage here already, and all existing tests pass :)

mathewc · 2016-10-05T17:04:05Z

sample/host.json

@@ -4,7 +4,11 @@
    "sendGrid": {
        "from": "Azure Functions <samples@functions.com>"
    },
+    "http": {


If customers want to remove the "api" prefix completely, they can set this to empty here

mathewc · 2016-10-05T17:05:25Z

src/WebJobs.Script.WebHost/WebScriptHostManager.cs

-
-                HttpFunctions.TryGetValue(route.ToLowerInvariant(), out function);
+                HttpFunctions.TryGetValue(routeData.Route, out function);
+                request.Properties.Add(ScriptConstants.AzureFunctionsHttpRouteDataKey, routeData.Values);


Pass the parsed route data through via request params so our http binding has access to it for binding

mathewc · 2016-10-05T17:06:08Z

src/WebJobs.Script/Binding/Http/HttpRouteFactory.cs

+
+        public HttpRouteFactory(string prefix)
+        {
+            var constraintResolver = new DefaultInlineConstraintResolver();


We're using Asp.Net core types for all template/route handling :)

mathewc · 2016-10-05T17:08:43Z

src/WebJobs.Script/Binding/Http/HttpTriggerAttributeBindingProvider.cs

                }
+
+                // add any route parameters to the contract
+                if (!string.IsNullOrEmpty(attribute.RouteTemplate))


Adding route parameters to this static binding contract is required to support allowing users to include function parameters mapping to route parameters

mathewc · 2016-10-05T17:12:15Z

src/WebJobs.Script/Description/PowerShell/PowerShellFunctionInvoker.cs

-            var msg = source[e.Index].ToString();
-
-            traceWriter.Info(msg);
+            var data = source[e.Index];


In some PowerShell testing, I was getting null refs here, e.g. if the powershell function writes a null output line

mathewc · 2016-10-05T17:13:24Z

src/WebJobs.Script/PropertyHelper.cs

+
+namespace Microsoft.Azure.WebJobs.Script
+{
+    // Original code here: https://github.com/aspnet/Common/blob/dev/src/Microsoft.Extensions.PropertyHelper.Sources/PropertyHelper.cs


This is a common helper class floating around in a lot of MSFT projects. We have one in WebJobs SDK as well, but it is an incomplete early version of this (it didn't have setter support).

I'm pulling this in to allow us to do performant property sets on user pocos, when aggregating binding data onto the instance

mathewc · 2016-10-05T17:15:51Z

test/WebJobs.Script.Tests/PowerShellEndToEndTests.cs

@@ -46,7 +46,7 @@ public PowerShellEndToEndTests(TestFixture fixture)

            HttpRequestMessage request = new HttpRequestMessage
            {
-                RequestUri = new Uri("http://localhost/api/httptrigger-powershell?code=1388a6b0d05eca2237f10e4a4641260b0a08f3a5&name=testuser"),


I just cleaned up the PS sample functions - they didn't need to have Powershell in their names, since they're in a Powershell folder. Same pattern as all other tests.

mathewc · 2016-10-05T17:16:45Z

...WebJobs.Script.Tests/TestScripts/PowerShell/HttpTrigger-WithError/modules/Get-DateToday.psm1

@@ -1,6 +1,6 @@
-Function Get-DateToday


These are all just renames

mathewc · 2016-10-05T17:17:46Z

test/WebJobs.Script.Tests/WebScriptHostManagerTests.cs

@@ -124,36 +125,6 @@ public void SecretFilesArePurgedOnStartup()
        }

        [Fact]
-        public void GetHttpFunctionOrNull_DecodesUriProperly()


We used to allow spaces in function names, which required URL decode on function names. I cleaned that code up and removed it, so this test is no longer needed.

mathewc · 2016-10-05T17:22:09Z

src/WebJobs.Script.WebHost/WebScriptHostManager.cs

-            string route = HttpUtility.UrlDecode(uri.AbsolutePath);
-            int idx = route.ToLowerInvariant().IndexOf("api", StringComparison.OrdinalIgnoreCase);
-            if (idx > 0)
+            var routeData = HttpRoutes.GetRouteData(request);


We let HttpRouteCollection to do all routing and parsing for us. If we have a match, this returns non-null, and we have all the route data

Does this handle duplicate route templates automatically? I think Web API will throw some 'ambiguous route' exception if it can't figure out which route to pick -- we should make sure we at least handle that similarly.

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

The behavior is (I just wrote a test for it) is that the first match is used. No error is thrown. I'm going to open a follow up issue to investigate what validations we can perform and improve this.

brettsam · 2016-10-05T17:46:17Z

src/WebJobs.Script/Binding/Http/HttpRouteFactory.cs

+        }
+
+        private static IEnumerable<string> ParseRouteParameters(string routeTemplate)
+        {


So the inline constraints like {id:int} are already stripped out before they get here?

brettsam · 2016-10-05T17:54:20Z

sample/HttpTrigger-CSharp-CustomRoute/run.csx

+}
+
+public static ProductInfo Run(ProductInfo info, string category, string id, TraceWriter log)
+{


I haven't gotten this far yet -- but are you setting Category and Id on the info parameter?

Yep :) This is just core SDK functionality. Any data that is part of the static binding contract for a trigger can be used as parameters. E.g. queue trigger dequeue count, etc.

mathewc · 2016-10-05T18:31:04Z

sample/HttpTrigger-CustomRoute/function.json

+            "name": "req",
+            "direction": "in",
+            "methods": [ "get" ],
+            "route": "node/products/{category:alpha}/{id:int?}"


Demonstrates an optional binding - the function returns a singleton or array based on this

brettsam · 2016-10-05T19:14:11Z

src/WebJobs.Script/Binding/Http/HttpTriggerAttributeBindingProvider.cs

+                    foreach (string parameterName in routeParameters)
+                    {
+                        aggregateDataContract[parameterName] = typeof(string);
+                    }


This will override any existing property type in the contract. Does that matter?

Good question. I've put a guard here so we only add if not already present. That way the original Type is retained.

brettsam · 2016-10-05T19:31:36Z

sample/HttpTrigger-CSharp-CustomRoute/function.json

@@ -0,0 +1,16 @@
+{


Could you add/modify a sample that shows using a docdb/table/etc input binding using {Id} to perform the lookup directly for you?

brettsam · 2016-10-05T19:46:15Z

src/WebJobs.Script/Host/ScriptHost.cs

+                if (configSection.TryGetValue("routePrefix", out value))
+                {
+                    routePrefix = (string)value;
+                }


We don't want to allow this to start with 'admin', do we? Although, I think we overwrite it later anyway. Or maybe that validation happens in WebScriptHostManager somewhere already.

The plan was to prevent any routes from using the "admin" prefix, as among other things, this could have potentially unintended consequences.
Validation and early failure here would be a good idea. Perhaps some tests to make sure those routes are disallowed as well?

Might be a good idea to add a route that catches all requests to unregistered routes under admin and responds appropriately (404?)

I opened #748 to track this and the duplicate route issue. I'll address that in a follow up.

brettsam · 2016-10-05T19:48:42Z

src/WebJobs.Script.WebHost/WebScriptHostManager.cs

-        private IDictionary<string, FunctionDescriptor> HttpFunctions { get; set; }
+        private IDictionary<IHttpRoute, FunctionDescriptor> HttpFunctions { get; set; }
+
+        private HttpRouteCollection HttpRoutes { get; set; }


Any reason these are properties instead of fields?

Probably for testing purposes at one time. Changing them to fields, since they're not used from outside.

fabiocav · 2016-10-05T20:36:19Z

sample/HttpTrigger-CSharp-CustomRoute/run.csx

+
+public static ProductInfo Run(ProductInfo info, string category, string id, TraceWriter log)
+{
+    log.Info($"category: {category} id: {id}");


Would be good to have logging showing that ProductInfo was populated as well

fabiocav · 2016-10-05T20:36:44Z

sample/HttpTrigger-CSharp-CustomRoute/function.json

+            "name": "info",
+            "direction": "in",
+            "methods": [ "get" ],
+            "route": "csharp/products/{Category:alpha}/{Id:int}"


These are not case sensitive, correct? Should we have this lower cased?

Correct. I've made it lower.

fabiocav · 2016-10-05T20:41:41Z

src/WebJobs.Script.WebHost/WebScriptHostManager.cs


-                    HttpFunctions.Add(route.ToLowerInvariant(), function);
+                    var httpRoute = _httpRouteFactory.AddRoute(functionName, route, HttpRoutes);


I'm assuming the ambiguous route validation @brettsam mentioned above would be done here. It would be good to make sure that is indeed the case and we have clear exceptions showing that.

fabiocav · 2016-10-05T20:44:36Z

src/WebJobs.Script/Binding/Http/HttpRouteFactory.cs

+        public IEnumerable<string> GetRouteParameters(string routeTemplate)
+        {
+            var routeBuilder = CreateRouteBuilder(routeTemplate);
+            return ParseRouteParameters(routeBuilder.Template);


Is the routeTemplate different than what the routeBuilder.Template property returns here? (does it deal with the constraints in this case?)

Yes, it is different - routeBuilder.Template has all the constraints removed after they are parsed out. I'll add a code comment.

fabiocav · 2016-10-05T20:46:15Z

src/WebJobs.Script/Binding/Http/HttpRouteFactory.cs

+                    if (segment.StartsWith("{") && segment.EndsWith("}"))
+                    {
+                        string parameter = segment.Substring(1, segment.Length - 2);
+                        routeParameters.Add(parameter);


I thought we could get this information from the ASP.NET Routing types, guessing that wasn't the case... :(

No, I looked.

fabiocav · 2016-10-05T20:57:12Z

src/WebJobs.Script/Binding/Http/HttpTriggerAttributeBindingProvider.cs

-                return new TriggerData(valueProvider, bindingData);
+                // create a modifiable collection of binding data and
+                // copy in any initial binding data from the poco
+                Dictionary<string, object> aggregateBindingData = new Dictionary<string, object>(StringComparer.OrdinalIgnoreCase);


fabiocav · 2016-10-05T20:58:40Z

src/WebJobs.Script/Binding/Http/HttpTriggerAttributeBindingProvider.cs

+                aggregateBindingData.AddRange(userTypeBindingData);
+
+                // Apply additional binding data coming from request route, query params, etc.
+                var requestBindingData = await GetRequestBindingDataAsync(request);


Awesome to see this being combined now! :) 👍

fabiocav · 2016-10-05T21:00:17Z

src/WebJobs.Script/Binding/Http/HttpTriggerAttributeBindingProvider.cs

-                    // pull binding data from the query string
-                    var queryParameters = request.GetQueryNameValuePairs();
-                    foreach (var pair in queryParameters)
+                    if (string.Compare("code", pair.Key, StringComparison.OrdinalIgnoreCase) == 0)


Should we be filtering the recently introduced clientid here as well?

Perhaps. However, clientId won't have any secrets, so I don't see it being that important? Maybe there's scenarios for binding to it?

That's a good point. Probably a good idea to let it flow.

fabiocav · 2016-10-05T21:05:42Z

test/WebJobs.Script.Tests/HttpTriggerBindingTests.cs

+
+            Assert.Equal("Ted", poco.Name);
+            Assert.Equal("Seattle", poco.Location);
+            Assert.Equal(25, poco.Age);  // verifies string was converted


fabiocav · 2016-10-05T21:11:01Z

src/WebJobs.Script/Host/ScriptHost.cs

+                if (configSection.TryGetValue("routePrefix", out value))
+                {
+                    routePrefix = (string)value;
+                }


The plan was to prevent any routes from using the "admin" prefix, as among other things, this could have potentially unintended consequences.
Validation and early failure here would be a good idea. Perhaps some tests to make sure those routes are disallowed as well?

mathewc · 2016-10-05T23:23:53Z

Follow up items are tracked via #748 and will be addressed soon.

brettsam · 2016-10-05T23:31:23Z

ahmelsayed added the in progress label Oct 5, 2016

dnfclas added the cla-already-signed label Oct 5, 2016

mathewc commented Oct 5, 2016

View reviewed changes

mathewc force-pushed the http-routes branch from addfbdd to cb4e637 Compare October 5, 2016 17:07

mathewc commented Oct 5, 2016

View reviewed changes

brettsam reviewed Oct 5, 2016

View reviewed changes

Adding support for custom http routes

0f3a271

mathewc force-pushed the http-routes branch from cb4e637 to 0f3a271 Compare October 5, 2016 18:30

mathewc commented Oct 5, 2016

View reviewed changes

brettsam reviewed Oct 5, 2016

View reviewed changes

Adding more tests / addressing CR feedback

d5a2eaa

fabiocav reviewed Oct 5, 2016

View reviewed changes

mathewc mentioned this pull request Oct 5, 2016

HttpRouting Followup Changes #748

Closed

fabiocav approved these changes Oct 5, 2016

View reviewed changes

More CR feedback

7a69c38

mathewc closed this Oct 5, 2016

ahmelsayed removed the in progress label Oct 5, 2016

mathewc mentioned this pull request Oct 21, 2016

Added expressjs style api for http responses #781

Merged

mikezks mentioned this pull request Oct 29, 2016

Customize default route on domain root level #848

Closed

4 tasks

mathewc deleted the http-routes branch December 14, 2016 19:15


		HttpFunctions.Add(route.ToLowerInvariant(), function);
		var httpRoute = _httpRouteFactory.AddRoute(functionName, route, HttpRoutes);

Adding support for custom http routes #743

Adding support for custom http routes #743

Conversation

mathewc commented Oct 5, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathewc Oct 5, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathewc Oct 5, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathewc Oct 5, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathewc Oct 5, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathewc commented Oct 5, 2016

brettsam commented Oct 5, 2016

mathewc commented Oct 5, 2016 •

edited

mathewc Oct 5, 2016 •

edited

mathewc Oct 5, 2016 •

edited

mathewc Oct 5, 2016 •

edited

mathewc Oct 5, 2016 •

edited