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

cloudevents.kafka.to_binary checks for a content-type attribute, which would not be a valid attribute name #211

Closed
aucampia opened this issue May 3, 2023 · 5 comments · Fixed by #232
Closed

cloudevents.kafka.to_binary checks for a content-type attribute, which would not be a valid attribute name #211

aucampia opened this issue May 3, 2023 · 5 comments · Fixed by #232
Labels
bug Something isn't working

Comments

@aucampia
Copy link

aucampia commented May 3, 2023 •
edited

sdk-python/cloudevents/kafka/conversion.py

Line 90 in ef98274

if event["content-type"]:

This will call CloudEvent._get_attributes()["content-type"] but a content-type attribute will violate the attribute naming rules:

https://github.com/cloudevents/spec/blob/30f1a081743bfe06b45b85519dd145f14a1ad22c/cloudevents/spec.md?plain=1#L173-L175

CloudEvents attribute names MUST consist of lower-case letters ('a' to 'z') or digits ('0' to '9') from the ASCII character set. Attribute names SHOULD be descriptive and terse and SHOULD NOT exceed 20 characters in length.

It is not really that clear to me why this is being done, but it also causes some problems when event is a Pydantic cloud event, but regardless of that it seems pretty clear that it is wrong.
@xSAVIKx xSAVIKx added the bug Something isn't working label May 3, 2023
@xSAVIKx
Copy link
Member

xSAVIKx commented May 3, 2023 •
edited

Hey @aucampia, thx for raising this. I believe you're correct and the event itself must not contain the content-type attribute. The conversion here should convert the datacontenttype of the event to the content-type header of the Kafka message.

Ans similarly the backwards conversion must take care of the respective mapping.

Here's the supportive docs section: https://github.com/cloudevents/spec/blob/30f1a081743bfe06b45b85519dd145f14a1ad22c/cloudevents/bindings/kafka-protocol-binding.md#321-content-type

@duglin
Copy link
Contributor

duglin commented May 4, 2023

someone want to PR this?

@aucampia
Copy link
Author

aucampia commented May 4, 2023

I want to, but realistically I probably won't get to it before the end of next week, though I may look at it then. Will notify before I start working on it though.

@xSAVIKx
Copy link
Member

xSAVIKx commented May 4, 2023

I'm out of capacity till the end of next week as well. Even though it's a small change in the codebase, I'd appreciate a PR. If none will start working on it, I'll find some time next week.

@MaryamTaj
Copy link

Hi! I would be happy to work on this!

@vivjd vivjd mentioned this issue Jan 6, 2024
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Modified content-type to abide by attribute naming conventions for cloudevents vivjd/sdk-python
4 participants
@duglin @aucampia @xSAVIKx @MaryamTaj