sharding: fail fast on invalid bucket_id in data or opts

- Added early validation of bucket_id in tuple/object and in opts
  inside tuple_set_and_return_bucket_id().
- Error messages now specify where the invalid value was provided:
  * "Invalid bucket_id in data: expected unsigned, got %s"
  * "Invalid bucket_id in opts: expected unsigned, got %s"
- Extended integration tests for insert/replace/upsert to cover these cases.

Author: vakhov

Makefile

@@ -28,7 +28,7 @@ sdk: sdk-2 sdk-3
 	chmod 644 sdk-3/rocks/* && \
 	tt rocks make_manifest sdk-3/rocks
 
-lint: .rocks
+lint:
 	source sdk-2/env.sh && .rocks/bin/luacheck .
 
 .PHONY: test

crud/common/sharding/init.lua

@@ -82,11 +82,13 @@ function sharding.tuple_get_bucket_id(vshard_router, tuple, space, specified_buc
     }
 end
 
-function sharding.validate_bucket_id(bucket_id)
+function sharding.validate_bucket_id(bucket_id, where)
+    where = where or 'opts'
+
     if not utils.is_uint(bucket_id) or bucket_id < 1 then
         return BucketIDError:new(
-                "Invalid bucket_id: expected unsigned, got %s",
-                type(bucket_id)
+                "Invalid bucket_id in %s: expected unsigned, got %s",
+                where, type(bucket_id)
         )
     end
 end
@@ -97,7 +99,19 @@ function sharding.tuple_set_and_return_bucket_id(vshard_router, tuple, space, sp
         return nil, BucketIDError:new("Failed to get bucket ID fieldno: %s", err)
     end
 
+    if tuple[bucket_id_fieldno] ~= nil then
+        err = sharding.validate_bucket_id(tuple[bucket_id_fieldno], 'data')
+        if err ~= nil then
+            return nil, err
+        end
+    end
+
     if specified_bucket_id ~= nil then
+        err = sharding.validate_bucket_id(specified_bucket_id)
+        if err ~= nil then
+            return nil, err
+        end
+
         if tuple[bucket_id_fieldno] == nil then
             tuple[bucket_id_fieldno] = specified_bucket_id
         else
@@ -122,11 +136,6 @@ function sharding.tuple_set_and_return_bucket_id(vshard_router, tuple, space, sp
         sharding_data.skip_sharding_hash_check = true
     end
 
-    err = sharding.validate_bucket_id(sharding_data.bucket_id)
-    if err ~= nil then
-        return nil, err
-    end
-
     return sharding_data
 end
 

test/integration/count_test.lua

@@ -925,7 +925,7 @@ pgroup.test_invalid_bucket_id_in_opts = function(g)
 
     for _, bucket_id in ipairs(invalid_values) do
         local expected_err = string.format(
-            "Invalid bucket_id: expected unsigned, got %s",
+            "Invalid bucket_id in opts: expected unsigned, got %s",
             type(bucket_id)
         )
 

test/integration/pairs_readview_test.lua

@@ -957,7 +957,7 @@ pgroup.test_invalid_bucket_id_in_readview_pairs = function(g)
 
     for _, opts in ipairs(invalid_opts_list) do
         local expected_err = string.format(
-            "Invalid bucket_id: expected unsigned, got %s",
+            "Invalid bucket_id in opts: expected unsigned, got %s",
             type(opts.bucket_id)
         )
         local _, err = g.router:eval([[

test/integration/pairs_test.lua

@@ -964,7 +964,7 @@ pgroup.test_invalid_bucket_id_pairs = function(g)
 
     for _, opts in ipairs(invalid_opts_list) do
         local expected_err = string.format(
-            "Invalid bucket_id: expected unsigned, got %s",
+            "Invalid bucket_id in opts: expected unsigned, got %s",
             type(opts.bucket_id)
         )
         t.assert_error_msg_contains(expected_err, function()

test/integration/select_readview_test.lua

@@ -2575,7 +2575,7 @@ pgroup.test_invalid_bucket_id_in_readview = function(g)
 
     for _, opts in ipairs(invalid_opts_list) do
         local expected_err = string.format(
-            "Invalid bucket_id: expected unsigned, got %s",
+            "Invalid bucket_id in opts: expected unsigned, got %s",
             type(opts.bucket_id)
         )
         local _, err = g.router:eval([[

test/integration/select_test.lua

@@ -2339,7 +2339,7 @@ pgroup.test_select_invalid_bucket_id = function(g)
         opts.fullscan = true
 
         local expected_err = string.format(
-            "Invalid bucket_id: expected unsigned, got %s",
+            "Invalid bucket_id in opts: expected unsigned, got %s",
             type(opts.bucket_id)
         )
 

test/integration/simple_operations_test.lua

@@ -1743,48 +1743,103 @@ pgroup.test_invalid_bucket_id_operations = function(g)
     helpers.truncate_space_on_cluster(g.cluster, 'customers')
 
     local invalid_bucket_id = 'bad-id'
-    local expected_err = string.format('Invalid bucket_id: expected unsigned, got %s', type(invalid_bucket_id))
+    local ERR_IN_DATA = string.format('Invalid bucket_id in data: expected unsigned, got %s', type(invalid_bucket_id))
+    local ERR_IN_OPTS = string.format('Invalid bucket_id in opts: expected unsigned, got %s', type(invalid_bucket_id))
 
-    local key = 1
-    local tuple = {key, invalid_bucket_id, 'Test', 42}
-    local tuple_clean = {key, box.NULL, 'Test', 42}
-    local object = {id = key + 1000, bucket_id = invalid_bucket_id, name = 'Test', age = 42}
-    local object_clean = {id = key + 1001, name = 'Test', age = 42}
-    local operations = {{'=', 'name', 'NewName'}}
+    local base = 10000
 
-    local crud_calls = {
-        {'crud.insert', {'customers', tuple, {}}},
-        {'crud.insert', {'customers', tuple_clean, {bucket_id = invalid_bucket_id}}},
-
-        {'crud.insert_object', {'customers', object, {}}},
-        {'crud.insert_object', {'customers', object_clean, {bucket_id = invalid_bucket_id}}},
-
-        {'crud.replace', {'customers', tuple, {}}},
-        {'crud.replace', {'customers', tuple_clean, {bucket_id = invalid_bucket_id}}},
-
-        {'crud.replace_object', {'customers', object, {}}},
-        {'crud.replace_object', {'customers', object_clean, {bucket_id = invalid_bucket_id}}},
-
-        {'crud.upsert', {'customers', tuple, operations, {}}},
-        {'crud.upsert', {'customers', tuple_clean, operations, {bucket_id = invalid_bucket_id}}},
+    local tuple_bad = { base + 1, invalid_bucket_id, 'Test', 42 }
+    local tuple_clean = { base + 2, box.NULL, 'Test', 42 }
+    local obj_bad = { id = base + 1001, bucket_id = invalid_bucket_id, name = 'Test', age = 42 }
+    local obj_clean = { id = base + 1002, name = 'Test', age = 42 }
+    local ops = { { '=', 'name', 'NewName' } }
 
-        {'crud.upsert_object', {'customers', object, operations, {}}},
-        {'crud.upsert_object', {'customers', object_clean, operations, {bucket_id = invalid_bucket_id}}},
-
-        {'crud.update', {'customers', key, operations, {bucket_id = invalid_bucket_id}}},
-        {'crud.delete', {'customers', key, {bucket_id = invalid_bucket_id}}},
+    local crud_calls = {
+        {
+            func = 'crud.insert',
+            args = { 'customers', tuple_bad, {} },
+            expected = ERR_IN_DATA
+        },
+        {
+            func = 'crud.insert',
+            args = { 'customers', tuple_clean, { bucket_id = invalid_bucket_id } },
+            expected = ERR_IN_OPTS
+        },
+        {
+            func = 'crud.insert_object',
+            args = { 'customers', obj_bad, {} },
+            expected = ERR_IN_DATA
+        },
+        {
+            func = 'crud.insert_object',
+            args = { 'customers', obj_clean, { bucket_id = invalid_bucket_id } },
+            expected = ERR_IN_OPTS
+        },
+        {
+            func = 'crud.replace',
+            args = { 'customers', { base + 3, invalid_bucket_id, 'Test', 42 }, {} },
+            expected = ERR_IN_DATA
+        },
+        {
+            func = 'crud.replace',
+            args = { 'customers', { base + 4, box.NULL, 'Test', 42 }, { bucket_id = invalid_bucket_id } },
+            expected = ERR_IN_OPTS
+        },
+        {
+            func = 'crud.replace_object',
+            args = { 'customers', { id = base + 1003, bucket_id = invalid_bucket_id, name = 'Test', age = 42 }, {} },
+            expected = ERR_IN_DATA
+        },
+        {
+            func = 'crud.replace_object',
+            args = { 'customers', { id = base + 1004, name = 'Test', age = 42 }, { bucket_id = invalid_bucket_id } },
+            expected = ERR_IN_OPTS
+        },
+        {
+            func = 'crud.upsert',
+            args = { 'customers', { base + 5, invalid_bucket_id, 'Test', 42 }, ops, {} },
+            expected = ERR_IN_DATA
+        },
+        {
+            func = 'crud.upsert',
+            args = { 'customers', { base + 6, box.NULL, 'Test', 42 }, ops, { bucket_id = invalid_bucket_id } },
+            expected = ERR_IN_OPTS
+        },
+        {
+            func = 'crud.upsert_object',
+            args = { 'customers',
+                     { id = base + 1005, bucket_id = invalid_bucket_id, name = 'Test', age = 42 }, ops, {}
+            },
+            expected = ERR_IN_DATA
+        },
+        {
+            func = 'crud.upsert_object',
+            args = { 'customers',
+                     { id = base + 1006, name = 'Test', age = 42 }, ops, { bucket_id = invalid_bucket_id }
+            },
+            expected = ERR_IN_OPTS
+        },
+        {
+            func = 'crud.update',
+            args = { 'customers', base + 7, ops, { bucket_id = invalid_bucket_id } },
+            expected = ERR_IN_OPTS
+        },
+        {
+            func = 'crud.delete',
+            args = { 'customers', base + 8, { bucket_id = invalid_bucket_id } },
+            expected = ERR_IN_OPTS
+        },
     }
 
     for _, call in ipairs(crud_calls) do
-        local func, args = call[1], call[2]
-        local _, err = g.router:call(func, args)
-        t.assert_str_contains(err.err or err[1].err, expected_err, func)
+        local _, err = g.router:call(call.func, call.args)
+        t.assert_str_contains(err.err or err[1].err, call.expected, call.func)
     end
 end
 
 pgroup.test_invalid_bucket_id_many_operations = function(g)
     local invalid_bucket_id = 'bad-id'
-    local expected_err = string.format('Invalid bucket_id: expected unsigned, got %s', type(invalid_bucket_id))
+    local expected_err = string.format('Invalid bucket_id in data: expected unsigned, got %s', type(invalid_bucket_id))
 
     local many_calls = {
         {
@@ -1818,7 +1873,7 @@ end
 
 pgroup.test_invalid_bucket_id_object_many_operations = function(g)
     local invalid_bucket_id = 'bad-id'
-    local expected_err = string.format('Invalid bucket_id: expected unsigned, got %s', type(invalid_bucket_id))
+    local expected_err = string.format('Invalid bucket_id in data: expected unsigned, got %s', type(invalid_bucket_id))
 
     local object_many_calls = {
         {
@@ -1860,7 +1915,7 @@ pgroup.test_get_invalid_bucket_id = function(g)
 
     for _, bucket_id in ipairs(invalid_values) do
         local expected_err = string.format(
-            "Invalid bucket_id: expected unsigned, got %s",
+            "Invalid bucket_id in opts: expected unsigned, got %s",
             type(bucket_id)
         )
         local result, err = g.router:call('crud.get', {
@@ -1871,4 +1926,3 @@ pgroup.test_get_invalid_bucket_id = function(g)
         t.assert_str_contains(err.err or err.str, expected_err)
     end
 end
-