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

WritePubSubMessage requires pubsubMessage to have Attributes #1092

Open
toshi38 opened this issue Aug 29, 2024 · 4 comments
Open

WritePubSubMessage requires pubsubMessage to have Attributes #1092

toshi38 opened this issue Aug 29, 2024 · 4 comments

Comments

@toshi38
Copy link

toshi38 commented Aug 29, 2024

Since the change in #976 it seems that Attributes property needs to be defined on the output message prior to calling WritePubSubMessage previously this wasn't strictly needed (at least it didn't exist this way in our code).

Which raises the question, should I need to have an Attributes field prior to calling WritePubSubMessage() or should it work with an empty message?

Alternatively is there a proper way that I should instantiate a pubsub.Message other than how I have below?

Broken:

import("github.com/cloudevents/sdk-go/v2/binding")
	
func writeMsg(ctx context.Context, in binding.Message) {
  msg := &pubsub.Message{}

  err := WritePubSubMessage(ctx, in, msg);  // Results in Panic
  
  //Do something with msg

}

Working:

import("github.com/cloudevents/sdk-go/v2/binding")
	
func writeMsg(ctx context.Context, in binding.Message) {
msg := &pubsub.Message{
	Attributes: AttributesFrom(ctx),
}

  err := WritePubSubMessage(ctx, in, msg); // Does not result in Panic
  
  //Do something with msg

}

@jannemagnusson
Copy link

jannemagnusson commented Aug 29, 2024

Calling WritePubSubMessage will eventually lead to a call of message.ReadBinary(). This method tries to iterate over the Attribute map that could be uninitialized since the binding.Start() method no longer adds an empty map.
As far as I can tell, there should be a 'nil' check before trying to iterate over the map.
There is actually a whole slew of potential access violations where the Attributes struct member is accessed without nil check.

@embano1
Copy link
Member

embano1 commented Sep 1, 2024

@toshi38 thx for flagging! Quick question: what's your use case for directly using the low-level protocol calls instead of the higher-level protocol Send or CloudEvents abstractions? I agree this was a breaking change on the protocol implementation which we should have flagged in the release notes.

cc/ @AndreasBergmeier6176

@jannemagnusson
Copy link

tbh, the likely answer is, we didn't know better.

@embano1
Copy link
Member

embano1 commented Sep 2, 2024

OK. So would you be open to adapt the "recommended" approach using the higher-level components the SDK offers? Yes, it's also a code change, but perhaps for the better?

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

No branches or pull requests

3 participants