feat: implement CloudEvent down Conversion & Adds Tests #262

grant · 2021-03-23T20:25:20Z

Refactors event conversion and implements partial event conversion.

Enables linting on full source code
Adds many tests to invoker
Implements CE downcasting
Refactors CloudEvent conversion
No external changes to existing functions interface

Test Output

CloudEvents: Utility methods
    ✓ should correctly eval if a CloudEvent is "binary"
    ✓ should correctly eval if a CloudEvent is "structured"
    ✓ should convert requests to a structured CE
    ✓ should get a binary CE from a request

  Invoker: CE -> CE
    ✓ should receive cloudevent from CloudEvents v1.0 structured content mode (39ms)
    ✓ should receive cloudevent from CloudEvents v1.0 binary content mode
    ✓ should receive cloudevent from CloudEvents v1.0 binary content mode with only required fields

  EventConverter: CE -> Legacy
    ✓ should downcast events

  Invoker: CE -> Legacy
    ✓ should receive data and context from CloudEvents v1.0 structured content mode

  EventConverter: Legacy -> CE
    ✓ should upcast Pub/Sub events
    ✓ should upcast legacy Pub/Sub events
    ✓ should upcast Firebase Auth events

  Invoker: HTTP
    ✓ should return the correct HTTP status and call count: empty path
    ✓ should return the correct HTTP status and call count: simple path
    ✓ should return the correct HTTP status and call count: with favicon.ico
    ✓ should return the correct HTTP status and call count: with robots.txt

  Invoker: Legacy Event -> Legacy Event
    ✓ should receive data and context from GCF event
    ✓ should receive data and context from GCF legacy event
    ✓ should receive data and context from GCF event with resource JSON

  Function loader
    ✓ should load function without "main" in package.json
    ✓ should load function with "main" in package.json


  21 passing (107ms)

(Reference) Previous Test Output

request to HTTP function
    ✓ should return transformed body: empty path (43ms)
    ✓ should return transformed body: simple path
    ✓ should return transformed body: with favicon.ico
    ✓ should return transformed body: with robots.txt

  GCF event request to event function
    ✓ should receive data and context from GCF event
    ✓ should receive data and context from GCF legacy event
    ✓ should receive data and context from GCF event with resource JSON

  CloudEvents request to event function
    ✓ should receive data and context from CloudEvents v1.0 structured content mode
    ✓ should receive data and context from CloudEvents v1.0 binary content mode

  CloudEvents request to cloudevent function
    ✓ should receive data and context from CloudEvents v1.0 structured content mode
    ✓ should receive data and context from CloudEvents v1.0 binary content mode

  loading function
    ✓ should load function without "main" in package.json
    ✓ should load function with "main" in package.json


  13 passing (99ms)

Signed-off-by: Grant Timmerman <timmerman+devrel@google.com>

grant · 2021-04-06T23:13:37Z

Should be ready to review @mtraver. Pardon the large PR.
We can move parts of the changes to a separate PR is needed.

matthewrobertson · 2021-04-13T19:35:20Z

.github/workflows/conformance.yml

      with:
        functionType: 'http'
+        useBuildpacks: false


It seems like this should be false by default, no? Any reason to add it in this PR?

Yeah, it's not needed. We can remove.

matthewrobertson · 2021-04-13T19:40:15Z

package.json

    "check": "gts check",
    "clean": "gts clean",
    "compile": "tsc -p .",
+    "watch": "tsc -p . -w",


maybe DRY this up with:

"watch": "npm run compile -- --watch",

matthewrobertson · 2021-04-13T19:43:46Z

src/cloudevents.ts

