crud: fix explicit bucket_id _many operations

In case `insert_many`, `insert_object_many`, `replace_many`, `replace_object_many`, `upsert_many` or `upsert_object_many` has been called on a space with custom sharding info and every tuple/object in the request had `bucket_id`, `ShardingHashMismatchError` has been returned even though there are no issues with sharding info. For example, some users met this issue while working with tt-ee `tt crud import` command. The reason is as follows. To ensure sharding info consistency between the storage and the router, some metainfo is calculated for a request in case bucket_id is generated. In case no bucket_id is generated, no sharding info is passed and consistency check is skipped (since it is not needed). Before this patch, `*_many` operations haven't skipped consistency check when it was expected due to improper `skip_sharding_hash_check` flag setup. Closes #437
tarantool · May 17, 2024 · cfa496d · cfa496d
1 parent 435b61e
commit cfa496d
Show file tree

Hide file tree

Showing 7 changed files with 196 additions and 6 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -5,6 +5,14 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/)
 and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).
 
+## Unreleased
+
+### Fixed
+* `insert_many`, `insert_object_many`, `replace_many`, `replace_object_many`,
+  `upsert_many`, `upsert_object_many` operations no longer fail with
+  `ShardingHashMismatchError` if a space has custom sharding info and
+  every tuple/object in the request has `bucket_id` set (#437).
+
 ## [1.5.1] - 27-04-24
 
 ### Added

diff --git a/crud/common/sharding/init.lua b/crud/common/sharding/init.lua
@@ -222,13 +222,11 @@ function sharding.split_tuples_by_replicaset(vshard_router, tuples, space, opts)
 
     local batches = {}
 
-    local sharding_func_hash
-    local sharding_key_hash
-    local skip_sharding_hash_check
-    local sharding_data
-    local err
+    local sharding_func_hash = nil
+    local sharding_key_hash = nil
+    local skip_sharding_hash_check = true
     for i, tuple in ipairs(tuples) do
-        sharding_data, err = sharding.tuple_set_and_return_bucket_id(vshard_router, tuple, space)
+        local sharding_data, err = sharding.tuple_set_and_return_bucket_id(vshard_router, tuple, space)
         if err ~= nil then
             return nil, BucketIDError:new("Failed to get bucket ID: %s", err)
         end

diff --git a/test/entrypoint/srv_batch_operations/storage.lua b/test/entrypoint/srv_batch_operations/storage.lua
@@ -47,6 +47,46 @@ return {
             unique = true,
             if_not_exists = true,
         })
+
+        local customers_sharded_by_age_space = box.schema.space.create('customers_sharded_by_age', {
+            format = {
+                {name = 'id', type = 'unsigned'},
+                {name = 'bucket_id', type = 'unsigned'},
+                {name = 'name', type = 'string'},
+                {name = 'age', type = 'number'},
+            },
+            if_not_exists = true,
+            engine = engine,
+        })
+        customers_sharded_by_age_space:create_index('id', {
+            parts = { {field = 'id'} },
+            if_not_exists = true,
+        })
+        customers_sharded_by_age_space:create_index('bucket_id', {
+            parts = { {field = 'bucket_id'} },
+            unique = false,
+            if_not_exists = true,
+        })
+
+        -- https://github.com/tarantool/migrations/blob/a7c31a17f6ac02d4498b4203c23e495856861444/migrator/utils.lua#L35-L53
+        if box.space._ddl_sharding_key == nil then
+            local sharding_space = box.schema.space.create('_ddl_sharding_key', {
+                format = {
+                    {name = 'space_name', type = 'string', is_nullable = false},
+                    {name = 'sharding_key', type = 'array', is_nullable = false}
+                },
+                if_not_exists = true,
+            })
+            sharding_space:create_index(
+                'space_name', {
+                    type = 'TREE',
+                    unique = true,
+                    parts = {{'space_name', 'string', is_nullable = false}},
+                    if_not_exists = true,
+                }
+            )
+        end
+        box.space._ddl_sharding_key:replace{'customers_sharded_by_age', {'age'}}
     end),
     wait_until_ready = helper.wait_schema_init,
 }
diff --git a/test/integration/insert_many_test.lua b/test/integration/insert_many_test.lua
@@ -2,6 +2,7 @@ local t = require('luatest')
 local crud = require('crud')
 
 local helpers = require('test.helper')
+local write_scenario = require('test.integration.write_scenario')
 
 local batching_utils = require('crud.common.batching_utils')
 
@@ -19,6 +20,7 @@ end)
 
 pgroup.before_each(function(g)
     helpers.truncate_space_on_cluster(g.cluster, 'customers')
+    helpers.truncate_space_on_cluster(g.cluster, 'customers_sharded_by_age')
 end)
 
 pgroup.test_non_existent_space = function(g)
@@ -2051,3 +2053,27 @@ pgroup.test_zero_objects = function(g)
     t.assert_str_contains(errs[1].err, "At least one object expected")
     t.assert_equals(result, nil)
 end
+
+pgroup.test_gh_437_insert_many_explicit_bucket_ids = function(g)
+    write_scenario.gh_437_many_explicit_bucket_ids(g, 'insert_many')
+end
+
+pgroup.test_gh_437_insert_many_partial_explicit_bucket_ids = function(g)
+    write_scenario.gh_437_many_explicit_bucket_ids(
+        g,
+        'insert_many',
+        {partial_explicit_bucket_ids = true}
+    )
+end
+
+pgroup.test_gh_437_insert_object_many_explicit_bucket_ids = function(g)
+    write_scenario.gh_437_many_explicit_bucket_ids(g, 'insert_object_many', {objects = true})
+end
+
+pgroup.test_gh_437_insert_object_many_partial_explicit_bucket_ids = function(g)
+    write_scenario.gh_437_many_explicit_bucket_ids(
+        g,
+        'insert_object_many',
+        {objects = true, partial_explicit_bucket_ids = true}
+    )
+end
diff --git a/test/integration/replace_many_test.lua b/test/integration/replace_many_test.lua
@@ -3,6 +3,7 @@ local t = require('luatest')
 local crud = require('crud')
 
 local helpers = require('test.helper')
+local write_scenario = require('test.integration.write_scenario')
 
 local batching_utils = require('crud.common.batching_utils')
 
@@ -20,6 +21,7 @@ end)
 
 pgroup.before_each(function(g)
     helpers.truncate_space_on_cluster(g.cluster, 'developers')
+    helpers.truncate_space_on_cluster(g.cluster, 'customers_sharded_by_age')
 end)
 
 pgroup.test_non_existent_space = function(g)
@@ -2062,3 +2064,27 @@ pgroup.test_zero_objects = function(g)
     t.assert_str_contains(errs[1].err, "At least one object expected")
     t.assert_equals(result, nil)
 end
+
+pgroup.test_gh_437_replace_many_explicit_bucket_ids = function(g)
+    write_scenario.gh_437_many_explicit_bucket_ids(g, 'replace_many')
+end
+
+pgroup.test_gh_437_replace_many_partial_explicit_bucket_ids = function(g)
+    write_scenario.gh_437_many_explicit_bucket_ids(
+        g,
+        'replace_many',
+        {partial_explicit_bucket_ids = true}
+    )
+end
+
+pgroup.test_gh_437_replace_object_many_explicit_bucket_ids = function(g)
+    write_scenario.gh_437_many_explicit_bucket_ids(g, 'replace_object_many', {objects = true})
+end
+
+pgroup.test_gh_437_replace_object_many_partial_explicit_bucket_ids = function(g)
+    write_scenario.gh_437_many_explicit_bucket_ids(
+        g,
+        'replace_object_many',
+        {objects = true, partial_explicit_bucket_ids = true}
+    )
+end
diff --git a/test/integration/upsert_many_test.lua b/test/integration/upsert_many_test.lua
@@ -1,6 +1,7 @@
 local t = require('luatest')
 
 local helpers = require('test.helper')
