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.

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

Support UNSPECIFIED payload format in CloudEvents mapping #248

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sophokles73
Copy link
Contributor

This is a follow up of
#247

@sophokles73 sophokles73 added enhancement New feature or request breaking change This pull request introduces a change to public API which is not backwards compatible labels Nov 11, 2024
@sophokles73
Copy link
Contributor Author

@stevenhartley would you mind taking a look?

Copy link
Contributor

@stevenhartley stevenhartley left a comment

Choose a reason for hiding this comment

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

We cannot get rid of datacontenttype. If we want to add a new one called pformat that we could but datacontenttype is defined in CE specs and it is needed for backwards compatibility with older versions of the protocol

@sophokles73
Copy link
Contributor Author

We cannot get rid of datacontenttype. If we want to add a new one called pformat that we could but datacontenttype is defined in CE specs and it is needed for backwards compatibility with older versions of the protocol

What older version of which protocol are you referring to? An old version of uProtocol? If so, what exactly are the expectations regarding the use of datacontenttype in that old version?

CloudEvents itself does not actually define datacontenttype to contain a value, based on my understanding of the spec ...

@stevenhartley
Copy link
Contributor

We cannot get rid of datacontenttype. If we want to add a new one called pformat that we could but datacontenttype is defined in CE specs and it is needed for backwards compatibility with older versions of the protocol

What older version of which protocol are you referring to? An old version of uProtocol? If so, what exactly are the expectations regarding the use of datacontenttype in that old version?

CloudEvents itself does not actually define datacontenttype to contain a value, based on my understanding of the spec ...

Older versions of the protocol would reject anything that was not application/x-protobuf (what was in production) because AFAIK the PR I made to remove this attribute (and assume it is this type all the time) did not make it into the production code in the cloud.

Cloudevent specifications does define the attribute as optional here.

Having said that, I am simply asking that we do not remove the attribute definition and in fact leave the "if it is missing it is assumed to be application/x-protobuf).

If we want to add a new attribute that sends an int in lie of a string, that is ok, but don't change the behavior of datacontenttype

@sophokles73
Copy link
Contributor Author

If we want to add a new attribute that sends an int in lie of a string, that is ok, but don't change the behavior of datacontenttype

It was actually you who added the pformat property in May 2024. FMPOV we can either use datacontenttype for indicating the payload format or we can use pformat (using the proto3 identifier of UPayloadFormat), but not both. That would only confuse users and leaves us with tricky situations like what to do when both are set and are not consistent?

If we want to stick with datacontentype (only), then we would need to

  1. live with its verbosity, and
  2. either break backwards compatibility by assuming the content type to be Unknown if datacontenttype is unset, or define a dedicated content type for Unknown/Unspecified_ instead.

WDYT?

@stevenhartley
Copy link
Contributor

2. unset

I think we leave it as is meaning we keep datacontenttype and if it is not present we assume it is UPAYLOAD_PROTOBUF_WRAPPED_IN_ANY.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change This pull request introduces a change to public API which is not backwards compatible enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants