Re: [Tarantool-patches] [PATCH v2 1/2] uuid: support comparison of uuid values in Lua

[Oleg Babin <olegrok@tarantool.org>]

Hi! Thanks for comments!

On 24/11/2020 00:58, Vladislav Shpilevoy wrote:
> Hi! Thanks for the fixes!
>
>> diff --git a/src/lua/uuid.lua b/src/lua/uuid.lua
>> index 08991cfeb..275f4e5c5 100644
>> --- a/src/lua/uuid.lua
>> +++ b/src/lua/uuid.lua
>> @@ -93,13 +93,20 @@ local uuid_isnil = function(uu)
>>       return builtin.tt_uuid_is_nil(uu)
>>   end
>>
>> +local check_uuid = function(value, index)
>> +    if is_uuid(value) then
>> +        return value
>> +    end
>> +
>> +    local err_fmt = 'incorrect value to compare with uuid as %d argument'
>> +    error(err_fmt:format(index), 4)
>> +end
>> +
>>   local uuid_eq = function(lhs, rhs)
>>       if not is_uuid(rhs) then
>>           return false
>>       end
>> -    if not is_uuid(lhs) then
>> -        return error('Usage: uuid == var')
>> -    end
>> +    lhs = check_uuid(lhs, 1)
> Check_uuid() here uses incorrect error index. check_uuid()
> raises error(..., 4). So check_uuid() itself is level 1.
> uuid_eq() is level 2. And the calling code is level 3. You
> will raise one level beyond the caller code.
>
> For uuid_lt and uuid_le it works, because check_uuid() is
> level 1, uuid_cmp() is level 2, uuid_lt/le() is level 3, and
> the calling code is level 4.


Good catch! Thanks. Fixed in the following way:

diff --git a/src/lua/uuid.lua b/src/lua/uuid.lua
index 275f4e5c5..1171dfcc4 100644
--- a/src/lua/uuid.lua
+++ b/src/lua/uuid.lua
@@ -93,20 +93,20 @@ local uuid_isnil = function(uu)
      return builtin.tt_uuid_is_nil(uu)
  end

-local check_uuid = function(value, index)
+local check_uuid = function(value, index, err_lvl)
      if is_uuid(value) then
          return value
      end

      local err_fmt = 'incorrect value to compare with uuid as %d argument'
-    error(err_fmt:format(index), 4)
+    error(err_fmt:format(index), err_lvl)
  end

  local uuid_eq = function(lhs, rhs)
      if not is_uuid(rhs) then
          return false
      end
-    lhs = check_uuid(lhs, 1)
+    lhs = check_uuid(lhs, 1, 3)
      return builtin.tt_uuid_is_equal(lhs, rhs)
  end

@@ -128,8 +128,8 @@ local uuid_new_str = function()
  end

  local uuid_cmp = function(lhs, rhs)
-    lhs = check_uuid(lhs, 1)
-    rhs = check_uuid(rhs, 2)
+    lhs = check_uuid(lhs, 1, 4)
+    rhs = check_uuid(rhs, 2, 4)
      return builtin.tt_uuid_compare(lhs, rhs)
  end
  local uuid_lt = function(lhs, rhs)

> But I can't give you a test, since I have no idea how to provide
> lhs with non-uuid type. Igor said we can give metatable of uuid to
> some non-uuid value, but I failed to assign uuid metatable to
> anything else.

It's possible but I don't think it's really needed. I put small snippet 
below:

uuid = require('uuid')
eq = debug.getmetatable(uuid()).__eq
eq('', uuid()) -- error: incorrect value to compare with uuid as 1 argument
proxy = newproxy()
debug.setmetatable(proxy, {__eq = eq})
proxy == uuid() -- error: '[string "return proxy == uuid()"]:1: 
incorrect value to compare with uuid as 1 argument'

I can add it to our test suite but it looks really strange

> After you fix this (but I don't insist on adding a test, because
> I don't know how), I suggest you to get a second review from Igor.

Ok. I add Igor to CC.

Igor, could you provide the second review?