[tarantool-patches] [PATCH] net.box: fix 'unique' index flag in net.box schema

[tarantool-patches] [PATCH] net.box: fix 'unique' index flag in net.box schema

[Alexander Turenko]

Before this commit it always returns false.

Fixes #4091.
---

https://github.com/tarantool/tarantool/issues/4091
https://github.com/tarantool/tarantool/tree/Totktonada/gh-4091-net.box-indexes-are-always-non-unique

 src/box/lua/net_box.lua                       |  2 +-
 ...box-indexes-are-always-non-unique.test.lua | 30 +++++++++++++++++++
 2 files changed, 31 insertions(+), 1 deletion(-)
 create mode 100755 test/box-tap/gh-4091-net.box-indexes-are-always-non-unique.test.lua

diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
index b3139a3f5..e1c4b652b 100644
--- a/src/box/lua/net_box.lua
+++ b/src/box/lua/net_box.lua
@@ -1253,7 +1253,7 @@ function remote_methods:_install_schema(schema_version, spaces, indices)
                 }
                 idx.parts[k] = pk
             end
-            idx.unique = not not index[OPTS].is_unique
+            idx.unique = not not index[OPTS].unique
         end
 
         if sl[idx.space] ~= nil then
diff --git a/test/box-tap/gh-4091-net.box-indexes-are-always-non-unique.test.lua b/test/box-tap/gh-4091-net.box-indexes-are-always-non-unique.test.lua
new file mode 100755
index 000000000..04c576915
--- /dev/null
+++ b/test/box-tap/gh-4091-net.box-indexes-are-always-non-unique.test.lua
@@ -0,0 +1,30 @@
+#!/usr/bin/env tarantool
+
+local tap = require('tap')
+local net_box = require('net.box')
+local urilib = require('uri')
+
+box.cfg({
+    listen = os.getenv('LISTEN') or 'localhost:3301',
+})
+
+box.schema.user.grant('guest', 'read,write,execute', 'universe')
+
+box.schema.create_space('s')
+box.space.s:create_index('pk')
+box.space.s:create_index('sk', {unique = false})
+
+local test = tap.test('gh-4091-net.box-indexes-are-always-non-unique')
+test:plan(2)
+
+local uri = urilib.parse(box.cfg.listen)
+local uri = ('%s:%s'):format(uri.host or 'localhost', uri.service)
+local conn = net_box.connect(uri)
+
+test:is(conn.space.s.index.pk.unique, true, 'unique index')
+test:is(conn.space.s.index.sk.unique, false, 'non-unique index')
+
+box.space.s:drop()
+box.schema.user.revoke('guest', 'read,write,execute', 'universe')
+
+os.exit(test:check() == true and 0 or 1)
-- 
2.20.1

[tarantool-patches] Re: [PATCH] net.box: fix 'unique' index flag in net.box schema

[Vladislav Shpilevoy]

> diff --git a/test/box-tap/gh-4091-net.box-indexes-are-always-non-unique.test.lua b/test/box-tap/gh-4091-net.box-indexes-are-always-non-unique.test.lua
> new file mode 100755
> index 000000000..04c576915
> --- /dev/null
> +++ b/test/box-tap/gh-4091-net.box-indexes-are-always-non-unique.test.lua

Please, just add a one line to box/net.box.lua. It does not help at all
to have a new file per each issue. This way looks bulky and hard to search
for something.

> @@ -0,0 +1,30 @@
> +#!/usr/bin/env tarantool
> +
> +local tap = require('tap')
> +local net_box = require('net.box')
> +local urilib = require('uri')
> +
> +box.cfg({
> +    listen = os.getenv('LISTEN') or 'localhost:3301',
> +})
> +
> +box.schema.user.grant('guest', 'read,write,execute', 'universe')
> +
> +box.schema.create_space('s')
> +box.space.s:create_index('pk')
> +box.space.s:create_index('sk', {unique = false})
> +
> +local test = tap.test('gh-4091-net.box-indexes-are-always-non-unique')
> +test:plan(2)
> +
> +local uri = urilib.parse(box.cfg.listen)
> +local uri = ('%s:%s'):format(uri.host or 'localhost', uri.service)
> +local conn = net_box.connect(uri)
> +
> +test:is(conn.space.s.index.pk.unique, true, 'unique index')
> +test:is(conn.space.s.index.sk.unique, false, 'non-unique index')
> +
> +box.space.s:drop()
> +box.schema.user.revoke('guest', 'read,write,execute', 'universe')
> +
> +os.exit(test:check() == true and 0 or 1)
> -- 
> 2.20.1
> 

[tarantool-patches] Re: [PATCH] net.box: fix 'unique' index flag in net.box schema

[Alexander Turenko]

On Mon, Apr 01, 2019 at 11:42:03AM +0300, Vladislav Shpilevoy wrote:
> 
> > diff --git a/test/box-tap/gh-4091-net.box-indexes-are-always-non-unique.test.lua b/test/box-tap/gh-4091-net.box-indexes-are-always-non-unique.test.lua
> > new file mode 100755
> > index 000000000..04c576915
> > --- /dev/null
> > +++ b/test/box-tap/gh-4091-net.box-indexes-are-always-non-unique.test.lua
> 
> Please, just add a one line to box/net.box.lua. It does not help at all
> to have a new file per each issue. This way looks bulky and hard to search
> for something.

I disagree. Summarized here:
https://www.freelists.org/post/tarantool-patches/PATCH-sql-make-SQL-BIND-optional-in-an-iproto-request,2

Anyway, if you feel all-in-one tests as the better approach, I don't
want to spent time to discuss it.

Changed. The new patch is below.

> 
> > @@ -0,0 +1,30 @@
> > +#!/usr/bin/env tarantool
> > +
> > +local tap = require('tap')
> > +local net_box = require('net.box')
> > +local urilib = require('uri')
> > +
> > +box.cfg({
> > +    listen = os.getenv('LISTEN') or 'localhost:3301',
> > +})
> > +
> > +box.schema.user.grant('guest', 'read,write,execute', 'universe')
> > +
> > +box.schema.create_space('s')
> > +box.space.s:create_index('pk')
> > +box.space.s:create_index('sk', {unique = false})
> > +
> > +local test = tap.test('gh-4091-net.box-indexes-are-always-non-unique')
> > +test:plan(2)
> > +
> > +local uri = urilib.parse(box.cfg.listen)
> > +local uri = ('%s:%s'):format(uri.host or 'localhost', uri.service)
> > +local conn = net_box.connect(uri)
> > +
> > +test:is(conn.space.s.index.pk.unique, true, 'unique index')
> > +test:is(conn.space.s.index.sk.unique, false, 'non-unique index')
> > +
> > +box.space.s:drop()
> > +box.schema.user.revoke('guest', 'read,write,execute', 'universe')
> > +
> > +os.exit(test:check() == true and 0 or 1)
> > -- 
> > 2.20.1
> > 

----

commit 9a78b65026714d8abfc9246de7e02475c7ce6727
Author: Alexander Turenko <alexander.turenko@tarantool.org>
Date:   Mon Apr 1 07:54:51 2019 +0300

    net.box: fix 'unique' index flag in net.box schema
    
    Before this commit it always returns false.
    
    Fixes #4091.

diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
index b3139a3f5..e1c4b652b 100644
--- a/src/box/lua/net_box.lua
+++ b/src/box/lua/net_box.lua
@@ -1253,7 +1253,7 @@ function remote_methods:_install_schema(schema_version, spaces, indices)
                 }
                 idx.parts[k] = pk
             end
-            idx.unique = not not index[OPTS].is_unique
+            idx.unique = not not index[OPTS].unique
         end
 
         if sl[idx.space] ~= nil then
diff --git a/test/box/net.box.result b/test/box/net.box.result
index aecaf9436..8fc668606 100644
--- a/test/box/net.box.result
+++ b/test/box/net.box.result
@@ -3526,3 +3526,35 @@ s:drop()
 box.cfg{readahead = readahead}
 ---
 ...
