refactor: add class that wraps CloudEvent into CloudEventInterface #113
Conversation
anniefu
requested review from
grant,
KaylaNguyen,
matthewrobertson and
bshaffer
This is groundwork to enabling function signatures that expect the official PHP CloudEvent SDKs [CloudEventInterface](https://github.com/cloudevents/sdk-php/blob/master/src/V1/CloudEventInterface.php) instead of the Function Framework's hand-rolled class.
| * Wraps a Google\CloudFunctions\CloudEvent to comply with | ||
| * CloudEvents\V1\CloudEventInterface. | ||
| */ | ||
| class CloudEventSdkCompliant implements JsonSerializable, CloudEventInterface |
There was a problem hiding this comment.
nit: newline spacing is inconsistent with these functions
There was a problem hiding this comment.
I'm not sure how this is going to be used.
Do we want to implement CloudEventImmutable in addition to CloudEventInterface?
There was a problem hiding this comment.
I think it's the getters don't have newlines in between them, but other functions do.
CloudEventInterface is going to be what's passed to CloudEvent Functions with the new declarative function signatures. This is similar to how ServerRequestInterface is passed to HTTP functions today.
CloudEventImmutable is not an interface, so we can't implement it. I don't think we would benefit from inheriting or using it within this class, since we are not using any other part of the CloudEvents SDK today that can serialize/deserialize to CloudEvent or CloudEventImmutable.
Using the CloudEventInterface will put us a step in the right direction, because it means we can swap our implementation to use the official CloudEvent SDK in the future, without rewiring everything all at once.
There was a problem hiding this comment.
Here's the next PR queue'd up which makes use of this new class, you can see the new function stub in this test:
https://github.com/anniefu/functions-framework-php/pull/1/files#diff-479b688739465688c5b60552045cb322a656e9bd47a2b9b12a05f8c4f76d3bd6R74
| $this->cloudevent = new CloudEvent( | ||
| '1413058901901494', | ||
| '//pubsub.googleapis.com/projects/MY-PROJECT/topics/MY-TOPIC', | ||
| '1.0', | ||
| 'com.google.cloud.pubsub.topic.publish', | ||
| 'application/json', | ||
| 'type.googleapis.com/google.logging.v2.LogEntry', | ||
| 'My Subject', | ||
| '2020-12-08T20:03:19.162Z', | ||
| [ | ||
| "message" => [ | ||
| "data" => "SGVsbG8gdGhlcmU=", | ||
| "messageId" => "1408577928008405", | ||
| "publishTime" => "2020-08-06T22:31:14.536Z" | ||
| ], | ||
| "subscription" => "projects/MY-PROJECT/subscriptions/MY-SUB" | ||
| ] | ||
| ); |
There was a problem hiding this comment.
Rather than using the constructor, which is pretty fragile given the number of arguments (and is not copy-pastable), could we deserialized a JSON payload?
JsonDeserializer::create()->deserializeStructured($payload);
There was a problem hiding this comment.
The CloudEvents SDK can't parse this payload due to a bug. I filed a bug on them: cloudevents/sdk-php#35
Until they fix it, the best we can do is provide the same interface.
There was a problem hiding this comment.
Should we add a TODO, or do you think using this constructor is fine?
| $this->cloudevent = new CloudEvent( | ||
| '1413058901901494', | ||
| '//pubsub.googleapis.com/projects/MY-PROJECT/topics/MY-TOPIC', | ||
| '1.0', | ||
| 'com.google.cloud.pubsub.topic.publish', | ||
| 'application/json', | ||
| 'type.googleapis.com/google.logging.v2.LogEntry', | ||
| 'My Subject', | ||
| '2020-12-08T20:03:19.162Z', | ||
| [ | ||
| "message" => [ | ||
| "data" => "SGVsbG8gdGhlcmU=", | ||
| "messageId" => "1408577928008405", | ||
| "publishTime" => "2020-08-06T22:31:14.536Z" | ||
| ], | ||
| "subscription" => "projects/MY-PROJECT/subscriptions/MY-SUB" | ||
| ] | ||
| ); |
There was a problem hiding this comment.
Should we add a TODO, or do you think using this constructor is fine?
|
I think that constructor's fine, it's internal only in practice. We should move onto the CloudEvents SDK if we can, so we shouldn't invest too much time in improving our FF implementation unless we have to. |
|
Forcing the merge because the it's only waiting on PHP 7.2/7.3 workflows that were removed. |
This is groundwork to enabling function signatures that expect the
official PHP CloudEvent SDKs CloudEventInterface
instead of the Function Framework's hand-rolled class.
This also gives us an interface to support CloudEvent extensions properly #112