+local write_scenario = require('test.integration.write_scenario')
 
 local batching_utils = require('crud.common.batching_utils')
 
@@ -18,6 +19,7 @@ end)
 
 pgroup.before_each(function(g)
     helpers.truncate_space_on_cluster(g.cluster, 'customers')
+    helpers.truncate_space_on_cluster(g.cluster, 'customers_sharded_by_age')
 end)
 
 pgroup.test_non_existent_space = function(g)
@@ -2063,3 +2065,31 @@ pgroup.test_zero_objects = function(g)
     t.assert_str_contains(errs[1].err, "At least one object expected")
     t.assert_equals(result, nil)
 end
+
+pgroup.test_gh_437_upsert_many_explicit_bucket_ids = function(g)
+    write_scenario.gh_437_many_explicit_bucket_ids(g, 'upsert_many', {upsert = true})
+end
+
+pgroup.test_gh_437_upsert_many_partial_explicit_bucket_ids = function(g)
+    write_scenario.gh_437_many_explicit_bucket_ids(
+        g,
+        'upsert_many',
+        {upsert = true, partial_explicit_bucket_ids = true}
+    )
+end
+
+pgroup.test_gh_437_upsert_object_many_explicit_bucket_ids = function(g)
+    write_scenario.gh_437_many_explicit_bucket_ids(
+        g,
+        'upsert_object_many',
+        {upsert = true, objects = true}
+    )
+end
+
+pgroup.test_gh_437_upsert_object_many_partial_explicit_bucket_ids = function(g)
+    write_scenario.gh_437_many_explicit_bucket_ids(
+        g,
+        'upsert_object_many',
+        {upsert = true, objects = true, partial_explicit_bucket_ids = true}
+    )
+end
diff --git a/test/integration/write_scenario.lua b/test/integration/write_scenario.lua
@@ -0,0 +1,62 @@
+local t = require('luatest')
+local checks = require('checks')
+
+-- Scenario is for 'srv_batch_operations' entrypoint.
+local function gh_437_many_explicit_bucket_ids(cg, operation, opts)
+    checks('table', 'string', {
+        objects = '?boolean',
+        upsert = '?boolean',
+        partial_explicit_bucket_ids = '?boolean',
+    })
+
+    opts = opts or {}
+
+    local rows = {
+        {1, 1, 'Kumiko', 18},
+        {2, 2, 'Reina', 19},
+        {3, 3, 'Shuuichi', 18},
+    }
+
+    if opts.partial_explicit_bucket_ids then
+        rows[2][2] = box.NULL
+    end
+
+    local objects = {}
+    for k, v in ipairs(rows) do
+        objects[k] = {id = v[1], bucket_id = v[2], name = v[3], age = v[4]}
+    end
+
+    local data
+    if opts.objects then
+        data = objects
+    else
+        data = rows
+    end
+
+    if opts.upsert then
+        local update_operations = {}
+        for k, v in ipairs(data) do
+            data[k] = {v, update_operations}
+        end
+    end
+
+    local args = {'customers_sharded_by_age', data}
+
+    local result, errs = cg.router:call('crud.' .. operation, args)
+    t.assert_equals(errs, nil)
+
+    local result_rows = table.deepcopy(rows)
+    if opts.partial_explicit_bucket_ids then
+        result_rows[2][2] = 1325
+    end
+    if opts.upsert then
+        -- upsert never return anything.
+        t.assert_equals(result.rows, nil)
+    else
+        t.assert_items_equals(result.rows, result_rows)
+    end
+end
+
+return {
+    gh_437_many_explicit_bucket_ids = gh_437_many_explicit_bucket_ids,
+}