From cc9974979fa37d066e2ecb4e9e53916afee84a6d Mon Sep 17 00:00:00 2001 From: congqixia Date: Tue, 19 Sep 2023 10:05:22 +0800 Subject: [PATCH] Add staticcheck linter and fix existing problems (#27174) Signed-off-by: Congqi Xia --- .golangci.yml | 6 ++++++ internal/datacoord/compaction_test.go | 2 +- internal/datacoord/compaction_trigger_test.go | 2 -- internal/datacoord/meta_test.go | 1 + internal/distributed/proxy/httpserver/utils.go | 16 ++++++++++------ .../distributed/proxy/httpserver/utils_test.go | 4 ++-- .../metastore/kv/datacoord/kv_catalog_test.go | 4 ---- .../parser/planparserv2/plan_parser_v2_test.go | 2 +- internal/proxy/mock_test.go | 2 ++ internal/proxy/reducer_test.go | 6 ++++-- internal/proxy/util.go | 5 ++--- .../balance/score_based_balancer_test.go | 2 -- internal/querycoordv2/meta/replica_manager.go | 6 +++--- internal/querynodev2/delegator/delegator_data.go | 1 - internal/querynodev2/segments/manager.go | 2 +- internal/querynodev2/services_test.go | 1 + internal/rootcoord/import_manager_test.go | 6 ++++++ internal/rootcoord/mock_test.go | 3 --- internal/storage/binlog_test.go | 4 ++-- internal/storage/vector_chunk_manager_test.go | 2 +- internal/util/importutil/json_parser.go | 1 + pkg/log/mlogger_test.go | 1 + pkg/mq/msgstream/mq_msgstream.go | 1 - pkg/util/cache/local_cache_test.go | 4 ++-- pkg/util/etcd/etcd_util_test.go | 7 +++---- 25 files changed, 50 insertions(+), 41 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index f9c363419c..d11131df21 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -11,6 +11,7 @@ run: linters: disable-all: true enable: + - staticcheck - typecheck - goimports - misspell @@ -82,6 +83,11 @@ issues: - G402 # Use of weak random number generator math/rand - G404 + # Unused parameters + - SA1019 + # defer return errors + - SA5001 + # Maximum issues count per one linter. Set to 0 to disable. Default is 50. max-issues-per-linter: 0 # Maximum count of issues with the same text. Set to 0 to disable. Default is 3. diff --git a/internal/datacoord/compaction_test.go b/internal/datacoord/compaction_test.go index 8e62389e51..2ca1a98081 100644 --- a/internal/datacoord/compaction_test.go +++ b/internal/datacoord/compaction_test.go @@ -419,7 +419,7 @@ func TestCompactionPlanHandler_handleMergeCompactionResult(t *testing.T) { require.True(t, has) call.Unset() - call = mockDataNode.EXPECT().SyncSegments(mock.Anything, mock.Anything).Run(func(ctx context.Context, req *datapb.SyncSegmentsRequest) {}).Return(&commonpb.Status{ErrorCode: commonpb.ErrorCode_UnexpectedError}, nil) + mockDataNode.EXPECT().SyncSegments(mock.Anything, mock.Anything).Run(func(ctx context.Context, req *datapb.SyncSegmentsRequest) {}).Return(&commonpb.Status{ErrorCode: commonpb.ErrorCode_UnexpectedError}, nil) err = c.handleMergeCompactionResult(plan, compactionResult2) assert.Error(t, err) } diff --git a/internal/datacoord/compaction_trigger_test.go b/internal/datacoord/compaction_trigger_test.go index 625d443f95..cf0b3234fe 100644 --- a/internal/datacoord/compaction_trigger_test.go +++ b/internal/datacoord/compaction_trigger_test.go @@ -465,9 +465,7 @@ func Test_compactionTrigger_force(t *testing.T) { }) t.Run(tt.name+" with DiskANN index", func(t *testing.T) { - segmentIDs := make([]int64, 0) for _, segment := range tt.fields.meta.segments.GetSegments() { - segmentIDs = append(segmentIDs, segment.GetID()) // Collection 1000 means it has DiskANN index segment.CollectionID = 1000 } diff --git a/internal/datacoord/meta_test.go b/internal/datacoord/meta_test.go index 636ac18a6f..ef9a8962df 100644 --- a/internal/datacoord/meta_test.go +++ b/internal/datacoord/meta_test.go @@ -351,6 +351,7 @@ func TestMeta_Basic(t *testing.T) { catalog = datacoord.NewCatalog(metakv, "", "") meta, err = newMeta(context.TODO(), catalog, nil) assert.NoError(t, err) + assert.NotNil(t, meta) }) t.Run("Test GetCount", func(t *testing.T) { diff --git a/internal/distributed/proxy/httpserver/utils.go b/internal/distributed/proxy/httpserver/utils.go index fd29eb9389..f6f138cda5 100644 --- a/internal/distributed/proxy/httpserver/utils.go +++ b/internal/distributed/proxy/httpserver/utils.go @@ -11,6 +11,7 @@ import ( "strings" "github.com/milvus-io/milvus/pkg/util/parameterutil.go" + "go.uber.org/zap" "github.com/milvus-io/milvus/pkg/util/merr" @@ -274,20 +275,23 @@ func checkAndSetData(body string, collDescResp *milvuspb.DescribeCollectionRespo for mapKey, mapValue := range data.Map() { if !containsString(fieldNames, mapKey) { mapValueStr := mapValue.String() - if mapValue.Type == gjson.True || mapValue.Type == gjson.False { + switch mapValue.Type { + case gjson.True, gjson.False: reallyData[mapKey] = cast.ToBool(mapValueStr) - } else if mapValue.Type == gjson.String { + case gjson.String: reallyData[mapKey] = mapValueStr - } else if mapValue.Type == gjson.Number { + case gjson.Number: if strings.Contains(mapValue.Raw, ".") { reallyData[mapKey] = cast.ToFloat64(mapValue.Raw) } else { reallyData[mapKey] = cast.ToInt64(mapValueStr) } - } else if mapValue.Type == gjson.JSON { + case gjson.JSON: reallyData[mapKey] = mapValue.Value() - } else { - + case gjson.Null: + // skip null + default: + log.Warn("unknown json type found", zap.Int("mapValue.Type", int(mapValue.Type))) } } } diff --git a/internal/distributed/proxy/httpserver/utils_test.go b/internal/distributed/proxy/httpserver/utils_test.go index cb10e4aa02..62249d30ca 100644 --- a/internal/distributed/proxy/httpserver/utils_test.go +++ b/internal/distributed/proxy/httpserver/utils_test.go @@ -321,7 +321,7 @@ func TestPrimaryField(t *testing.T) { } func TestInsertWithDynamicFields(t *testing.T) { - body := "{\"data\": {\"id\": 0, \"book_id\": 1, \"book_intro\": [0.1, 0.2], \"word_count\": 2}}" + body := "{\"data\": {\"id\": 0, \"book_id\": 1, \"book_intro\": [0.1, 0.2], \"word_count\": 2, \"classified\": false, \"databaseID\": null}}" req := InsertReq{} coll := generateCollectionSchema(false) err := checkAndSetData(body, &milvuspb.DescribeCollectionResponse{ @@ -336,7 +336,7 @@ func TestInsertWithDynamicFields(t *testing.T) { assert.Equal(t, err, nil) assert.Equal(t, fieldsData[len(fieldsData)-1].IsDynamic, true) assert.Equal(t, fieldsData[len(fieldsData)-1].Type, schemapb.DataType_JSON) - assert.Equal(t, string(fieldsData[len(fieldsData)-1].GetScalars().GetJsonData().GetData()[0]), "{\"id\":0}") + assert.Equal(t, string(fieldsData[len(fieldsData)-1].GetScalars().GetJsonData().GetData()[0]), "{\"classified\":false,\"id\":0}") } func TestSerialize(t *testing.T) { diff --git a/internal/metastore/kv/datacoord/kv_catalog_test.go b/internal/metastore/kv/datacoord/kv_catalog_test.go index 856706de1f..de7fa38bbd 100644 --- a/internal/metastore/kv/datacoord/kv_catalog_test.go +++ b/internal/metastore/kv/datacoord/kv_catalog_test.go @@ -372,10 +372,6 @@ func Test_AlterSegments(t *testing.T) { opGroupCount := 0 metakv := mocks.NewMetaKv(t) metakv.EXPECT().MultiSave(mock.Anything).RunAndReturn(func(m map[string]string) error { - var ks []string - for k := range m { - ks = append(ks, k) - } maps.Copy(savedKvs, m) opGroupCount++ return nil diff --git a/internal/parser/planparserv2/plan_parser_v2_test.go b/internal/parser/planparserv2/plan_parser_v2_test.go index 2383bc848a..b3cb0f76fa 100644 --- a/internal/parser/planparserv2/plan_parser_v2_test.go +++ b/internal/parser/planparserv2/plan_parser_v2_test.go @@ -1794,7 +1794,7 @@ func Test_JSONContainsAny(t *testing.T) { assert.False(t, plan.GetVectorAnns().GetPredicates().GetJsonContainsExpr().GetElementsSameType()) expr = `JSON_CONTAINS_ANY(A, 1)` - plan, err = CreateSearchPlan(schema, expr, "FloatVectorField", &planpb.QueryInfo{ + _, err = CreateSearchPlan(schema, expr, "FloatVectorField", &planpb.QueryInfo{ Topk: 0, MetricType: "", SearchParams: "", diff --git a/internal/proxy/mock_test.go b/internal/proxy/mock_test.go index 88d68e7346..f3f54d2a8f 100644 --- a/internal/proxy/mock_test.go +++ b/internal/proxy/mock_test.go @@ -207,6 +207,8 @@ func newMockDmlTask(ctx context.Context) *mockDmlTask { return &mockDmlTask{ mockTask: newMockTask(ctx), + vchans: vchans, + pchans: pchans, } } diff --git a/internal/proxy/reducer_test.go b/internal/proxy/reducer_test.go index 3405e3e391..58795450cf 100644 --- a/internal/proxy/reducer_test.go +++ b/internal/proxy/reducer_test.go @@ -1,6 +1,7 @@ package proxy import ( + "context" "testing" "github.com/milvus-io/milvus/internal/proto/planpb" @@ -16,13 +17,14 @@ func Test_createMilvusReducer(t *testing.T) { }, } var r milvusReducer + ctx := context.Background() - r = createMilvusReducer(nil, nil, nil, nil, n, "") + r = createMilvusReducer(ctx, nil, nil, nil, n, "") _, ok := r.(*defaultLimitReducer) assert.True(t, ok) n.Node.(*planpb.PlanNode_Query).Query.IsCount = true - r = createMilvusReducer(nil, nil, nil, nil, n, "") + r = createMilvusReducer(ctx, nil, nil, nil, n, "") _, ok = r.(*cntReducer) assert.True(t, ok) } diff --git a/internal/proxy/util.go b/internal/proxy/util.go index 3552dc61f7..4c1649fbb4 100644 --- a/internal/proxy/util.go +++ b/internal/proxy/util.go @@ -525,10 +525,9 @@ func validateSchema(coll *schemapb.CollectionSchema) error { if err4 != nil { return err4 } - } else { - // in C++, default type will be specified - // do nothing } + // in C++, default type will be specified + // do nothing } else { if len(field.IndexParams) != 0 { return fmt.Errorf("index params is not empty for scalar field: %s(%d)", field.Name, field.FieldID) diff --git a/internal/querycoordv2/balance/score_based_balancer_test.go b/internal/querycoordv2/balance/score_based_balancer_test.go index c65d5dfc2c..14aa5c5dff 100644 --- a/internal/querycoordv2/balance/score_based_balancer_test.go +++ b/internal/querycoordv2/balance/score_based_balancer_test.go @@ -397,10 +397,8 @@ func (suite *ScoreBasedBalancerTestSuite) TestBalanceMultiRound() { balancer := suite.balancer //1. set up target for multi collections - collections := make([]*meta.Collection, 0, len(balanceCase.collectionIDs)) for i := range balanceCase.collectionIDs { collection := utils.CreateTestCollection(balanceCase.collectionIDs[i], int32(balanceCase.replicaIDs[i])) - collections = append(collections, collection) suite.broker.EXPECT().GetRecoveryInfoV2(mock.Anything, balanceCase.collectionIDs[i]).Return( nil, balanceCase.collectionsSegments[i], nil) diff --git a/internal/querycoordv2/meta/replica_manager.go b/internal/querycoordv2/meta/replica_manager.go index 0bc7de359d..53376ad6db 100644 --- a/internal/querycoordv2/meta/replica_manager.go +++ b/internal/querycoordv2/meta/replica_manager.go @@ -54,7 +54,7 @@ func (replica *Replica) AddNode(nodes ...int64) { func (replica *Replica) GetNodes() []int64 { replica.rwmutex.RLock() defer replica.rwmutex.RUnlock() - if replica != nil { + if replica.nodes != nil { return replica.nodes.Collect() } return nil @@ -63,7 +63,7 @@ func (replica *Replica) GetNodes() []int64 { func (replica *Replica) Len() int { replica.rwmutex.RLock() defer replica.rwmutex.RUnlock() - if replica != nil { + if replica.nodes != nil { return replica.nodes.Len() } @@ -73,7 +73,7 @@ func (replica *Replica) Len() int { func (replica *Replica) Contains(node int64) bool { replica.rwmutex.RLock() defer replica.rwmutex.RUnlock() - if replica != nil { + if replica.nodes != nil { return replica.nodes.Contain(node) } diff --git a/internal/querynodev2/delegator/delegator_data.go b/internal/querynodev2/delegator/delegator_data.go index e9db866852..5accd82202 100644 --- a/internal/querynodev2/delegator/delegator_data.go +++ b/internal/querynodev2/delegator/delegator_data.go @@ -575,7 +575,6 @@ func (sd *shardDelegator) readDeleteFromMsgstream(ctx context.Context, position // reach safe ts if safeTs <= msgPack.EndPositions[0].GetTimestamp() { hasMore = false - break } } } diff --git a/internal/querynodev2/segments/manager.go b/internal/querynodev2/segments/manager.go index 205db118f3..2f620d5564 100644 --- a/internal/querynodev2/segments/manager.go +++ b/internal/querynodev2/segments/manager.go @@ -152,7 +152,7 @@ func (mgr *segmentManager) Put(segmentType SegmentType, segments ...Segment) { var replacedSegment []Segment mgr.mu.Lock() defer mgr.mu.Unlock() - targetMap := mgr.growingSegments + var targetMap map[int64]Segment switch segmentType { case SegmentTypeGrowing: targetMap = mgr.growingSegments diff --git a/internal/querynodev2/services_test.go b/internal/querynodev2/services_test.go index 378d1f1045..474f0e2ffc 100644 --- a/internal/querynodev2/services_test.go +++ b/internal/querynodev2/services_test.go @@ -1771,6 +1771,7 @@ func (suite *ServiceSuite) TestSyncDistribution_Normal() { req.Actions = []*querypb.SyncAction{syncVersionAction} status, err = suite.node.SyncDistribution(ctx, req) suite.NoError(err) + suite.Equal(commonpb.ErrorCode_Success, status.GetErrorCode()) } func (suite *ServiceSuite) TestSyncDistribution_ReleaseResultCheck() { diff --git a/internal/rootcoord/import_manager_test.go b/internal/rootcoord/import_manager_test.go index 57b88f2b63..379ae943b4 100644 --- a/internal/rootcoord/import_manager_test.go +++ b/internal/rootcoord/import_manager_test.go @@ -573,6 +573,7 @@ func TestImportManager_ImportJob(t *testing.T) { rowReq.Files = []string{"f1.json"} mgr = newImportManager(context.TODO(), mockKv, idAlloc, importServiceFunc, callGetSegmentStates, nil, nil) resp = mgr.importJob(context.TODO(), rowReq, colID, 0) + assert.Equal(t, commonpb.ErrorCode_Success, resp.GetStatus().GetErrorCode()) assert.Equal(t, len(rowReq.Files), len(mgr.pendingTasks)) assert.Equal(t, 0, len(mgr.workingTasks)) @@ -586,6 +587,7 @@ func TestImportManager_ImportJob(t *testing.T) { // since the importServiceFunc return error, tasks will be kept in pending list mgr = newImportManager(context.TODO(), mockKv, idAlloc, importServiceFunc, callGetSegmentStates, nil, nil) resp = mgr.importJob(context.TODO(), colReq, colID, 0) + assert.Equal(t, commonpb.ErrorCode_Success, resp.GetStatus().GetErrorCode()) assert.Equal(t, 1, len(mgr.pendingTasks)) assert.Equal(t, 0, len(mgr.workingTasks)) @@ -600,12 +602,14 @@ func TestImportManager_ImportJob(t *testing.T) { // row-based case, since the importServiceFunc return success, tasks will be sent to working list mgr = newImportManager(context.TODO(), mockKv, idAlloc, importServiceFunc, callGetSegmentStates, nil, nil) resp = mgr.importJob(context.TODO(), rowReq, colID, 0) + assert.Equal(t, commonpb.ErrorCode_Success, resp.GetStatus().GetErrorCode()) assert.Equal(t, 0, len(mgr.pendingTasks)) assert.Equal(t, len(rowReq.Files), len(mgr.workingTasks)) // column-based case, since the importServiceFunc return success, tasks will be sent to working list mgr = newImportManager(context.TODO(), mockKv, idAlloc, importServiceFunc, callGetSegmentStates, nil, nil) resp = mgr.importJob(context.TODO(), colReq, colID, 0) + assert.Equal(t, commonpb.ErrorCode_Success, resp.GetStatus().GetErrorCode()) assert.Equal(t, 0, len(mgr.pendingTasks)) assert.Equal(t, 1, len(mgr.workingTasks)) @@ -630,9 +634,11 @@ func TestImportManager_ImportJob(t *testing.T) { // the first task is sent to working list, and 1 task left in pending list mgr = newImportManager(context.TODO(), mockKv, idAlloc, importServiceFunc, callGetSegmentStates, nil, nil) resp = mgr.importJob(context.TODO(), rowReq, colID, 0) + assert.Equal(t, commonpb.ErrorCode_Success, resp.GetStatus().GetErrorCode()) assert.Equal(t, 0, len(mgr.pendingTasks)) assert.Equal(t, 1, len(mgr.workingTasks)) resp = mgr.importJob(context.TODO(), rowReq, colID, 0) + assert.Equal(t, commonpb.ErrorCode_Success, resp.GetStatus().GetErrorCode()) assert.Equal(t, 1, len(mgr.pendingTasks)) assert.Equal(t, 1, len(mgr.workingTasks)) diff --git a/internal/rootcoord/mock_test.go b/internal/rootcoord/mock_test.go index f1ebe477f0..d4fd3389e4 100644 --- a/internal/rootcoord/mock_test.go +++ b/internal/rootcoord/mock_test.go @@ -1107,21 +1107,18 @@ func newMockStepExecutor() *mockStepExecutor { func (m mockStepExecutor) Start() { if m.StartFunc != nil { m.StartFunc() - } else { } } func (m mockStepExecutor) Stop() { if m.StopFunc != nil { m.StopFunc() - } else { } } func (m mockStepExecutor) AddSteps(s *stepStack) { if m.AddStepsFunc != nil { m.AddStepsFunc(s) - } else { } } diff --git a/internal/storage/binlog_test.go b/internal/storage/binlog_test.go index a55a83c0b7..4e1833139a 100644 --- a/internal/storage/binlog_test.go +++ b/internal/storage/binlog_test.go @@ -1101,7 +1101,7 @@ func TestIndexFileBinlog(t *testing.T) { //descriptor data fix, field id fID := UnsafeReadInt64(buf, pos) - assert.Equal(t, fieldID, fieldID) + assert.Equal(t, fieldID, fID) pos += int(unsafe.Sizeof(fID)) //descriptor data fix, start time stamp @@ -1230,7 +1230,7 @@ func TestIndexFileBinlogV2(t *testing.T) { //descriptor data fix, field id fID := UnsafeReadInt64(buf, pos) - assert.Equal(t, fieldID, fieldID) + assert.Equal(t, fieldID, fID) pos += int(unsafe.Sizeof(fID)) //descriptor data fix, start time stamp diff --git a/internal/storage/vector_chunk_manager_test.go b/internal/storage/vector_chunk_manager_test.go index eb875a4319..0c1c026610 100644 --- a/internal/storage/vector_chunk_manager_test.go +++ b/internal/storage/vector_chunk_manager_test.go @@ -465,7 +465,7 @@ func TestVectorChunkManager_Read(t *testing.T) { r, err = vcm.Mmap(ctx, "not exist") assert.Error(t, err) - assert.Nil(t, nil) + assert.Nil(t, r) } content, err = vcm.ReadAt(ctx, "109", 9999, 8*4) diff --git a/internal/util/importutil/json_parser.go b/internal/util/importutil/json_parser.go index a0332d4c4e..5667847dc1 100644 --- a/internal/util/importutil/json_parser.go +++ b/internal/util/importutil/json_parser.go @@ -322,6 +322,7 @@ func (p *JSONParser) ParseRows(reader *IOReader, handler JSONRowHandler) error { return errors.New("import task was canceled") } + // nolint // this break means we require the first node must be RowRootNode // once the RowRootNode is parsed, just finish break diff --git a/pkg/log/mlogger_test.go b/pkg/log/mlogger_test.go index fcdf9339c5..63a71f7140 100644 --- a/pkg/log/mlogger_test.go +++ b/pkg/log/mlogger_test.go @@ -28,6 +28,7 @@ func TestExporterV2(t *testing.T) { ts.assertMessagesContains("traceID=mock-trace") ts.CleanBuffer() + // nolint Ctx(nil).Info("empty context") ts.assertMessagesNotContains("traceID") diff --git a/pkg/mq/msgstream/mq_msgstream.go b/pkg/mq/msgstream/mq_msgstream.go index 864d3aab53..e7f3bcb6d2 100644 --- a/pkg/mq/msgstream/mq_msgstream.go +++ b/pkg/mq/msgstream/mq_msgstream.go @@ -860,7 +860,6 @@ func (ms *MqTtMsgStream) Seek(ctx context.Context, msgPositions []*msgpb.MsgPosi } if tsMsg.Type() == commonpb.MsgType_TimeTick && tsMsg.BeginTs() >= mp.Timestamp { runLoop = false - break } else if tsMsg.BeginTs() > mp.Timestamp { ctx, _ := ExtractCtx(tsMsg, msg.Properties()) tsMsg.SetTraceCtx(ctx) diff --git a/pkg/util/cache/local_cache_test.go b/pkg/util/cache/local_cache_test.go index 508e1495c2..07c7345383 100644 --- a/pkg/util/cache/local_cache_test.go +++ b/pkg/util/cache/local_cache_test.go @@ -353,12 +353,12 @@ func TestGetIfPresentExpired(t *testing.T) { c := NewCache(WithExpireAfterWrite[int, string](1*time.Second), WithInsertionListener(insFunc)) defer c.Close() - v, ok := c.GetIfPresent(0) + _, ok := c.GetIfPresent(0) assert.False(t, ok) wg.Add(1) c.Put(0, "0") - v, ok = c.GetIfPresent(0) + v, ok := c.GetIfPresent(0) assert.True(t, ok) assert.Equal(t, "0", v) diff --git a/pkg/util/etcd/etcd_util_test.go b/pkg/util/etcd/etcd_util_test.go index ca67454604..86a60ae4ea 100644 --- a/pkg/util/etcd/etcd_util_test.go +++ b/pkg/util/etcd/etcd_util_test.go @@ -42,27 +42,26 @@ func TestEtcd(t *testing.T) { assert.False(t, resp.Count < 1) assert.Equal(t, string(resp.Kvs[0].Value), "value") - etcdCli, err = GetEtcdClient(false, true, []string{}, + _, err = GetEtcdClient(false, true, []string{}, "../../../configs/cert/client.pem", "../../../configs/cert/client.key", "../../../configs/cert/ca.pem", "some not right word") assert.Error(t, err) - etcdCli, err = GetEtcdClient(false, true, []string{}, + _, err = GetEtcdClient(false, true, []string{}, "../../../configs/cert/client.pem", "../../../configs/cert/client.key", "wrong/file", "1.2") assert.Error(t, err) - etcdCli, err = GetEtcdClient(false, true, []string{}, + _, err = GetEtcdClient(false, true, []string{}, "wrong/file", "../../../configs/cert/client.key", "../../../configs/cert/ca.pem", "1.2") assert.Error(t, err) - } func Test_buildKvGroup(t *testing.T) {