refactor: create abstract cloudevent #186

sasha-tkachev · 2022-07-22T21:26:00Z

Changes

Created an abstract cloudevent class to be used in #182

Tests pass
Appropriate changes to README are included in PR

…udevents#171) Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>

Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>

for more information, see https://pre-commit.ci

Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>

for more information, see https://pre-commit.ci

Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>

xSAVIKx

@sasha-tkachev thx for your PR. There are some good ideas in this approach, but I don't get some of the decisions being made. Please see some comments and suggestions noted below.

cloudevents/abstract/event.py

xSAVIKx · 2022-07-23T18:34:10Z

cloudevents/abstract/event.py

+    @property
+    @abc.abstractmethod
+    def _attributes_read_model(self) -> typing.Dict[str, typing.Any]:
+        """
+        :return: attributes of this event
+
+        you MUST NOT mutate this dict
+        implementation MAY assume the dic will not be mutated
+        """
+        pass


Why don't we go with public attributes property? This way each implementation will expose the attributes which may be beneficial.

it is to prevent confision. Documented in the function this reason

What kind of confusion? We're basically talking about having an interface that exposes desired public APIs.

It is beneficial to discuss which properties must be exposed and which not.

* What is the difference between `event.get("myattr")` and `event.attributes.get("myattr")` * What SHOULD I use `event["myattr"]` or `event.attributes["myattr"]` ?

I'm not saying that one should not use .get way. I'm saying that it may be beneficial to expose attributes sometimes.

OK fine with me

xSAVIKx · 2022-07-23T18:34:37Z

cloudevents/abstract/event.py

+    @property
+    @abc.abstractmethod
+    def _data_read_model(self) -> typing.Optional[typing.Any]:
+        """
+        :return: data value of the  event
+        you MUST NOT mutate this value
+        implementation MAY assume the value will not be mutated
+        """
+        pass


The same question here. I'd rather have it here as just data and hide data field in the implementation.

I can't use the data property because in pydantic i need to declare it as a field and not a property

Well, I'm not sure that's a valid reason to drop this approach. Maybe worth naming data differently in Pydantic model instead?

but then it will be serialized funky in json form

i think maybe we should define the cloudevent as protocol

this will make much more sense dont you think?

Protocol would be nice, but it's only available in python 3.8+.
And we currently support 3.6+.

but then it will be serialized funky in json form

I guess it may be configured. E.g. by using alias for the data.

E.g. name the field "payload" and use data as alias

good idea, i will do that

did not work, see below, i found a better way

cloudevents/abstract/event.py

xSAVIKx · 2022-07-23T18:38:01Z

cloudevents/abstract/event.py

+        pass
+
+    def __setitem__(self, key: str, value: typing.Any) -> None:
+        raise NotImplementedError()


Can't we use self._data_read_model()[key]=value?

I don't really get why we want to have a read model.

It MAY not be possible to do so.
For example - if my cloudevent is a database row, or a cached value.
I can read the value, but setting it MAY not be possible in the same way.

updated the docs so it will be more clear

Again, it is up to us to define the contract. I'd rather have the default implementation here and rely on whatever we present in the contract.
It's kinda the implementation problem of how to present the data.
While we're kinda obliged to follow the existing API till we move to v2, I'd rather keep it simple and leave it to the implementation to follow.

I'd probably go with a fully immutable class in v2 and then define some mutation helpers, but that's a different story.

It's pretty simple to read one way and write another way
its CQRS.
if you wish to change it, i can

I don't think this is cqrs. Haven't ever heard of using it in the context of a single entity.

We may not provide a mutation API at all at this level.

I think that you are right. i will remove the del and set from this level

cloudevents/abstract/event.py

cloudevents/abstract/http_methods.py

xSAVIKx · 2022-07-23T18:42:57Z

cloudevents/abstract/json_methods.py

