KaymeKaydex / go-vshard-router

go vshard-router implementation for tarantool
MIT License
21 stars 3 forks source link

A problem with decoding vshard storage answers #42

Closed nurzhan-saktaganov closed 2 months ago

nurzhan-saktaganov commented 3 months ago

Problem 1

RouterMapCallRWImpl lacks checks for len(respData) before addressing its elements.

For example, here https://github.com/KaymeKaydex/go-vshard-router/blob/84c49a6fdfd472916879a9e6cef22c60df380139/api.go#L295

and here https://github.com/KaymeKaydex/go-vshard-router/blob/84c49a6fdfd472916879a9e6cef22c60df380139/api.go#L298

and so on...

Problem 2

In ReplicaCall, we consider an error if len(respData) != 2. But, if fnc is completed successfully but does not return anything (i.e. returns nil), vshard does not send the second element (i.e. does not send nil):

https://github.com/KaymeKaydex/go-vshard-router/blob/84c49a6fdfd472916879a9e6cef22c60df380139/replicaset.go#L109

The fix is expected to be similar to PR #35.

nurzhan-saktaganov commented 3 months ago

The same problem here:

https://github.com/KaymeKaydex/go-vshard-router/blob/84c49a6fdfd472916879a9e6cef22c60df380139/discovery.go#L184

nurzhan-saktaganov commented 3 months ago

There is another more recommended patch from colleague

From 58a807944e510f0fe403fc081e10f98f01ebe726 Mon Sep 17 00:00:00 2001
From: Vladimir Patikov <v.patikov@corp.mail.ru>
Date: Tue, 3 Sep 2024 17:32:50 +0300
Subject: [PATCH] ReplicaCall response parsing fix

---
 replicaset.go | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/replicaset.go b/replicaset.go
index e7f4a00..d5e880f 100644
--- a/replicaset.go
+++ b/replicaset.go
@@ -119,12 +119,12 @@ func (rs *Replicaset) ReplicaCall(
            continue
        }

-       if len(respData) != 2 {
-           err = fmt.Errorf("invalid length of response data: must be = 2, current: %d", len(respData))
+       if len(respData) < 1 {
+           err = fmt.Errorf("response data is empty")
            continue
        }

-       if respData[1] != nil {
+       if len(respData) > 1 && respData[1] != nil {
            assertErr := &StorageCallAssertError{}

            err = mapstructure.Decode(respData[1], assertErr)
-- 
1.8.3.1
nurzhan-saktaganov commented 2 months ago

We still have decoding problems in func (rs *Replicaset) ReplicaCall, there will be another PR with solution