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

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

[Roman Khabibov]

Ban ability to modify format or field count of a view. Since a
view is a named select, and not a space, in fact, changing view
format is not a valid operation.
---

Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/ban-view-mod

@ChangeLog
- Ban ability to modify view's format and field count.

 src/box/alter.cc                          | 40 +++++++++++++
 test/box/view-dont-modify-format.result   | 69 +++++++++++++++++++++++
 test/box/view-dont-modify-format.test.lua | 29 ++++++++++
 3 files changed, 138 insertions(+)
 create mode 100644 test/box/view-dont-modify-format.result
 create mode 100644 test/box/view-dont-modify-format.test.lua

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
@@ -2086,6 +2086,32 @@ update_view_references(struct Select *select, int update_value,
 	return 0;
 }
 
+/**
+ * Check whether space format is changed during UPDATE/REPLACE.
+ *
+ * Used to disallow view's format modification.
+ *
+ * @param old_tuple Tuple of a space before UPDATE/REPLACE.
+ * @param new_tuple Tuple of a space after UPDATE/REPLACE.
+ */
+static bool
+format_is_changed(struct tuple *old_tuple, struct tuple *new_tuple)
+{
+	const char *old_format = tuple_field(old_tuple, BOX_SPACE_FIELD_FORMAT);
+	const char *new_format = tuple_field(new_tuple, BOX_SPACE_FIELD_FORMAT);
+	assert(old_format != NULL && new_format != NULL);
+	const char *end = old_format;
+	mp_next(&end);
+	uint32_t length = end - old_format;
+	end = new_format;
+	mp_next(&end);
+	if (length != end - new_format)
+		return true;
+	if (memcmp(old_format, new_format, length) != 0)
+		return true;
+	return false;
+}
+
 /**
  * Trigger which is fired to commit creation of new SQL view.
  * Its purpose is to release memory of SELECT.
@@ -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;
+		}
 		/*
 		 * Allow change of space properties, but do it
 		 * in WAL-error-safe mode.
diff --git a/test/box/view-dont-modify-format.result b/test/box/view-dont-modify-format.result
new file mode 100644
index 000000000..0d8c50601
--- /dev/null
+++ b/test/box/view-dont-modify-format.result
@@ -0,0 +1,69 @@
+-- test-run result file version 2
+--
+-- Make sure we can't modify a view format.
+--
+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 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'': can not modify field count of a view'
+ | ...
+
+--
+-- Try to modify format only.
+--
+view = box.space.V
+ | ---
+ | ...
+view:format{}
+ | ---
+ | - error: 'Can''t modify space ''V'': can not modify field count of a view'
+ | ...
+
+view_format = box.space.V:format()
+ | ---
+ | ...
+view_format[1].name = 'B'
+ | ---
+ | ...
+view:format(view_format)
+ | ---
+ | - error: 'Can''t modify space ''V'': can not modify format of a view'
+ | ...
+
+box.execute("DROP VIEW v;")
+ | ---
+ | - row_count: 1
+ | ...
+box.execute("DROP TABLE t1;")
+ | ---
+ | - row_count: 1
+ | ...
diff --git a/test/box/view-dont-modify-format.test.lua b/test/box/view-dont-modify-format.test.lua
new file mode 100644
index 000000000..e553e7012
--- /dev/null
+++ b/test/box/view-dont-modify-format.test.lua
@@ -0,0 +1,29 @@
+--
+-- Make sure we can't modify a view format.
+--
+box.execute("CREATE TABLE t1 (a INT PRIMARY KEY);")
+box.execute("CREATE VIEW v AS SELECT * FROM t1;")
+
+--
+-- 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;")
-- 
2.24.3 (Apple Git-128)

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

[Vladislav Shpilevoy]

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.

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

[Roman Khabibov]

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;")

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

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

LGTM.

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

[Roman Khabibov]

Hi! Thanks.

Nikita, could you, please, look through?

> On Sep 25, 2020, at 23:14, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
> 
> Hi! Thanks for the patch!
> 
> LGTM.

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

[Nikita Pettik]

On 26 Sep 16:01, Roman Khabibov wrote:
> Hi! Thanks.
> 
> Nikita, could you, please, look through?

Changed error msg to 'view can not be altered' and pushed to master.
Changelog for 2.6 is updated; branch is dropped.
 
> > On Sep 25, 2020, at 23:14, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
> > 
> > Hi! Thanks for the patch!
> > 
> > LGTM.
> 

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

[Vladislav Shpilevoy]

It seems this is a bugfix and should be pushed to the older version too.

On 28.09.2020 17:52, Nikita Pettik wrote:
> On 26 Sep 16:01, Roman Khabibov wrote:
>> Hi! Thanks.
>>
>> Nikita, could you, please, look through?
> 
> Changed error msg to 'view can not be altered' and pushed to master.
> Changelog for 2.6 is updated; branch is dropped.
>  
>>> On Sep 25, 2020, at 23:14, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
>>>
>>> Hi! Thanks for the patch!
>>>
>>> LGTM.
>>

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

[Nikita Pettik]

On 28 Sep 20:41, Vladislav Shpilevoy wrote:
> It seems this is a bugfix and should be pushed to the older version too.

I consider this change to be very minor and don't think it is worth
backporting. If you really feel that it must be backported - feel free to do it.
 
> On 28.09.2020 17:52, Nikita Pettik wrote:
> > On 26 Sep 16:01, Roman Khabibov wrote:
> >> Hi! Thanks.
> >>
> >> Nikita, could you, please, look through?
> > 
> > Changed error msg to 'view can not be altered' and pushed to master.
> > Changelog for 2.6 is updated; branch is dropped.
> >  
> >>> On Sep 25, 2020, at 23:14, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
> >>>
> >>> Hi! Thanks for the patch!
> >>>
> >>> LGTM.
> >>

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

[Vladislav Shpilevoy]

On 29.09.2020 01:50, Nikita Pettik wrote:
> On 28 Sep 20:41, Vladislav Shpilevoy wrote:
>> It seems this is a bugfix and should be pushed to the older version too.
> 
> I consider this change to be very minor and don't think it is worth
> backporting. If you really feel that it must be backported - feel free to do it.

Я не догоняю че-то. У нас есть четкая политика - все багфиксы
пропушиваются на все поддерживаемые версии, где эти баги есть. Тут
нет правила, что ну баг вроде мелкий, можно забить. Нет никакой
двузначности. Тут даже конфликтов нет, это тупо черри-пик.

Я пропушил в 2.5 и 2.4, обновил changelog.

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

[Nikita Pettik]

On 02 Oct 00:35, Vladislav Shpilevoy wrote:
> On 29.09.2020 01:50, Nikita Pettik wrote:
> > On 28 Sep 20:41, Vladislav Shpilevoy wrote:
> >> It seems this is a bugfix and should be pushed to the older version too.
> > 
> > I consider this change to be very minor and don't think it is worth
> > backporting. If you really feel that it must be backported - feel free to do it.
> 
> Я не догоняю че-то. У нас есть четкая политика - все багфиксы
> пропушиваются на все поддерживаемые версии, где эти баги есть. Тут
> нет правила, что ну баг вроде мелкий, можно забить. Нет никакой
> двузначности. Тут даже конфликтов нет, это тупо черри-пик.
> 
> Я пропушил в 2.5 и 2.4, обновил changelog.

Влад, отстрелить себе ногу (руку, голову, палец соседа) в СКЛ можно 1001
способом, но это не значит, что на каждый случай стоит того, чтобы писать
очередную затычку (по моему мнению) и заносить ее во все ветки.
Но спасибо, что сделал это.