Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Pydantic v2 native implementation #219

Merged
merged 19 commits into from
Sep 20, 2023
Merged

Pydantic v2 native implementation #219

merged 19 commits into from
Sep 20, 2023

Conversation

febus982
Copy link
Contributor

@febus982 febus982 commented Aug 31, 2023 •
edited
Loading

Fixes #215
Iterates over #218

Changes

Pydantic V2 implementation of CloudEvent class:

  • Importing the class/functions from cloudevents.pydantic module will import the correct version
  • It's possible to import class/functions from cloudevents.pydantic.v1 or cloudevents.pydantic.v2 modules. (There's no check on users with V1 deciding to import the V2 module)
  • Tests are parametric on both the implementations

One line description for the changelog

Pydantic V2 implementation of CloudEvent class

  • Tests pass
  • Appropriate changes to README are included in PR

@febus982
Add missing Copyright
7b1e4a7 
Signed-off-by: Federico Busetti <[email protected]>
@febus982 febus982 marked this pull request as draft August 31, 2023 21:32
@febus982
Add missing docstring
ae4b362 
Signed-off-by: Federico Busetti <[email protected]>
@febus982
Remove test leftover
7272113 
Signed-off-by: Federico Busetti <[email protected]>
@febus982 febus982 marked this pull request as ready for review August 31, 2023 22:55
@febus982
Copy link
Contributor Author

@xSAVIKx Here it is! I left comments in the parts with non-trivial changes.

One of them is a question for the last tiny piece of code still missing.

@xSAVIKx xSAVIKx self-requested a review September 4, 2023 18:47
xSAVIKx
xSAVIKx reviewed Sep 4, 2023
View reviewed changes
Copy link
Member

@xSAVIKx xSAVIKx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@febus982 thx for the PR. Please take a look the comments noted below.

cloudevents/pydantic/__init__.py Outdated
from pydantic import VERSION as PYDANTIC_VERSION

pydantic_major_version = PYDANTIC_VERSION.split(".")[0]
if pydantic_major_version == "2":
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like if here should be flipped, no? You're importing from v1 package when Pydantic v2 is used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. I'll also add a test to make sure this is correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not been able to test this but it is fixed.

cloudevents/pydantic/fields_docs.py Outdated
Comment on lines 17 to 22
"""
This contains title, description, example and other NON-FUNCTIONAL data
for pydantic fields. It could be potentially used across all the SDK.
Functional field configuration (e.g. defaults) is still defined in the
pydantic model classes.
"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please move this documentation under the field declaration? That's where pydocs should be placed.

cloudevents/pydantic/v1/__init__.py
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd probably add "exports" here in the same style as we have it in pydantic.__init__,py.

cloudevents/pydantic/v2/conversion.py Show resolved Hide resolved
cloudevents/pydantic/v2/__init__.py
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same regarding the exports

cloudevents/pydantic/v2/event.py Outdated
Comment on lines 154 to 167
@model_validator(mode="before")
@classmethod
def check_base64_data_input(cls, data: typing.Any) -> typing.Any:
"""Populates the `data` property if the model gets created using `data_base64`.

:param data: Input data.

