Skip to content
Snippets Groups Projects
Commit 9d403f00 authored by Georges Racinet's avatar Georges Racinet
Browse files

Missing repository argument: proper error codes.

As it was uncovered by experimental implementation in RHGitaly of
`RepositoryExists`, it's possible that the `repository` argument
is entirely missing. In that case, we are now raising `ValueError`
internally, and convert it to the gRPC code
`HGitalyServicer.STATUS_CODE_STORAGE_NOT_FOUND`.
parent 215369c1
No related branches found
No related tags found
1 merge request!141Better error compliance and comparison tests refactor
......@@ -169,6 +169,8 @@
f'GetStorageByName: no such storage: "{exc.args[1]}"'
)
exists = False
except ValueError as exc:
context.abort(StatusCode.INVALID_ARGUMENT, exc.args[0])
return RepositoryExistsResponse(exists=exists)
......@@ -373,11 +375,9 @@
try:
repo_path = self.repo_disk_path(request.repository, context)
except KeyError as exc:
if exc.args[0] == 'storage':
context.abort(StatusCode.INVALID_ARGUMENT,
"Unkbown storage %r" % exc.args[1])
raise # pragma no cover (not triggerable at this point)
self.handle_key_error(context, exc.args)
except ValueError as exc:
self.handle_value_error(context, exc.args)
if not os.path.exists(repo_path):
# same error message as Gitaly (probably no need to repeat
......
......@@ -259,6 +259,12 @@
fixture.exists()
assert exc_info.value.code() == grpc.StatusCode.INVALID_ARGUMENT
# missing repo argument
fixture.grpc_repo = None
with pytest.raises(grpc.RpcError) as exc_info:
fixture.exists()
assert exc_info.value.code() == grpc.StatusCode.INVALID_ARGUMENT
def test_has_local_branches(fixture_with_repo):
fixture = fixture_with_repo
......@@ -273,6 +279,12 @@
assert not fixture.has_local_branches()
# missing repo argument
fixture.grpc_repo = None
with pytest.raises(grpc.RpcError) as exc_info:
fixture.has_local_branches()
assert exc_info.value.code() == grpc.StatusCode.INVALID_ARGUMENT
def test_write_ref(fixture_with_repo):
fixture = fixture_with_repo
......@@ -561,6 +573,12 @@
relative_path='no/such/path'))
assert exc_info.value.code() == grpc.StatusCode.NOT_FOUND
# missing repo argument
fixture.grpc_repo = None
with pytest.raises(grpc.RpcError) as exc_info:
fixture.remove_repository(grpc_repo=None)
assert exc_info.value.code() == grpc.StatusCode.INVALID_ARGUMENT
def test_set_full_path(fixture_with_repo):
fixture = fixture_with_repo
......
......@@ -62,6 +62,7 @@
STATUS_CODE_STORAGE_NOT_FOUND = StatusCode.NOT_FOUND
STATUS_CODE_REPO_NOT_FOUND = StatusCode.NOT_FOUND
STATUS_CODE_MISSING_REPO_ARG = StatusCode.INVALID_ARGUMENT
def __init__(self, storages):
self.storages = storages
......@@ -111,6 +112,11 @@
return self.load_repo_inner(repository, context)
except KeyError as exc:
self.handle_key_error(context, exc.args)
except ValueError as exc:
self.handle_value_error(context, exc.args)
def handle_value_error(self, context, exc_args):
context.abort(self.STATUS_CODE_MISSING_REPO_ARG, exc_args[0])
def handle_key_error(self, context, exc_args):
ktype = exc_args[0]
......@@ -165,6 +171,11 @@
If the storage is unknown, this raises
``KeyError('storage', storage_name)``
"""
if not storage_name:
# this is the best detection of a missing `repository` field
# in request, without having the request object itself
raise ValueError('empty repository')
root_dir = self.storages.get(storage_name)
if root_dir is None:
raise KeyError('storage', storage_name)
......@@ -197,6 +208,8 @@
return self.temp_dir_inner(storage_name, context, ensure=ensure)
except KeyError as exc:
self.handle_key_error(context, exc.args)
except ValueError as exc:
self.handle_value_error(context, exc.args)
except OSError as exc:
context.abort(StatusCode.INTERNAL,
"Error ensuring temporary dir: %s" % exc)
......
......@@ -164,6 +164,12 @@
servicer.temp_dir('unknown', context)
assert context.code == grpc.StatusCode.NOT_FOUND
# this is how it looks with a missing repository argument in the
# gRPC request, and temp_dir(request.repository.storage_name)
with pytest.raises(AbortContext) as exc_info:
servicer.temp_dir('', context)
assert context.code == grpc.StatusCode.INVALID_ARGUMENT
def test_temp_dir_failed_creation(tmpdir):
# context is used for error raising only
......
......@@ -188,6 +188,59 @@
relative_path='no/such/path'))
def test_repository_exists(gitaly_comparison, server_repos_root):
fixture = gitaly_comparison
grpc_repo = fixture.gitaly_repo
rpc_helper = fixture.rpc_helper(
stub_cls=RepositoryServiceStub,
method_name='RepositoryExists',
request_cls=RemoveRepositoryRequest,
)
assert_compare = rpc_helper.assert_compare
assert_compare_errors = rpc_helper.assert_compare_errors
assert_compare(repository=grpc_repo)
assert_compare(
repository=Repository(storage_name=grpc_repo.storage_name,
relative_path='no/such/path'))
# missing repo *message* (!)
assert_compare_errors(repository=None, same_details=False)
# unknown storage
assert_compare_errors(same_details=False,
repository=Repository(storage_name='unknown',
relative_path='/some/path'))
def test_has_local_branches(gitaly_comparison, server_repos_root):
fixture = gitaly_comparison
grpc_repo = fixture.gitaly_repo
rpc_helper = fixture.rpc_helper(
stub_cls=RepositoryServiceStub,
method_name='HasLocalBranches',
request_cls=RemoveRepositoryRequest,
)
assert_compare = rpc_helper.assert_compare
assert_compare_errors = rpc_helper.assert_compare_errors
assert_compare(repository=grpc_repo)
# repo does not exist
assert_compare_errors(
same_details=False,
repository=Repository(storage_name=grpc_repo.storage_name,
relative_path='no/such/path'))
# missing repo *message* (!)
assert_compare_errors(repository=None, same_details=False)
# unknown storage
assert_compare_errors(same_details=False,
repository=Repository(storage_name='unknown',
relative_path='/some/path'))
def test_full_path(gitaly_comparison, server_repos_root):
fixture = gitaly_comparison
grpc_repo = fixture.gitaly_repo
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment