Skip to content

CloudEvent class needs better access to cloudevent fields #49

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

Closed
cumason123 opened this issue Jun 30, 2020 · 9 comments · Fixed by #165
Closed

CloudEvent class needs better access to cloudevent fields #49

cumason123 opened this issue Jun 30, 2020 · 9 comments · Fixed by #165
Labels
enhancement New feature or request

Comments

@cumason123
Copy link
Contributor

cumason123 commented Jun 30, 2020

Expected Behavior

event = CloudEvent(headers, data)
event.specversion

Actual Behavior

event = CloudEvent(headers, data)
event['specversion']

Steps to Reproduce the Problem

from cloudevents.sdk import converters
from cloudevents.sdk.http_events import CloudEvent
import requests


attributes = {
    "Content-Type": "application/json",
    "type": "README.sample.binary",
    "id": "binary-event",
    "source": "README",
}
data = {"message": "Hello World!"}

event = CloudEvent(attributes, data)
event['specversion']
@evankanderson
Copy link
Contributor

I'm not sure that you want attribute accessors, given that the set of CE attributes are unknown a priori due to the possible presence of extensions. Using attribute access may also make iterating over all the attributes, or adding additional methods on the class more difficult.

@cumason123
Copy link
Contributor Author

cumason123 commented Jul 10, 2020

I was thinking about providing an interface from CloudEvent to the existing python properties within the base event class such as:

cloudevent = CloudEvent(attributes, data)
cloudevent.event.specversion

I don't want access properties to the _attribute field if that's what you thought I meant. I acknowledge this will take a bit of refactoring/design decision rework if we agreed to go this route because we'd likely have to instantiate some form of events during the constructor call which will require discussion.

For extensions, which are unknown, you can access them through the above example by doing:

cloudevent.event.extensions['key']

This naming schema feels a tad bit inconvenient so perhaps we can make adjustments here but overall I think it would be nice for users to be able to auto complete fields through python properties, specifically for the required and optional cloudevent metadata fields. It would reduce obscurity during event consumption and make code slightly more verbose in my opinion.

Aside from that we will want to revise the README to have example usage of the getitem overload. I sort of want to overhaul the README in general. I don't think we want to recommend users to use the base event classes atm. I'll issue a follow up PR on README changes but lmk your thoughts on the above @evankanderson

@evankanderson
Copy link
Contributor

All the old base stuff needs to disappear or at least be hidden and not recommended for external use. You'll note that I didn't hold on to the base event in http_events.py after parsing. I just wasn't quite ready to bite the bullet of restructuring all the parsing code.

One thing I would try to do before declaring a v1 API would be to also separate out the HTTP components from the event class. At the very least, I'd hope to have the following to/from static (non-class) methods:

  • to_json - converts to transport-agnostic JSON (for example, if you want to store in a file)
  • to_structured_http - probably the equivalent of .to_http(converters.TypeStructured)
  • to_binary_http - ditto for .to_http(converters.TypeBinary)
  • from_json - converts to and from transport-agnostic JSON
  • from_http - handles both structured and binary HTTP; structured will be easy with from_json

@evankanderson
Copy link
Contributor

I'm -1 on having a sub-attribute named event on a CloudEvent. attribute would be slightly better, but I don't think nested objects is the way to go.

If we want to do attribute access, they should just be top-level properties, since the name data is (or should be) treated as reserved from an extension PoV -- it would break JSON encoding to try to add a data extension).

If you do want to be able to use attribute access instead of map-style access, I think the following should be sufficient:

@evankanderson
Copy link
Contributor

The point on autocomplete is interesting -- I haven't looked much at how to hint that in various editors.

@cumason123
Copy link
Contributor Author

I'm also -1 on the event name also hence me saying the naming schema I gave above sounds inconvenient. Top level properties rather than nested objects is definitely preferable although marginally more annoying to implement. I further agree we should keep the getitem overload regardless.

The [a-z0-9] naming schema is an interesting point I was unaware of. I would like to know why the spec permits this anyways, although there are no existing attributes where the first character is a number that I know of.

I also agree the http function handles you described should be separate from the event itself. Perhaps we should break this issue up until several smaller issues.

@matejvasek
Copy link

What about just adding properties for the six known attributes (id,type,specversion,source,time,subject)?
I really like dot access more because of IDE completion and type hinting.

See: https://github.com/cloudevents/sdk-python/blob/4030c61368889fb8301de2bb60eae0f381915809/cloudevents/http/event.py

@matejvasek
Copy link

Also Java or Go sdk allow dot access on the known attributes.

@grant
Copy link
Member

grant commented Dec 10, 2021

@ace-n also mentions a .get('specversion') option would suffice as a better accessor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants