add json serialization by duglin · Pull Request #124 · cloudevents/spec

duglin · 2018-03-24T03:15:49Z

Closes #17

I would actually prefer if we just move this into the main spec itself since its so small, but for now I started out with it being in a separate doc with the assumption that we can decide on whether we want to merge it later.

Signed-off-by: Doug Davis dug@us.ibm.com

SeanFeldman

A few minor comments.

SeanFeldman · 2018-03-24T14:42:06Z

+  "cloud-events-version": "cloud-events-version value",
+  "event-id": "event-id value",
+  "namespace": "namespace value",
+  "source": {


@duglin isn't source a URI?

Not yet :-)

Fair enough. Pending #123 🙂

SeanFeldman · 2018-03-24T14:43:06Z

+}
+```
+
+Note:


nitpicking - sais Note, but these are notes.

SeanFeldman · 2018-03-24T14:43:41Z

+```
+
+Note:
+- the use of whitespace is not significant


Capitalize?

markpeek · 2018-03-24T18:41:36Z

@@ -0,0 +1,43 @@
+# CloudEvents Serialization Profile - Version 0.1
+
+This document specified how a CloudEvent is to be serialized into certain


Should this be applied here? s/specified/specifies/

fixed the "specified" typo. What did you mean by "should this applied here?" ?

markpeek · 2018-03-24T18:54:23Z

+}
+```
+
+Notes:


Should there be a comment about extra fields in the serialized data? Such as:

- It is undefined whether extra fields are passed along or removed depending on the usage role and implementation. Those fields are best placed in the extensions map.

I expanded on the thought a little - see what you think.

Thanks for the update. LGTM.

markpeek · 2018-03-24T21:41:03Z

+}
+```
+
+Notes:


Thanks for the update. LGTM.

markpeek · 2018-03-24T21:44:34Z

 CloudEvents seeks to ease event declaration and delivery across services,
 platforms and beyond.

+The [Serialization Profile](serialization.md) specified how to


This might be better as "specifies". (Or am I being too tense?)

No, you're right - just a disconnect between the brain and fingers.

inlined · 2018-03-28T22:16:51Z

+
+```
+{
+  "cloud-events-version": "cloud-events-version value",


I'm curious whether it would be more useful to use valid fake values rather than prose in quotes.

I struggled with this because if we put fake values in there then we need something to say "property X from the spec goes into json property 'x'". While in many cases it should be really really obvious, I felt that a spec should be more explicit about it. If all we cared about was JSON I actually would have proposed that we have the serialization of each property next to its definition in the spec itself. But I didn't want to assume everyone would be ok with JSON getting special treatment.

@inlined I added an example to try to address your concern. See if that helps.

inlined · 2018-03-28T22:21:17Z

+  "extensions": {
+    ... extensions values ...
+  },
+  "data": ... data value ...


As an FYI: though Google&Firebase started out with a model like this where all context is at the top level with one magic field called "data", we generally considered this to be a mistake. Many languages have better ways to expose context (e.g. Golang's Context.context, or JavaScript's variadic parameter system) so we are migrating to a world where data & context are exposed separately/natively our runtimes. Alpha testers have given very positive feedback for this because it allows any existing function(SomeNativeType) to be a CloudFunction<SomeNativeType>.

To match this, the REST JSON is being migrated to a two-field JSON object with {"context", "data"}

I could live with that. What do other people think?

@inlined would extensions be under context or another sibling at the root level?

@inlined where would "could-events-version" go? I can see it being useful as a top-level thing since we may choose to rename data or context in a future version, but I can also see a need for that info to be passed downstream so people know how to interpret the context attributes based on our spec's version number.

I do want to explore this option, it feels cleaner - would make for a great follow-on PR

@duglin I agree with @inlined that context and data should be on the same level as he suggested.

One more thing:
Do we need rules how to encode data, if the content-type is something binary or text/xml?

would that be something that the definition of the content-type (e.g. json spec) would need to define?

afaik json spec does not cover this explicitly. Most people seem to base64 encode data.

yea, I did a quick search too and didn't find a lot. I guess we could offer some recommendations...

Honestly I don't have the practical experience with content-type and cloud-events-version that I do with the other fields. Here's my first SWAG:

In my opinion, content-type is/should be meta to the whole spec (why are we dictating the content type within a payload that's already explicitly JSON encoded?). This should be dictated in the transport layer, not the encoding layer.

cloud-events-version looks cleanest at the top level in JSON though if it's going to show up in the programming model it might be easier to put it in context.

I have the strong opinion (weakly held) that extensions should be in context. The developer is likely to depend on these extensions and they should be easily accessible.

@inlined My understanding is that this PR is about how to package an arbitrary cloudevent into json. I was just wondering what happens if the content-type (the context attribute) is text/xml or image/png. The result is supposed to be a valid json object. So data would need to be encoded somehow. I don't see how this can be delegated to another layer. For events that use json for their data section, there is of course no need for encoding.

Signed-off-by: Doug Davis <dug@us.ibm.com>

duglin · 2018-03-29T13:21:07Z

Added an example and one line to make it clear that optional fields are still optional, but if present then they MUST adhere to the rules defined. I think it was implied before but now its explicitly stated.

ultrasaurus · 2018-03-29T16:15:47Z

This will have to be modified to be consistent with changed accepted today

duglin · 2018-03-29T17:45:22Z

Approved on 3/29 call

duglin force-pushed the serialize branch 2 times, most recently from 9a6e2c7 to 3d5a1cd Compare March 24, 2018 03:53

SeanFeldman reviewed Mar 24, 2018

View reviewed changes

duglin force-pushed the serialize branch 2 times, most recently from 349dab1 to c4fd2d5 Compare March 24, 2018 14:47

markpeek reviewed Mar 24, 2018

View reviewed changes

duglin force-pushed the serialize branch from c4fd2d5 to 588f5f8 Compare March 24, 2018 19:12

markpeek reviewed Mar 24, 2018

View reviewed changes

duglin force-pushed the serialize branch from 588f5f8 to a94019c Compare March 24, 2018 23:14

inlined reviewed Mar 28, 2018

View reviewed changes

add json serialization

6e74891

Signed-off-by: Doug Davis <dug@us.ibm.com>

duglin force-pushed the serialize branch from eac2547 to 08e2f31 Compare March 29, 2018 13:15

add example and make it clear optional stuff is optional

ae3820b

Signed-off-by: Doug Davis <dug@us.ibm.com>

duglin force-pushed the serialize branch from 08e2f31 to ae3820b Compare March 29, 2018 13:16

duglin merged commit 1db322f into cloudevents:master Mar 29, 2018

duglin deleted the serialize branch March 31, 2018 12:14

duglin mentioned this pull request Mar 31, 2018

method context attribute #26

Closed

+              }
+              ```
+              Note:

		@@ -0,0 +1,43 @@
		# CloudEvents Serialization Profile - Version 0.1

		This document specified how a CloudEvent is to be serialized into certain

+              }
+              ```
+              Notes:

Conversation

duglin commented Mar 24, 2018

Uh oh!

SeanFeldman left a comment

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!

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!

inlined Mar 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

duglin commented Mar 29, 2018

Uh oh!

ultrasaurus commented Mar 29, 2018

Uh oh!

duglin commented Mar 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

inlined Mar 28, 2018 •

edited

Loading