Consider an API with a request object for Select

[Totktonada]

To be honest, I going to the docs each time to look at which positions offset, limit, iterator type and key reside. I would like to offer a more friendly API, where one can leave most of options by default and provide just space and key to make EQ request on a primary index of the space without offset and limit.

Of course, I want this feature to work in parallel with existing API and don't break anything.

One of possible approaches is construction of a request object and passing it to a Select variant. See tarantool/tarantool-java#247 for example.

Ideas about the best API are welcome.

[oleg-jukovec]

Request type + the builder pattern + Connection.Exec()/Connection.ExecAsync() methods.

Something similar has been implemented by FZamiba's fork. But it doesn't use the builder pattern to build a request.

But we can do it:

request := conn.Select(space)
                         .Index(I)
                         .Offset(O)
                         .Limit(L)
                         .Iterator(I)
                         .Key(k)
                         .Build()

response := conn.Exec(request)
future := conn.ExecAsync(request)
conn.ExecTyped(&tuple)

An advantage: it will be easy to extend public API and to add a callback for the synchronous case of IPROTO_PUSH (or something else in the future), see #67:

request := conn.Select(space)
                         ...
                         .OnPush(callback)
                         .OnPushCtx(callbackCtx)
                         .SomethingElse()
                         .Build()

[oleg-jukovec]

This is a more detailed proposal.

For the base Request class, we can take a FZambia's implementation:

I have added some comments to the FZambia's type:

type Request struct {
	// the field not required and may be deleted in our implementation
	requestCode int32
	// ok
	sendFunc    func(conn *Connection) (func(enc *msgpack.Encoder) error, error)
	// something like that can be added at the future for handle push messages
	// in synchonous case
	push        func([]interface{})
	pushTyped   func(func(interface{}) error)
}

The first approach it to add several methods and fields into the Request struct

type Request struct {
	requestCode int32
	...
	// args
	space, index, key interface{}
	offset, limit, iterator uint32 
}

func (req *Request) Offset(offset uint32) {
	req.offset = offset
}
//...
func (req *Request) Key(key interface{}) {
	req.key = key
}

For a client code, it will look like that:

request := conn.Select(space)
               .Offset(10)
               .Limit(20)
               .Iterator(IterEq)
               .Key(key)
resp, err := conn.Exec(request) // 'build' request and executes

The problems begin if we will want to extend the approach to all Connection's methods. As can be seen from the table, all Connection's methods have different arguments:

argument	used by
space	Select, Insert, Replace, Delete, Update, Upsert
index	Select, Delete, Update
offset	Select
limit	Select
iterator	Select
key	Select, Delete, Update
tuple	Insert, Replace, Upsert
ops	Update, Upsert
functionName	Call, Call17
expr	Eval, Execute
args	Call, Call17, Eval, Execute

All of them will need to be added to the Request type. For some of them (which are not mandatory), we will need to write a setter. The user of public API will be even more confused than before. He will not understand which setter can be called for a request. IDE will not help to him.

It will be difficult to maintain and extend this approach.

The second possible approach is to extend the basic Request type

type SelectRequest struct {
	Request
	space, index, key interface{}
	offset, limit, iterator uint32
}
func (req *SelectRequest) Index(index interface{}) {
	req.index = index
}
//...
func (req *SelectRequest) Key(key interface{}) {
	req.key = key
}
//...
type UpdateRequest struct {
	Request
	space, index, key, ops interface{} 
}
func (req *UpdateRequest) Index(index interface{}) {
	req.index = index
}
//...
func (req *UpdateRequest) Key(key interface{}) {
	req.key = key
}

For a client code, it will look like that:

// returns SelectRequest
request := conn.Select(space)
               .Offset(10)
               .Limit(20)
               .Iterator(IterEq)
               .Key(key)
resp, err := conn.Exec(request) // 'build' request and executes

There will be a lot of duplicate code and reflection inside the Connection.Exec* methods. But the approach is extensible and IDE will help to user of public API.

The third approach is to use The Builder design pattern