+--
+-- gh-4091: index unique flag is always false
+--
+s = box.schema.space.create("test")
+---
+...
+_ = s:create_index("pk")
+---
+...
+_ = s:create_index("sk", {unique = false})
+---
+...
+box.schema.user.grant("guest", "read", "space", "test")
+---
+...
+c = net.connect(box.cfg.listen)
+---
+...
+c.space.test.index.pk.unique -- true
+---
+- true
+...
+c.space.test.index.sk.unique -- false
+---
+- false
+...
+box.schema.user.revoke("guest", "read", "space", "test")
+---
+...
+s:drop()
+---
+...
diff --git a/test/box/net.box.test.lua b/test/box/net.box.test.lua
index 04d6c1903..b81bed239 100644
--- a/test/box/net.box.test.lua
+++ b/test/box/net.box.test.lua
@@ -1435,3 +1435,16 @@ test_run:wait_log('default', 'readahead limit is reached', 1024, 0.1)
 
 s:drop()
 box.cfg{readahead = readahead}
+
+--
+-- gh-4091: index unique flag is always false
+--
+s = box.schema.space.create("test")
+_ = s:create_index("pk")
+_ = s:create_index("sk", {unique = false})
+box.schema.user.grant("guest", "read", "space", "test")
+c = net.connect(box.cfg.listen)
+c.space.test.index.pk.unique -- true
+c.space.test.index.sk.unique -- false
+box.schema.user.revoke("guest", "read", "space", "test")
+s:drop()

[tarantool-patches] Re: [PATCH] net.box: fix 'unique' index flag in net.box schema

[Vladislav Shpilevoy]

On 01/04/2019 13:08, Alexander Turenko wrote:
> On Mon, Apr 01, 2019 at 11:42:03AM +0300, Vladislav Shpilevoy wrote:
>>
>>> diff --git a/test/box-tap/gh-4091-net.box-indexes-are-always-non-unique.test.lua b/test/box-tap/gh-4091-net.box-indexes-are-always-non-unique.test.lua
>>> new file mode 100755
>>> index 000000000..04c576915
>>> --- /dev/null
>>> +++ b/test/box-tap/gh-4091-net.box-indexes-are-always-non-unique.test.lua
>>
>> Please, just add a one line to box/net.box.lua. It does not help at all
>> to have a new file per each issue. This way looks bulky and hard to search
>> for something.
> 
> I disagree. Summarized here:
> https://www.freelists.org/post/tarantool-patches/PATCH-sql-make-SQL-BIND-optional-in-an-iproto-request,2

I had read that, and it was discussed so many times that I do not
see a sense of doing that again. Someone should just prohibit one
of the ways, and standardize the other. Until it is done, any reviewer
can stick to his own favorite way, and mine is to do not clog test
directories with hundreds and thousands of files.

> 
> Anyway, if you feel all-in-one tests as the better approach, I don't
> want to spent time to discuss it.
> 
> Changed. The new patch is below.

It is changed, but in fact it is still the same. When you have one file,
you can reuse some of existing places to make new tests shorter and
simpler. Strictly speaking for such a bug I wouldn't add a test at all.
It is rather an old typo than a real bug which could return in future.

My diff is below. The same diff is in a separate commit on top of your
branch.

=======================================================================

diff --git a/test/box/net.box.result b/test/box/net.box.result
index 8fc668606..c23ab7a3a 100644
--- a/test/box/net.box.result
+++ b/test/box/net.box.result
@@ -2001,6 +2001,13 @@ c.space.test:format({})
 ---
 - error: net.box does not support setting space format
 ...
+--
+-- gh-4091: index unique flag is always false.
+--
+c.space.test.index.primary.unique
+---
+- true
+...
 c:close()
 ---
 ...
@@ -3526,35 +3533,3 @@ s:drop()
 box.cfg{readahead = readahead}
 ---
 ...
