diff --git a/CHANGELOG.md b/CHANGELOG.md index 8a798113..75d45c7e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ### Fixed +* Fixed not finding field in tuple on ``crud.update`` if + there are ``is_nullable`` fields in front of it that were added + when the schema was changed. * Fixed select crash when dropping indexes * Using outdated schema on router-side * Sparsed tuples generation that led to "Tuple/Key must be MsgPack array" error diff --git a/crud/common/utils.lua b/crud/common/utils.lua index 29610d1e..6991b0df 100644 --- a/crud/common/utils.lua +++ b/crud/common/utils.lua @@ -199,11 +199,70 @@ function utils.tarantool_supports_uuids() return enabled_tarantool_features.uuids end -function utils.convert_operations(user_operations, space_format) +local function add_nullable_fields_recursive(operations, operations_map, space_format, tuple, id) + if id < 2 or tuple[id - 1] ~= box.NULL then + return operations + end + + if space_format[id - 1].is_nullable and not operations_map[id - 1] then + table.insert(operations, {'=', id - 1, box.NULL}) + return add_nullable_fields_recursive(operations, operations_map, space_format, tuple, id - 1) + end + + return operations +end + +-- Tarantool < 2.3 has no fields `box.error.NO_SUCH_FIELD_NO` and `box.error.NO_SUCH_FIELD_NAME`. +if _TARANTOOL >= "2.3" then + function utils.is_field_not_found(err_code) + return err_code == box.error.NO_SUCH_FIELD_NO or err_code == box.error.NO_SUCH_FIELD_NAME + end +else + function utils.is_field_not_found(err_code) + return err_code == box.error.NO_SUCH_FIELD + end +end + +local function get_operations_map(operations) + local map = {} + for _, operation in ipairs(operations) do + map[operation[2]] = true + end + + return map +end + +function utils.add_intermediate_nullable_fields(operations, space_format, tuple) + if tuple == nil then + return operations + end + + -- If tarantool doesn't supports the fieldpaths, we already + -- have converted operations (see this function call in update.lua) if utils.tarantool_supports_fieldpaths() then - return user_operations + local formatted_operations, err = utils.convert_operations(operations, space_format) + if err ~= nil then + return operations + end + + operations = formatted_operations + end + + -- We need this map to check if there is a field update + -- operation with constant complexity + local operations_map = get_operations_map(operations) + for _, operation in ipairs(operations) do + operations = add_nullable_fields_recursive( + operations, operations_map, + space_format, tuple, operation[2] + ) end + table.sort(operations, function(v1, v2) return v1[2] < v2[2] end) + return operations +end + +function utils.convert_operations(user_operations, space_format) local converted_operations = {} for _, operation in ipairs(user_operations) do diff --git a/crud/update.lua b/crud/update.lua index fd331964..2c403f29 100644 --- a/crud/update.lua +++ b/crud/update.lua @@ -24,10 +24,32 @@ local function update_on_storage(space_name, key, operations, field_names) -- add_space_schema_hash is false because -- reloading space format on router can't avoid update error on storage - return schema.wrap_box_space_func_result(space, 'update', {key, operations}, { + local res, err = schema.wrap_box_space_func_result(space, 'update', {key, operations}, { add_space_schema_hash = false, field_names = field_names, }) + + if err ~= nil then + return nil, err + end + + if res.err == nil then + return res, nil + end + + -- We can only add fields to end of the tuple. + -- If schema is updated and nullable fields are added, then we will get error. + -- Therefore, we need to add filling of intermediate nullable fields. + -- More details: https://github.com/tarantool/tarantool/issues/3378 + if utils.is_field_not_found(res.err.code) then + operations = utils.add_intermediate_nullable_fields(operations, space:format(), space:get(key)) + res, err = schema.wrap_box_space_func_result(space, 'update', {key, operations}, { + add_space_schema_hash = false, + field_names = field_names, + }) + end + + return res, err end function update.init() @@ -46,7 +68,11 @@ local function call_update_on_router(space_name, key, user_operations, opts) opts = opts or {} - local space = utils.get_space(space_name, vshard.router.routeall()) + local space, err = utils.get_space(space_name, vshard.router.routeall()) + if err ~= nil then + return nil, UpdateError:new("Failed to get space %q: %s", space_name, err), true + end + if space == nil then return nil, UpdateError:new("Space %q doesn't exist", space_name), true end @@ -57,9 +83,12 @@ local function call_update_on_router(space_name, key, user_operations, opts) key = key:totable() end - local operations, err = utils.convert_operations(user_operations, space_format) - if err ~= nil then - return nil, UpdateError:new("Wrong operations are specified: %s", err), true + local operations = user_operations + if not utils.tarantool_supports_fieldpaths() then + operations, err = utils.convert_operations(user_operations, space_format) + if err ~= nil then + return nil, UpdateError:new("Wrong operations are specified: %s", err), true + end end local bucket_id = sharding.key_get_bucket_id(key, opts.bucket_id) diff --git a/crud/upsert.lua b/crud/upsert.lua index 30979837..00216526 100644 --- a/crud/upsert.lua +++ b/crud/upsert.lua @@ -61,9 +61,12 @@ local function call_upsert_on_router(space_name, tuple, user_operations, opts) end local space_format = space:format() - local operations, err = utils.convert_operations(user_operations, space_format) - if err ~= nil then - return nil, UpsertError:new("Wrong operations are specified: %s", err), true + local operations = user_operations + if not utils.tarantool_supports_fieldpaths() then + operations, err = utils.convert_operations(user_operations, space_format) + if err ~= nil then + return nil, UpsertError:new("Wrong operations are specified: %s", err), true + end end local bucket_id, err = sharding.tuple_set_and_return_bucket_id(tuple, space, opts.bucket_id) diff --git a/test/entrypoint/srv_simple_operations.lua b/test/entrypoint/srv_simple_operations.lua index b437afe0..3ea1705f 100755 --- a/test/entrypoint/srv_simple_operations.lua +++ b/test/entrypoint/srv_simple_operations.lua @@ -32,6 +32,30 @@ package.preload['customers-storage'] = function() if_not_exists = true, }) + local developers_space = box.schema.space.create('developers', { + format = { + {name = 'id', type = 'unsigned'}, + {name = 'bucket_id', type = 'unsigned'}, + }, + if_not_exists = true, + engine = engine, + }) + developers_space:create_index('id', { + parts = { {field = 'id'} }, + if_not_exists = true, + }) + developers_space:create_index('bucket_id', { + parts = { {field = 'bucket_id'} }, + unique = false, + if_not_exists = true, + }) + + rawset(_G, 'add_extra_field', function(name) + local new_format = box.space.developers:format() + table.insert(new_format, {name = name, type = 'any', is_nullable = true}) + box.space.developers:format(new_format) + end) + -- Space with huge amount of nullable fields -- an object that inserted in such space should get -- explicit nulls in absence fields otherwise diff --git a/test/integration/simple_operations_test.lua b/test/integration/simple_operations_test.lua index e9f18eb3..c727c718 100644 --- a/test/integration/simple_operations_test.lua +++ b/test/integration/simple_operations_test.lua @@ -392,6 +392,92 @@ pgroup:add('test_upsert', function(g) t.assert_equals(result.rows, {{67, 1143, 'Mikhail Saltykov-Shchedrin', 63}}) end) +pgroup:add('test_intermediate_nullable_fields_update', function(g) + local result, err = g.cluster.main_server.net_box:call( + 'crud.insert', {'developers', {1, box.NULL}}) + t.assert_equals(err, nil) + + local objects = crud.unflatten_rows(result.rows, result.metadata) + t.assert_equals(objects, { + { + id = 1, + bucket_id = 477 + } + }) + + helpers.call_on_servers(g.cluster, {'s1-master', 's2-master'}, function(server) + for i = 1, 12 do + server.net_box:call('add_extra_field', {'extra_' .. tostring(i)}) + end + end) + + local result, err = g.cluster.main_server.net_box:call('crud.update', + {'developers', 1, {{'=', 'extra_3', { a = { b = {} } } }}}) + t.assert_equals(err, nil) + objects = crud.unflatten_rows(result.rows, result.metadata) + t.assert_equals(objects, { + { + id = 1, + bucket_id = 477, + extra_1 = box.NULL, + extra_2 = box.NULL, + extra_3 = {a = {b = {}}}, + } + }) + + -- This tests use jsonpath updates. + if _TARANTOOL >= "2.3" then + local _, err = g.cluster.main_server.net_box:call('crud.update', + {'developers', 1, {{'=', '[5].a.b[1]', 3}, {'=', 'extra_5', 'extra_value_5'}}}) + t.assert_equals(err.err, "Failed to update: Field ''extra_5'' was not found in the tuple") + end + + result, err = g.cluster.main_server.net_box:call('crud.update', + {'developers', 1, {{'=', 5, 'extra_value_3'}, {'=', 7, 'extra_value_5'}}}) + t.assert_equals(err, nil) + objects = crud.unflatten_rows(result.rows, result.metadata) + t.assert_equals(objects, { + { + id = 1, + bucket_id = 477, + extra_1 = box.NULL, + extra_2 = box.NULL, + extra_3 = 'extra_value_3', + extra_4 = box.NULL, + extra_5 = 'extra_value_5', + } + }) + + result, err = g.cluster.main_server.net_box:call('crud.update', + {'developers', 1, { + {'=', 14, 'extra_value_12'}, + {'=', 'extra_9', 'extra_value_9'}, + {'=', 'extra_3', 'updated_extra_value_3'} + } + }) + + t.assert_equals(err, nil) + objects = crud.unflatten_rows(result.rows, result.metadata) + t.assert_equals(objects, { + { + id = 1, + bucket_id = 477, + extra_1 = box.NULL, + extra_2 = box.NULL, + extra_3 = 'updated_extra_value_3', + extra_4 = box.NULL, + extra_5 = 'extra_value_5', + extra_6 = box.NULL, + extra_7 = box.NULL, + extra_8 = box.NULL, + extra_9 = 'extra_value_9', + extra_10 = box.NULL, + extra_11 = box.NULL, + extra_12 = 'extra_value_12' + } + }) +end) + pgroup:add('test_object_with_nullable_fields', function(g) -- Insert local result, err = g.cluster.main_server.net_box:call(