provide publish/subscribe options for MQTT protocol

[yanmxa]

Signed-off-by: myan myan@redhat.com

Provide more flexible parameter configurations when using the MQTT protocol to send events.

[yanmxa]

1. Why someone would use the context vs. New() to configure these settings?

When using MQTT sends a message, you can configure many parameters according to your needs. We provide the ProtocolConext here to support users to pass all the parameters to MQTT publisher. For example, let's create a ProtocolContext and then add it to the ctx.

  qos := 0
  retained := false
  packetId := uint16(123)
  properties := &paho.PublishProperties{
      TopicAlias: paho.Uint16(3),
  }
  pctx := cemqtt.ProtocolContext{
       Topic: "test",
        QoS : &qos,
        Retained: &retained,
        PacketID: &packetId,
        PublishProperties: properties
  }
  ctx = cemqtt.WithProtocolContext(ctx, pctx)

But in most cases, users only specify topic/qos/retain when publishing messages, and do not set packet-related parameters, so I provide a New method here to deal with such cases

  if result := c.Send(
      cemqtt.WithProtocolContext(ctx, cemqtt.NewProtocolContext("test-topic", 0, false)),
      e,
  );

2. Whether there generic MQTT options/property struct we should allow (use) to influence all parameters in Send() instead of handpicking a couple of options?

Yes. There is a generic MQTT option struct when sending the message, But in its implementation, both payload and properties are placed in the struct. That's the reason I created the ProtocolContext to map all the properties, instead of using its own option struct.

[embano1]

1. Why someone would use the context vs. New() to configure these settings?

When using MQTT sends a message, you can configure many parameters according to your needs. We provide the ProtocolConext here to support users to pass all the parameters to MQTT publisher. For example, let's create a ProtocolContext and then add it to the ctx. But in most cases, users only specify topic/qos/retain when publishing messages, and do not set packet-related parameters, so I provide a New method here to deal with such cases

I don't think this is the right use case for Context here. We generally use a specific Context to overwrite certain behavior for individual Send calls. In your case, I'd suggest using a default New constructor with functional options. I'm not 100% clear on whether the topic value will always come from cecontext or must be passed to New(). WDYT?

2. Whether there generic MQTT options/property struct we should allow (use) to influence all parameters in Send() instead of handpicking a couple of options?

Yes. There is a generic MQTT option struct when sending the message, But in its implementation, both payload and properties are placed in the struct. That's the reason I created the ProtocolContext to map all the properties, instead of using its own option struct.

I just checked our implementation and IMHO it would be fine to have the Context use the whole struct assuming a user would know how to use it (since he's intentionally overwriting behavior) and would not pass a payload (which would anyways be overwritten during SetData).

[yanmxa]

1. Why someone would use the context vs. New() to configure these settings? When using MQTT sends a message, you can configure many parameters according to your needs. We provide the ProtocolConext here to support users to pass all the parameters to MQTT publisher. For example, let's create a ProtocolContext and then add it to the ctx. But in most cases, users only specify topic/qos/retain when publishing messages, and do not set packet-related parameters, so I provide a New method here to deal with such cases

I don't think this is the right use case for Context here. We generally use a specific Context to overwrite certain behavior for individual Send calls. In your case, I'd suggest using a default New constructor with functional options. I'm not 100% clear on whether the topic value will always come from cecontext or must be passed to New(). WDYT?

2. Whether there generic MQTT options/property struct we should allow (use) to influence all parameters in Send() instead of handpicking a couple of options? Yes. There is a generic MQTT option struct when sending the message, But in its implementation, both payload and properties are placed in the struct. That's the reason I created the ProtocolContext to map all the properties, instead of using its own option struct.

I just checked our implementation and IMHO it would be fine to have the Context use the whole struct assuming a user would know how to use it (since he's intentionally overwriting behavior) and would not pass a payload (which would anyways be overwritten during SetData).

Agree with that. we can keep using function options with other parameters. At the same time, discard protocolContext and use the struct option as the value.

[yanmxa]

@embano1 If I append the mqtt function option in the file v1/context/context.go, then we have to update so many go.mod and go.sum files just like this PR. Another way is to add this MQTT function option to the directory protocol/mqtt_phao/v2, this only requires updating a few files. So which one do you think is better?

[embano1]

Sorry, I think I may have confused you somehow. I did not suggest using cecontext for this, but to implement everything in func New() of the protocol API e.g., using functional options and defaults. See http protocol implementation for how this is done there.

[embano1]

@yanmxa looks like you reverted to the mqtt context approach instead of what I meant following other protocol implementations e.g., http. Can you please review https://github.com/cloudevents/sdk-go/blob/3dfc0335ba8a9ed559fda3d32ea1b2a18b17f5da/v2/protocol/http/protocol.go#L95 and see if this approach would work for your proposal? I know it's a breaking change but we haven't cut a release with mqtt anyways so strictly it's non-breaking.

[yanmxa]

Thanks @embano1! I misunderstood before. Also, I think we can support setting the options in both theNew() protocol and Send()(cause the settings sent each time may be different from the ones at initialization) at the same time. What do you think?

[embano1]

Thanks @embano1! I misunderstood before.

Also, I think we can support setting the options in both theNew() protocol and Send()(cause the settings sent each time may be different from the ones at initialization) at the same time. What do you think?

Send is an interface implementation. For those cases context is typically used. Can we start with New() in this PR and create a follow up PR if needed?

[embano1]

@yanmxa what do you think of using functional options in Mew() like other protocols do to reduce API surface, allow for API evolution, set defaults, etc.?

[yanmxa]

@yanmxa what do you think of using functional options in Mew() like other protocols do to reduce API surface, allow for API evolution, set defaults, etc.?

@embano1 Do you mean define the functional options in New() like this?

Now MQTT protocol mainly has four parts option struct. In this way, we can reduce to provide only one necessary clientConfig each time, and then override or add other options as needed. This is a great idea! I'll try to implement it tomorrow.

[embano1]

Great! Excited!

[embano1]

Are there any doc-related changes needed for this?

[yanmxa]

Great work! Can you please squash the commits?

Done!

[yanmxa]

Are there any doc-related changes needed for this?

The protocol here has not changed, and it seems that there is no need to change the document now.