Added expressjs style api for http responses #781

mamaso · 2016-10-19T18:47:35Z

resolves #160

Went with a very basic wrapper, can add functionality as necessary but these basic functions cover most use cases.

If there is only one output http binding, we populate context.res, otherwise we populate context.bindings.

Not a breaking change as these response objects can be overwritten with { headers: ..., status: ..., body: ... } objects

mamaso · 2016-10-19T19:45:32Z

src/WebJobs.Script/Binding/HttpBinding.cs

-                        } 
+                        }
+
+                        if (jo.TryGetValue("statusCode", StringComparison.OrdinalIgnoreCase, out value) && value is JValue)


needed to add statusCode so that the response object could use the express status(...) function

Can you merge this if with the previous one?

mathewc · 2016-10-21T16:44:51Z

src/WebJobs.Script/Content/Script/functions.js

+function attachResponseObjects(context) {
+    if (context._httpOutputBindings) {
+        var bindings = context._httpOutputBindings;
+        // if there is only one output http binding use the context.res shortcut


We really only support a single http output binding right now. Not sure if we actually prevent multiple (we should probably prevent this) but multiple won't actually make sense, since there can only be one response. If a user added multiple, our current behavior would be unpredictable. Let's make this a singleton.

yep makes sense, for some reason had it in my head that http output bindings could act as a httpclient and fire off new requests

mathewc · 2016-10-21T16:45:14Z

src/WebJobs.Script/Content/Script/functions.js

+        var bindings = context._httpOutputBindings;
+        // if there is only one output http binding use the context.res shortcut
+        if (bindings.length === 1) {
+            context['res'] = res();


can't this be context.res?

it is context.res, yep

mathewc · 2016-10-21T16:59:56Z

src/WebJobs.Script/Description/Node/NodeFunctionInvoker.cs

+                                                    .Select((FunctionBinding binding) => binding.Metadata.Name)
+                                                    .ToArray();
+
+            context["_httpOutputBindings"] = httpOutputBindings;


can we name this _httpResponseName?

changed so it's a simple boolean _http

mathewc · 2016-10-21T17:03:00Z

src/WebJobs.Script/Description/Node/NodeFunctionInvoker.cs

@@ -371,6 +371,12 @@ protected override void OnScriptFileChanged(object sender, FileSystemEventArgs e

            bindings.Add(_trigger.Name, input);

+            var httpOutputBindings = _outputBindings.Where((FunctionBinding binding) => binding.Metadata.Type == "http")


Since we should just make this a .First(...) since we only really support one currently

make the string comparison OrdinalIgnoreCase

nit: use Linq short form .Where(p => p.Metadata.Type) ...

mathewc · 2016-10-21T18:25:16Z

src/WebJobs.Script/Content/Script/functions.js

@@ -69,6 +70,11 @@ function createFunction(f) {
        inputs.unshift(context);
        delete context._inputs;

+        if (context._http) {


perhaps generalize this to context._triggerType so it can be used for other things in the future? So the invoker adds the trigger type, and we remove the property after use, before passing context to script.

sounds reasonable. I was originally thinking more in terms of output bindings, but as there's 1:1 mapping for httptrigger -> httpoutputbinding this work

mathewc · 2016-10-21T18:31:02Z

src/WebJobs.Script/Description/Node/NodeFunctionInvoker.cs

@@ -371,6 +371,11 @@ protected override void OnScriptFileChanged(object sender, FileSystemEventArgs e

            bindings.Add(_trigger.Name, input);

+            if (_outputBindings.Any((FunctionBinding binding) => binding.Metadata.Type == "http"))


nit: _outputBindings.Any(p => string.Compare(binding.Metadata.Type, "http", StringComparison.OrdinalIgnoreCase) == 0)

not nit: _outputBindings.Any(p => string.Compare(binding.Metadata.Type, "http", StringComparison.OrdinalIgnoreCase) == 0) :)

mathewc · 2016-10-21T18:41:53Z

test/WebJobs.Script.Tests/TestScripts/Node/HttpTriggerExpressApi/index.js

+module.exports = function (context, req) {
+    context.log('Node.js HttpTrigger function invoked.');
+
+    context.res.status(200)


What about a corresponding programming model for request (req)? E.g. Express has a bunch of interesting methods that are applicable like req.accepts(), req.get(), req.params(), etc. I think if we're adding a model for response, we should for requests as well.

yeah, I'll expand this, accepts and get are definitely useful. Correct me if I'm wrong, but we don't support params for httptriggers (params like functionurl/:id, not query params)

Yes we do: see #743

slick, I'll add

I'm already adding captured route params to req.params here

fabiocav · 2016-10-21T18:48:11Z

src/WebJobs.Script/Content/Script/res.js

+// Licensed under the MIT License. See License.txt in the project root for license information.
+
+module.exports = () => {
+    var res = {};


Can we rename the file/module to something more descriptive (e.g. response, httpresponse, funchttpresponse, etc.)?

Pretty standard practice in express: http://expressjs.com/en/api.html#res, but not opposed to changing to response

@mathewc brought that up as well so I think we're on the same page and could move forward with the rename, unless if you feel strongly that it should remain res.

fabiocav · 2016-10-21T18:49:34Z

src/WebJobs.Script/Description/Node/NodeFunctionInvoker.cs

@@ -371,6 +371,11 @@ protected override void OnScriptFileChanged(object sender, FileSystemEventArgs e

            bindings.Add(_trigger.Name, input);

+            if (_outputBindings.Any((FunctionBinding binding) => binding.Metadata.Type == "http"))


not nit: _outputBindings.Any(p => string.Compare(binding.Metadata.Type, "http", StringComparison.OrdinalIgnoreCase) == 0) :)

mathewc · 2016-10-21T20:54:14Z

src/WebJobs.Script/Content/Script/res.js

+        return res;
+    };
+
+    res.send = res.json = (body) => {


Do we need res.end()? Not sure if this is something people expect.

Also, doesn't res.json have behavior (e.g. set application/json content type)? Otherwise, why have two different methods?

So - with single http output res.end is great, because now I can automatically wire up context.done to it.

Debated on what to do for res.json as there's a bunch of logic for processing body type in HttpBinding.cs. To avoid confusion & try to express intent I should set content-type to application/json anyway and then the c# webapi stuff can do its content negotiation.

Express sets text/plain for strings, application/octet-stream for buffer, etc. We should just make sure that these simple helpful behaviors are there. Might be simpler to add these to this node module, as long as our existing logic in httpbinding won't conflict. Also need to make sure overrides work as expected - e.g. if they called set to set content type explicitly, don't set it, etc.

@mamaso , I might be missing your point, but I'm confused about this statement:

... I should set content-type to application/json anyway and then the c# webapi stuff can do its content negotiation.

We need to set the content type to application/json if that content type is what is expected when using express and res.json, and in that case, there would be no conneg. The runtime would honor the content-type set in the function and that would be used no matter what the Accept header stated.

If, on the other hand, we do not set the content-type header when using res.json (and the others mentioned by Mathew above), then we would end up relying on conneg and the result could be unexpected if the intention is to match the express behavior (as what the client specifies in the accept header would be evaluated and you could end up with a different content-type)

This is unrelated to your changes, but before commenting I ran a few tests with our current logic that would trigger content negotiation in Node and we have some issues we need to address. We can chat with the larger group tomorrow and open issues to document them.

mathewc · 2016-10-21T20:54:36Z

src/WebJobs.Script/Content/Script/res.js

+    };
+
+    res.set = res.header = (field, val) => {
+        if (!res.headers)


nit: missing braces

mathewc · 2016-10-21T21:11:26Z

src/WebJobs.Script/Content/Script/functions.js

@@ -69,6 +70,11 @@ function createFunction(f) {
        inputs.unshift(context);
        delete context._inputs;

+        if (context._http) {
+            context.res = res();


Take a look at the crappy logic we have here: https://github.com/Azure/azure-webjobs-sdk-script/blob/dev/src/WebJobs.Script/Binding/HttpBinding.cs#L60 where we're trying to sniff whether the returned object is a json response object v.s. a user defined object. The flaw with the existing logic is that it hinges on a single 'body' property. However, that is faulty if someone tries to return a custom poco with a body property that ISN'T intended to be an http response.

Can your new type help here? E.g. if your context.res module object had a private field that we checked for here, we'd know the intent was for this to be a response object.

ah, so at present to get around that you'd need to nest your poco like...

body: { //poco stuff body: 'poco property' }

I'll think about it, certainly could improve the logic a bit

Here's the question: do we want to make this a breaking change and force everyone to migrate to using the express-like api & response objects?

Otherwise we'll still need to assume that a returned object could be poco or a response object via the above logic, I don't see a way around it.

mathewc · 2016-10-23T19:41:43Z

test/WebJobs.Script.Tests/TestScripts/Node/HttpTriggerExpressApi/index.js

+    context.res.status(200)
+        .set('test-header', 'Test Response Header')
+        .set('Content-Type', 'application/json; charset=utf-8')
+        .send({


In Express, send will actually send the response, right? Whereas here, this is just a builder, and we don't actually send the response until much later, after context.done() is called, and all output bindings are processed?

Should we tie send/end to context.done?

I think that tying send & end to context.done is the right move, it will match the express model. Potential negative is if there are multiple output bindings, could get a little confusing.

yvele · 2016-10-25T13:26:53Z

Should Buffer be supported as a body object? With utf-8 as default encoding? See #814

Express documentation:

The body parameter can be a Buffer object, a String, an object, or an Array.

mamaso · 2016-10-25T16:29:48Z

Yep @yvele, buffer should be supported (and is pretty straightforward https://github.com/expressjs/express/blob/master/lib/response.js#L147)

Edit: unfortunately, not so straightforward due to our node -> c# marshaling. I think I'll push buffers to a later PR

fabiocav

Let's get this in for 0.8 as some of the other changes discussed are a bit out of scope for this PR.

yvele · 2016-11-02T11:04:55Z

Just to let you know that I've open sourced my Azure Function to Express adapter: https://github.com/yvele/azure-function-express 😆

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

ahmelsayed added the in progress label Oct 19, 2016

mamaso commented Oct 19, 2016

View reviewed changes

mathewc reviewed Oct 21, 2016

View reviewed changes

fabiocav reviewed Oct 21, 2016

View reviewed changes

mathewc reviewed Oct 21, 2016

View reviewed changes

src/WebJobs.Script/Content/Script/res.js

};

res.set = res.header = (field, val) => {

if (!res.headers)

Copy link

Member

mathewc Oct 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: missing braces

mathewc reviewed Oct 21, 2016

View reviewed changes

mathewc reviewed Oct 23, 2016

View reviewed changes

yvele mentioned this pull request Oct 25, 2016

[Node] HTTP response with a native Buffer object results gives base64 #814

Closed

fabiocav approved these changes Oct 27, 2016

View reviewed changes

Added expressjs style api for http responses

3381c2e

mamaso force-pushed the expressApi branch from 24d8e41 to 3381c2e Compare October 27, 2016 00:22

mamaso merged commit 50f7de2 into Azure:dev Oct 27, 2016

ahmelsayed removed the in progress label Oct 27, 2016

kshmir mentioned this pull request Nov 14, 2016

Support streams in NodeJS #660

Closed

yvele mentioned this pull request Oct 18, 2017

Support for multi-part file uploads yvele/azure-function-express#8

Open

		@@ -371,6 +371,12 @@ protected override void OnScriptFileChanged(object sender, FileSystemEventArgs e

		bindings.Add(_trigger.Name, input);

		var httpOutputBindings = _outputBindings.Where((FunctionBinding binding) => binding.Metadata.Type == "http")

		@@ -371,6 +371,11 @@ protected override void OnScriptFileChanged(object sender, FileSystemEventArgs e

		bindings.Add(_trigger.Name, input);

		if (_outputBindings.Any((FunctionBinding binding) => binding.Metadata.Type == "http"))

Added expressjs style api for http responses #781

Added expressjs style api for http responses #781

Conversation

mamaso commented Oct 19, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathewc Oct 21, 2016 • edited Loading

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 Oct 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mamaso Oct 21, 2016 • edited Loading

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 21, 2016 • edited Loading

Choose a reason for hiding this comment

mamaso Oct 21, 2016 • edited Loading

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

yvele commented Oct 25, 2016

mamaso commented Oct 25, 2016 • edited Loading

fabiocav left a comment

Choose a reason for hiding this comment

yvele commented Nov 2, 2016

mamaso commented Oct 19, 2016 •

edited

Loading

mathewc Oct 21, 2016 •

edited

Loading

mathewc Oct 21, 2016 •

edited

Loading

mamaso Oct 21, 2016 •

edited

Loading

mathewc Oct 21, 2016 •

edited

Loading

mamaso Oct 21, 2016 •

edited

Loading

mamaso commented Oct 25, 2016 •

edited

Loading