Permit per-connection Callbacks to allow per-connection state

[andykellr]

This adds a ConnectionHandler field to the ConnectionResponse struct. If nil, the Callbacks provided with StartSettings would be used.

I don't have time at the moment to provide a working demonstration of maintaining per-connection state, but hopefully this is obvious.

The implementation would look something like this:

// connectionState contains any state the implementer wishes to preserve per-connection.
// It also implements types.ConnectionCallbacks
type connectionState struct {
	connectedAt time.Time
}

func (c *connectionState) OnConnected(conn types.Connection) {
	c.connectedAt = time.Now()
}

func (c *connectionState) OnMessage(conn types.Connection, message *protobufs.AgentToServer) *protobufs.ServerToAgent {
	// TODO: real message handling
	return &protobufs.ServerToAgent{}
}

func (c *connectionState) OnConnectionClose(conn types.Connection) {
}

The callbacks passed to StartSettings.Settings would construct a new connectionState in OnConnecting.

callbacks := CallbacksStruct{
	OnConnectingFunc: func(request *http.Request) types.ConnectionResponse {
		return types.ConnectionResponse{
			Accept:             true,
			ConnectionCallbacks:  &connectionState{},
		}
	},
}

[codecov]

Codecov Report

Base: 76.11% // Head: 75.54% // Decreases project coverage by -0.57% ⚠️

Coverage data is based on head (74b5957) compared to base (efddaa2).
Patch coverage: 92.30% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #148      +/-   ##
==========================================
- Coverage   76.11%   75.54%   -0.57%     
==========================================
  Files          24       24              
  Lines        1834     1865      +31     
==========================================
+ Hits         1396     1409      +13     
- Misses        326      339      +13     
- Partials      112      117       +5     

Impacted Files	Coverage Δ
client/internal/mockserver.go	82.35% <85.71%> (+0.31%)	⬆️
server/serverimpl.go	58.92% <94.73%> (+0.37%)	⬆️
client/httpclient.go	95.00% <100.00%> (+0.26%)	⬆️
client/internal/httpsender.go	72.04% <100.00%> (+1.08%)	⬆️
server/callbacks.go	68.18% <100.00%> (-4.55%)	⬇️
client/internal/receivedprocessor.go	69.28% <0.00%> (-15.00%)	⬇️
client/wsclient.go	89.02% <0.00%> (+4.26%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[tigrannajaryan]

I like this due to stronger typing.

Earlier I said for WebSocket we may want to avoid keeping this but I think in real world scenarios you inevitably need to keep carry this state for any transport. You typically authenticate in OnConnecting and need to save the tenant id that you get as a result of authenticating somewhere since it is necessary later in OnMessage.

Should we get rid of the callback in Settings and replace it with this per connection Callbacks?

[andykellr]

I like this due to stronger typing.

I agree.

Earlier I said for WebSocket we may want to avoid keeping this but I think in real world scenarios you inevitably need to keep carry this state for any transport. You typically authenticate in OnConnecting and need to save the tenant id that you get as a result of authenticating somewhere since it is necessary later in OnMessage.

Good example. I think it is critical that we at least figure out a way to connect OnConnecting and OnMessage and this approach feels right.

Should we get rid of the callback in Settings and replace it with this per connection Callbacks?

I considered that, but wanted to make sure this PR was small and easy to understand. Changing the callbacks requires changes to the CallbacksStruct, etc.

Edit: Before actually merging this PR, I plan to add tests for the specific behavior and am happy to refactor the Callbacks.

One benefit of this implementation is compatibility with existing configurations, but there probably aren't many out there and if the same implementation is used for both callbacks, it's easy enough to return it in the ConnectionResponse.

[andykellr]

@tigrannajaryan I finally had a chance to refactor this PR to separate the Callbacks and ConnectionCallbacks and update the tests. Please review.

[tigrannajaryan]

Generally I like this direction, just left some minor comments.

[tigrannajaryan]

Please add some comments to explain how the ConnectionHandler field in the returned struct is used, can it be nil or must be set, etc. Or perhaps add comments to the ConnectionResponse struct to explain it there.

[andykellr]

Added that it MUST be set if Accept=true and added a comment to the ConnectionCallbacks interface.

[tigrannajaryan]

Nit: should the field be also named ConnectionCallbacks? Or rename ConnectionCallbacks struct to ConnectionHandler?
The names being different somehow is not helping me to read.

[tigrannajaryan]

We probably need to document somewhere that the connection callback is required for plain http connections.

[tigrannajaryan]

@andykellr I think this can be merged.