v2: ability to mock Connector.Do()/Future/Response in tests

[oleg-jukovec]

At now it is required to write integration tests with Tarantool to test the code written using go-tarantool. I saw this problem while writing tests for tt. I couldn't mock the workflow:

ret := some_code_to_test(mockConnectorObject) {
    // some code to test
    future := mockConnectorObject.Do(someRequest)
    mock_response, mock_err := future.Get()
    // more code to test
}
if ret != expected {
    t.Errorf("test failed")
}

It would be great to have that ability.

[oleg-jukovec]

At now the code must be changed when adding new requests.

go-tarantool/response.go

Lines 112 to 143 in 265f96c

	func (resp *Response) decodeHeader(d *decoder) (err error) {
	var l int
	d.Reset(&resp.buf)
	if l, err = d.DecodeMapLen(); err != nil {
	return
	}
	for ; l > 0; l-- {
	var cd int
	if cd, err = resp.smallInt(d); err != nil {
	return
	}
	switch cd {
	case KeySync:
	var rid uint64
	if rid, err = d.DecodeUint64(); err != nil {
	return
	}
	resp.RequestId = uint32(rid)
	case KeyCode:
	var rcode uint64
	if rcode, err = d.DecodeUint64(); err != nil {
	return
	}
	resp.Code = uint32(rcode)
	default:
	if err = d.Skip(); err != nil {
	return
	}
	}
	}
	return nil
	}

go-tarantool/response.go

Lines 145 to 248 in 265f96c

	func (resp *Response) decodeBody() (err error) {
	if resp.buf.Len() > 2 {
	offset := resp.buf.Offset()
	defer resp.buf.Seek(offset)
	var l, larr int
	var stmtID, bindCount uint64
	var serverProtocolInfo ProtocolInfo
	var feature ProtocolFeature
	var errorExtendedInfo *BoxError = nil
	d := newDecoder(&resp.buf)
	if l, err = d.DecodeMapLen(); err != nil {
	return err
	}
	for ; l > 0; l-- {
	var cd int
	if cd, err = resp.smallInt(d); err != nil {
	return err
	}
	switch cd {
	case KeyData:
	var res interface{}
	var ok bool
	if res, err = d.DecodeInterface(); err != nil {
	return err
	}
	if resp.Data, ok = res.([]interface{}); !ok {
	return fmt.Errorf("result is not array: %v", res)
	}
	case KeyError:
	if errorExtendedInfo, err = decodeBoxError(d); err != nil {
	return err
	}
	case KeyError24:
	if resp.Error, err = d.DecodeString(); err != nil {
	return err
	}
	case KeySQLInfo:
	if err = d.Decode(&resp.SQLInfo); err != nil {
	return err
	}
	case KeyMetaData:
	if err = d.Decode(&resp.MetaData); err != nil {
	return err
	}
	case KeyStmtID:
	if stmtID, err = d.DecodeUint64(); err != nil {
	return err
	}
	case KeyBindCount:
	if bindCount, err = d.DecodeUint64(); err != nil {
	return err
	}
	case KeyVersion:
	if err = d.Decode(&serverProtocolInfo.Version); err != nil {
	return err
	}
	case KeyFeatures:
	if larr, err = d.DecodeArrayLen(); err != nil {
	return err
	}
	serverProtocolInfo.Features = make([]ProtocolFeature, larr)
	for i := 0; i < larr; i++ {
	if err = d.Decode(&feature); err != nil {
	return err
	}
	serverProtocolInfo.Features[i] = feature
	}
	default:
	if err = d.Skip(); err != nil {
	return err
	}
	}
	}
	if stmtID != 0 {
	stmt := &Prepared{
	StatementID: PreparedID(stmtID),
	ParamCount: bindCount,
	MetaData: resp.MetaData,
	}
	resp.Data = []interface{}{stmt}
	}
	// Tarantool may send only version >= 1
	if (serverProtocolInfo.Version != ProtocolVersion(0)) || (serverProtocolInfo.Features != nil) {
	if serverProtocolInfo.Version == ProtocolVersion(0) {
	return fmt.Errorf("No protocol version provided in Id response")
	}
	if serverProtocolInfo.Features == nil {
	return fmt.Errorf("No features provided in Id response")
	}
	resp.Data = []interface{}{serverProtocolInfo}
	}
	if resp.Code != OkCode && resp.Code != PushCode {
	resp.Code &^= ErrorCodeBit
	err = Error{resp.Code, resp.Error, errorExtendedInfo}
	}
	}
	return
	}

It's better to redesign the interface (add ability to decode header/body per request?) a bit and get rid of it. This may also be related to the issue:

#129

[oleg-jukovec]

It would be also nice to highlight/fix that Future.Err() does not return an error for all cases. From the Telegram chat:

-- доброго дня, странная ситуация. видимо кончилась память, узнал я об этом только через консоль тарантула. при всем при этом гошный коннектор ошибку не выдает а просто принимает данные как будто все хорошо. не подскажите куда копать что бы ошибка явно приходила? (the user uses Future.Err() to check the result of a request).
-- future.Err() не декодирует тело ответа msgpack, а ошибка может быть там. Нужно future.Get()/future.GetTyped() вызвать и ловить ошибку от них.
-- ого. очень и очень не очевидно. спасибо

[DerekBum]

As a first priority task we want to do something with the Response struct. Here's some thoughts on how to redesign this struct:

The first approach is to create the Response interface.

We can create an interface:

type Response interface {
	Decode() ([]interface{}, error)
	DecodeTyped(res interface{}) error

	Pos() ([]byte, error) // just []byte
	MetaData() ([]ColumnMetaData, error) // just []ColumnMetaData
	SQLInfo() (SQLInfo, error) // just SQLInfo
}

And make the current Response implementation a part of it:

type baseResponse struct {
	requestId uint32
	code      uint32
	// Error contains an error message.
	error string
	buf   smallBuf
	data []interface // how it is already
	sqlInfo // // how it is already
	metadata // // how it is already
	pos // how it is already
}

After that, some changes to the Future methods needs to be done: all methods that return *Response needs to return just the Response. User could access old Pos, MetaData and SQLInfo fields via Pos(), MetaData() or SQLInfo methods call respectively.
Also, we could redesign Get() and GetTyped methods for the Future to return Data from the response (and not the response itself). To get the response new method could be added: GetResponse.

As example:

data, err := conn.Do(...).Get()
if err != nil {
        ...
}
// Do something with the response data.

resp, err := conn.Do(...).GetResponse()
if err != nil {
        ...
}
sqlInfo, err := resp.SQLInfo()
if err != nil {
        ...
}
// Do something with the response sql info.

Same goes with the Pos and MetaData.

The second approach is to create the Response interface and create different Response types for each Request type.

The problem with the first approach is that every request will have Pos, MetaData and SQLInfo methods regardless the actual need in them. Responses on some requests does not have those fields, some have only part of them.

So the idea is to create something like current Requests structure:

go-tarantool/request.go

Lines 820 to 824 in b2b800b

	type baseRequest struct {
	rtype iproto.Type
	async bool
	ctx context.Context
	}

go-tarantool/request.go

Lines 841 to 844 in b2b800b

	type spaceRequest struct {
	baseRequest
	space interface{}
	}

go-tarantool/request.go

Lines 850 to 853 in b2b800b

	type spaceIndexRequest struct {
	spaceRequest
	index interface{}
	}

All in all, the response structure will be something like that:

type Response interface {
	Decode() ([]interface{}, error)
	DecodeTyped(res interface{}) error
}

type baseResponse struct {
	requestId uint32
	code      uint32
	// Error contains an error message.
	error string
	buf   smallBuf
}

type SelectResponse struct {
	baseResponse
        pos []byte
}

type ExecuteResponse struct {
	baseResponse
        metaData []ColumnMetaData
        sqlInfo []SQLInfo
}

But, there is also a downside from the user experience. Since the interface is minimalistic, to get the Pos, MetaData or SQLInfo, the user needs to try to convert the Request instance to the corresponding type first.
As an example:

resp, err := conn.Do(InsertRequest).GetResponse()
if err != nil {
        ...
}
data, err := resp.Decode()
if err != nil {
        ...
}
// Do something with the response data.

Data is easy accessible for every kind of response (no need to convert to anything). However,

resp, err := conn.Do(SelectRequest).GetResponse()
if err != nil {
        ...
}
selectResp, err := resp.(SelectResponse)
if err != nil {
        ...
}
pos := selectResp.GetPos()
// Do something with the response pos.

resp, err := conn.Do(ExecuteRequest).GetResponse()
if err != nil {
        ...
}
selectResp, err := resp.(ExecuteResponse)
if err != nil {
        ...
}
metaInfo := selectResp.GetMetaInfo()
sqlInfo := selectResp.GetSQLInfo()
// Do something.

There is no performance penalty in this method compared to the previous one.

For the developer-side: we also could create for each Request a new method: CreateResponse([]byte) (Response, error) that will return corresponding response.

What about user experience?

Which method is more user-friendly? (in case of any more discovered approaches to this problem, I will add them to this Issue).