Skip to content

feat: handle CloudEvent and Message responses from function invocation #68

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Oct 30, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 7 additions & 10 deletions lib/context.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
const Spec = require('../lib/ce-constants.js').Spec;
const { CloudEvent } = require('cloudevents');

class Context {
constructor(request) {
Expand All @@ -24,34 +24,31 @@ class CloudEventResponse {
#response;

constructor(response) {
this.#response = response;
if (!this.#response.headers) {
this.#response.headers = [];
}
this.#response = { data: response };
}

version(version) {
this.#response.headers[Spec.version] = version;
this.#response.specversion = version;
return this;
}

id(id) {
this.#response.headers[Spec.id] = id;
this.#response.id = id;
return this;
}

type(type) {
this.#response.headers[Spec.type] = type;
this.#response.type = type;
return this;
}

source(source) {
this.#response.headers[Spec.source] = source;
this.#response.source = source;
return this;
}

response() {
return this.#response;
return new CloudEvent(this.#response);
}
}

Expand Down
53 changes: 41 additions & 12 deletions lib/invoker.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
'use strict';
const { CloudEvent, HTTP } = require('cloudevents');

module.exports = function invoker(func) {
return async function invokeFunction(context, log) {
Expand All @@ -7,10 +8,10 @@ module.exports = function invoker(func) {
code: 200,
response: undefined,
headers: {
'Content-Type': 'application/json; charset=utf8',
'Access-Control-Allow-Methods':
'content-type': 'application/json; charset=utf8',
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change, shouldn't it be case insensitive?

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right - it should be, and it is here where we merge user headers with event headers https://github.com/boson-project/faas-js-runtime/pull/68/files#diff-89ae878b1a78765add577244271796f156213cdd0ab6722f641ef96926f38674R54.

That change to lowercase is kind of immaterial since we do lowercase all headers later. It was left over from some debugging I was doing when I realized that I was not doing case-insensitive comparisons. But I thought it would be fine to keep. 🤷

'access-control-allow-methods':
'OPTIONS, GET, DELETE, POST, PUT, HEAD, PATCH',
'Access-Control-Allow-Origin': '*'
'access-control-allow-origin': '*'
}
};

Expand All @@ -25,11 +26,7 @@ module.exports = function invoker(func) {
payload.response = await func(context);
}
} catch (err) {
log.error(err);
payload.response = {
statusCode: err.code ? err.code : 500,
statusMessage: err.message
};
payload.response = handleError(err, log);
}

// Return 204 No Content if the function returns
Expand All @@ -39,17 +36,49 @@ module.exports = function invoker(func) {
return payload;
}

// Check for user defined headers
if (typeof payload.response.headers === 'object') {
const headers = {};
// normalize the headers as lowercase
for (const header in payload.response.headers) {
headers[header.toLocaleLowerCase()] = payload.response.headers[header];
}
payload.headers = { ...payload.headers, ...headers };
delete payload.response.headers;
}

// If the response is a CloudEvent, we need to convert it
// to a Message first and respond with the headers/body
if (payload.response instanceof CloudEvent) {
try {
const message = HTTP.binary(payload.response);
payload.headers = {...payload.headers, ...message.headers};
payload.response = message.body;
} catch (err) {
payload.response = handleError(err, log);
return payload;
}
}

// Check for user defined status code
if (payload.response.statusCode) {
payload.code = payload.response.statusCode;
delete payload.response.statusCode;
}

// Check for user defined headers
if (typeof payload.response.headers === 'object') {
Object.assign(payload.headers, payload.response.headers);
delete payload.response.headers;
// Check for user supplied body
if (payload.response.body !== undefined) {
payload.response = payload.response.body;
delete payload.response.body;
}
return payload;
};
};

function handleError(err, log) {
log.error(err);
return {
statusCode: err.code ? err.code : 500,
statusMessage: err.message
};
}
3 changes: 2 additions & 1 deletion lib/request-handler.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ module.exports = function(fastify, opts, done) {
};

function sendReply(reply, payload) {
if (payload.headers['Content-Type'].startsWith('text/plain')) {
const contentType = payload.headers['content-type'];
if (contentType.startsWith('text/plain') && (typeof payload.response !== 'string')) {
payload.response = JSON.stringify(payload.response);
}
return reply
Expand Down
20 changes: 10 additions & 10 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
"bin": "./bin/cli.js",
"dependencies": {
"chalk": "^4.1.0",
"cloudevents": "^3.1.0",
"cloudevents": "^3.2.0",
"commander": "^6.1.0",
"death": "^1.1.0",
"fastify": "^3.3.0",
Expand Down
2 changes: 1 addition & 1 deletion test/fixtures/query-params/index.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
module.exports = function testFunc(context) {
return context;
return { ...context.query };
};
4 changes: 2 additions & 2 deletions test/test-context.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,8 @@ test('Provides HTTP request query parameters with the context parameter', t => {
.expect('Content-Type', /json/)
.end((err, res) => {
t.error(err, 'No error');
t.equal(res.body.query.lunch, 'tacos');
t.equal(res.body.query.supper, 'burgers');
t.equal(res.body.lunch, 'tacos');
t.equal(res.body.supper, 'burgers');
t.end();
server.close();
});
Expand Down
77 changes: 57 additions & 20 deletions test/test.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ const Spec = require('../lib/ce-constants.js').Spec;
const { existsSync, readdirSync } = require('fs');
const { execSync } = require('child_process');
const path = require('path');
const { CloudEvent } = require('cloudevents');
const { CloudEvent, HTTP } = require('cloudevents');

// Ensure fixture dependencies are installed
const fixtureDir = path.join(__dirname, 'fixtures');
Expand Down Expand Up @@ -173,6 +173,62 @@ test('Responds to 1.0 structured cloud events', t => {
}, { log: false });
});

test('Handles 1.0 CloudEvent responses', t => {
framework(_ => {
return new CloudEvent({
source: 'test',
type: 'test-type',
data: 'some data',
datacontenttype: 'text/plain'
});
}, server => {
request(server)
.post('/')
.send({ message: 'hello' })
.set(Spec.id, '1')
.set(Spec.source, 'integration-test')
.set(Spec.type, 'dev.knative.example')
.set(Spec.version, '1.0')
.expect(200)
.expect('Content-Type', /text/)
.end((err, res) => {
t.error(err, 'No error');
t.equal(res.text, 'some data');
t.end();
server.close();
});
},
{ log: false });
});

test('Handles 1.0 CloudEvent Message responses', t => {
framework(_ => {
return HTTP.binary(new CloudEvent({
source: 'test',
type: 'test-type',
data: 'some data',
datacontenttype: 'text/plain'
}));
}, server => {
request(server)
.post('/')
.send({ message: 'hello' })
.set(Spec.id, '1')
.set(Spec.source, 'integration-test')
.set(Spec.type, 'dev.knative.example')
.set(Spec.version, '1.0')
.expect(200)
.expect('Content-Type', /text/)
.end((err, res) => {
t.error(err, 'No error');
t.equal(res.text, 'some data');
t.end();
server.close();
});
},
{ log: false });
});

test('Extracts event data as the first parameter to a function', t => {
const data = {
lunch: "tacos"
Expand Down Expand Up @@ -226,25 +282,6 @@ test('Successfully handles events with no data', t => {
});
});

test('Responds with error code (4xx or 5xx) to malformed cloud events', t => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why deleting this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I probably should have mentioned that in the description. I don't think we should fail if we can help it. If we can recover by creating a CE that is correct, in spite of the malformation, then I think we should do that.

Copy link
Member Author

@lance lance Oct 30, 2020

Choose a reason for hiding this comment

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

@matejvasek I will elaborate a little more here. The rationale behind this change is rooted in changes that were made in the upstream cloudevents module and which recently landed in v3.2.0. That version bump of the module introduced the HTTP.binary() API method (and others) that I have made use of in this PR.

But a bigger thing that it did was change how the receiver handles malformed events. A summary of the wider community discussion on why receiving a CloudEvent should provide loose validation is in this comment.

Because of this change in the upstream validation behavior, an exception is never thrown on our end when receiving an event. In cloudevents 3.1.x it would have been and was, and was why this test worked. However, in order to maintain the existing behavior in this module, I would need to add a call to event.validate() when receiving them. To me that seems to land more in the user/customer domain. I thought a lot about this while making the upstream changes, and it's been pretty well hashed out in the community. So I think this module should follow the behavior upstream.

const func = require(`${__dirname}/fixtures/cloud-event/`);
framework(func, server => {
request(server)
.post('/')
.send({ message: 'hello' })
.set(Spec.id, '1')
.set(Spec.source, 'integration-test')
.set(Spec.version, '0.3')
.set('ce-datacontenttype', 'application/json')
.expect('Content-Type', /json/)
.end((err, res) => {
t.assert(res.statusCode >= 400 && res.statusCode <= 599, 'Error code 4xx or 5xx expected.');
t.end();
server.close();
});
}, { log: false });
});

test('Responds with 406 Not Acceptable to unknown cloud event versions', t => {
const func = require(`${__dirname}/fixtures/cloud-event/`);
framework(func, server => {
Expand Down