---
--- gh-4091: index unique flag is always false
---
-s = box.schema.space.create("test")
----
-...
-_ = s:create_index("pk")
----
-...
-_ = s:create_index("sk", {unique = false})
----
-...
-box.schema.user.grant("guest", "read", "space", "test")
----
-...
-c = net.connect(box.cfg.listen)
----
-...
-c.space.test.index.pk.unique -- true
----
-- true
-...
-c.space.test.index.sk.unique -- false
----
-- false
-...
-box.schema.user.revoke("guest", "read", "space", "test")
----
-...
-s:drop()
----
-...
diff --git a/test/box/net.box.test.lua b/test/box/net.box.test.lua
index b81bed239..556d48420 100644
--- a/test/box/net.box.test.lua
+++ b/test/box/net.box.test.lua
@@ -814,6 +814,11 @@ format[1].type == "unsigned"
 
 c.space.test:format({})
 
+--
+-- gh-4091: index unique flag is always false.
+--
+c.space.test.index.primary.unique
+
 c:close()
 space:drop()
 
@@ -1435,16 +1440,3 @@ test_run:wait_log('default', 'readahead limit is reached', 1024, 0.1)
 
 s:drop()
 box.cfg{readahead = readahead}
-
---
--- gh-4091: index unique flag is always false
---
-s = box.schema.space.create("test")
-_ = s:create_index("pk")
-_ = s:create_index("sk", {unique = false})
-box.schema.user.grant("guest", "read", "space", "test")
-c = net.connect(box.cfg.listen)
-c.space.test.index.pk.unique -- true
-c.space.test.index.sk.unique -- false
-box.schema.user.revoke("guest", "read", "space", "test")
-s:drop()

[tarantool-patches] Re: [PATCH] net.box: fix 'unique' index flag in net.box schema

[Alexander Turenko]

Applied your diff.

I still think this is the mess, sorry.

WBR, Alexander Turenko.

On Mon, Apr 01, 2019 at 06:32:44PM +0300, Vladislav Shpilevoy wrote:
> 
> 
> On 01/04/2019 13:08, Alexander Turenko wrote:
> > On Mon, Apr 01, 2019 at 11:42:03AM +0300, Vladislav Shpilevoy wrote:
> >>
> >>> diff --git a/test/box-tap/gh-4091-net.box-indexes-are-always-non-unique.test.lua b/test/box-tap/gh-4091-net.box-indexes-are-always-non-unique.test.lua
> >>> new file mode 100755
> >>> index 000000000..04c576915
> >>> --- /dev/null
> >>> +++ b/test/box-tap/gh-4091-net.box-indexes-are-always-non-unique.test.lua
> >>
> >> Please, just add a one line to box/net.box.lua. It does not help at all
> >> to have a new file per each issue. This way looks bulky and hard to search
> >> for something.
> > 
> > I disagree. Summarized here:
> > https://www.freelists.org/post/tarantool-patches/PATCH-sql-make-SQL-BIND-optional-in-an-iproto-request,2
> 
> I had read that, and it was discussed so many times that I do not
> see a sense of doing that again. Someone should just prohibit one
> of the ways, and standardize the other. Until it is done, any reviewer
> can stick to his own favorite way, and mine is to do not clog test
> directories with hundreds and thousands of files.
> 
> > 
> > Anyway, if you feel all-in-one tests as the better approach, I don't
> > want to spent time to discuss it.
> > 
> > Changed. The new patch is below.
> 
> It is changed, but in fact it is still the same. When you have one file,
> you can reuse some of existing places to make new tests shorter and
> simpler. Strictly speaking for such a bug I wouldn't add a test at all.
> It is rather an old typo than a real bug which could return in future.
> 
> My diff is below. The same diff is in a separate commit on top of your
> branch.
> 
> =======================================================================
> 
> diff --git a/test/box/net.box.result b/test/box/net.box.result
> index 8fc668606..c23ab7a3a 100644
> --- a/test/box/net.box.result
> +++ b/test/box/net.box.result
> @@ -2001,6 +2001,13 @@ c.space.test:format({})
>  ---
>  - error: net.box does not support setting space format
>  ...
> +--
> +-- gh-4091: index unique flag is always false.
> +--
> +c.space.test.index.primary.unique
> +---
> +- true
> +...
>  c:close()
>  ---
>  ...
> @@ -3526,35 +3533,3 @@ s:drop()
>  box.cfg{readahead = readahead}
>  ---
>  ...
> ---
> --- gh-4091: index unique flag is always false
> ---
> -s = box.schema.space.create("test")
> ----
> -...
> -_ = s:create_index("pk")
> ----
> -...
> -_ = s:create_index("sk", {unique = false})
> ----
> -...
> -box.schema.user.grant("guest", "read", "space", "test")
> ----
> -...
> -c = net.connect(box.cfg.listen)
> ----
> -...
> -c.space.test.index.pk.unique -- true
> ----
> -- true
> -...
> -c.space.test.index.sk.unique -- false
> ----
> -- false
> -...
> -box.schema.user.revoke("guest", "read", "space", "test")
> ----
> -...
> -s:drop()
> ----
> -...
> diff --git a/test/box/net.box.test.lua b/test/box/net.box.test.lua
> index b81bed239..556d48420 100644
> --- a/test/box/net.box.test.lua
> +++ b/test/box/net.box.test.lua
> @@ -814,6 +814,11 @@ format[1].type == "unsigned"
>  
>  c.space.test:format({})
>  
> +--
> +-- gh-4091: index unique flag is always false.
> +--
> +c.space.test.index.primary.unique
> +
>  c:close()
>  space:drop()
>  
> @@ -1435,16 +1440,3 @@ test_run:wait_log('default', 'readahead limit is reached', 1024, 0.1)
>  
>  s:drop()
>  box.cfg{readahead = readahead}
> -
> ---
> --- gh-4091: index unique flag is always false
> ---
> -s = box.schema.space.create("test")
> -_ = s:create_index("pk")
> -_ = s:create_index("sk", {unique = false})
> -box.schema.user.grant("guest", "read", "space", "test")
> -c = net.connect(box.cfg.listen)
> -c.space.test.index.pk.unique -- true
> -c.space.test.index.sk.unique -- false
> -box.schema.user.revoke("guest", "read", "space", "test")
> -s:drop()

[tarantool-patches] Re: [PATCH] net.box: fix 'unique' index flag in net.box schema

[Vladislav Shpilevoy]

LGTM.

On 01/04/2019 19:15, Alexander Turenko wrote:
> Applied your diff.
> 
> I still think this is the mess, sorry.
> 
> WBR, Alexander Turenko.
> 
> On Mon, Apr 01, 2019 at 06:32:44PM +0300, Vladislav Shpilevoy wrote:
>>
>>
>> On 01/04/2019 13:08, Alexander Turenko wrote:
>>> On Mon, Apr 01, 2019 at 11:42:03AM +0300, Vladislav Shpilevoy wrote:
>>>>
>>>>> diff --git a/test/box-tap/gh-4091-net.box-indexes-are-always-non-unique.test.lua b/test/box-tap/gh-4091-net.box-indexes-are-always-non-unique.test.lua
>>>>> new file mode 100755
>>>>> index 000000000..04c576915
>>>>> --- /dev/null
>>>>> +++ b/test/box-tap/gh-4091-net.box-indexes-are-always-non-unique.test.lua
>>>>
>>>> Please, just add a one line to box/net.box.lua. It does not help at all
>>>> to have a new file per each issue. This way looks bulky and hard to search
>>>> for something.
>>>
>>> I disagree. Summarized here:
>>> https://www.freelists.org/post/tarantool-patches/PATCH-sql-make-SQL-BIND-optional-in-an-iproto-request,2
>>
>> I had read that, and it was discussed so many times that I do not
>> see a sense of doing that again. Someone should just prohibit one
>> of the ways, and standardize the other. Until it is done, any reviewer
>> can stick to his own favorite way, and mine is to do not clog test
>> directories with hundreds and thousands of files.
>>
>>>
>>> Anyway, if you feel all-in-one tests as the better approach, I don't
>>> want to spent time to discuss it.
>>>
>>> Changed. The new patch is below.
>>
>> It is changed, but in fact it is still the same. When you have one file,
>> you can reuse some of existing places to make new tests shorter and
>> simpler. Strictly speaking for such a bug I wouldn't add a test at all.
>> It is rather an old typo than a real bug which could return in future.
>>
>> My diff is below. The same diff is in a separate commit on top of your
>> branch.
>>
>> =======================================================================
>>
>> diff --git a/test/box/net.box.result b/test/box/net.box.result
>> index 8fc668606..c23ab7a3a 100644
>> --- a/test/box/net.box.result
>> +++ b/test/box/net.box.result
>> @@ -2001,6 +2001,13 @@ c.space.test:format({})
>>  ---
>>  - error: net.box does not support setting space format
>>  ...
>> +--
>> +-- gh-4091: index unique flag is always false.
>> +--
>> +c.space.test.index.primary.unique
>> +---
>> +- true
>> +...
>>  c:close()
>>  ---
>>  ...
>> @@ -3526,35 +3533,3 @@ s:drop()
>>  box.cfg{readahead = readahead}
>>  ---
>>  ...
>> ---
>> --- gh-4091: index unique flag is always false
>> ---
>> -s = box.schema.space.create("test")
>> ----
>> -...
>> -_ = s:create_index("pk")
>> ----
>> -...
>> -_ = s:create_index("sk", {unique = false})
>> ----
>> -...
>> -box.schema.user.grant("guest", "read", "space", "test")
>> ----
>> -...
>> -c = net.connect(box.cfg.listen)
>> ----
>> -...
>> -c.space.test.index.pk.unique -- true
>> ----
>> -- true
>> -...
>> -c.space.test.index.sk.unique -- false
>> ----
>> -- false
>> -...
>> -box.schema.user.revoke("guest", "read", "space", "test")
>> ----
>> -...
>> -s:drop()
>> ----
>> -...
>> diff --git a/test/box/net.box.test.lua b/test/box/net.box.test.lua
>> index b81bed239..556d48420 100644
>> --- a/test/box/net.box.test.lua
>> +++ b/test/box/net.box.test.lua
>> @@ -814,6 +814,11 @@ format[1].type == "unsigned"
>>  
>>  c.space.test:format({})
>>  
>> +--
>> +-- gh-4091: index unique flag is always false.
>> +--
>> +c.space.test.index.primary.unique
>> +
>>  c:close()
>>  space:drop()
>>  
>> @@ -1435,16 +1440,3 @@ test_run:wait_log('default', 'readahead limit is reached', 1024, 0.1)
>>  
>>  s:drop()
>>  box.cfg{readahead = readahead}
>> -
>> ---
>> --- gh-4091: index unique flag is always false
>> ---
>> -s = box.schema.space.create("test")
>> -_ = s:create_index("pk")
>> -_ = s:create_index("sk", {unique = false})
>> -box.schema.user.grant("guest", "read", "space", "test")
>> -c = net.connect(box.cfg.listen)
>> -c.space.test.index.pk.unique -- true
>> -c.space.test.index.sk.unique -- false
>> -box.schema.user.revoke("guest", "read", "space", "test")
>> -s:drop()

[tarantool-patches] Re: [PATCH] net.box: fix 'unique' index flag in net.box schema

[Kirill Yukhin]

Hello,

On 01 апр 08:08, Alexander Turenko wrote:
> Before this commit it always returns false.
> 
> Fixes #4091.
> ---
> 
> https://github.com/tarantool/tarantool/issues/4091
> https://github.com/tarantool/tarantool/tree/Totktonada/gh-4091-net.box-indexes-are-always-non-unique

I've checked your patch into master & 2.1 branch.

--
Regards, Kirill Yukhin