+from cloudevents.abstract.http_methods import to_structured, from_http
+
+
+def to_json(


I'd say it worth moving these http_methods and json_methods to the cloudevents package level. These have nothing "abstract` in them. They don't belong here.

Isn't it confusing?

what is the difference between cloudevents.http.http_methods and cloudevents.http_methods?

Well, there's somewhat confusing APIs all over the package. But these are definitely not abstract. They look more like generic or standard implementations.

Maybe even name the file with these APIs as conversion.py or convertor.py. This will then make a bit more sense.

Maybe we should rename the whole module to generic?

Generic is a bad name for a module IMO

Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>

Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com> # Conflicts: # CHANGELOG.md # cloudevents/http/event.py # cloudevents/tests/test_http_cloudevent.py

Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>

This reverts commit 89d30eb. Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>

Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>

sasha-tkachev · 2022-07-24T22:25:03Z

@xSAVIKx i tried to implement the alias trick but pydantic does not like it at all
and i thought about a new way of implementing it

super simple - get_data_ and get_attributes classmethods

simple, polymorphic and implementation independent

Also i dont need to think about the mutability of the resulted attributes

Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>

cloudevents/abstract/__init__.py

xSAVIKx · 2022-07-25T10:57:29Z

cloudevents/abstract/event.py

+        we MUST NOT assume how the __init__ function looks across different python
+        frameworks
+        """
+        raise NotImplementedError()


Why don't we mark this method as abstract?

because it causes issues in the code coverage tests.
I cannot trigger the line to be called because an abstract method call fails before the function is called

Yeah, sure, but that's not a valid reason to make smth not abstract. You can just create a test no-op or error-throwing implementation directly in the test case.

I tried testing the code directly but it did not actually executed the pass line of the function

@abstractmethod def dostuff(): pass

because the abstract method raised an exception before reaching the pass.

I will do the thing that is mentioned here
https://stackoverflow.com/questions/9202723/excluding-abstractproperties-from-coverage-reports

xSAVIKx · 2022-07-25T10:58:34Z

cloudevents/abstract/event.py

+    @classmethod
+    def get_attributes(cls, event: "CloudEvent") -> typing.Dict[str, typing.Any]:
+        """
+        :return: Attributes of this event.
+
+        You MUST NOT mutate this dict.
+        Implementation MAY assume the dict will not be mutated.
+        """
+        raise NotImplementedError()
+
+    @classmethod
+    def get_data(cls, event: "CloudEvent") -> typing.Optional[typing.Any]:
+        """
+        :return: Data value of the  event.
+        You MUST NOT mutate this dict.
+        Implementation MAY assume the dict will not be mutated.
+        """
+        raise NotImplementedError()


Why have classmethod? What's the benefit?

Why can't it be just an abstract method?

It is my way of handling the pydantic edge case for the data field. I tried making an alias but it messes everything up.
I don't see any reason for forcing any data attributes or member functions for our concrete event type.
All I want is the generic access of the values im interested in especially if this is for a internal use only.

I think I may make it private to prevent confusion.

I still don't get the reasoning. Just `my way of handling is not a good enough reason.

I don't see a reason why get_data(event) can be anyhow different from event.get_data. The latter one looks and feels more natural. Moreover, it is the event instance behavior to return some part of its state to the caller. Even if we make these APIs private (with __) or protected (with _), they still look very much like some abstract instance APIs, not class-level ones.

And also, why don't we return a copy of the internal state instead of stating that one MUST NOT mutate the state? (that's in case we want this to be part of the public API).

I have no problem to create protected event._get_data and event._get_attributes.
this will solve the confusion i was afraid of

I thought about copying the data, but was afraid of the efficiency implication though it may be premature optimization.

xSAVIKx · 2022-07-26T15:39:12Z

@sasha-tkachev please let me know if you'd like to continue on this one. I can jump in if needed.

sasha-tkachev · 2022-07-27T19:09:17Z

@xSAVIKx It's ok, I have a-lot of motivation for this project. Just did not have enough free time for it in the past few days.

sasha-tkachev · 2022-08-05T12:58:23Z

@xSAVIKx waiting for your answers

xSAVIKx · 2022-08-05T13:13:19Z

@sasha-tkachev I'm sorry for the late reply. Somehow I missed the notifications.

You can also find me in CNCF Slack workspace if you need to poke me. Join here: https://communityinviter.com/apps/cloud-native/cncf

…ions instead of classmethods Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>

Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>

for more information, see https://pre-commit.ci

xSAVIKx

@sasha-tkachev overall LGTM. I've modified the docs to be a bit more reader-friendly.

cloudevents/abstract/event.py

cloudevents/conversion.py

Signed-off-by: Yurii Serhiichuk <savik.ne@gmail.com>

for more information, see https://pre-commit.ci

sasha-tkachev and others added 24 commits July 11, 2022 14:04

fix: non-cloudevents values must not equal to cloudevents values (clo…

9422e62

…udevents#171) Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>

test: refactor move fixtures to beginning

6c6508b

Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>

test: cloudevent equality bug regression (cloudevents#171)

494170b

Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>

style: remove redundent else

202c987

Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>

test: remove redundent test

8f98d65

Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>

test: refactor non_cloudevent_value into a parameterization

781d914

Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>

docs: update changelog

fe3a2d0

Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>

Merge branch 'master' into master

188e7b7

[pre-commit.ci] auto fixes from pre-commit.com hooks

39c838a

for more information, see https://pre-commit.ci

docs: fix bad merge

5c0af53

Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>

[pre-commit.ci] auto fixes from pre-commit.com hooks

f55d247

for more information, see https://pre-commit.ci

Merge branch 'master' into master

aaa8098

[pre-commit.ci] auto fixes from pre-commit.com hooks

207c4a8

for more information, see https://pre-commit.ci

feat: abstract event

41a9af2

Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>

feat: add missing return type

5f8399f

Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>

feat: create function

2b64830

Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>

feat: any cloud event

4488201

Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>

refactor: move to abstract

f1ff009

Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>

refactor: integrate abstract event

c747f59

Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>

refactor: create abstract cloudevent package

6588577

Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>

docs: abstract cloudevent

01041e7

Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>

feat: simplify data attributes

5648968

Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>

fix: intengrate data read model

09062e3

Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>

feat: define abstract methods

a96bd6c

Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>

xSAVIKx reviewed Jul 23, 2022

View reviewed changes

sasha-tkachev added 5 commits July 24, 2022 21:48

refactor: use anycloudevent for generics

3845aa7

Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>

docs: getitem documentation

fb4f993

Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>

docs: better cloudevent explenation

1187600

Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>

docs: explain read model

ecf9418

Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>

docs: not implemented errors

b9e8763

Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>

sasha-tkachev added 12 commits July 24, 2022 23:00

test: add abstract cloudevent coverage tests

a22efbd

Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>

refactor: rename abstract to generic

89d30eb

Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>

Merge branch 'main' into feature/abstract-cloudevent

d303eae

Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com> # Conflicts: # CHANGELOG.md # cloudevents/http/event.py # cloudevents/tests/test_http_cloudevent.py

refactor: cloudevent is no longer absctract

ba16cdd

Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>

test: fix broken test

ea19f7d

Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>

Revert "refactor: rename abstract to generic"

f47087d

This reverts commit 89d30eb. Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>

refactor: move all abstract conversion logic under conversion

41c5f59

Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>

test: rename badly named test

5d0882d

Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>

refactor: add default value for conversions

6a92016

Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>

docs: remove inconsistent types

1109bc5

Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>

refactor: remove mutation variables from contract

c0b5413

Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>

refactor: expose data and attributes in class

065ef91

Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>

sasha-tkachev force-pushed the feature/abstract-cloudevent branch from 684a62c to 065ef91 Compare July 24, 2022 22:04

test: remove broken tests

a8872b9

Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>

sasha-tkachev added 2 commits July 25, 2022 01:26

refactor: use classmethods

fbc0632

Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>

refactor: remove optional type

0c2bafc

Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>

xSAVIKx reviewed Jul 25, 2022

View reviewed changes

sasha-tkachev and others added 3 commits August 5, 2022 17:09

refactor: convert get_data and get_attributes to private member funct…

4e00b55

…ions instead of classmethods Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>

build: ignore not-implemented functions in coverage

96c41a1

Signed-off-by: Alexander Tkachev <sasha64sasha@gmail.com>

[pre-commit.ci] auto fixes from pre-commit.com hooks

d129e6a

for more information, see https://pre-commit.ci

xSAVIKx approved these changes Aug 6, 2022

View reviewed changes

xSAVIKx and others added 2 commits August 6, 2022 14:50

docs: mentioned default branch change in the changelog

48de8e1

Signed-off-by: Yurii Serhiichuk <savik.ne@gmail.com>

[pre-commit.ci] auto fixes from pre-commit.com hooks

26e551b

for more information, see https://pre-commit.ci

xSAVIKx force-pushed the feature/abstract-cloudevent branch from dab07d2 to 26e551b Compare August 6, 2022 11:50

xSAVIKx merged commit 785bfe7 into cloudevents:main Aug 6, 2022

		from cloudevents.abstract.http_methods import to_structured, from_http


		def to_json(

refactor: create abstract cloudevent #186

refactor: create abstract cloudevent #186

Conversation

sasha-tkachev commented Jul 22, 2022

Changes

xSAVIKx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xSAVIKx Jul 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xSAVIKx Jul 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sasha-tkachev commented Jul 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xSAVIKx commented Jul 26, 2022

sasha-tkachev commented Jul 27, 2022

sasha-tkachev commented Aug 5, 2022

xSAVIKx commented Aug 5, 2022 • edited Loading

xSAVIKx left a comment

Choose a reason for hiding this comment

xSAVIKx Jul 24, 2022 •

edited

Loading

xSAVIKx Jul 24, 2022 •

edited

Loading

sasha-tkachev commented Jul 24, 2022 •

edited

Loading

xSAVIKx commented Aug 5, 2022 •

edited

Loading