cloudevents/sdk-go

WritePubSubMessage requires pubsubMessage to have Attributes

toshi38 opened this issue · 4 comments

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

}

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.

@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

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

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?