# HG changeset patch
# User Georges Racinet <georges.racinet@octobus.net>
# Date 1679480576 -3600
#      Wed Mar 22 11:22:56 2023 +0100
# Branch stable
# Node ID 215369c1779812ee1add88100017ad0c7cbef825
# Parent  008b961e55653912d8d4a3a94e77df7011e2017a
servicer.load_repo: completed error treatment.

Up to know, we were thinking that raising `KeyError` and setting
code and details on the context was enough, but apparently, it
is at least problematic with streaming gRPC methods.

We're instead using the two-layer organisation that was suggested
as comment in `service.repository`. As it turns out, the error
code to set in case of unknown storage depends at least on the
service. This was already visible in the error treatment of the
full path methods of `RepositoryService`, which does not need
to be specific any more.

The new `STATUS_CODE_*`class attributes make it easy to tweak the
error code in service implementations. Specific methods could override
them with instance attributes if needed.

In the process, we're basically giving up on providing the same
`details` as Gitaly: these are highly dependent on implementation
(it's just stderr if the implementation relies on a
`git` subprocess) and chances that it may matter are low, given
that GitLab these days pushes towards "structured errors", i.e. with
programmatically usable details provided in metadata.
As a result, the normalization of error details in `assert_compare_errors`
is not used any more. We're keeping it in the code base, as it may
turn useful for forthcoming tests were the details would not be a
simple stderr echo.

diff --git a/hgitaly/service/ref.py b/hgitaly/service/ref.py
--- a/hgitaly/service/ref.py
+++ b/hgitaly/service/ref.py
@@ -127,11 +127,7 @@
             request: FindDefaultBranchNameRequest,
             context) -> FindDefaultBranchNameResponse:
         logger = LoggerAdapter(base_logger, context)
-        try:
-            repo = self.load_repo(request.repository, context)
-        except KeyError as exc:
-            context.abort(StatusCode.NOT_FOUND,
-                          "repository not found: " + repr(exc.args))
+        repo = self.load_repo(request.repository, context)
 
         branch = get_default_gitlab_branch(repo)
         if branch is None:
diff --git a/hgitaly/service/repository.py b/hgitaly/service/repository.py
--- a/hgitaly/service/repository.py
+++ b/hgitaly/service/repository.py
@@ -131,6 +131,8 @@
     """RepositoryServiceService implementation.
     """
 
+    STATUS_CODE_STORAGE_NOT_FOUND = StatusCode.INVALID_ARGUMENT
+
     def FindMergeBase(self,
                       request: FindMergeBaseRequest,
                       context) -> FindMergeBaseResponse:
@@ -158,16 +160,15 @@
                          request: RepositoryExistsRequest,
                          context) -> RepositoryExistsResponse:
         try:
-            self.load_repo(request.repository, context)
+            self.load_repo_inner(request.repository, context)
             exists = True
-        except KeyError:
+        except KeyError as exc:
+            if exc.args[0] == 'storage':
+                context.abort(
+                    StatusCode.INVALID_ARGUMENT,
+                    f'GetStorageByName: no such storage: "{exc.args[1]}"'
+                )
             exists = False
-            # TODO would be better to have a two-layer helper
-            # in servicer: load_repo() for proper gRPC error handling and
-            # load_repo_raw_exceptions() (name to be improved) to get the
-            # raw exceptions
-            context.set_code(StatusCode.OK)
-            context.set_details('')
 
         return RepositoryExistsResponse(exists=exists)
 
@@ -399,21 +400,7 @@
 
     def SetFullPath(self, request: SetFullPathRequest,
                     context) -> SetFullPathResponse:
-        try:
-            repo = self.load_repo(request.repository, context)
-        except KeyError as exc:
-            kind, what = exc.args
-            if kind == 'storage':
-                context.abort(StatusCode.INVALID_ARGUMENT,
-                              'setting config: GetStorageByName: '
-                              'no such storage: "%s"' % what)
-            else:
-                # (H)Gitaly relative paths are always ASCII, but the
-                # root might not be (Gitaly does disclose the full expected
-                # path in the error message)
-                context.abort(StatusCode.NOT_FOUND,
-                              'setting config: GetRepoPath: not a Mercurial '
-                              'repository: "%s"' % os.fsdecode(what))
+        repo = self.load_repo(request.repository, context)
 
         if not request.path:
             context.abort(StatusCode.INVALID_ARGUMENT, "no path provided")
