GODRIVER-3058 Centralize x-package Connection interface as a struct #1475
Conversation
prestonvasquez
requested review from
qingyang-hu
and removed request for
a team
prestonvasquez
requested review from
matthewdale
and removed request for
qingyang-hu and
matthewdale
prestonvasquez
changed the title
API Change Report./x/mongo/driverincompatible changesCompressor: removed ./x/mongo/driver/authincompatible changes##Config.Connection: changed from ./x/mongo/driver.Connection to *./x/mongo/driver/mnet.Connection ./x/mongo/driver/drivertestincompatible changes(*ChannelConn).ReadWireMessage: removed compatible changes(*ChannelConn).Read: added ./x/mongo/driver/mnetcompatible changespackage added ./x/mongo/driver/operationincompatible changes##(*Hello).FinishHandshake: changed from func(context.Context, ./x/mongo/driver.Connection) error to func(context.Context, *./x/mongo/driver/mnet.Connection) error ./x/mongo/driver/sessionincompatible changesLoadBalancedTransactionConnection.ReadWireMessage: removed ./x/mongo/driver/topologyincompatible changes(*Connection).ReadWireMessage: removed compatible changes(*Connection).Read: added |
prestonvasquez
changed the title
prestonvasquez
changed the title
prestonvasquez
changed the title
| func newProcessErrorTestConn(wireVersion *description.VersionRange, stale bool) *processErrorTestConn { | ||
| return &processErrorTestConn{ | ||
| func newProcessErrorTestConn(t *testing.T, wireVersion *description.VersionRange, stale bool) *mnet.Connection { | ||
| t.Helper() |
There was a problem hiding this comment.
Is there a reason to call t.Helper here? Typically the purpose is to allow failed test assertions to point to a more relevant line number, but t is not used to make any assertions in this function.
x/mongo/driver/mnet/connection.go
Outdated
| type WireMessageReader interface { | ||
| Read(ctx context.Context) ([]byte, error) | ||
| } | ||
|
|
||
| // WireMessageWriter represents a Connection where server operations can be | ||
| // written to. | ||
| type WireMessageWriter interface { | ||
| Write(ctx context.Context, wm []byte) error | ||
| } |
There was a problem hiding this comment.
Optional: These interfaces are only used in WireMessageReadWriteCloser. Consider putting the Read and Write method definitions in that interface instead.
x/mongo/driver/mnet/connection.go
Outdated
|
|
||
| // WireMessageReadWriteCloser represents a Connection where server operations | ||
| // can read from, written to, and closed. | ||
| type WireMessageReadWriteCloser interface { |
There was a problem hiding this comment.
Optional: Since mnet is dedicated to MongoDB connections, the WireMessage in WireMessageReadWriteCloser is redundant. Consider condensing the type name to ReadWriteCloser.
| if describer, ok := component.(Describer); ok { | ||
| conn.Describer = describer | ||
| } |
There was a problem hiding this comment.
For all of the other interfaces (Streamer, Compressor, etc), code that calls the associated methods performs nil checks first. However, code that calls the Describer methods do not perform nil checks and just assumes it's available. That's a valid assumption for now, but could break unexpectedly.
There are a few options to fix that:
- Make the minimum interface be the union of
WireMessageReadWriteCloserandDescriber. - Add
nilchecks to all the calls toDescribermethods. - Add wrapper methods for all the
Describermethod calls that handle thenilcase. For example:
func (c *Connection) Description() description.Server {
if c.describer == nil {
return description.Server{}
}
return c.describer.Description()
}There was a problem hiding this comment.
I think unioning is the best solution if we are certain that objects need to describe the connection for which they are establishing. However, a Describer is a pretty complicated interface which makes constructing a connection more strict. For robustness, the other two solutions also seem like a good idea. What are your thoughts?
There was a problem hiding this comment.
I think 3. is probably the best change because it's easy and enables simpler connection implementations.
Edit: I see you went with option 1. We should keep that implementation for now because it should prevent bugs caused by a missing Description method, which is the main issue. These are internal APIs, so we can simplify things in the future without breaking changes.
| @@ -76,26 +75,15 @@ func (s TransactionState) String() string { | |||
| } | |||
| } | |||
|
|
|||
| var _ mnet.Pinner = (LoadBalancedTransactionConnection)(nil) | |||
There was a problem hiding this comment.
Optional: This seems redundant because the interface description below includes mnet.Pinner. Consider removing it.
| if !ok { | ||
| refConn := info.Connection.Pinner | ||
| if refConn == nil { | ||
| //debug.PrintStack() | ||
| return CursorResponse{}, fmt.Errorf("expected Connection used to establish a cursor to implement PinnedConnection, but got %T", info.Connection) |
There was a problem hiding this comment.
Optional: Make error message more correct.
| return CursorResponse{}, fmt.Errorf("expected Connection used to establish a cursor to implement PinnedConnection, but got %T", info.Connection) | |
| return CursorResponse{}, fmt.Errorf("expected Connection used to establish a cursor (type %T) to to support pinning, but it does not", info.Connection) |
| if !ok { | ||
| refConn := info.Connection.Pinner | ||
| if refConn == nil { | ||
| //debug.PrintStack() |
There was a problem hiding this comment.
Optional: Remove commented-out code.
| //debug.PrintStack() |
| @@ -499,7 +501,8 @@ func (bc *BatchCursor) getOperationDeployment() Deployment { | |||
| // handled for these commands in this mode. | |||
| type loadBalancedCursorDeployment struct { | |||
| errorProcessor ErrorProcessor | |||
| conn PinnedConnection | |||
| //conn PinnedConnection | |||
There was a problem hiding this comment.
Optional: Remove commented-out code.
| //conn PinnedConnection |
GODRIVER-3058
Summary
Move all of the connection logic from interfaces in the driver package to the
mnetpackage.Background & Motivation
Context
The driver.Connection interface is returned by various functions in the x package, which is a pattern in conflict with the Go idiom of "accept interfaces, return structs" defined in the wiki . And generally violates the robustness principle.
More importantly, this change will allow us to centralize the usage of description.Server. If we were to migrate the description package to the internal package, then we could create a subset of the fields from description.Server that are required in the experimental API in the mnet package:
Functions that implement the
Describerwill need to change their signature to usemnet.Serverand apply the internal analogue under the hood to the entailed methods.