+    return ce;
+  } else {
+    // Structured event. Just return the event, which is stored in the HTTP body.
+    return req.body;


how does this pass the typechecker right now?

matthewrobertson · 2021-04-13T20:02:18Z

src/cloudevents.ts

+ * @return True if the request is a CloudEvents event in structured content mode,
+ *     false otherwise.
+ */
+export function isStructuredCloudEvent(req: express.Request): boolean {


Is there an opportunity to use a type predicate here? Something like:

interface StructuredCloudEventRequest extends express.Request { body: CloudEventsContext; } export function isStructuredCloudEvent(req: express.Request) req is StructuredCloudEventRequest

matthewrobertson · 2021-04-13T20:32:15Z

test/legacyEvents.ts

@@ -0,0 +1,113 @@
+// Copyright 2021 Google LLC


as above: test/integration/legacyEvents.ts?

matthewrobertson · 2021-04-13T20:47:09Z

test/eventConverterDowncast.ts

+//   TEST_CLOUDEVENT_STRUCTURED,
+// } from './data/testHTTPData';
+
+describe('EventConverter: CE -> Legacy', () => {


as below, if you programatically skip, you don't have comment out the imports:

describe.skip('EventConverter: CE -> Legacy', () => {

matthewrobertson · 2021-04-13T20:56:24Z

src/cloudevents.ts

+    const ce: CloudEventsContext = {
+      specversion: req.header('ce-specversion'),
+      type: req.header('ce-type'),
+      source: req.header('ce-source'),
+      subject: req.header('ce-subject'),
+      id: req.header('ce-id'),
+      time: req.header('ce-time'),
+      datacontenttype: req.header('ce-datacontenttype'),
+      data: req.body,
+    };
+    // Remove undefined keys from CE object
+    Object.keys(ce).forEach((key: string) =>
+      ce[key] === undefined ? delete ce[key] : {}
+    );
+    return ce;


I might write this a bit differently. Rather than going through and deleting undefined keys, just don't set them in the first place:

const ce: CloudEventsContext = { data: req.body, }; [ 'specversion', 'type', 'source', 'subject', 'id', 'time', 'datacontenttype', ].forEach((key) => { if (req.header(`ce-${key}`) !== undefined) { ce[key] = req.header(`ce-${key}`); } }

even better would be to move that array to a constant somewhere

matthewrobertson · 2021-04-13T21:09:43Z

src/cloudevents.ts

 /**
 * Returns a CloudEvents context from the given CloudEvents request. Context
 * attributes are retrieved from request headers.
 *
 * @param req Express request object.
 * @return CloudEvents context.
 */
-export function getBinaryCloudEventContext(
+export function getStructuredCloudEventContext(


I am a bit confused about how this function is different from convertRequestToStructuredCE

matthewrobertson · 2021-04-13T21:31:02Z

src/invoker.ts

+        // Set data and context normally
+        data = req.body.data;
+        context = req.body.context;


Looks like these variables are already initialized to exactly these values on line 186/187. Can we just remove the else block here or remove the initial values above?

matthewrobertson · 2021-04-13T21:31:42Z

src/invoker.ts

+    } else {
+      // If a really legacy event, convert it to a normal event.
+      // See the tests for more details.
+      if (!context) {


nit: flatten to } else if (!context) {

matthewrobertson

Formally requesting changes because I am sure if you saw my review.

grant · 2021-04-19T22:20:34Z

Thanks. I'll have to apply the comments and likely break this PR into some smaller PRs.

Signed-off-by: Grant Timmerman <timmerman+devrel@google.com>

grant · 2021-04-21T17:51:54Z

I've pulled out a few changes into separate PRs. I think it will make things cleaner and safer:

Signed-off-by: Grant Timmerman <timmerman+devrel@google.com>

grant · 2021-05-06T19:37:21Z

Going to close this PR. Other PRs will be able to complete the conversions. Feel free to ping me if there are questions.

grant added 3 commits March 23, 2021 12:05

ci: bump conforamnce version

939f072

Signed-off-by: Grant Timmerman <timmerman+devrel@google.com>

ci: run all the time

05d20b2

Signed-off-by: Grant Timmerman <timmerman+devrel@google.com>

ci: validate mapping cloudevent

a208e49

Signed-off-by: Grant Timmerman <timmerman+devrel@google.com>

grant self-assigned this Mar 23, 2021

google-cla bot added the cla: yes label Mar 23, 2021

grant added 6 commits March 31, 2021 12:20

feat: partial ce support

b4f3a45

Signed-off-by: Grant Timmerman <timmerman+devrel@google.com>

fix: improve the whole test suite

d2b6adc

Signed-off-by: Grant Timmerman <timmerman+devrel@google.com>

feat: update event conversions

114d235

Signed-off-by: Grant Timmerman <timmerman+devrel@google.com>

ci: do not use buildpacks

81329a9

Signed-off-by: Grant Timmerman <timmerman+devrel@google.com>

ci: pass linter

1e37372

Signed-off-by: Grant Timmerman <timmerman+devrel@google.com>

fchore: disable console log

f5c8d94

Signed-off-by: Grant Timmerman <timmerman+devrel@google.com>

grant changed the title ~~ci: conformance action 2~~ feat: implement CloudEvent down Conversion & Adds Tests Apr 6, 2021

grant requested a review from mtraver April 6, 2021 23:11

grant marked this pull request as ready for review April 6, 2021 23:11

grant requested a review from matthewrobertson April 8, 2021 18:04

matthewrobertson reviewed Apr 13, 2021

View reviewed changes

matthewrobertson requested changes Apr 19, 2021

View reviewed changes

ci: use it.skip

13b82c8

Signed-off-by: Grant Timmerman <timmerman+devrel@google.com>

revert: undo changes pulled into separate prs

b378ccf

Signed-off-by: Grant Timmerman <timmerman+devrel@google.com>

grant force-pushed the grant_ci_update_2 branch from acb2053 to b378ccf Compare April 21, 2021 18:01

grant added 2 commits April 21, 2021 13:03

revert: undo changes pulled into separate prs

f151018

Signed-off-by: Grant Timmerman <timmerman+devrel@google.com>

revert: undo changes pulled into separate prs

e696ee1

Signed-off-by: Grant Timmerman <timmerman+devrel@google.com>

grant closed this May 6, 2021

grant deleted the grant_ci_update_2 branch May 6, 2021 19:37

feat: implement CloudEvent down Conversion & Adds Tests #262

feat: implement CloudEvent down Conversion & Adds Tests #262

Uh oh!

Conversation

grant commented Mar 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Output

(Reference) Previous Test Output

Uh oh!

grant commented Apr 6, 2021

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!

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!

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!

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!

matthewrobertson left a comment

Choose a reason for hiding this comment

Uh oh!

grant commented Apr 19, 2021

Uh oh!

grant commented Apr 21, 2021

Uh oh!

grant commented May 6, 2021

Uh oh!

Uh oh!

grant commented Mar 23, 2021 •

edited

Loading