@@ -423,21 +410,7 @@
 
     def FullPath(self, request: FullPathRequest,
                  context) -> FullPathResponse:
-        try:
-            repo = self.load_repo(request.repository, context)
-        except KeyError as exc:
-            kind, what = exc.args
-            if kind == 'storage':
-                context.abort(StatusCode.INVALID_ARGUMENT,
-                              'fetch config: GetStorageByName: '
-                              'no such storage: "%s"' % what)
-            else:
-                # (H)Gitaly relative paths are always ASCII, but the
-                # root might not be (Gitaly does disclose the full expected
-                # path in the error message)
-                context.abort(StatusCode.NOT_FOUND,
-                              'fetch config: GetRepoPath: not a Mercurial '
-                              'repository: "%s"' % os.fsdecode(what))
+        repo = self.load_repo(request.repository, context)
 
         path = get_gitlab_project_full_path(repo)
         if not path:  # None or (not probable) empty string
diff --git a/hgitaly/service/tests/test_repository_service.py b/hgitaly/service/tests/test_repository_service.py
--- a/hgitaly/service/tests/test_repository_service.py
+++ b/hgitaly/service/tests/test_repository_service.py
@@ -253,6 +253,12 @@
     fixture.grpc_repo.relative_path = 'does/not/exist'
     assert not fixture.exists()
 
+    # storage does not exist
+    fixture.grpc_repo.storage_name = 'dream'
+    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
diff --git a/hgitaly/servicer.py b/hgitaly/servicer.py
--- a/hgitaly/servicer.py
+++ b/hgitaly/servicer.py
@@ -60,6 +60,9 @@
       uis are derived. In particular, it bears the common configuration.
     """
 
+    STATUS_CODE_STORAGE_NOT_FOUND = StatusCode.NOT_FOUND
+    STATUS_CODE_REPO_NOT_FOUND = StatusCode.NOT_FOUND
+
     def __init__(self, storages):
         self.storages = storages
         self.init_ui()
@@ -101,8 +104,33 @@
         Error treatment: the caller doesn't have to do anything specific,
         the status code and the details are already set in context, and these
         are automatically propagated to the client (see corresponding test
-        in `test_servicer.py`). Still, the caller can still catch the
-        raised exception and change the code and details as they wish.
+        in `test_servicer.py`). For specific error treatment, use
+        :meth:`load_repo_inner` and catch the exceptions it raises.
+        """
+        try:
+            return self.load_repo_inner(repository, context)
+        except KeyError as exc:
+            self.handle_key_error(context, exc.args)
+
+    def handle_key_error(self, context, exc_args):
+        ktype = exc_args[0]
+        if ktype == 'storage':
+            context.abort(self.STATUS_CODE_STORAGE_NOT_FOUND,
+                          "No storage named %r" % exc_args[1])
+        elif ktype == 'repo':
+            context.abort(self.STATUS_CODE_REPO_NOT_FOUND,
+                          exc_args[1])
+
+    def load_repo_inner(self, repository: Repository, context):
+        """Load the repository from storage name and relative path
+
+        :param repository: Repository Gitaly Message, encoding storage name
+            and relative path
+        :param context: gRPC context (used in error raising)
+        :raises:
+          - ``KeyError('storage', storage_name)`` if storage is not found
+          - ``KeyError('repo', path, details)`` if repo not found or
+            cannot be loaded.
         """
         global repos_counter
         if repos_counter % GARBAGE_COLLECTING_RATE_GEN2 == 0:
@@ -121,9 +149,8 @@
         try:
             repo = hg.repository(self.ui, repo_path)
         except error.RepoError as exc:
-            context.set_code(StatusCode.NOT_FOUND)
-            context.set_details(repr(exc.args))
-            raise KeyError('repo', repo_path)
+            raise KeyError('repo', repo_path, repr(exc.args))
+
         weakref.finalize(repo, clear_repo_class, repo.unfiltered().__class__)
         srcrepo = hg.sharedreposource(repo)
         if srcrepo is not None:
@@ -135,13 +162,11 @@
     def storage_root_dir(self, storage_name, context):
         """Return the storage directory.
 
-        If the storage is unknown, this sets error code and details
-        on the context and raises ``KeyError('storage', storage_name)``
+        If the storage is unknown, this raises
+        ``KeyError('storage', storage_name)``
         """
         root_dir = self.storages.get(storage_name)
         if root_dir is None:
-            context.set_code(StatusCode.NOT_FOUND)
-            context.set_details("No storage named %r" % storage_name)
             raise KeyError('storage', storage_name)
         return root_dir
 
@@ -167,8 +192,28 @@
 
         :param bool ensure: if ``True``, the temporary directory is created
            if it does not exist yet.
+        """
+        try:
+            return self.temp_dir_inner(storage_name, context, ensure=ensure)
+        except KeyError as exc:
+            self.handle_key_error(context, exc.args)
+        except OSError as exc:
+            context.abort(StatusCode.INTERNAL,
+                          "Error ensuring temporary dir: %s" % exc)
+
+    def temp_dir_inner(self, storage_name, context, ensure=True):
+        """Return the path to temporary directory for the given storage
+
+        Similar to what Gitaly uses, with a dedicated path in order
+        to be really sure not to overwrite anything. The important feature
+        is that the temporary directory is under the root directory of
+        the storage, hence on the same file system (atomic renames of
+        other files from the storage, etc.)
+
+        :param bool ensure: if ``True``, the temporary directory is created
+           if it does not exist yet.
         :raises KeyError: if the storage is unknown
-        :raises OSError: if creation fail
+        :raises OSError: if creation fails.
         """
         temp_dir = os.path.join(self.storage_root_dir(storage_name, context),
                                 b'+hgitaly', b'tmp')
@@ -180,19 +225,14 @@
         # work well)
         to_create = []
         current = temp_dir
-        try:
-            while not os.path.exists(current):
-                to_create.append(current)
-                current = os.path.dirname(current)
+
+        while not os.path.exists(current):
+            to_create.append(current)
+            current = os.path.dirname(current)
 
-            while to_create:
-                # same mode as in Gitaly, hence we don't care about groups
-                # although this does propagate the setgid bit
-                os.mkdir(to_create.pop(), mode=0o755)
-        except OSError as exc:
-            context.set_code(StatusCode.INTERNAL)
-            context.set_details("Error ensuring temporary dir %r: %s" % (
-                temp_dir, exc))
-            raise
+        while to_create:
+            # same mode as in Gitaly, hence we don't care about groups
+            # although this does propagate the setgid bit
+            os.mkdir(to_create.pop(), mode=0o755)
 
         return temp_dir
diff --git a/hgitaly/tests/test_servicer.py b/hgitaly/tests/test_servicer.py
--- a/hgitaly/tests/test_servicer.py
+++ b/hgitaly/tests/test_servicer.py
@@ -31,8 +31,21 @@
 from ..stub.repository_pb2_grpc import RepositoryServiceStub
 
 
+class AbortContext(Exception):
+    """Raised by FakeContext.abort.
+
+    gRPC's context.abort raises `Exception()` (sic), which is
+    inconvenient for testing.
+    """
+
+
 class FakeContext(FakeServicerContext):
 
+    def abort(self, code, message):
+        self.code = code
+        self.message = message
+        raise AbortContext()
+
     def set_code(self, code):
         self.code = code
 
@@ -61,10 +74,15 @@
     assert loaded.root == wrapper.repo.root
 
     with pytest.raises(KeyError) as exc_info:
+        servicer.load_repo_inner(Repository(storage_name='dream',
+                                            relative_path='dream-proj.hg'),
+                                 context)
+    assert exc_info.value.args == ('storage', 'dream')
+
+    with pytest.raises(AbortContext):
         servicer.load_repo(Repository(storage_name='dream',
                                       relative_path='dream-proj.hg'),
                            context)
-    assert exc_info.value.args == ('storage', 'dream')
     assert context.code == grpc.StatusCode.NOT_FOUND
 
 
@@ -102,7 +120,7 @@
     assert len(frt) == 0
 
 
-def test_not_found_propagation(grpc_channel, server_repos_root):
+def test_errors_propagation(grpc_channel, server_repos_root):
     # Taking a random RPC to check that the client receives the
     # proper error response
     repo_stub = RepositoryServiceStub(grpc_channel)
@@ -112,7 +130,7 @@
             repository=Repository(storage_name='dream', relative_path='')))
     exc = exc_info.value
 
-    assert exc.code() == grpc.StatusCode.NOT_FOUND
+    assert exc.code() == grpc.StatusCode.INVALID_ARGUMENT
     assert 'dream' in exc.details()
 
     with pytest.raises(grpc.RpcError) as exc_info:
@@ -139,8 +157,11 @@
     assert path.exists()
 
     with pytest.raises(KeyError) as exc_info:
+        servicer.temp_dir_inner('unknown', context)
+    assert exc_info.value.args == ('storage', 'unknown')
+
+    with pytest.raises(AbortContext) as exc_info:
         servicer.temp_dir('unknown', context)
-    assert exc_info.value.args == ('storage', 'unknown')
     assert context.code == grpc.StatusCode.NOT_FOUND
 
 
@@ -152,6 +173,6 @@
 
     servicer = HGitalyServicer(dict(broken=as_bytes(broken)))
 
-    with pytest.raises(OSError):
+    with pytest.raises(AbortContext):
         servicer.temp_dir('broken', context, ensure=True)
     assert context.code == grpc.StatusCode.INTERNAL
diff --git a/tests_with_gitaly/comparison.py b/tests_with_gitaly/comparison.py
--- a/tests_with_gitaly/comparison.py
+++ b/tests_with_gitaly/comparison.py
@@ -263,7 +263,7 @@
             norm = self.error_details_normalizer
             hg_details = exc_hg.details()
             git_details = exc_git.details()
-            if norm is not None:
+            if norm is not None:  # pragma no cover
                 hg_details = norm(hg_details, vcs='hg')
                 git_details = norm(git_details, vcs='git')
             assert hg_details == git_details
diff --git a/tests_with_gitaly/test_blob_tree.py b/tests_with_gitaly/test_blob_tree.py
--- a/tests_with_gitaly/test_blob_tree.py
+++ b/tests_with_gitaly/test_blob_tree.py
@@ -169,6 +169,7 @@
     # precondition for the test: mirror worked
     assert fixture.git_repo.branch_titles() == {b'branch/default': b"zebar"}
 
+    # basic case
     for path in (b'sub', b'sub/bar', b'sub/', b'.', b'do-not-exist'):
         fixture.assert_compare_tree_entry(path)
 
diff --git a/tests_with_gitaly/test_repository_service.py b/tests_with_gitaly/test_repository_service.py
--- a/tests_with_gitaly/test_repository_service.py
+++ b/tests_with_gitaly/test_repository_service.py
@@ -192,14 +192,10 @@
     fixture = gitaly_comparison
     grpc_repo = fixture.gitaly_repo
 
-    def normalizer(msg, **kw):
-        return msg.replace('Mercurial', 'git')
-
     rpc_helper = fixture.rpc_helper(
         stub_cls=RepositoryServiceStub,
         method_name='FullPath',
         request_cls=FullPathRequest,
-        error_details_normalizer=normalizer,
     )
     assert_compare_errors = rpc_helper.assert_compare_errors
 
@@ -211,6 +207,7 @@
                           repository=Repository(storage_name='unknown',
                                                 relative_path='/some/path'))
     assert_compare_errors(
+        same_details=False,
         repository=Repository(storage_name=grpc_repo.storage_name,
                               relative_path='no/such/path'))
 
@@ -218,14 +215,10 @@
 def test_set_full_path(gitaly_comparison, server_repos_root):
     fixture = gitaly_comparison
 
-    def normalize_repo_not_found(msg, **kw):
-        return msg.replace('git repo', 'Mercurial repo')
-
     rpc_helper = fixture.rpc_helper(
         stub_cls=RepositoryServiceStub,
         method_name='SetFullPath',
         request_cls=SetFullPathRequest,
-        error_details_normalizer=normalize_repo_not_found,
     )
     assert_compare = rpc_helper.assert_compare
     assert_error_compare = rpc_helper.assert_compare_errors
@@ -240,9 +233,11 @@
                          repository=Repository(
                              storage_name=fixture.gitaly_repo.storage_name,
                              relative_path='no/such/repo'),
+                         same_details=False,
                          )
 
     assert_error_compare(path='some/full/path',
+                         same_details=False,
                          repository=Repository(
                              relative_path=fixture.gitaly_repo.relative_path,
                              storage_name='unknown'))