:return: Event serialized as a standard CloudEvent dict with user specific
parameters.
"""
if isinstance(data, dict) and data.get("data_base64") is not None:
data["data"] = base64.b64decode(data["data_base64"])
del data["data_base64"]
return data
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so why exactly would we like to have this? I believe this is already handled by the base CloudEvent implementation.

Copy link
Contributor Author

@febus982 febus982 Sep 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why these 2 functions are here:

The base64 data is handled if only when serialising the event using cloudevents.conversion.to_structured function.

The original pydantic v1 implementation had tests covering both serialisation and json deserialisation without passing through the conversion module but using the two pydantic functions:

  • Basemodel.parse_raw() to deserialise from a json string.
  • Basemodel.json() to serialize to a json string.

I suspect it was done to support HTTP frameworks using pydantic serialisation logic (e.g. FastAPI) without explicitly calling the conversion.* functions.

This function, and the following one (serialize_model) are to support this scenario.

=====

Reasoning about the implementation:

The original pydantic v1 serialisation implementation (_ce_json_loads and _ce_json_dumps functions in the V1 implementation) was relying on the http module conversion logic.

I did rewrite the functions decoupling them from the http module as it seemed an anti-pattern, and using json.dumps would have worse performance if compared to the pydantic v2 json serializer.

I see now it doesn't make much sense to have a duplicate implementation and it's better to rely on the existing cloudevents.conversion module, so we have a single implementation. I'll address this (and document in the code why we decided to use the conversion module).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So yes, the reasoning for using _ce_ implementations is to make sure the behavior is consistent. We may provide "extra" APIs that are not part of the SDK for advanced/customized use cases, but IMO the standard conversion should be the default one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have addressed both serialisation and deserialization. It now uses the methods from conversion module.

cloudevents/pydantic/v2/event.py Outdated Show resolved Hide resolved
cloudevents/tests/test_pydantic_conversions.py Outdated Show resolved Hide resolved
@febus982
Remove failing test for unsupported scenario
85faf67 
Fix typo

Signed-off-by: Federico Busetti <[email protected]>
@febus982
Use SDK json serialization logic
f32ee91 
Signed-off-by: Federico Busetti <[email protected]>
@febus982
No need to filter base64_data
a790f2f 
Signed-off-by: Federico Busetti <[email protected]>
@febus982
Use SDK json deserialization logic
b2e84cd 
Signed-off-by: Federico Busetti <[email protected]>
@febus982
Fix imports
f4265a1 
Signed-off-by: Federico Busetti <[email protected]>
@febus982
Move docs after field declarations
bb7c70e 
Signed-off-by: Federico Busetti <[email protected]>
@febus982
Copy link
Contributor Author

Hey @xSAVIKx I addressed all the comments (I left them unresolved so you can double-check).

I don't think the typing error can be addressed because json.loads returns Any but we know well we're getting a dictionary there. I could add a check on the result being an instance of dict but I think adding a # type: ignore is safe enough. I'm also not sure why self is identified with a type and not an object, it might be a mypy issue. Let mw know if you have a better recommendation for this.

xSAVIKx
xSAVIKx reviewed Sep 20, 2023
View reviewed changes
Copy link
Member

@xSAVIKx xSAVIKx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@febus982 overall LGTM. There's some pre-commit thing failing, will you take a look? Also, if'd like I can jump in and fix the thing as well as my comments.

cloudevents/pydantic/v1/__init__.py Outdated
Comment on lines 15 to 16
from .conversion import from_dict, from_http, from_json
from .event import CloudEvent
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please use fully-qualified imports instead?

cloudevents/pydantic/v2/__init__.py Outdated
Comment on lines 15 to 18
from .conversion import from_dict, from_http, from_json
from .event import CloudEvent

__all__ = ["CloudEvent", "from_json", "from_dict", "from_http"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, please use fully-qualified imports

@febus982
Use fully qualified imports
b38efed 
Signed-off-by: Federico Busetti <[email protected]>
@febus982
Ignore typing error
e73e3ab 
Signed-off-by: Federico Busetti <[email protected]>
@febus982
Copy link
Contributor Author

@xSAVIKx Ready to be merged, all comments addressed.

@xSAVIKx xSAVIKx merged commit 5a1063e into cloudevents:main Sep 20, 2023
17 checks passed
@benvdh-incentro
Copy link

@xSAVIKx I noticed there hasn't been a release of the package since january 2023, though some very useful PRs (among them this one) got merged to main. I noticed the CI also fails on the fact that 1.9.0 already exists on PyPI, so likely it's just a version bump that's required.

So my question is: are there any plans to get this code released to PyPI ?

@xSAVIKx
Copy link
Member

xSAVIKx commented Sep 22, 2023

@xSAVIKx I noticed there hasn't been a release of the package since january 2023, though some very useful PRs (among them this one) got merged to main. I noticed the CI also fails on the fact that 1.9.0 already exists on PyPI, so likely it's just a version bump that's required.

So my question is: are there any plans to get this code released to PyPI ?

Hey @benvdh-incentro, sure thing 🙂 I was holding the release till this particular PR is implemented and we should get one probably on the weekend or early next week

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xSAVIKx xSAVIKx xSAVIKx approved these changes

Assignees

@febus982 febus982

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support for pydantic v2
3 participants
@febus982 @benvdh-incentro @xSAVIKx