Consider a more Pythonic API #26
Comments
|
Seems like there is a similar discussion on the Java SDK here: cloudevents/sdk-java#108 (cc @slinkydeveloper) |
|
Here is a super basic implementation of a more pythonic CloudEvent Expand to seefrom datetime import datetime
from datetime import timezone
from typing import Any
from typing import Dict
from typing import Iterable
from typing import Optional
_EVENT_KNOWN_ATTRIBUTES: Iterable[str] = (
"spec_version",
"id",
"source",
"type",
"subject",
"time",
"data_content_type",
"data_schema",
"data",
)
_MANGLED_EVENT_KNOWN_ATTRIBUTES: Iterable[str] = tuple([f"_Event__{attr}" for attr in _EVENT_KNOWN_ATTRIBUTES])
class Event:
__spec_version: str = "1.0"
__id: str
__source: str
__type: str
__subject: Optional[str] = None
__time: Optional[datetime] = None
__data_content_type: Optional[str] = None
__data_schema: Optional[str] = None
__data: Optional[Any] = None
__extensions: Optional[Dict[str, Any]] = {}
def __init__(
self,
spec_version: str,
id: str,
source: str,
type: str,
subject: Optional[str] = None,
time: Optional[datetime] = None,
data_content_type: Optional[str] = None,
data_schema: Optional[str] = None,
data: Optional[Any] = None,
**extensions: Any
) -> None:
self.__spec_version = spec_version
self.__id = id
self.__source = source
self.__type = type
self.__time = time or datetime.now(timezone.utc)
if subject is not None:
self.__subject = subject
if data_content_type is not None:
self.__data_content_type = data_content_type
if data_schema is not None:
self.__data_schema = data_schema
if data is not None:
self.__data = data
if extensions is not None:
for extension_key, extension_value in extensions.items():
setattr(self, extension_key, extension_value)
@property
def spec_version(self) -> str:
return self.__spec_version
@spec_version.setter
def spec_version(self, value: str) -> None:
self.__spec_version = value
@property
def id(self) -> str:
return self.__id
@id.setter
def id(self, value: str) -> None:
self.__id = value
@property
def source(self) -> str:
return self.__source
@source.setter
def source(self, value: str) -> None:
self.__source = value
@property
def type(self) -> str:
return self.__type
@type.setter
def type(self, value: str) -> None:
self.__type = value
@property
def subject(self) -> Optional[str]:
return self.__subject
@subject.setter
def subject(self, value: str) -> None:
self.__subject = value
@property
def time(self) -> Optional[datetime]:
return self.__time
@time.setter
def time(self, value: datetime) -> None:
self.__time = value
@property
def data_content_type(self) -> Optional[str]:
return self.__data_content_type
@data_content_type.setter
def data_content_type(self, value: str) -> None:
self.__data_content_type = value
@property
def data_schema(self) -> Optional[str]:
return self.__data_schema
@data_schema.setter
def data_schema(self, value: str) -> None:
self.__data_schema = value
@property
def data(self) -> Optional[Any]:
return self.__data
@data.setter
def data(self, value: Any) -> None:
self.__data = value
def __getattr__(self, item: str) -> Any:
return self.__extensions[item]
def __setattr__(self, key: str, value: Any) -> None:
if key in _EVENT_KNOWN_ATTRIBUTES or key in _MANGLED_EVENT_KNOWN_ATTRIBUTES:
super().__setattr__(key, value)
else:
self.__extensions[key] = valueThis implementation allow to use pythonic style Event creation with keyword arguments for all Required, Optional or Extras attributes. event = Event(spec_version="v1",
id=str(uuid.uuid4()),
source="event-source",
type="event-type",
subject="event-subject",
time=datetime(2020, 4, 10, tzinfo=timezone.utc),
data=["abc", 123, True],
extra_parameter="extra-parameter-value")
print(event.spec_version) # --> v1
print(event.subject) # --> event-subject
print(event.time.isoformat()) # --> 2020-04-10T00:00:00+00:00
print(event.data) # --> ['abc', 123, True]
print(event.extra_parameter) # --> extra-parameter-value
# Changing subject (Normal attributes)
event.subject = "new-subject"
print(event.subject) # --> new-subject
# Adding extra attributes
event.other_parameter = "other-value"
print(event.other_parameter) # --> other-valueThe big missing part is the serialization and deserialization |
|
One other thing to deal with when talking about the API is to be careful not to lock-in the HTTP binding's headers as the interchange format. We are using this SDK with RabbitMQ, and the AMQP bindings have different header mappings in Binary mode, for example. The JSON Event Format support for the Structured content mode also assumes HTTP (it's in the name of the class...) but happens to have the same header name in AMQP, so we're lucky there. At some point soon, we'll be using this with Kafka (eventually replacing RabbitMQ) and I haven't checked how well those bindings align in Structured mode. As part of this, we may want the AVRO Event Format support too. This might mean that the current converter/marshaller/event relationship needs readjustment, since Structured mode is co-operation between an Event Format and a Protocol Binding, while Binary mode is entirely up to the Protocol Binding. Currently the JSON Event Format implementation has HTTP-specific bits inside. It should be returning a collection of Cloud Events attributes, not HTTP headers. Note: I've only looked at the v1.0 spec, it's possible that this stuff was differently-structured in earlier releases. |
|
A slight caveat on #26 (comment) It might make more sense to clearly split the Context Attributes from the Event Payload. It would also be handy to default or require the required attributes, which are:
|
|
@di The current CloudEvent class #47 allows users to pass in http headers/data content as dictionaries and it'll automatically parse the dicts for cloudevent data. event = CloudEvent(data, headers=headers)But this issue declares a more pythonic constructor to be the following: event = CloudEvent(spec_version="v1",
id=str(uuid.uuid4()),
source="event-source",
type="event-type",
subject="event-subject",
time=datetime(2020, 4, 10, tzinfo=timezone.utc),
data=["abc", 123, True],
extra_parameter="extra-parameter-value",
isbinary=True
)This constructor while more descriptive of what it wants as arguments, can provide a little more overhead when receiving events as a user must first extract data from the http request. Naturally, **kwargs could provide a slightly more convenient constructor via: event = CloudEvent(**headers, **body, isbinary=True)So I suppose my question is I plan on refactoring the current CloudEvent constructor to take more key named arguments instead of a vague dict, but for canonical sample code is it appropriate to show instantiating CloudEvents with **kwargs for convenience? |
|
Coming here from #47 , I think there's been a bit of a misunderstanding (either by myself or by one of the other authors here). Looking at the new According to my reading of the spec, the CloudEvent payload is not always JSON:
Furthermore, when encoding a CloudEvent with non-JSON data, the data may need to be converted to a base-64 encoded string (if Binary), or serialized as a JSON string (if not). I suspect that the current signature of "CloudEvent" in #47 and mentioned in #26 (comment) probably won't work for e.g. |
|
I'm probably going to try to add this to #34, since I'm needing to rework |
|
Proposed API. Note that the core data is version- and encoding- agnostic, based on the v1 interface, and that serialization specifies (with a default) whether structured or binary encoding should be used. class CloudEvents():
@classmethod
def FromHttp(cls, headers, body, unmarshaller=json.loads) -> CloudEvent:
"""Automatically determines version and encoding, decodes to standard format.
Throws an exception if required fields are missing."""
def __init__(self, attributes:dict={}, data=typing.Any):
"""Constructs an object from cloudevents attributes (no "ce-" prefix) and optional data.
Fills in defaults as needed."""
def ToHTTP(self, format: base.Converter=converters.HTTPStructured) -> (dict, typing.IO):
"Creates HTTP header & payload for the specified encoding format."
# The `data` attribute in the CluodEvent may be used to access the stored data.
# Implement map type for attributes
def __getitem__(self, attribute: string) -> string
def __setitem__(self, attribute: string, value: string)
def __delitem__(self, attribute: string)
def __iter__(self) -> typing.Iterable[string]
def __len__(self) -> int
def __contains__(self, attribute: string) -> bool |
|
Rationale for implementing Mapping rather than free-form attributes with |
|
I've updated #34 with an implementation of my proposed API. |
|
Thanks for the comments and raising the regression @evankanderson. I've pinged @cumason123 to look at #34 and we should aim to merge that PR next. |
|
is there a demo for use this with kafka like https://github.com/cloudevents/sdk-java/tree/master/examples/kafka |
|
Not as far as I know. As I noted earlier (and it hasn't changed, that I can see) the only implementations for both 'structured' and 'binary' Content Modes are for the HTTP Protocol Binding. |
Hi all, first: thanks for providing this library. It's going to be really useful to have a standard way to work with CloudEvents from Python.
I took a preliminary look at it and my first impression was that the interface provided by this library isn't very "Pythonic", meaning that it doesn't provide an interface in a way that (I believe) would be expected by most Python developers. There are some small things (like using method names like
FromRequestinstead offrom_request, usingNewin class/method names) but also larger things (like the use of method chaining, setters/getters, etc.)I'm hoping that since this project is still in alpha, there's still time to improve the interface here (and that y'all are open to suggestions). I'll try to provide some examples below of what I'm talking about but I'm happy to clarify if anything's unclear. I'm also not intimately familiar with the CloudEvents spec, so I may have some misunderstandings as well.
Parsing upstream
Eventfrom HTTP RequestThe current example for turning an HTTP request into an event:
HTTPMarshaller, and if it's the default maybe we should just get it by default when creating anEvent, instead of making the user initialize it every time?data_unmarshalleris maybe not necessary? We're taking a bytestring, turning it into aBytesIOobject to pass asdata, and then usingdata_unmarshallerto turn it back into a bytestring. Why not just have the user do any data unmarshalling themselves before constructing the event?Instead, consider:
Event, detects which spec it matches, and gives back a correspondingEventsubclass. (If the user knew which spec they were supporting, instead of constructing anEventthey could construct av02.Eventand skip the event detection)HTTPMarshalleris used by default. This also helps avoid issues like Reusing a marshaller causes failures? #12.data_unmarshallerfield is removed (or at the very least, optional)Creating a minimal CloudEvent
The provided example is:
Instead, consider the following based on the same
Eventclass proposed above:Eventobject can be created with a single method call. If the user needs to modify the event after initialization, they can act on the attributes directly (e.g.event.event_time = "yesterday").Getting event attributes
Currently, after creating an event, if the user wanted to get one of the fields such as the event time, the user would have to do something like:
ce-prefix is a bit unexpected and the double-underscore is unconventional.valueto get the value is also unexpectedInstead, consider something like:
Final thoughts
I realize this is proposing a pretty big overhaul of the current interface, but I think doing so would probably go a long way to lend towards the usability and maintainability of this library, and it'd be better to do it sooner than later. I'm happy to help out here, including designing/implementing these changes and helping maintain them afterwards, if it makes sense. Thanks!
The text was updated successfully, but these errors were encountered: