Skip to content

Conversation

not4x217
Copy link
Contributor

Currently for all queue implementations driver.new() does not validate queue space, created by driver.create_space() method. However corrupted space can lead to unexpected behavior (for example, see #134),

This one adds check to ensure, that indexes, created by driver.create_space(), really exist.

for all queue drivers new method() now checks that indexes,
whihc are crate by create_space() method really exist
Copy link
Contributor

@LeonidVas LeonidVas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! Thanks for the patch!
May be add the validation to create_space (case if_not_exists == true)
It's looks like a fix for #62

end

-- validate space of queue
local function validate_space(space)
Copy link
Contributor

@LeonidVas LeonidVas Feb 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, WET practice can be used here.
But I suggest using something like:

local function validate_space(space, indexes)
	local indexes = {'task_id', 'status'}
	for _, index in pairs(indexes) do
		if space.index[index] == nil then
			error('space does not have ' .. index .. ' index')
		end
	end

test:test('task_id index does not exist', function(test)
test:plan(2)

space.index.task_id = nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not proper use of the index lua api and it is not guaranteed to work in a future. OTOH, I don't see anything that would block usage of <index_object>:drop() and then re-creation of the space for a next test.

@Totktonada
Copy link
Contributor

@olmiik I want to kindly remind you about this PR. There are a couple of small comments. It would be good to resolve them and finally fix the problem for users.

@LeonidVas
Copy link
Contributor

Continued in PR #136

@LeonidVas LeonidVas closed this Oct 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants