From 29166c1152e2edef745ad59b102aeeb7f75634cb Mon Sep 17 00:00:00 2001 From: Alexey Romanov Date: Thu, 18 Feb 2021 17:59:34 +0300 Subject: [PATCH 01/18] Added intermediate nullable fields when update --- crud/common/utils.lua | 71 +++++++++++++++++++++ crud/update.lua | 9 +++ test/entrypoint/srv_simple_operations.lua | 24 +++++++ test/integration/simple_operations_test.lua | 16 +++++ 4 files changed, 120 insertions(+) diff --git a/crud/common/utils.lua b/crud/common/utils.lua index 29610d1e..a74fbe92 100644 --- a/crud/common/utils.lua +++ b/crud/common/utils.lua @@ -199,6 +199,77 @@ function utils.tarantool_supports_uuids() return enabled_tarantool_features.uuids end +function utils.table_to_string(tbl) + local result = "{" + for k, v in pairs(tbl) do + -- Check the key type (ignore any numerical keys - assume its an array) + if type(k) == "string" then + result = result.."[\""..k.."\"]".."=" + end + + -- Check the value type + if type(v) == "table" then + result = result..utils.table_to_string(v) + elseif type(v) == "boolean" then + result = result..tostring(v) + else + result = result.."\""..v.."\"" + end + result = result.."," + end + -- Remove leading commas from the result + if result ~= "" then + result = result:sub(1, result:len()-1) + end + return result.."}" +end + +function utils.add_intermediate_nullable_fields(user_operations, space_format) + local math = require('math') + local formatted_operations = {} + local max_field_id = 0 + local file = io.open('test_form', 'a+') + io.output(file) + + for _, operation in ipairs(user_operations) do + if type(operation[2]) == 'string' then + local field_id + for fieldno, field_format in ipairs(space_format) do + io.write(table_to_string(field_format)) + if field_format.name == operation[2] then + field_id = fieldno + break + end + io.write(field_format.name) + end + + if field_id == nil then + return nil, ParseOperationsError:new( + "Space format doesn't contain field named %q", operation[2]) + end + + max_field_id = math.max(field_id, max_field_id) + table.insert(formatted_operations, { + operation[1], field_id, operation[3] + }) + else + max_field_id = math.max(operation[2], max_field_id) + table.insert(formatted_operations, operation) + end + end + + if max_field_id == #space_format then + for fieldno, field_format in ipairs(space_format) do + if field_format.is_nullable == true and fieldno ~= max_field_id then + table.insert(formatted_operations, {'=', fieldno, box.NULL}) + end + end + end + + table.sort(formatted_operations, function(v1, v2) return v1[2] < v2[2] end) + return formatted_operations +end + function utils.convert_operations(user_operations, space_format) if utils.tarantool_supports_fieldpaths() then return user_operations diff --git a/crud/update.lua b/crud/update.lua index fd331964..4b048566 100644 --- a/crud/update.lua +++ b/crud/update.lua @@ -57,7 +57,16 @@ local function call_update_on_router(space_name, key, user_operations, opts) key = key:totable() end + --local user_operations, err = utils.add_intermediate_nullable_fields(user_operations, space_format) + --if err ~= nil then + --return nil, UpdateError:new("%s", err), true + --end local operations, err = utils.convert_operations(user_operations, space_format) + local file = io.open('test1.txt', 'a+') + io.output(file) + io.write('\n\n') + io.write(utils.table_to_string(space_format)) + io.write('\n\n') if err ~= nil then return nil, UpdateError:new("Wrong operations are specified: %s", err), true end diff --git a/test/entrypoint/srv_simple_operations.lua b/test/entrypoint/srv_simple_operations.lua index b437afe0..7f9e4d66 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 = 'string', 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..ea6afc80 100644 --- a/test/integration/simple_operations_test.lua +++ b/test/integration/simple_operations_test.lua @@ -392,6 +392,22 @@ 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) + + helpers.call_on_servers(g.cluster, {'s1-master', 's2-master'}, function(server) + server.net_box:call('add_extra_field', {'extra_1'}) + server.net_box:call('add_extra_field', {'extra_2'}) + end) + + local result, err = g.cluster.main_server.net_box:call('crud.update', + {'developers', 1, {{'=', 'extra_2', 'extra_value'}}}) + t.assert_equals(err, nil) + --t.assert_equals(result, nil) +end) + pgroup:add('test_object_with_nullable_fields', function(g) -- Insert local result, err = g.cluster.main_server.net_box:call( From 2d3131b90ba886ee75cffc8a960110870a3aeef0 Mon Sep 17 00:00:00 2001 From: Alexey Romanov Date: Sat, 20 Feb 2021 04:13:43 +0300 Subject: [PATCH 02/18] Update add intermediate nullable fields --- crud/common/utils.lua | 89 +++++++-------------- crud/update.lua | 15 ++-- test/integration/simple_operations_test.lua | 50 +++++++++++- 3 files changed, 80 insertions(+), 74 deletions(-) diff --git a/crud/common/utils.lua b/crud/common/utils.lua index a74fbe92..6fa03575 100644 --- a/crud/common/utils.lua +++ b/crud/common/utils.lua @@ -199,82 +199,47 @@ function utils.tarantool_supports_uuids() return enabled_tarantool_features.uuids end -function utils.table_to_string(tbl) - local result = "{" - for k, v in pairs(tbl) do - -- Check the key type (ignore any numerical keys - assume its an array) - if type(k) == "string" then - result = result.."[\""..k.."\"]".."=" +local function id_not_in_table(operations, id) + for _, operation in ipairs(operations) do + if operation[2] == id then + return false end - - -- Check the value type - if type(v) == "table" then - result = result..utils.table_to_string(v) - elseif type(v) == "boolean" then - result = result..tostring(v) - else - result = result.."\""..v.."\"" - end - result = result.."," - end - -- Remove leading commas from the result - if result ~= "" then - result = result:sub(1, result:len()-1) end - return result.."}" + + return true end -function utils.add_intermediate_nullable_fields(user_operations, space_format) - local math = require('math') - local formatted_operations = {} - local max_field_id = 0 - local file = io.open('test_form', 'a+') - io.output(file) +local function add_nullable_fields_rec(operations, space_format, tuple, id) + if id < 2 or tuple[id - 1] ~= box.NULL then + return operations + end - for _, operation in ipairs(user_operations) do - if type(operation[2]) == 'string' then - local field_id - for fieldno, field_format in ipairs(space_format) do - io.write(table_to_string(field_format)) - if field_format.name == operation[2] then - field_id = fieldno - break - end - io.write(field_format.name) - end + if space_format[id - 1].is_nullable and id_not_in_table(operations, id - 1) then + table.insert(operations, {'=', id - 1, box.NULL}) + return add_nullable_fields_rec(operations, space_format, tuple, id - 1) + end - if field_id == nil then - return nil, ParseOperationsError:new( - "Space format doesn't contain field named %q", operation[2]) - end + return operations +end - max_field_id = math.max(field_id, max_field_id) - table.insert(formatted_operations, { - operation[1], field_id, operation[3] - }) - else - max_field_id = math.max(operation[2], max_field_id) - table.insert(formatted_operations, operation) +function utils.add_intermediate_nullable_fields(operations, space_format, tuple) + if tuple ~= nil then + for i = 1, #operations do + operations = add_nullable_fields_rec( + operations, + space_format, + tuple, + operations[i][2] + ) end - end - if max_field_id == #space_format then - for fieldno, field_format in ipairs(space_format) do - if field_format.is_nullable == true and fieldno ~= max_field_id then - table.insert(formatted_operations, {'=', fieldno, box.NULL}) - end - end + table.sort(operations, function(v1, v2) return v1[2] < v2[2] end) end - table.sort(formatted_operations, function(v1, v2) return v1[2] < v2[2] end) - return formatted_operations + return operations end function utils.convert_operations(user_operations, space_format) - if utils.tarantool_supports_fieldpaths() then - return user_operations - end - local converted_operations = {} for _, operation in ipairs(user_operations) do diff --git a/crud/update.lua b/crud/update.lua index 4b048566..99542cf6 100644 --- a/crud/update.lua +++ b/crud/update.lua @@ -57,20 +57,17 @@ local function call_update_on_router(space_name, key, user_operations, opts) key = key:totable() end - --local user_operations, err = utils.add_intermediate_nullable_fields(user_operations, space_format) - --if err ~= nil then - --return nil, UpdateError:new("%s", err), true - --end local operations, err = utils.convert_operations(user_operations, space_format) - local file = io.open('test1.txt', 'a+') - io.output(file) - io.write('\n\n') - io.write(utils.table_to_string(space_format)) - io.write('\n\n') if err ~= nil then return nil, UpdateError:new("Wrong operations are specified: %s", err), true end + if type(key) == 'table' or type(key) == 'number' then + local tuple = space:get{key} + -- See https://github.com/tarantool/crud/issues/113 + operations = utils.add_intermediate_nullable_fields(operations, space_format, tuple) + end + local bucket_id = sharding.key_get_bucket_id(key, opts.bucket_id) local storage_result, err = call.rw_single( bucket_id, UPDATE_FUNC_NAME, diff --git a/test/integration/simple_operations_test.lua b/test/integration/simple_operations_test.lua index ea6afc80..a8a8e768 100644 --- a/test/integration/simple_operations_test.lua +++ b/test/integration/simple_operations_test.lua @@ -397,15 +397,59 @@ pgroup:add('test_intermediate_nullable_fields_update', function(g) '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) server.net_box:call('add_extra_field', {'extra_1'}) server.net_box:call('add_extra_field', {'extra_2'}) + server.net_box:call('add_extra_field', {'extra_3'}) + server.net_box:call('add_extra_field', {'extra_4'}) + server.net_box:call('add_extra_field', {'extra_5'}) + server.net_box:call('add_extra_field', {'extra_6'}) end) - local result, err = g.cluster.main_server.net_box:call('crud.update', - {'developers', 1, {{'=', 'extra_2', 'extra_value'}}}) + -- TODO: delete this, when issue (https://github.com/tarantool/crud/issues/98) will be closed + g.cluster.main_server.net_box:call('crud.select', + {'developers', nil}) + + result, err = g.cluster.main_server.net_box:call('crud.update', + {'developers', 1, {{'=', 'extra_3', '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 = 'extra_value_3', + } + }) + + result, err = g.cluster.main_server.net_box:call('crud.update', + {'developers', 1, {{'=', 8, 'extra_value_6'}}}) -- update extra_6 field + t.assert_equals(err, nil) - --t.assert_equals(result, 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 = box.NULL, + extra_6 = 'extra_value_6' + } + }) end) pgroup:add('test_object_with_nullable_fields', function(g) From d1a98e76cfcafea15dbd910203a08e8e6c596fd0 Mon Sep 17 00:00:00 2001 From: Alexey Romanov Date: Sat, 20 Feb 2021 18:36:51 +0300 Subject: [PATCH 03/18] Fix PR issues --- crud/update.lua | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/crud/update.lua b/crud/update.lua index 99542cf6..307108a0 100644 --- a/crud/update.lua +++ b/crud/update.lua @@ -24,10 +24,23 @@ 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 result = schema.wrap_box_space_func_result(space, 'update', {key, operations}, { add_space_schema_hash = false, field_names = field_names, }) + + local err = result.err + if err ~= nil and (err.code == box.error.NO_SUCH_FIELD_NO or err.code == box.error.NO_SUCH_FIELD_NAME) then + local tuple = space:get(key) + operations = utils.add_intermediate_nullable_fields(operations, space:format(), tuple) + + result = schema.wrap_box_space_func_result(space, 'update', {key, operations}, { + add_space_schema_hash = false, + field_names = field_names, + }) + end + + return result end function update.init() @@ -62,12 +75,6 @@ local function call_update_on_router(space_name, key, user_operations, opts) return nil, UpdateError:new("Wrong operations are specified: %s", err), true end - if type(key) == 'table' or type(key) == 'number' then - local tuple = space:get{key} - -- See https://github.com/tarantool/crud/issues/113 - operations = utils.add_intermediate_nullable_fields(operations, space_format, tuple) - end - local bucket_id = sharding.key_get_bucket_id(key, opts.bucket_id) local storage_result, err = call.rw_single( bucket_id, UPDATE_FUNC_NAME, From 8a9eef243235e5e08195281a459fbbe8600d38d0 Mon Sep 17 00:00:00 2001 From: Alexey Romanov Date: Sat, 20 Feb 2021 20:36:13 +0300 Subject: [PATCH 04/18] Tests fix --- crud/update.lua | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/crud/update.lua b/crud/update.lua index 307108a0..b5f83765 100644 --- a/crud/update.lua +++ b/crud/update.lua @@ -24,23 +24,27 @@ 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 - local result = 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, }) - local err = result.err + if err ~= nil then + return nil, err + end + + local err = res.err if err ~= nil and (err.code == box.error.NO_SUCH_FIELD_NO or err.code == box.error.NO_SUCH_FIELD_NAME) then local tuple = space:get(key) operations = utils.add_intermediate_nullable_fields(operations, space:format(), tuple) - result = schema.wrap_box_space_func_result(space, 'update', {key, operations}, { + res = schema.wrap_box_space_func_result(space, 'update', {key, operations}, { add_space_schema_hash = false, field_names = field_names, }) end - return result + return res end function update.init() From bcb9fdbb535c05a733dad855695b65d174031297 Mon Sep 17 00:00:00 2001 From: Alexey Romanov Date: Sun, 21 Feb 2021 22:51:18 +0300 Subject: [PATCH 05/18] Fix some logic and update test --- crud/common/utils.lua | 26 +++++++++++-------- crud/update.lua | 10 +++++--- test/integration/simple_operations_test.lua | 28 +++++++++++++++++++++ 3 files changed, 50 insertions(+), 14 deletions(-) diff --git a/crud/common/utils.lua b/crud/common/utils.lua index 6fa03575..3eb5df04 100644 --- a/crud/common/utils.lua +++ b/crud/common/utils.lua @@ -223,19 +223,25 @@ local function add_nullable_fields_rec(operations, space_format, tuple, id) end function utils.add_intermediate_nullable_fields(operations, space_format, tuple) - if tuple ~= nil then - for i = 1, #operations do - operations = add_nullable_fields_rec( - operations, - space_format, - tuple, - operations[i][2] - ) - end + if tuple == nil then + return operations + end + + local operations, err = utils.convert_operations(operations, space_format) + if err ~= nil then + return nil, UpdateError:new("Wrong operations are specified: %s", err), true + end - table.sort(operations, function(v1, v2) return v1[2] < v2[2] end) + for i = 1, #operations do + operations = add_nullable_fields_rec( + operations, + space_format, + tuple, + operations[i][2] + ) end + table.sort(operations, function(v1, v2) return v1[2] < v2[2] end) return operations end diff --git a/crud/update.lua b/crud/update.lua index b5f83765..23864454 100644 --- a/crud/update.lua +++ b/crud/update.lua @@ -74,15 +74,17 @@ 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 + if not utils.tarantool_supports_fieldpaths() then + local user_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) local storage_result, err = call.rw_single( bucket_id, UPDATE_FUNC_NAME, - {space_name, key, operations, opts.fields}, + {space_name, key, user_operations, opts.fields}, {timeout = opts.timeout} ) diff --git a/test/integration/simple_operations_test.lua b/test/integration/simple_operations_test.lua index a8a8e768..875691b9 100644 --- a/test/integration/simple_operations_test.lua +++ b/test/integration/simple_operations_test.lua @@ -412,6 +412,11 @@ pgroup:add('test_intermediate_nullable_fields_update', function(g) server.net_box:call('add_extra_field', {'extra_4'}) server.net_box:call('add_extra_field', {'extra_5'}) server.net_box:call('add_extra_field', {'extra_6'}) + server.net_box:call('add_extra_field', {'extra_7'}) + server.net_box:call('add_extra_field', {'extra_8'}) + server.net_box:call('add_extra_field', {'extra_9'}) + server.net_box:call('add_extra_field', {'extra_10'}) + server.net_box:call('add_extra_field', {'extra_11'}) end) -- TODO: delete this, when issue (https://github.com/tarantool/crud/issues/98) will be closed @@ -450,6 +455,29 @@ pgroup:add('test_intermediate_nullable_fields_update', function(g) extra_6 = 'extra_value_6' } }) + + result, err = g.cluster.main_server.net_box:call('crud.update', + {'developers', 1, {{'=', 13, 'extra_value_11'}, {'=', 'extra_8', 'extra_value_8'}}}) + + 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 = box.NULL, + extra_6 = 'extra_value_6', + extra_7 = box.NULL, + extra_8 = 'extra_value_8', + extra_9 = box.NULL, + extra_10 = box.NULL, + extra_11 = 'extra_value_11' + } + }) end) pgroup:add('test_object_with_nullable_fields', function(g) From 4ea9060525f672c4b0404e1df8d21ab85558e903 Mon Sep 17 00:00:00 2001 From: Alexey Romanov Date: Sun, 21 Feb 2021 23:03:16 +0300 Subject: [PATCH 06/18] Style fix --- crud/common/utils.lua | 2 +- crud/update.lua | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/crud/common/utils.lua b/crud/common/utils.lua index 3eb5df04..fa405189 100644 --- a/crud/common/utils.lua +++ b/crud/common/utils.lua @@ -229,7 +229,7 @@ function utils.add_intermediate_nullable_fields(operations, space_format, tuple) local operations, err = utils.convert_operations(operations, space_format) if err ~= nil then - return nil, UpdateError:new("Wrong operations are specified: %s", err), true + return operations end for i = 1, #operations do diff --git a/crud/update.lua b/crud/update.lua index 23864454..1d111e8d 100644 --- a/crud/update.lua +++ b/crud/update.lua @@ -74,8 +74,9 @@ local function call_update_on_router(space_name, key, user_operations, opts) key = key:totable() end + local err if not utils.tarantool_supports_fieldpaths() then - local user_operations, err = utils.convert_operations(user_operations, space_format) + user_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 From 8b4984216e67de238d775e7479f5e7b0fb5930f4 Mon Sep 17 00:00:00 2001 From: Alexey Romanov Date: Mon, 22 Feb 2021 03:02:13 +0300 Subject: [PATCH 07/18] Fix update/upsert tests --- crud/update.lua | 24 ++++++++++++++---------- crud/upsert.lua | 10 ++++++---- 2 files changed, 20 insertions(+), 14 deletions(-) diff --git a/crud/update.lua b/crud/update.lua index 1d111e8d..b66e5b7b 100644 --- a/crud/update.lua +++ b/crud/update.lua @@ -33,18 +33,20 @@ local function update_on_storage(space_name, key, operations, field_names) return nil, err end - local err = res.err - if err ~= nil and (err.code == box.error.NO_SUCH_FIELD_NO or err.code == box.error.NO_SUCH_FIELD_NAME) then - local tuple = space:get(key) - operations = utils.add_intermediate_nullable_fields(operations, space:format(), tuple) + if res.err == nil then + return res, nil + end + + if (res.err.code == box.error.NO_SUCH_FIELD_NO or res.err.code == box.error.NO_SUCH_FIELD_NAME) then + operations = utils.add_intermediate_nullable_fields(operations, space:format(), space:get(key)) - res = schema.wrap_box_space_func_result(space, 'update', {key, operations}, { + res, err = schema.wrap_box_space_func_result(space, 'update', {key, operations}, { add_space_schema_hash = false, field_names = field_names, }) end - return res + return res, err end function update.init() @@ -63,18 +65,20 @@ 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 - local space_format = space:format() - if box.tuple.is(key) then key = key:totable() end - local err + local space_format = space:format() if not utils.tarantool_supports_fieldpaths() then user_operations, err = utils.convert_operations(user_operations, space_format) if err ~= nil then diff --git a/crud/upsert.lua b/crud/upsert.lua index 30979837..b0b4e8ab 100644 --- a/crud/upsert.lua +++ b/crud/upsert.lua @@ -61,9 +61,11 @@ 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 + if not utils.tarantool_supports_fieldpaths() then + user_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) @@ -73,7 +75,7 @@ local function call_upsert_on_router(space_name, tuple, user_operations, opts) local storage_result, err = call.rw_single( bucket_id, UPSERT_FUNC_NAME, - {space_name, tuple, operations}, + {space_name, tuple, user_operations}, {timeout = opts.timeout} ) From 9447a74427f41999addb6058b1cd6b64a2041b8f Mon Sep 17 00:00:00 2001 From: Alexey Romanov Date: Mon, 22 Feb 2021 04:18:55 +0300 Subject: [PATCH 08/18] Tests fix --- crud/common/utils.lua | 14 +++++++------- crud/update.lua | 3 +-- test/integration/simple_operations_test.lua | 3 +-- 3 files changed, 9 insertions(+), 11 deletions(-) diff --git a/crud/common/utils.lua b/crud/common/utils.lua index fa405189..3405d264 100644 --- a/crud/common/utils.lua +++ b/crud/common/utils.lua @@ -227,22 +227,22 @@ function utils.add_intermediate_nullable_fields(operations, space_format, tuple) return operations end - local operations, err = utils.convert_operations(operations, space_format) + local formatted_operations, err = utils.convert_operations(operations, space_format) if err ~= nil then return operations end - for i = 1, #operations do - operations = add_nullable_fields_rec( - operations, + for i = 1, #formatted_operations do + formatted_operations = add_nullable_fields_rec( + formatted_operations, space_format, tuple, - operations[i][2] + formatted_operations[i][2] ) end - table.sort(operations, function(v1, v2) return v1[2] < v2[2] end) - return operations + table.sort(formatted_operations, function(v1, v2) return v1[2] < v2[2] end) + return formatted_operations end function utils.convert_operations(user_operations, space_format) diff --git a/crud/update.lua b/crud/update.lua index b66e5b7b..056d1627 100644 --- a/crud/update.lua +++ b/crud/update.lua @@ -37,9 +37,8 @@ local function update_on_storage(space_name, key, operations, field_names) return res, nil end - if (res.err.code == box.error.NO_SUCH_FIELD_NO or res.err.code == box.error.NO_SUCH_FIELD_NAME) then + if res.err.code == box.error.NO_SUCH_FIELD_NO or res.err.code == box.error.NO_SUCH_FIELD_NAME 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, diff --git a/test/integration/simple_operations_test.lua b/test/integration/simple_operations_test.lua index 875691b9..e6eae8b4 100644 --- a/test/integration/simple_operations_test.lua +++ b/test/integration/simple_operations_test.lua @@ -420,8 +420,7 @@ pgroup:add('test_intermediate_nullable_fields_update', function(g) end) -- TODO: delete this, when issue (https://github.com/tarantool/crud/issues/98) will be closed - g.cluster.main_server.net_box:call('crud.select', - {'developers', nil}) + g.cluster.main_server.net_box:call('crud.select', {'developers'}) result, err = g.cluster.main_server.net_box:call('crud.update', {'developers', 1, {{'=', 'extra_3', 'extra_value_3'}}}) From 074d75c259d4a8633e17fb8764929336c9709aa1 Mon Sep 17 00:00:00 2001 From: Romanov Alexey Date: Mon, 22 Feb 2021 15:44:13 +0300 Subject: [PATCH 09/18] Add tarantool < 2 support --- crud/common/utils.lua | 18 ++++++++++++++++++ crud/update.lua | 6 +++++- 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/crud/common/utils.lua b/crud/common/utils.lua index 3405d264..1fb45871 100644 --- a/crud/common/utils.lua +++ b/crud/common/utils.lua @@ -222,6 +222,24 @@ local function add_nullable_fields_rec(operations, space_format, tuple, id) return operations end +-- Tarantool < 2 has no fields `box.error.NO_SUCH_FIELD_NO` and `box.error.NO_SUCH_FIELD_NAME`. +function utils.is_field_not_found(err_code) + local patch_parts = _G._TARANTOOL:split('-', 1)[1]:split('.', 2) + local major = tonumber(patch_parts[1]) + + if major >= 2 then + if err_code == box.error.NO_SUCH_FIELD_NO or err_code == box.error.NO_SUCH_FIELD_NAME then + return true + end + else + if err_code == box.error.NO_SUCH_FIELD then + return true + end + end + + return false +end + function utils.add_intermediate_nullable_fields(operations, space_format, tuple) if tuple == nil then return operations diff --git a/crud/update.lua b/crud/update.lua index 056d1627..e26da260 100644 --- a/crud/update.lua +++ b/crud/update.lua @@ -37,7 +37,11 @@ local function update_on_storage(space_name, key, operations, field_names) return res, nil end - if res.err.code == box.error.NO_SUCH_FIELD_NO or res.err.code == box.error.NO_SUCH_FIELD_NAME then + -- We can only add fields to end of the tuple. + -- If schema is updated and intermediate null fields are added, then we will get error. + -- Therefore, we need to add filling of intermediate null fields. + -- More details: https://github.com/tarantool/crud/issues/113 + 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, From 4c460e597a88237352771e51550b0fcfbd4a2735 Mon Sep 17 00:00:00 2001 From: Alexey Romanov Date: Wed, 24 Feb 2021 12:46:25 +0300 Subject: [PATCH 10/18] Fix some PR issues --- crud/common/utils.lua | 23 +++++++-------------- test/integration/simple_operations_test.lua | 3 --- 2 files changed, 8 insertions(+), 18 deletions(-) diff --git a/crud/common/utils.lua b/crud/common/utils.lua index 1fb45871..3c86db48 100644 --- a/crud/common/utils.lua +++ b/crud/common/utils.lua @@ -222,22 +222,15 @@ local function add_nullable_fields_rec(operations, space_format, tuple, id) return operations end --- Tarantool < 2 has no fields `box.error.NO_SUCH_FIELD_NO` and `box.error.NO_SUCH_FIELD_NAME`. -function utils.is_field_not_found(err_code) - local patch_parts = _G._TARANTOOL:split('-', 1)[1]:split('.', 2) - local major = tonumber(patch_parts[1]) - - if major >= 2 then - if err_code == box.error.NO_SUCH_FIELD_NO or err_code == box.error.NO_SUCH_FIELD_NAME then - return true - end - else - if err_code == box.error.NO_SUCH_FIELD then - return true - 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 - - return false end function utils.add_intermediate_nullable_fields(operations, space_format, tuple) diff --git a/test/integration/simple_operations_test.lua b/test/integration/simple_operations_test.lua index e6eae8b4..c6559fdd 100644 --- a/test/integration/simple_operations_test.lua +++ b/test/integration/simple_operations_test.lua @@ -419,9 +419,6 @@ pgroup:add('test_intermediate_nullable_fields_update', function(g) server.net_box:call('add_extra_field', {'extra_11'}) end) - -- TODO: delete this, when issue (https://github.com/tarantool/crud/issues/98) will be closed - g.cluster.main_server.net_box:call('crud.select', {'developers'}) - result, err = g.cluster.main_server.net_box:call('crud.update', {'developers', 1, {{'=', 'extra_3', 'extra_value_3'}}}) From 10af29bb64575479fb42ff120503ffa6d86927eb Mon Sep 17 00:00:00 2001 From: Alexey Romanov Date: Wed, 24 Feb 2021 14:30:48 +0300 Subject: [PATCH 11/18] Fix update with json-path updates --- crud/common/utils.lua | 43 ++++++++------- test/entrypoint/srv_simple_operations.lua | 2 +- test/integration/simple_operations_test.lua | 59 ++++++++++++--------- 3 files changed, 60 insertions(+), 44 deletions(-) diff --git a/crud/common/utils.lua b/crud/common/utils.lua index 3c86db48..0eb6523d 100644 --- a/crud/common/utils.lua +++ b/crud/common/utils.lua @@ -199,24 +199,23 @@ function utils.tarantool_supports_uuids() return enabled_tarantool_features.uuids end -local function id_not_in_table(operations, id) +local function exists_operations_id_map(operations) + local map = {} for _, operation in ipairs(operations) do - if operation[2] == id then - return false - end + map[operation[2]] = true end - return true + return map end -local function add_nullable_fields_rec(operations, space_format, tuple, id) +local function add_nullable_fields_rec(operations, operations_id_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 id_not_in_table(operations, id - 1) then + if space_format[id - 1].is_nullable and not operations_id_map[id - 1] then table.insert(operations, {'=', id - 1, box.NULL}) - return add_nullable_fields_rec(operations, space_format, tuple, id - 1) + return add_nullable_fields_rec(operations, operations_id_map, space_format, tuple, id - 1) end return operations @@ -238,22 +237,29 @@ function utils.add_intermediate_nullable_fields(operations, space_format, tuple) return operations end - local formatted_operations, err = utils.convert_operations(operations, space_format) - if err ~= nil then - return operations + if utils.tarantool_supports_fieldpaths() then + local formatted_operations, err = utils.convert_operations(operations, space_format) + if err ~= nil then + return operations + end + + operations = formatted_operations end - for i = 1, #formatted_operations do - formatted_operations = add_nullable_fields_rec( - formatted_operations, + -- We need this map to check existence of an operation in constant complexity + local operations_id_map = exists_operations_id_map(operations) + for i = 1, #operations do + operations = add_nullable_fields_rec( + operations, + operations_id_map, space_format, tuple, - formatted_operations[i][2] + operations[i][2] ) end - table.sort(formatted_operations, function(v1, v2) return v1[2] < v2[2] end) - return formatted_operations + table.sort(operations, function(v1, v2) return v1[2] < v2[2] end) + return operations end function utils.convert_operations(user_operations, space_format) @@ -263,7 +269,8 @@ function utils.convert_operations(user_operations, space_format) if type(operation[2]) == 'string' then local field_id for fieldno, field_format in ipairs(space_format) do - if field_format.name == operation[2] then + local operation_name = operation[2]:split('.', 1)[1] + if field_format.name == operation_name then field_id = fieldno break end diff --git a/test/entrypoint/srv_simple_operations.lua b/test/entrypoint/srv_simple_operations.lua index 7f9e4d66..3ea1705f 100755 --- a/test/entrypoint/srv_simple_operations.lua +++ b/test/entrypoint/srv_simple_operations.lua @@ -52,7 +52,7 @@ package.preload['customers-storage'] = function() rawset(_G, 'add_extra_field', function(name) local new_format = box.space.developers:format() - table.insert(new_format, {name = name, type = 'string', is_nullable = true}) + table.insert(new_format, {name = name, type = 'any', is_nullable = true}) box.space.developers:format(new_format) end) diff --git a/test/integration/simple_operations_test.lua b/test/integration/simple_operations_test.lua index c6559fdd..705a1875 100644 --- a/test/integration/simple_operations_test.lua +++ b/test/integration/simple_operations_test.lua @@ -406,22 +406,27 @@ pgroup:add('test_intermediate_nullable_fields_update', function(g) }) helpers.call_on_servers(g.cluster, {'s1-master', 's2-master'}, function(server) - server.net_box:call('add_extra_field', {'extra_1'}) - server.net_box:call('add_extra_field', {'extra_2'}) - server.net_box:call('add_extra_field', {'extra_3'}) - server.net_box:call('add_extra_field', {'extra_4'}) - server.net_box:call('add_extra_field', {'extra_5'}) - server.net_box:call('add_extra_field', {'extra_6'}) - server.net_box:call('add_extra_field', {'extra_7'}) - server.net_box:call('add_extra_field', {'extra_8'}) - server.net_box:call('add_extra_field', {'extra_9'}) - server.net_box:call('add_extra_field', {'extra_10'}) - server.net_box:call('add_extra_field', {'extra_11'}) + for i = 1, 12 do + server.net_box:call('add_extra_field', {'extra_' .. tostring(i)}) + end end) result, err = g.cluster.main_server.net_box:call('crud.update', - {'developers', 1, {{'=', 'extra_3', 'extra_value_3'}}}) + {'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 = {}}}, + } + }) + result, err = g.cluster.main_server.net_box:call('crud.update', + {'developers', 1, {{'=', 'extra_3.a.b[1]', 2}, {'=', 'extra_5', 'extra_value_5'}}}) t.assert_equals(err, nil) objects = crud.unflatten_rows(result.rows, result.metadata) t.assert_equals(objects, { @@ -430,12 +435,14 @@ pgroup:add('test_intermediate_nullable_fields_update', function(g) bucket_id = 477, extra_1 = box.NULL, extra_2 = box.NULL, - extra_3 = 'extra_value_3', + extra_3 = 2, + extra_4 = box.NULL, + extra_5 = 'extra_value_5' } }) result, err = g.cluster.main_server.net_box:call('crud.update', - {'developers', 1, {{'=', 8, 'extra_value_6'}}}) -- update extra_6 field + {'developers', 1, {{'=', 9, 'extra_value_7'}}}) -- update extra_7 field t.assert_equals(err, nil) objects = crud.unflatten_rows(result.rows, result.metadata) @@ -445,15 +452,16 @@ pgroup:add('test_intermediate_nullable_fields_update', function(g) bucket_id = 477, extra_1 = box.NULL, extra_2 = box.NULL, - extra_3 = 'extra_value_3', + extra_3 = 2, extra_4 = box.NULL, - extra_5 = box.NULL, - extra_6 = 'extra_value_6' + extra_5 = 'extra_value_5', + extra_6 = box.NULL, + extra_7 = 'extra_value_7' } }) result, err = g.cluster.main_server.net_box:call('crud.update', - {'developers', 1, {{'=', 13, 'extra_value_11'}, {'=', 'extra_8', 'extra_value_8'}}}) + {'developers', 1, {{'=', 14, 'extra_value_12'}, {'=', 'extra_9', 'extra_value_9'}}}) t.assert_equals(err, nil) objects = crud.unflatten_rows(result.rows, result.metadata) @@ -463,15 +471,16 @@ pgroup:add('test_intermediate_nullable_fields_update', function(g) bucket_id = 477, extra_1 = box.NULL, extra_2 = box.NULL, - extra_3 = 'extra_value_3', + extra_3 = 2, extra_4 = box.NULL, - extra_5 = box.NULL, - extra_6 = 'extra_value_6', - extra_7 = box.NULL, - extra_8 = 'extra_value_8', - extra_9 = box.NULL, + extra_5 = 'extra_value_5', + extra_6 = box.NULL, + extra_7 = 'extra_value_7', + extra_8 = box.NULL, + extra_9 = 'extra_value_9', extra_10 = box.NULL, - extra_11 = 'extra_value_11' + extra_11 = box.NULL, + extra_12 = 'extra_value_12' } }) end) From dc5544f2554f7d709f2d4bbaffb2b657fc919335 Mon Sep 17 00:00:00 2001 From: Alexey Romanov Date: Wed, 24 Feb 2021 20:03:48 +0300 Subject: [PATCH 12/18] Change update operations logic --- crud/common/utils.lua | 106 +++++++++++---- crud/update.lua | 10 +- test/integration/simple_operations_test.lua | 138 ++++++++++++-------- 3 files changed, 162 insertions(+), 92 deletions(-) diff --git a/crud/common/utils.lua b/crud/common/utils.lua index 0eb6523d..c1fd1fe7 100644 --- a/crud/common/utils.lua +++ b/crud/common/utils.lua @@ -199,23 +199,23 @@ function utils.tarantool_supports_uuids() return enabled_tarantool_features.uuids end -local function exists_operations_id_map(operations) - local map = {} - for _, operation in ipairs(operations) do - map[operation[2]] = true +local function map_to_table(map) + local tab = {} + for _, value in pairs(map) do + table.insert(tab, value) end - return map + return tab end -local function add_nullable_fields_rec(operations, operations_id_map, space_format, tuple, id) +local function add_nullable_fields_recursive(operations, 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_id_map[id - 1] then - table.insert(operations, {'=', id - 1, box.NULL}) - return add_nullable_fields_rec(operations, operations_id_map, space_format, tuple, id - 1) + if space_format[id - 1].is_nullable and not operations[id - 1] then + operations[id - 1] = {'=', id - 1, box.NULL} + return add_nullable_fields_recursive(operations, space_format, tuple, id - 1) end return operations @@ -237,40 +237,58 @@ function utils.add_intermediate_nullable_fields(operations, space_format, tuple) return operations end - if utils.tarantool_supports_fieldpaths() then - local formatted_operations, err = utils.convert_operations(operations, space_format) - if err ~= nil then - return operations - end - - operations = formatted_operations + -- We need this map to check existence of an operation in constant complexity + local operations_map, err = utils.get_operations_map(operations, space_format) + if err ~= nil then + return operations end - -- We need this map to check existence of an operation in constant complexity - local operations_id_map = exists_operations_id_map(operations) - for i = 1, #operations do - operations = add_nullable_fields_rec( - operations, - operations_id_map, - space_format, - tuple, - operations[i][2] + for key, _ in pairs(operations_map) do + operations_map = add_nullable_fields_recursive( + operations_map, space_format, + tuple, key ) end - table.sort(operations, function(v1, v2) return v1[2] < v2[2] end) - return operations + return map_to_table(operations_map) +end + +local function convert_jsonpath_operation(operation) + local field_parts = operation[2]:split('.', 1) + local field_name = field_parts[1] + local field_id = tonumber(field_name:sub(2, -2)) + + if #field_parts > 1 or field_id == nil then + -- Checking '[4].a.b' case + if field_name:sub(1, 1) == '[' and field_name:sub(-1, -1) == ']' then + local field_id = tonumber(field_name:sub(2, -2)) + + if field_id == nil then + -- Checking '["field_name"]' case + if field_name:sub(2, 2) == '"' and field_name:sub(-2, -2) == '"' then + return field_name:sub(3, -3), nil + end + end + + return nil, tonumber(field_name:sub(2, -2)) + end + end + + return field_name, nil end function utils.convert_operations(user_operations, space_format) + if utils.tarantool_supports_fieldpaths() then + return user_operations + end + local converted_operations = {} for _, operation in ipairs(user_operations) do if type(operation[2]) == 'string' then local field_id for fieldno, field_format in ipairs(space_format) do - local operation_name = operation[2]:split('.', 1)[1] - if field_format.name == operation_name then + if field_format.name == operation[2] then field_id = fieldno break end @@ -292,6 +310,36 @@ function utils.convert_operations(user_operations, space_format) return converted_operations end +function utils.get_operations_map(user_operations, space_format) + local operations_map = {} + + for _, operation in ipairs(user_operations) do + if type(operation[2]) == 'string' then + local field_name, field_id = convert_jsonpath_operation(operation) + + if field_id == nil then + for fieldno, field_format in ipairs(space_format) do + if field_format.name == field_name then + field_id = fieldno + break + end + end + + if field_id == nil then + return nil, ParseOperationsError:new( + "Space format doesn't contain field named %q", operation[2]) + end + end + + operations_map[field_id] = operation + else + operations_map[operation[2]] = operation + end + end + + return operations_map +end + function utils.unflatten_rows(rows, metadata) if metadata == nil then return nil, UnflattenError:new('Metadata is not provided') diff --git a/crud/update.lua b/crud/update.lua index e26da260..12ff61a1 100644 --- a/crud/update.lua +++ b/crud/update.lua @@ -82,17 +82,15 @@ local function call_update_on_router(space_name, key, user_operations, opts) end local space_format = space:format() - if not utils.tarantool_supports_fieldpaths() then - user_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 + local formatted_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 local bucket_id = sharding.key_get_bucket_id(key, opts.bucket_id) local storage_result, err = call.rw_single( bucket_id, UPDATE_FUNC_NAME, - {space_name, key, user_operations, opts.fields}, + {space_name, key, formatted_operations, opts.fields}, {timeout = opts.timeout} ) diff --git a/test/integration/simple_operations_test.lua b/test/integration/simple_operations_test.lua index 705a1875..d42bbf3b 100644 --- a/test/integration/simple_operations_test.lua +++ b/test/integration/simple_operations_test.lua @@ -425,64 +425,88 @@ pgroup:add('test_intermediate_nullable_fields_update', function(g) } }) - result, err = g.cluster.main_server.net_box:call('crud.update', - {'developers', 1, {{'=', 'extra_3.a.b[1]', 2}, {'=', 'extra_5', '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 = 2, - extra_4 = box.NULL, - extra_5 = 'extra_value_5' - } - }) - - result, err = g.cluster.main_server.net_box:call('crud.update', - {'developers', 1, {{'=', 9, 'extra_value_7'}}}) -- update extra_7 field - - 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 = 2, - extra_4 = box.NULL, - extra_5 = 'extra_value_5', - extra_6 = box.NULL, - extra_7 = 'extra_value_7' - } - }) - - result, err = g.cluster.main_server.net_box:call('crud.update', - {'developers', 1, {{'=', 14, 'extra_value_12'}, {'=', 'extra_9', 'extra_value_9'}}}) + -- This tests use jsonpath updates. + if _TARANTOOL >= "2.3" then + result, 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, 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 = {3}}}, + extra_4 = box.NULL, + extra_5 = 'extra_value_5' + } + }) + + result, err = g.cluster.main_server.net_box:call('crud.update', + {'developers', 1, {{'=', 'extra_3.a.b', 'extra_value_3'}, {'=', 9, 'extra_value_7'}}}) + 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 = 'extra_value_3'}}, + extra_4 = box.NULL, + extra_5 = 'extra_value_5', + extra_6 = box.NULL, + extra_7 = 'extra_value_7' + } + }) - 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 = 2, - extra_4 = box.NULL, - extra_5 = 'extra_value_5', - extra_6 = box.NULL, - extra_7 = 'extra_value_7', - extra_8 = box.NULL, - extra_9 = 'extra_value_9', - extra_10 = box.NULL, - extra_11 = box.NULL, - extra_12 = 'extra_value_12' - } - }) + 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 = 'extra_value_7', + extra_8 = box.NULL, + extra_9 = 'extra_value_9', + extra_10 = box.NULL, + extra_11 = box.NULL, + extra_12 = 'extra_value_12' + } + }) + else + result, err = g.cluster.main_server.net_box:call('crud.update', + {'developers', 1, {{'=', 8, 'extra_value_6'}, {'=', 'extra_3', '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 = 'extra_value_3', + extra_4 = box.NULL, + extra_5 = box.NULL, + extra_6 = 'extra_value_6' + } + }) + end end) pgroup:add('test_object_with_nullable_fields', function(g) From f3da2778c181953201b2074d48367011ca9322bb Mon Sep 17 00:00:00 2001 From: Alexey Romanov Date: Wed, 24 Feb 2021 20:08:05 +0300 Subject: [PATCH 13/18] Remove usless if section in upsert function --- crud/update.lua | 4 ++-- crud/upsert.lua | 10 ++++------ 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/crud/update.lua b/crud/update.lua index 12ff61a1..84503243 100644 --- a/crud/update.lua +++ b/crud/update.lua @@ -82,7 +82,7 @@ local function call_update_on_router(space_name, key, user_operations, opts) end local space_format = space:format() - local formatted_operations, err = utils.convert_operations(user_operations, space_format) + 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 end @@ -90,7 +90,7 @@ local function call_update_on_router(space_name, key, user_operations, opts) local bucket_id = sharding.key_get_bucket_id(key, opts.bucket_id) local storage_result, err = call.rw_single( bucket_id, UPDATE_FUNC_NAME, - {space_name, key, formatted_operations, opts.fields}, + {space_name, key, operations, opts.fields}, {timeout = opts.timeout} ) diff --git a/crud/upsert.lua b/crud/upsert.lua index b0b4e8ab..30979837 100644 --- a/crud/upsert.lua +++ b/crud/upsert.lua @@ -61,11 +61,9 @@ local function call_upsert_on_router(space_name, tuple, user_operations, opts) end local space_format = space:format() - if not utils.tarantool_supports_fieldpaths() then - user_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 + 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 end local bucket_id, err = sharding.tuple_set_and_return_bucket_id(tuple, space, opts.bucket_id) @@ -75,7 +73,7 @@ local function call_upsert_on_router(space_name, tuple, user_operations, opts) local storage_result, err = call.rw_single( bucket_id, UPSERT_FUNC_NAME, - {space_name, tuple, user_operations}, + {space_name, tuple, operations}, {timeout = opts.timeout} ) From 15e36582445d201c7b8018db162a2452ffd43196 Mon Sep 17 00:00:00 2001 From: Alexey Romanov Date: Wed, 24 Feb 2021 20:19:24 +0300 Subject: [PATCH 14/18] Changelog --- CHANGELOG.md | 3 +++ crud/common/utils.lua | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8a798113..f4aba304 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 fining 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 c1fd1fe7..9b192a6c 100644 --- a/crud/common/utils.lua +++ b/crud/common/utils.lua @@ -261,7 +261,7 @@ local function convert_jsonpath_operation(operation) if #field_parts > 1 or field_id == nil then -- Checking '[4].a.b' case if field_name:sub(1, 1) == '[' and field_name:sub(-1, -1) == ']' then - local field_id = tonumber(field_name:sub(2, -2)) + field_id = tonumber(field_name:sub(2, -2)) if field_id == nil then -- Checking '["field_name"]' case From 251e2905676c5ea5fcc55fe5ed2f358e64bebd94 Mon Sep 17 00:00:00 2001 From: Alexey Romanov Date: Wed, 24 Feb 2021 20:25:53 +0300 Subject: [PATCH 15/18] Typo fix --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f4aba304..75d45c7e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ### Fixed -* Fixed not fining field in tuple on ``crud.update`` if +* 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 From 86d3b73db1e6a4458e45ab08394d96a7f4b5118e Mon Sep 17 00:00:00 2001 From: Alexey Romanov Date: Thu, 25 Feb 2021 17:47:08 +0300 Subject: [PATCH 16/18] Remove jsonpath support --- crud/common/utils.lua | 46 ++----- crud/update.lua | 3 +- test/integration/simple_operations_test.lua | 127 ++++++++------------ 3 files changed, 59 insertions(+), 117 deletions(-) diff --git a/crud/common/utils.lua b/crud/common/utils.lua index 9b192a6c..91ca563d 100644 --- a/crud/common/utils.lua +++ b/crud/common/utils.lua @@ -253,30 +253,6 @@ function utils.add_intermediate_nullable_fields(operations, space_format, tuple) return map_to_table(operations_map) end -local function convert_jsonpath_operation(operation) - local field_parts = operation[2]:split('.', 1) - local field_name = field_parts[1] - local field_id = tonumber(field_name:sub(2, -2)) - - if #field_parts > 1 or field_id == nil then - -- Checking '[4].a.b' case - if field_name:sub(1, 1) == '[' and field_name:sub(-1, -1) == ']' then - field_id = tonumber(field_name:sub(2, -2)) - - if field_id == nil then - -- Checking '["field_name"]' case - if field_name:sub(2, 2) == '"' and field_name:sub(-2, -2) == '"' then - return field_name:sub(3, -3), nil - end - end - - return nil, tonumber(field_name:sub(2, -2)) - end - end - - return field_name, nil -end - function utils.convert_operations(user_operations, space_format) if utils.tarantool_supports_fieldpaths() then return user_operations @@ -315,22 +291,18 @@ function utils.get_operations_map(user_operations, space_format) for _, operation in ipairs(user_operations) do if type(operation[2]) == 'string' then - local field_name, field_id = convert_jsonpath_operation(operation) - - if field_id == nil then - for fieldno, field_format in ipairs(space_format) do - if field_format.name == field_name then - field_id = fieldno - break - end - end - - if field_id == nil then - return nil, ParseOperationsError:new( - "Space format doesn't contain field named %q", operation[2]) + local field_id + for fieldno, field_format in ipairs(space_format) do + if field_format.name == operation[2] then + field_id = fieldno + break end end + if field_id == nil then + return nil, ParseOperationsError:new( + "Space format doesn't contain field named %q", operation[2]) + end operations_map[field_id] = operation else operations_map[operation[2]] = operation diff --git a/crud/update.lua b/crud/update.lua index 84503243..2e30a6ec 100644 --- a/crud/update.lua +++ b/crud/update.lua @@ -77,11 +77,12 @@ local function call_update_on_router(space_name, key, user_operations, opts) return nil, UpdateError:new("Space %q doesn't exist", space_name), true end + local space_format = space:format() + if box.tuple.is(key) then key = key:totable() end - local space_format = space:format() 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 diff --git a/test/integration/simple_operations_test.lua b/test/integration/simple_operations_test.lua index d42bbf3b..c727c718 100644 --- a/test/integration/simple_operations_test.lua +++ b/test/integration/simple_operations_test.lua @@ -411,7 +411,7 @@ pgroup:add('test_intermediate_nullable_fields_update', function(g) end end) - result, err = g.cluster.main_server.net_box:call('crud.update', + 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) @@ -427,86 +427,55 @@ pgroup:add('test_intermediate_nullable_fields_update', function(g) -- This tests use jsonpath updates. if _TARANTOOL >= "2.3" then - result, err = g.cluster.main_server.net_box:call('crud.update', + 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, 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 = {3}}}, - extra_4 = box.NULL, - extra_5 = 'extra_value_5' - } - }) - - result, err = g.cluster.main_server.net_box:call('crud.update', - {'developers', 1, {{'=', 'extra_3.a.b', 'extra_value_3'}, {'=', 9, 'extra_value_7'}}}) - 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 = 'extra_value_3'}}, - extra_4 = box.NULL, - extra_5 = 'extra_value_5', - extra_6 = box.NULL, - extra_7 = 'extra_value_7' - } - }) - - 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 = 'extra_value_7', - extra_8 = box.NULL, - extra_9 = 'extra_value_9', - extra_10 = box.NULL, - extra_11 = box.NULL, - extra_12 = 'extra_value_12' - } - }) - else - result, err = g.cluster.main_server.net_box:call('crud.update', - {'developers', 1, {{'=', 8, 'extra_value_6'}, {'=', 'extra_3', '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 = 'extra_value_3', - extra_4 = box.NULL, - extra_5 = box.NULL, - extra_6 = 'extra_value_6' - } - }) + 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) From 2094f54127798e8f07a011f3030b5d187e99d152 Mon Sep 17 00:00:00 2001 From: Alexey Romanov Date: Thu, 25 Feb 2021 23:40:19 +0300 Subject: [PATCH 17/18] Change some logic of update array forming --- crud/common/utils.lua | 57 +++++++++++++++++++++++-------------------- crud/update.lua | 15 +++++++----- crud/upsert.lua | 9 ++++--- 3 files changed, 46 insertions(+), 35 deletions(-) diff --git a/crud/common/utils.lua b/crud/common/utils.lua index 91ca563d..d5f09796 100644 --- a/crud/common/utils.lua +++ b/crud/common/utils.lua @@ -199,23 +199,14 @@ function utils.tarantool_supports_uuids() return enabled_tarantool_features.uuids end -local function map_to_table(map) - local tab = {} - for _, value in pairs(map) do - table.insert(tab, value) - end - - return tab -end - -local function add_nullable_fields_recursive(operations, space_format, tuple, id) +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[id - 1] then - operations[id - 1] = {'=', id - 1, box.NULL} - return add_nullable_fields_recursive(operations, space_format, tuple, id - 1) + 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 @@ -232,32 +223,46 @@ else 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 - -- We need this map to check existence of an operation in constant complexity - local operations_map, err = utils.get_operations_map(operations, space_format) - if err ~= nil then - return operations + -- 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 + local formatted_operations, err = utils.convert_operations(operations, space_format) + if err ~= nil then + return operations + end + + operations = formatted_operations end - for key, _ in pairs(operations_map) do - operations_map = add_nullable_fields_recursive( - operations_map, space_format, - tuple, key + -- 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 - return map_to_table(operations_map) + table.sort(operations, function(v1, v2) return v1[2] < v2[2] end) + return operations end function utils.convert_operations(user_operations, space_format) - if utils.tarantool_supports_fieldpaths() then - return user_operations - end - local converted_operations = {} for _, operation in ipairs(user_operations) do diff --git a/crud/update.lua b/crud/update.lua index 2e30a6ec..2c403f29 100644 --- a/crud/update.lua +++ b/crud/update.lua @@ -38,9 +38,9 @@ local function update_on_storage(space_name, key, operations, field_names) end -- We can only add fields to end of the tuple. - -- If schema is updated and intermediate null fields are added, then we will get error. - -- Therefore, we need to add filling of intermediate null fields. - -- More details: https://github.com/tarantool/crud/issues/113 + -- 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}, { @@ -83,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) From cde88ed23a3b5c5e4b0b4baf1a27198cb6b0ee08 Mon Sep 17 00:00:00 2001 From: Alexey Romanov Date: Thu, 25 Feb 2021 23:44:13 +0300 Subject: [PATCH 18/18] Remove useless function --- crud/common/utils.lua | 26 -------------------------- 1 file changed, 26 deletions(-) diff --git a/crud/common/utils.lua b/crud/common/utils.lua index d5f09796..6991b0df 100644 --- a/crud/common/utils.lua +++ b/crud/common/utils.lua @@ -291,32 +291,6 @@ function utils.convert_operations(user_operations, space_format) return converted_operations end -function utils.get_operations_map(user_operations, space_format) - local operations_map = {} - - for _, operation in ipairs(user_operations) do - if type(operation[2]) == 'string' then - local field_id - for fieldno, field_format in ipairs(space_format) do - if field_format.name == operation[2] then - field_id = fieldno - break - end - end - - if field_id == nil then - return nil, ParseOperationsError:new( - "Space format doesn't contain field named %q", operation[2]) - end - operations_map[field_id] = operation - else - operations_map[operation[2]] = operation - end - end - - return operations_map -end - function utils.unflatten_rows(rows, metadata) if metadata == nil then return nil, UnflattenError:new('Metadata is not provided')