Update function now adds intermediate nullable fields (if they needed) (#121)

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. Closes #113

Author: mRrvz

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

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

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)

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)

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

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(