Skip to content

Commit

Permalink
scan: forbid interval conditions
Browse files Browse the repository at this point in the history
As for Tarantool 3.0 and older, datetime intervals are
not comparable [1]. They are also don't supported in indexes. This patch
explicitly forbids to use them in conditions.
1. tarantool/tarantool#7659

Closes #373
  • Loading branch information
DifferentialOrange committed Mar 14, 2024
1 parent 7449ade commit 771904f
Show file tree
Hide file tree
Showing 11 changed files with 103 additions and 14 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.

## Unreleased

### Changed
* Explicitly forbid datetime interval conditions (#373).

### Fixed
* Working with datetime conditions in case of non-indexed fields or
non-iterating indexes (#373).
Expand Down
23 changes: 20 additions & 3 deletions crud/common/utils.lua
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
local bit = require('bit')
local errors = require('errors')
local fiber = require('fiber')
local ffi = require('ffi')
local vshard = require('vshard')
local fun = require('fun')
local bit = require('bit')
local vshard = require('vshard')
local log = require('log')

local datetime_supported, datetime = pcall(require, 'datetime')

local is_cartridge, cartridge = pcall(require, 'cartridge')
local is_cartridge_hotreload, cartridge_hotreload = pcall(require, 'cartridge.hotreload')

Expand All @@ -23,7 +26,6 @@ local NotInitializedError = errors.new_class('NotInitialized')
local StorageInfoError = errors.new_class('StorageInfoError')
local VshardRouterError = errors.new_class('VshardRouterError', {capture_stack = false})
local UtilsInternalError = errors.new_class('UtilsInternalError', {capture_stack = false})
local fiber = require('fiber')

local utils = {}

Expand Down Expand Up @@ -1335,6 +1337,21 @@ function utils.is_cartridge_hotreload_supported()
return true, cartridge_hotreload
end

local interval_supported = datetime_supported and (datetime.interval ~= nil)

if interval_supported then
-- https://github.com/tarantool/tarantool/blob/0510ffa07afd84a70c9c6f1a4c28aacd73a393d6/src/lua/datetime.lua#L175-179
local interval_t = ffi.typeof('struct interval')

utils.is_interval = function(o)
return ffi.istype(interval_t, o)
end
else
utils.is_interval = function()
return false
end
end

for k, v in pairs(require('crud.common.vshard_utils')) do
utils[k] = v
end
Expand Down
8 changes: 7 additions & 1 deletion crud/compare/filters.lua
Original file line number Diff line number Diff line change
Expand Up @@ -167,10 +167,16 @@ local function format_value(value)
return ("%q"):format(value)
elseif datetime_supported and datetime.is_datetime(value) then
return ("%q"):format(value:format())
elseif utils.is_interval(value) then
-- As for Tarantool 3.0 and older, datetime intervals
-- are not comparable. It's better to explicitly forbid them
-- for now.
-- https://github.com/tarantool/tarantool/issues/7659
GenFiltersError:assert(false, ('datetime interval conditions are not supported'))
elseif type(value) == 'cdata' then
return tostring(value)
end
assert(false, ('Unexpected value %s (type %s)'):format(value, type(value)))
GenFiltersError:assert(false, ('Unexpected value %s (type %s)'):format(value, type(value)))
end

local PARSE_ARGS_TEMPLATE = 'local tuple = ...'
Expand Down
28 changes: 27 additions & 1 deletion test/entrypoint/srv_select/storage_init.lua
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
local datetime_supported, _ = pcall(require, 'datetime')
local datetime_supported, datetime = pcall(require, 'datetime')
local decimal_supported, _ = pcall(require, 'decimal')

local crud_utils = require('crud.common.utils')
Expand Down Expand Up @@ -422,4 +422,30 @@ return function()
if_not_exists = true,
})
end

local interval_supported = datetime_supported and (datetime.interval ~= nil)
if interval_supported then
-- Interval is non-indexable.
local interval_space = box.schema.space.create('interval', {
if_not_exists = true,
engine = engine,
})

interval_space:format({
{name = 'id', type = 'unsigned'},
{name = 'bucket_id', type = 'unsigned'},
{name = 'interval_field', type = 'interval'},
})

interval_space:create_index('id_index', {
parts = { 'id' },
if_not_exists = true,
})

interval_space:create_index('bucket_id', {
parts = { 'bucket_id' },
unique = false,
if_not_exists = true,
})
end
end
6 changes: 6 additions & 0 deletions test/helper.lua
Original file line number Diff line number Diff line change
Expand Up @@ -968,6 +968,12 @@ function helpers.skip_datetime_unsupported()
t.skip_if(not module_available, 'datetime is not supported')
end

function helpers.skip_interval_unsupported()
local datetime_supported, datetime = pcall(require, 'datetime')
local interval_supported = datetime_supported and (datetime.interval ~= nil)
t.skip_if(not interval_supported, 'interval is not supported')
end

function helpers.merge_tables(t1, t2, ...)
if t2 == nil then
return t1
Expand Down
8 changes: 3 additions & 5 deletions test/integration/count_test.lua
Original file line number Diff line number Diff line change
Expand Up @@ -873,10 +873,7 @@ local read_impl = function(cg, space, conditions, opts)
opts = table.deepcopy(opts) or {}
opts.mode = 'write'

local resp, err = cg.cluster.main_server:call('crud.count', {space, conditions, opts})
t.assert_equals(err, nil)

return resp
return cg.cluster.main_server:call('crud.count', {space, conditions, opts})
end

pgroup.test_gh_418_count_with_secondary_noneq_index_condition = function(g)
Expand All @@ -885,7 +882,8 @@ end

local gh_373_types_cases = helpers.merge_tables(
read_scenario.gh_373_read_with_decimal_condition_cases,
read_scenario.gh_373_read_with_datetime_condition_cases
read_scenario.gh_373_read_with_datetime_condition_cases,
read_scenario.gh_373_read_with_interval_condition_cases
)

for case_name_template, case in pairs(gh_373_types_cases) do
Expand Down
3 changes: 2 additions & 1 deletion test/integration/pairs_readview_test.lua
Original file line number Diff line number Diff line change
Expand Up @@ -911,7 +911,8 @@ end

local gh_373_types_cases = helpers.merge_tables(
read_scenario.gh_373_read_with_decimal_condition_cases,
read_scenario.gh_373_read_with_datetime_condition_cases
read_scenario.gh_373_read_with_datetime_condition_cases,
read_scenario.gh_373_read_with_interval_condition_cases
)

for case_name_template, case in pairs(gh_373_types_cases) do
Expand Down
3 changes: 2 additions & 1 deletion test/integration/pairs_test.lua
Original file line number Diff line number Diff line change
Expand Up @@ -919,7 +919,8 @@ end

local gh_373_types_cases = helpers.merge_tables(
read_scenario.gh_373_read_with_decimal_condition_cases,
read_scenario.gh_373_read_with_datetime_condition_cases
read_scenario.gh_373_read_with_datetime_condition_cases,
read_scenario.gh_373_read_with_interval_condition_cases
)

for case_name_template, case in pairs(gh_373_types_cases) do
Expand Down
29 changes: 29 additions & 0 deletions test/integration/read_scenario.lua
Original file line number Diff line number Diff line change
Expand Up @@ -548,6 +548,34 @@ for space_kind, space_option in pairs(datetime_condition_space_options) do
end


local gh_373_read_with_interval_condition_cases = {
['gh_373_%s_with_interval_single_condition_is_forbidden'] = function(cg, read)
helpers.skip_interval_unsupported()

local _, err = read(cg,
'interval',
{{'>=', 'interval_field', datetime.interval.new{}}}
)
t.assert_not_equals(err, nil)

local err_msg = err.err or tostring(err)
t.assert_str_contains(err_msg, "datetime interval conditions are not supported")
end,
['gh_373_%s_with_interval_second_condition_is_forbidden'] = function(cg, read)
helpers.skip_interval_unsupported()

local _, err = read(cg,
'interval',
{{'>=', 'id', 1}, {'>=', 'interval_field', datetime.interval.new{}}}
)
t.assert_not_equals(err, nil)

local err_msg = err.err or tostring(err)
t.assert_str_contains(err_msg, "datetime interval conditions are not supported")
end,
}


local function before_merger_process_storage_error(cg)
helpers.call_on_storages(cg.cluster, function(server)
server:exec(function()
Expand Down Expand Up @@ -631,6 +659,7 @@ return {
gh_418_read_with_secondary_noneq_index_condition = gh_418_read_with_secondary_noneq_index_condition,
gh_373_read_with_decimal_condition_cases = gh_373_read_with_decimal_condition_cases,
gh_373_read_with_datetime_condition_cases = gh_373_read_with_datetime_condition_cases,
gh_373_read_with_interval_condition_cases = gh_373_read_with_interval_condition_cases,
before_merger_process_storage_error = before_merger_process_storage_error,
merger_process_storage_error = merger_process_storage_error,
after_merger_process_storage_error = after_merger_process_storage_error,
Expand Down
3 changes: 2 additions & 1 deletion test/integration/select_readview_test.lua
Original file line number Diff line number Diff line change
Expand Up @@ -2509,7 +2509,8 @@ end

local gh_373_types_cases = helpers.merge_tables(
read_scenario.gh_373_read_with_decimal_condition_cases,
read_scenario.gh_373_read_with_datetime_condition_cases
read_scenario.gh_373_read_with_datetime_condition_cases,
read_scenario.gh_373_read_with_interval_condition_cases
)

for case_name_template, case in pairs(gh_373_types_cases) do
Expand Down
3 changes: 2 additions & 1 deletion test/integration/select_test.lua
Original file line number Diff line number Diff line change
Expand Up @@ -2287,7 +2287,8 @@ end

local gh_373_types_cases = helpers.merge_tables(
read_scenario.gh_373_read_with_decimal_condition_cases,
read_scenario.gh_373_read_with_datetime_condition_cases
read_scenario.gh_373_read_with_datetime_condition_cases,
read_scenario.gh_373_read_with_interval_condition_cases
)

for case_name_template, case in pairs(gh_373_types_cases) do
Expand Down

0 comments on commit 771904f

Please sign in to comment.