Skip to content
This repository has been archived by the owner on Dec 14, 2020. It is now read-only.

Support for multiple data sections #38

Closed
vcabbage opened this issue Feb 15, 2018 · 4 comments
Closed

Support for multiple data sections #38

vcabbage opened this issue Feb 15, 2018 · 4 comments
Assignees

Comments

@vcabbage
Copy link
Owner

vcabbage commented Feb 15, 2018

Per discussion in #35, multiple data section support would be helpful for some use cases.

The implementation shouldn't be too difficult, but I'm not sure how the API should look.

Though not a firm requirement, I'd like to leave Message.Data as is. I suspect a single data payload is the most common case.


Idea A: Add an AdditionalData [][]byte field.

  • This would require the consumer to process data from Message.Data and Message.AdditionalData, which isn't particularly ergonomic but I don't think it's terrible either.

Idea B: Add MultiData [][]byte, where the first element is the same same as Message.Data.

  • I see this is a bit more appealing to a receiver as they would only have to deal with Message.Data or Message.MultiData instead of both.
  • It's not clear how a send operation would deal with Message.Data and Message.MultiData where the first MultiData element differs from Data. The API would likely end up asymmetric.

Idea C: A variation on the above MultiData [][]byte approach could be to only set Data or MultiData.

  • Attempting to send with both fields set would result in an error.

After rubber-ducking in this issue, I'm favoring Idea C. Other input is welcome.

/cc @devigned

@vcabbage vcabbage self-assigned this Feb 15, 2018
@devigned
Copy link
Contributor

I prefer C as well. I think the either, but not both, strategy disambiguates what is happening on the wire. The return of an error in the case of both instructs a sender without any odd side effects due to ambiguous usage.

@vcabbage
Copy link
Owner Author

I implemented C in https://github.com/vcabbage/amqp/tree/multidata

I'm not sold on the approach. Taking the case of batching, presumably it's possible for a batch to contain only a single data payload. In that case it would end up in the Data field and the consuming code would be required to check both Data and MultiData.

Going to think on this a bit longer to see if I (or anyone else) come up with a better approach.

@vcabbage
Copy link
Owner Author

I'm starting to think changing Message.Data to Data [][]byte is the right API choice. It removes all the issues of the above ideas.

The reason I didn't want to change Message.Data was that it is currently easy to handle the first data payload as json.Unmarshal(msg.Data, T). If it became Data [][]byte, safe code would be required to check the length of Data before doing json.Unmarshal(msg.Data[0], T).

This can be mitigating by providing an accessor method like:

func (m *Message) GetData() []byte {
    if len(m.Data) < 1 {
        return nil
    }
    return m.Data[0]
}

Then the consumer can do json.Unmarshal(msg.GetData(), T).

Other than the breaking change, I don't see a significant downside to this approach.

@vcabbage
Copy link
Owner Author

I updated the multidata branch with my latest idea for this API.

An unfortunate side effect of this approach is that constructing a message with a single payload requires a little more work.

msg := &amqp.Message{
	Data: data,
}

becomes

msg := &amqp.Message{
	Data: [][]byte{data},
}

To mitigate this I've added a constructor function:

// NewMessage returns a *Message with data as the payload.
//
// This constructor is intended as a helper for basic messages with a
// single data payload. It is valid to construct a message directly for
// more complex usages.
func NewMessage(data []byte) *Message {
	return &Message{
		Data: [][]byte{data},
	}
}

Since I don't personally use the sending functionality of this lib outside it's own integration tests, I'm not sure how useful the constructor will be in practice.

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

No branches or pull requests

2 participants