openfaas / faas

OpenFaaS - Serverless Functions Made Simple
https://www.openfaas.com
MIT License
25.16k stars 1.94k forks source link

Web UI shows undefined as response body #528

Closed maiermic closed 6 years ago

maiermic commented 6 years ago

Steps to Reproduce

Run the Web UI as decribed in Deployment guide for Docker Swarm

$ git clone https://github.com/openfaas/faas && \
  cd faas && \
  git checkout 0.7.0 && \
  ./deploy_stack.sh

Create Node.js function:

faas-cli new ui-bug --lang node

handler.js

"use strict"

module.exports = (context, callback) => {
    callback(false, '[\n  "foo" => "foo"\n]');
}
faas-cli build -f ui-bug.yml
faas-cli deploy -f ui-bug.yml

Open http://localhost:8080/ui/ and invoke a Text request for ui-bug function.

Note: Chrome Dev Tools shows response with response header Content-Type:text/plain and body

[
  "foo" => "foo"
]

Expected Behaviour

Web UI should show response body:

[
  "foo" => "foo"
]

Current Behaviour

Web UI shows response body:

undefined
undefined

Your Environment

alexellis commented 6 years ago

Hi thank you for filling out the whole template. I’d suggest you return an object and not a string as the response to the callback. Alex

alexellis commented 6 years ago

Derek add label: question

maiermic commented 6 years ago

I'm kind of confused. Why should I return an object? Do I always have to return an object? What kind of object is expected? I'd like to have plain text as response and not JSON. That's why I return a string in the handler and not an object. The response header is as I expect Content-Type:text/plain and not Content-Type:application/json. Does OpenFaaS (the Web UI) always expect plain text to be a sting in JSON format? I don't think so. I actually expect this to be a bug in the Web UI.

alexellis commented 6 years ago
$ cat ./nodetext/handler.js 
"use strict"

module.exports = (context, callback) => {
    callback(undefined, "Status is done");
}
screen shot 2018-02-22 at 8 56 16 am

This is what I'm getting when I return a string from the handler - same in Chrome and Safari.

alexellis commented 6 years ago

The default response type for the Node template is text/plain - you can override it via an environmental variable in your YAML file or at deployment time if you do want to change it to something else.

screen shot 2018-02-22 at 9 01 58 am
alexellis commented 6 years ago

@maiermic there may be an issue in Angular (not OpenFaaS) that means the framework tries to parse anything that looks remotely like JSON - even when it's not valid JSON. Let's see if the folks on the angular project are able to help.

Bottom line:

I'd like to fix your specific use-case if it is possible within the bounds on angular, but consider this low priority given the above points. cc @jrevillas @kenfdev

alexellis commented 6 years ago

One more thing - you return false as an error.. you should not do this in a purist view. The error should be null or undefined when there is no error. False may evaluate to negative with the ! operator - but best to be explicit.

maiermic commented 6 years ago

I'd like to fix your specific use-case if it is possible within the bounds on angular, but consider this low priority given the above points.

I totally agree. If it is a bug in angular, we still have to update the dependency when it is fixed.

One more thing - you return false as an error.

I see room for improvement in the documentation for handler.js. I had a look at the default template because I didn't know what to return as first argument. I saw it's the error parameter, but I still don't know exactly what kind of value is expected. I guess it has to be an error message, but maybe this error should be in a specific format so that it can be processed.

Have you considered to use (context: string) => Promise<string | object> instead of (context: string, callback: (error: any, result: string | object) => void) => void as type of the exported handler function?

jrevillas commented 6 years ago

Thanks for the report, @maiermic. This is something I saw when working on #398. As @alexellis said, Angular's $http service transforms response data when possible. I will do some research and tests to consolidate the experience in both CLI and UI.

kenfdev commented 6 years ago

@jrevillas I had some time so did a quick research.

As from this comment (Alex, you're fast) https://github.com/angular/angular.js/issues/15897#issuecomment-367672775

It seems like upgrading AngularJS to 1.6.6 or greater would solve this problem. I did a quick check and confirmed that it is working as below:

image

Some considerations are that there are breaking changes to the AngularJS API between 1.5 to 1.6 as mentioned here.

For example

$http's deprecated custom callback methods - success() and error() - have been removed. You can use the standard then()/catch() promise methods instead, but note that the method signatures and return values are different.

The UI is using the success method so we'll have to change it.

I think we should perhaps add another issue about migrating to 1.6 and consider what we need to change.

alexellis commented 6 years ago

Great input from #TeamServerless there - thank you.

@kenfdev or @jrevillas please could you co-ordinate and:

Whilst working on this - test out the store / manual deployment and auto-refresh behavior to see there is any regression or whether there are other concerns about moving up in version.

kenfdev commented 6 years ago

Raise an issue to migrate to 1.6.6 (ideally with the same CDN if available) Raise a PR changing the success/error promise to then/catch

I can work on this today

alexellis commented 6 years ago

@maiermic this has been fixed and is good to go in 0.7.2 - just do:

cd faas
git fetch origin
git checkout 0.7.2
./deploy_stack.sh

That should be it. Thank you for logging the issue.

alexellis commented 6 years ago
screen shot 2018-02-25 at 13 01 34

This is the function running in the new version by @kenfdev ^

maiermic commented 6 years ago

@alexellis @kenfdev Thank you very much :+1: