-
Notifications
You must be signed in to change notification settings - Fork 595
fix(protobuf)!: Expose CloudEvent structure with explicit fields #1354
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
Conversation
4c5050b
to
c62b56a
Compare
Previously, all optional CloudEvent attributes were hidden within a generic map structure alongside extension attributes. Signed-off-by: Yordis Prieto <[email protected]>
c62b56a
to
5226604
Compare
@@ -2,7 +2,8 @@ | |||
* CloudEvent Protobuf Format | |||
* | |||
* - Required context attributes are explicitly represented. | |||
* - Optional and Extension context attributes are carried in a map structure. | |||
* - Optional context attributes are explicitly represented. | |||
* - Extension context attributes are carried in a map structure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One of the things that CE has really pushed hard for is the idea that spec-defined attributes and extensions should be treated (and serialized) the same way. The idea of "extension buckets" is something we've avoided.
So, when I see the above text it makes me nervous. Could this be why it was done this way originally?
If, in the future, some extension becomes an OPTIONAL spec-defined attribute, what would change in the protobuf stuff? Ideally nothing, but I suspect with this PR that's not the case. Now, some formats may force us to violate our goal, and I know nothing about protobuf, but I wanted to mention this as we discuss this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most likely that was the intent, if such extension becomes part of the schema, you would need such schema version to understand the new message-level field
We discussed this at last Thursday's CloudEvents working group meeting. My feeling - which I seem to remember was broadly supported - was that we wouldn't want to take this pull request.
We can definitely revisit the format for v2 - Doug has added it to #1355 as something to consider if and when we go for a v2. Feel free to push back of course, but I'd suggest closing this PR for now. |
Could we take advantage of alpha/beta versioning and propose the v2 spec? I didn't know about the Avro compact schema, that give me a strong reason to try to find a solution that would normalize the long-term situation. Without side-tracking, even the JSON schema is frustrating to deal with, I wish it was like the Avro compact schema as well, but I digress. I am gonna try to join the next meeting. |
Can you elaborate on this point? What's the frustration? |
In the current format, extensions are not grouped under a predefined key, which complicates marshalling in languages that don't support structs or records with dynamic fields—Elixir, in my case. To work around this, we need to explicitly filter out the "standard" CloudEvents attributes and treat the remaining keys as extensions. This either requires manually dropping known fields and handling the rest as a separate map, or merging everything into a flat map—both are manageable in Elixir, but still less than ideal. We can not take advantage of built-in protocols because of this (I think latest Go jsonv2 solves this a bit nicer, but still) defmodule CloudEvent do
@enforce_keys [:id, :source, :specversion, :type]
defstruct [
:id,
:source,
:specversion,
:type,
:datacontenttype,
:dataschema,
:subject,
:time,
:data,
:data_base64,
extensions: %{}
]
defimpl Jason.Encoder do
def encode(cloud_event, opts) do
base_attrs =
cloud_event
|> Map.from_struct()
|> Map.delete(:extensions)
|> Enum.reject(&is_nil(elem(&1, 1)))
|> Map.new()
base_attrs
|> Map.merge(cloud_event.extensions) # Merge extensions into root
|> Jason.Encoder.encode(opts)
end
end
@non_extension_keys [
"id",
"source",
"specversion",
"type",
"datacontenttype",
"dataschema",
"subject",
"time",
"data",
"data_base64"
]
def from_json!(json) do
data = Jason.decode!(json)
%__MODULE__{
id: data["id"],
source: data["source"],
specversion: data["specversion"],
type: data["type"],
datacontenttype: data["datacontenttype"],
dataschema: data["dataschema"],
subject: data["subject"],
time: data["time"],
data: decode_data(data["type"], data["data"]),
data_base64: data["data_base64"],
extensions: Map.drop(data, @non_extension_keys) # Extract extensions
}
end
end Lastly, I actually prefer using the Protobuf schema alongside the JSON format. However, that's currently not feasible without custom code, since schema-driven tools don't handle unstructured, root-level extension fields well. Take it with a grain of salt, it could be me. |
Closing and adding to our v2 list: https://github.com/cloudevents/spec/blob/main/cloudevents/v2.md |
Previously, all optional CloudEvent attributes were hidden within a generic
map structure alongside extension attributes.
Signed-off-by: Yordis Prieto [email protected]