cloudevents / sdk-go

Go SDK for CloudEvents
https://cloudevents.github.io/sdk-go/
Apache License 2.0
835 stars 220 forks source link

WritePubSubMessage requires pubsubMessage to have Attributes #1092

Open toshi38 opened 2 months ago

toshi38 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

@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 commented 2 months ago

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

embano1 commented 2 months ago

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?