Check data type before metadata decode to avoid confusing error messa…

…ge (#2015)

src/storage/redis_db.cc

@@ -55,7 +55,8 @@ rocksdb::Status Database::ParseMetadata(RedisTypes types, Slice *bytes, Metadata
   });
 
   auto s = metadata->Decode(bytes);
-  if (!s.ok()) return s;
+  // delay InvalidArgument error check after type match check
+  if (!s.ok() && !s.IsInvalidArgument()) return s;
 
   if (metadata->Expired()) {
     // error discarded here since it already failed
@@ -69,6 +70,8 @@ rocksdb::Status Database::ParseMetadata(RedisTypes types, Slice *bytes, Metadata
     auto _ [[maybe_unused]] = metadata->Decode(old_metadata);
     return rocksdb::Status::InvalidArgument(kErrMsgWrongType);
   }
+  if (s.IsInvalidArgument()) return s;
+
   if (metadata->size == 0 && !metadata->IsEmptyableType()) {
     // error discarded here since it already failed
     auto _ [[maybe_unused]] = metadata->Decode(old_metadata);

src/storage/redis_metadata.cc

@@ -335,13 +335,13 @@ rocksdb::Status ListMetadata::Decode(Slice *input) {
   if (auto s = Metadata::Decode(input); !s.ok()) {
     return s;
   }
-  if (Type() == kRedisList) {
-    if (input->size() < 8 + 8) {
-      return rocksdb::Status::InvalidArgument(kErrMetadataTooShort);
-    }
-    GetFixed64(input, &head);
-    GetFixed64(input, &tail);
+
+  if (input->size() < 8 + 8) {
+    return rocksdb::Status::InvalidArgument(kErrMetadataTooShort);
   }
+  GetFixed64(input, &head);
+  GetFixed64(input, &tail);
+
   return rocksdb::Status::OK();
 }
 

tests/gocase/unit/type/types_test.go

@@ -0,0 +1,54 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package types
+
+import (
+	"context"
+	"testing"
+
+	"github.com/apache/kvrocks/tests/gocase/util"
+	"github.com/stretchr/testify/require"
+)
+
+func TestTypesError(t *testing.T) {
+	srv := util.StartServer(t, map[string]string{})
+	defer srv.Close()
+	ctx := context.Background()
+	rdb := srv.NewClient()
+	defer func() { require.NoError(t, rdb.Close()) }()
+
+	t.Run("Operate with wrong type", func(t *testing.T) {
+		message := "ERR Invalid argument: WRONGTYPE Operation against a key holding the wrong kind of value"
+		require.NoError(t, rdb.Set(ctx, "a", "hello", 0).Err())
+		require.EqualError(t, rdb.Do(ctx, "XADD", "a", "*", "a", "test").Err(), message)
+		require.EqualError(t, rdb.Do(ctx, "LPUSH", "a", 1).Err(), message)
+		require.EqualError(t, rdb.Do(ctx, "HSET", "a", "1", "2").Err(), message)
+		require.EqualError(t, rdb.Do(ctx, "SADD", "a", "1", "2").Err(), message)
+		require.EqualError(t, rdb.Do(ctx, "ZADD", "a", "1", "2").Err(), message)
+		require.EqualError(t, rdb.Do(ctx, "JSON.SET", "a", "$", "{}").Err(), message)
+		require.EqualError(t, rdb.Do(ctx, "BF.ADD", "a", "test").Err(), message)
+		require.EqualError(t, rdb.Do(ctx, "SADD", "a", 100).Err(), message)
+
+		require.NoError(t, rdb.LPush(ctx, "a1", "hello", 0).Err())
+		require.EqualError(t, rdb.Do(ctx, "SETBIT", "a1", 1, 1).Err(), message)
+		require.EqualError(t, rdb.Do(ctx, "GET", "a1").Err(), message)
+
+	})
+}