From f3760c1a98810e3b521617f67cd019ecaaa3498a Mon Sep 17 00:00:00 2001 From: Tatiana Nesterenko Date: Sun, 13 Aug 2023 21:33:49 +0100 Subject: [PATCH] rpcsrv: return ErrUnknownSession and ErrUnknownIterator Behaviour change. `terminatesession` returns ErrUnknownSession in case of impossibility of finding session, previously there was no-error response with `false` result. `traverseIterator`returns ErrUnknownSession in case of impossibility of finding session, previously there was no-error response with default result; `traverseIterator`returns ErrUnknownIterator, there were no such errors before. Accordingly to proposal: https://github.com/neo-project/proposals/pull/156 Also adding description of `traverseIterator` in docs/rpc.md. Signed-off-by: Tatiana Nesterenko --- docs/rpc.md | 5 ++++ pkg/services/rpcsrv/client_test.go | 6 +++-- pkg/services/rpcsrv/server.go | 28 +++++++++++++-------- pkg/services/rpcsrv/server_test.go | 40 +++++++++++++++++------------- 4 files changed, 49 insertions(+), 30 deletions(-) diff --git a/docs/rpc.md b/docs/rpc.md index 73b5b0a0b..13083e8e1 100644 --- a/docs/rpc.md +++ b/docs/rpc.md @@ -196,6 +196,11 @@ enabled in the server's protocol configuration. ##### `getnep11transfers` and `getnep17transfers` `transfernotifyindex` is not tracked by NeoGo, thus this field is always zero. +##### `traverseiterator` and `terminatesession` + +NeoGo returns an error when it is unable to find a session or iterator, unlike +the error-free C# response that provides a default result. + ##### `verifyProof` NeoGo can generate an error in response to an invalid proof, unlike diff --git a/pkg/services/rpcsrv/client_test.go b/pkg/services/rpcsrv/client_test.go index 73f36154f..a7e2b3784 100644 --- a/pkg/services/rpcsrv/client_test.go +++ b/pkg/services/rpcsrv/client_test.go @@ -1723,7 +1723,8 @@ func TestClient_IteratorSessions(t *testing.T) { require.True(t, ok) ok, err = c.TerminateSession(sID) - require.NoError(t, err) + require.Error(t, err) + require.ErrorIs(t, err, neorpc.ErrUnknownSession) require.False(t, ok) // session has already been terminated. }) t.Run("automatically", func(t *testing.T) { @@ -1746,7 +1747,8 @@ func TestClient_IteratorSessions(t *testing.T) { time.Duration(rpcSrv.config.SessionExpirationTime)*time.Second/4) ok, err := c.TerminateSession(sID) - require.NoError(t, err) + require.Error(t, err) + require.ErrorIs(t, err, neorpc.ErrUnknownSession) require.False(t, ok) // session has already been terminated. }) }) diff --git a/pkg/services/rpcsrv/server.go b/pkg/services/rpcsrv/server.go index 86c4f3dc3..45d629278 100644 --- a/pkg/services/rpcsrv/server.go +++ b/pkg/services/rpcsrv/server.go @@ -2351,7 +2351,7 @@ func (s *Server) traverseIterator(reqParams params.Params) (any, *neorpc.Error) session, ok := s.sessions[sID.String()] if !ok { s.sessionsLock.Unlock() - return []json.RawMessage{}, nil + return nil, neorpc.ErrUnknownSession } session.iteratorsLock.Lock() // Perform `till` update only after session.iteratorsLock is taken in order to have more @@ -2362,14 +2362,19 @@ func (s *Server) traverseIterator(reqParams params.Params) (any, *neorpc.Error) var ( iIDStr = iID.String() iVals []stackitem.Item + found bool ) for _, it := range session.iteratorIdentifiers { if iIDStr == it.ID { iVals = iterator.Values(it.Item, count) + found = true break } } session.iteratorsLock.Unlock() + if !found { + return nil, neorpc.ErrUnknownIterator + } result := make([]json.RawMessage, len(iVals)) for j := range iVals { @@ -2393,17 +2398,18 @@ func (s *Server) terminateSession(reqParams params.Params) (any, *neorpc.Error) s.sessionsLock.Lock() defer s.sessionsLock.Unlock() session, ok := s.sessions[strSID] - if ok { - // Iterators access Seek channel under the hood; finalizer closes this channel, thus, - // we need to perform finalisation under iteratorsLock. - session.iteratorsLock.Lock() - session.finalize() - if !session.timer.Stop() { - <-session.timer.C - } - delete(s.sessions, strSID) - session.iteratorsLock.Unlock() + if !ok { + return nil, neorpc.ErrUnknownSession } + // Iterators access Seek channel under the hood; finalizer closes this channel, thus, + // we need to perform finalisation under iteratorsLock. + session.iteratorsLock.Lock() + session.finalize() + if !session.timer.Stop() { + <-session.timer.C + } + delete(s.sessions, strSID) + session.iteratorsLock.Unlock() return ok, nil } diff --git a/pkg/services/rpcsrv/server_test.go b/pkg/services/rpcsrv/server_test.go index bc098e3ca..b50a4be1f 100644 --- a/pkg/services/rpcsrv/server_test.go +++ b/pkg/services/rpcsrv/server_test.go @@ -2582,6 +2582,13 @@ func testRPCProtocol(t *testing.T, doRPCCall func(string, string, *testing.T) [] return res.Session, *iterator.ID } t.Run("traverseiterator", func(t *testing.T) { + t.Run("sessions disabled", func(t *testing.T) { + _, _, httpSrv2 := initClearServerWithCustomConfig(t, func(c *config.Config) { + c.ApplicationConfiguration.RPC.SessionEnabled = false + }) + body := doRPCCall(`{"jsonrpc": "2.0", "id": 1, "method": "traverseiterator", "params": []}"`, httpSrv2.URL, t) + checkErrGetResult(t, body, true, neorpc.ErrSessionsDisabledCode) + }) t.Run("good", func(t *testing.T) { sID, iID := prepareIteratorSession(t) expectedCount := 99 @@ -2626,36 +2633,35 @@ func testRPCProtocol(t *testing.T, doRPCCall func(string, string, *testing.T) [] _, iID := prepareIteratorSession(t) rpc := fmt.Sprintf(`{"jsonrpc": "2.0", "id": 1, "method": "traverseiterator", "params": ["%s", "%s", %d]}"`, uuid.NewString(), iID.String(), 1) body := doRPCCall(rpc, httpSrv.URL, t) - resp := checkErrGetResult(t, body, false, 0) - res := new([]json.RawMessage) - require.NoError(t, json.Unmarshal(resp, res)) - require.Equal(t, 0, len(*res)) // No errors expected, no elements should be returned. + checkErrGetResult(t, body, true, neorpc.ErrUnknownSessionCode) }) t.Run("unknown iterator", func(t *testing.T) { sID, _ := prepareIteratorSession(t) rpc := fmt.Sprintf(`{"jsonrpc": "2.0", "id": 1, "method": "traverseiterator", "params": ["%s", "%s", %d]}"`, sID.String(), uuid.NewString(), 1) body := doRPCCall(rpc, httpSrv.URL, t) - resp := checkErrGetResult(t, body, false, 0) - res := new([]json.RawMessage) - require.NoError(t, json.Unmarshal(resp, res)) - require.Equal(t, 0, len(*res)) // No errors expected, no elements should be returned. + checkErrGetResult(t, body, true, neorpc.ErrUnknownIteratorCode) }) }) t.Run("terminatesession", func(t *testing.T) { - check := func(t *testing.T, id string, expected bool) { - rpc := fmt.Sprintf(`{"jsonrpc": "2.0", "id": 1, "method": "terminatesession", "params": ["%s"]}"`, id) - body := doRPCCall(rpc, httpSrv.URL, t) + rpc := `{"jsonrpc": "2.0", "id": 1, "method": "terminatesession", "params": ["%s"]}"` + t.Run("sessions disabled", func(t *testing.T) { + _, _, httpSrv2 := initClearServerWithCustomConfig(t, func(c *config.Config) { + c.ApplicationConfiguration.RPC.SessionEnabled = false + }) + body := doRPCCall(fmt.Sprintf(rpc, uuid.NewString()), httpSrv2.URL, t) + checkErrGetResult(t, body, true, neorpc.ErrSessionsDisabledCode) + }) + t.Run("true", func(t *testing.T) { + sID, _ := prepareIteratorSession(t) + body := doRPCCall(fmt.Sprintf(rpc, sID.String()), httpSrv.URL, t) resp := checkErrGetResult(t, body, false, 0) res := new(bool) require.NoError(t, json.Unmarshal(resp, res)) - require.Equal(t, expected, *res) - } - t.Run("true", func(t *testing.T) { - sID, _ := prepareIteratorSession(t) - check(t, sID.String(), true) + require.Equal(t, true, *res) }) t.Run("false", func(t *testing.T) { - check(t, uuid.NewString(), false) + body := doRPCCall(fmt.Sprintf(rpc, uuid.NewString()), httpSrv.URL, t) + checkErrGetResult(t, body, true, neorpc.ErrUnknownSessionCode) }) t.Run("expired", func(t *testing.T) { _, _ = prepareIteratorSession(t)