type RequestBuilder struct {
	// some common fields
	push        func([]interface{})
}
// some common functions
func (builder *RequestBuilder) OnPush(callback func([]interface{})) {
	builder.push = push
}
// a concrete builder
type SelectRequestBuilder struct {
	RequestBuilder
	space, index, key interface{}
	offset, limit, iterator uint32
}
func (build *SelectRequestBuilder) Index(index interface{}) {
	req.index = index
}
//...
func (builder *SelectRequestBuilder) Key(key interface{}) {
	req.key = key
}
func (builder *SelectRequestBuilder) Build() (req *Request, err error) {
	// some check and return err if fails
	return &Request {
		requestCode = InsertCode,
		sendFunc = ...,
	}
}

For a client code, it will look like that:

request, err := conn.SelectBuilder(space)
                    .Offset(20)
                    .Limit(10)
                    .Iterator(IterEq)
                    .Key(key)
                    .Build()
if err != nil {
	// can't build the response
} else {
	conn.Exec(request)
}

There will be some of duplicate code, but no reflection inside Connection.Exec* methods. The approach is extensible and IDE will help to user of public API.

Conclusion

I like the third approach. It seems over-engineering, but this is a familiar design pattern. The second approach is a compromise between complexity and extensibility. It seems to me that the first approach is the worst.

[rybakit]

@oleg-jukovec Have you considered the following api:

resp, err := client.Select(space, criteria
               .Offset(10)
               .Limit(20)
               .Iterator(IterEq)
               .Key(key))

It is as simple as the second approach, but it has no of its disadvantages (such as the use of reflection). You can extend this approach to other methods as well:

resp, err := client.Update(space, ops.Add(1, 5).Subtract(2, 4))

P.S. I'm not sure why you use conn.Exec(request) in your examples. AFAIU, this issue is about IPROTO select, but conn.Exec (or conn.Execute) is for executing arbitrary SQL statements.

[oleg-jukovec]

@oleg-jukovec Have you considered the following api:

resp, err := client.Select(space, criteria
               .Offset(10)
               .Limit(20)
               .Iterator(IterEq)
               .Key(key))

Yeah, I' ve been thinking about that too. It will be more like Lua-code implementation, but we will must to duplicate each call with *Typed(), *Async() versions: Select/SelectTyped/SelectAsync, Update/UpdateTyped/SelectAsync, etc.

It is as simple as the second approach, but it has no of its disadvantages (such as the use of reflection).

I think, we can get avoid of reflection in the second approach by using an interface (as in the Java implementation above). The updated version of the second approach (I could have incorrectly set the arguments and return type of the SendFunc function, but it does not matter):

type Request interface {
    SendFunc func(conn *Connection) (func(enc *msgpack.Encoder) error, error)
}
type BaseRequest struct {
	// some common fields
	requestCode int32
	push        func([]interface{})
	pushTyped   func(func(interface{}) error)
}
// some common methods
func (req *BaseRequest) OnPush(push func([]interface)) {
     req.push = push
}
// a concrete request
type SelectRequest struct {
	BaseRequest
	space, index, key interface{}
	offset, limit, iterator uint32
}
func (req *SelectRequest) Index(index interface{}) {
	req.index = index
}
//...
func (req *SelectRequest) Key(key interface{}) {
	req.key = key
}
//...
func (req *UpdateRequest) SendFunc(conn *Connection) (func(enc *msgpack.Encoder) error, error) {
}
// an another request
type UpdateRequest struct {
	BaseRequest
	space, index, key, ops interface{} 
}
func (req *UpdateRequest) Index(index interface{}) {
	req.index = index
}
//...
func (req *UpdateRequest) Key(key interface{}) {
	req.key = key
}
func (req *UpdateRequest) SendFunc(conn *Connection) (func(enc *msgpack.Encoder) error, error) {
}

P.S. I'm not sure why you use conn.Exec(request) in your examples. AFAIU, this issue is about IPROTO select, but conn.Exec (or conn.Execute) is for executing arbitrary SQL statements.

Thank you for noticing,I took the naming from FZambia's implementation. In our implementation It can be Do:

conn.Do(request)
conn.DoTyped(request, &tup)
conn.DoAsync(request)
conn.DoEtc(request)

A client code:

request, err := conn.Select(space)
                    .Offset(20)
                    .Limit(10)
                    .Iterator(IterEq)
                    .Key(key)
if err != nil {
	// can't build the response
} else {
	conn.Do(request)
}

[Totktonada]

I would prefer a 'request objects' implementation over a 'conditions and update statements objects', because the former allows more flexibility in a future. Say, it may allow us to make .OnPush() (#67) and .WithContext() (#48) APIs more convenient.