Re: [Tarantool-patches] [PATCH] box: disallow to modify format of a view

[Roman Khabibov <roman.habibov@tarantool.org>]

Hi! Thanks for the review.

> On Sep 24, 2020, at 23:59, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
> 
> Hi! Thanks for the patch!
> 
>> diff --git a/src/box/alter.cc b/src/box/alter.cc
>> index ba96d9c62..4d0da347a 100644
>> --- a/src/box/alter.cc
>> +++ b/src/box/alter.cc
>> @@ -2441,6 +2467,20 @@ on_replace_dd_space(struct trigger * /* trigger */, void *event)
>> 				  "view");
>> 			return -1;
>> 		}
>> +		if (def->opts.is_view && old_space->def->field_count !=
>> +		    def->field_count) {
>> +			diag_set(ClientError, ER_ALTER_SPACE,
>> +				 space_name(old_space),
>> +				 "can not modify field count of a view");
>> +			return -1;
>> +		}
>> +		if (def->opts.is_view && format_is_changed(old_tuple,
>> +							   new_tuple)) {
>> +			diag_set(ClientError, ER_ALTER_SPACE,
>> +				 space_name(old_space),
>> +				 "can not modify format of a view");
>> +			return -1;
>> +		}
> 
> Wouldn't it be easier to just prohibit update of such
> tuple completely? Raise an error if new_tuple != NULL and
> old_tuple != NULL and the space is a view.
For some reason, I thought that you can rename views, because some
vendors allow it in ALTER TABLE RENAME TO. But I looked at our
standard file: you really can't rename a view. So that, you are right.

commit 515f2b8315fd266a1e9a13ab1b3357c680786cd5
Author: Roman Khabibov <roman.habibov@tarantool.org>
Date:   Fri Sep 11 09:33:39 2020 +0300

    box: disallow to alter view
    
    Ban ability to modify view on box level. Since a view is a named
    select, and not a space, in fact, altering view is not a valid
    operation.

diff --git a/src/box/alter.cc b/src/box/alter.cc
index ba96d9c62..22578c9c4 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -2398,6 +2398,12 @@ on_replace_dd_space(struct trigger * /* trigger */, void *event)
 		}
 	} else { /* UPDATE, REPLACE */
 		assert(old_space != NULL && new_tuple != NULL);
+		if (old_space->def->opts.is_view) {
+			diag_set(ClientError, ER_ALTER_SPACE,
+				 space_name(old_space),
+				 "view may not be altered");
+			return -1;
+		}
 		struct space_def *def =
 			space_def_new_from_tuple(new_tuple, ER_ALTER_SPACE,
 						 region);
diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c
index 14f6c1a97..54a8d5b80 100644
--- a/src/box/sql/alter.c
+++ b/src/box/sql/alter.c
@@ -60,11 +60,6 @@ sql_alter_table_rename(struct Parse *parse)
 		diag_set(ClientError, ER_NO_SUCH_SPACE, tbl_name);
 		goto tnt_error;
 	}
-	if (space->def->opts.is_view) {
-		diag_set(ClientError, ER_ALTER_SPACE, tbl_name,
-			 "view may not be altered");
-		goto tnt_error;
-	}
 	sql_set_multi_write(parse, false);
 	/* Drop and reload the internal table schema. */
 	struct Vdbe *v = sqlGetVdbe(parse);
diff --git a/test/sql/view.result b/test/sql/view.result
index 1e73ff621..b556b5482 100644
--- a/test/sql/view.result
+++ b/test/sql/view.result
@@ -44,7 +44,7 @@ v1[6]['view'] = false;
 ...
 box.space._space:replace(v1);
 ---
-- error: 'Can''t modify space ''V1'': can not convert a space to a view and vice versa'
+- error: 'Can''t modify space ''V1'': view may not be altered'
 ...
 t1 = box.space._space.index[2]:select('T1')[1]:totable();
 ---
@@ -68,7 +68,7 @@ v1[6]['view'] = true;
 ...
 box.space._space:replace(v1);
 ---
-- error: Space declared as a view must have SQL statement
+- error: 'Can''t modify space ''V1'': view may not be altered'
 ...
 -- Views can't be created via space_create().
 box.schema.create_space('view', {view = true})
@@ -400,3 +400,106 @@ box.execute('DROP TABLE t1;')
 ---
 - row_count: 1
 ...
+--
+-- Make sure that we can't alter a view.
+--
+box.execute("CREATE TABLE t1 (a INT PRIMARY KEY);")
+---
+- row_count: 1
+...
+box.execute("CREATE VIEW v AS SELECT * FROM t1;")
+---
+- row_count: 1
+...
+--
+-- Try to change owner.
+--
+view = box.space._space.index[2]:select('V')[1]:totable()
+---
+...
+view[2] = 1
+---
+...
+box.space._space:replace(view)
+---
+- error: 'Can''t modify space ''V'': view may not be altered'
+...
+--
+-- Try to rename.
+--
+view = box.space._space.index[2]:select('V')[1]:totable()
+---
+...
+view[3] = 'a'
+---
+...
+box.space._space:replace(view)
+---
+- error: 'Can''t modify space ''V'': view may not be altered'
+...
+--
+-- Try to change engine.
+--
+view = box.space._space.index[2]:select('V')[1]:totable()
+---
+...
+view[4] = 'a'
+---
+...
+box.space._space:replace(view)
+---
+- error: 'Can''t modify space ''V'': view may not be altered'
+...
+--
+-- Try to add a field.
+--
+view = box.space._space.index[2]:select('V')[1]:totable()
+---
+...
+view_format = box.space.V:format()
+---
+...
+f = {type = 'string', nullable_action = 'none', name = 'B', is_nullable = true}
+---
+...
+table.insert(view_format, f)
+---
+...
+view[5] = 2
+---
+...
+view[7] = view_format
+---
+...
+box.space._space:replace(view)
+---
+- error: 'Can''t modify space ''V'': view may not be altered'
+...
+--
+-- Try to modify format only.
+--
+view = box.space.V
+---
+...
+view:format{}
+---
+- error: 'Can''t modify space ''V'': view may not be altered'
+...
+view_format = box.space.V:format()
+---
+...
+view_format[1].name = 'B'
+---
+...
+view:format(view_format)
+---
+- error: 'Can''t modify space ''V'': view may not be altered'
+...
+box.execute("DROP VIEW v;")
+---
+- row_count: 1
+...
+box.execute("DROP TABLE t1;")
+---
+- row_count: 1
+...
diff --git a/test/sql/view.test.lua b/test/sql/view.test.lua
index 47ca726af..2500c9e7a 100644
--- a/test/sql/view.test.lua
+++ b/test/sql/view.test.lua
@@ -138,3 +138,54 @@ box.execute('SELECT * FROM t1;')
 box.execute('DROP VIEW v;')
 box.execute('DROP TABLE t;')
 box.execute('DROP TABLE t1;')
+
+--
+-- Make sure that we can't alter a view.
+--
+box.execute("CREATE TABLE t1 (a INT PRIMARY KEY);")
+box.execute("CREATE VIEW v AS SELECT * FROM t1;")
+
+--
+-- Try to change owner.
+--
+view = box.space._space.index[2]:select('V')[1]:totable()
+view[2] = 1
+box.space._space:replace(view)
+
+--
+-- Try to rename.
+--
+view = box.space._space.index[2]:select('V')[1]:totable()
+view[3] = 'a'
+box.space._space:replace(view)
+
+--
+-- Try to change engine.
+--
+view = box.space._space.index[2]:select('V')[1]:totable()
+view[4] = 'a'
+box.space._space:replace(view)
+
+--
+-- Try to add a field.
+--
+view = box.space._space.index[2]:select('V')[1]:totable()
+view_format = box.space.V:format()
+f = {type = 'string', nullable_action = 'none', name = 'B', is_nullable = true}
+table.insert(view_format, f)
+view[5] = 2
+view[7] = view_format
+box.space._space:replace(view)
+
+--
+-- Try to modify format only.
+--
+view = box.space.V
+view:format{}
+
+view_format = box.space.V:format()
+view_format[1].name = 'B'
+view:format(view_format)
+
+box.execute("DROP VIEW v;")
+box.execute("DROP TABLE t1;")