[#106] Backend-agnostic listDirectoryContents
#107
Conversation
DK318
force-pushed
the
dk318/#106-backend-agnostic-listsecrets
branch
from
565e0aa
to
4857a8b
Compare
DK318
changed the title
listSecretslistDirectoryContents
| entry <- lift $ readEntry backend (Path.appendEntryName rootPath entryName) | ||
| case entry of | ||
| Just entry -> modify' (Dir.insertEntry entry) | ||
| Nothing -> pure () |
There was a problem hiding this comment.
@dcastro If I remember correctly, when we discussed line 528
Nothing -> pure ()
you said it is not correct to just ignore unexpected Nothing.
Maybe we can throw some internalError, cause contents ^. entryNames returned an entryName that couldn't be read properly by readEntry, which is unexpected.
There was a problem hiding this comment.
I just copy-pasted that code). Maybe we should really throw something here
There was a problem hiding this comment.
Yeah I remember saying that. I've been thinking about it a bit more, and maybe pure () is right after all 🤔
So, we're traversing a directory, listDirectoryContents gave us a list of entry names, and then readEntry returned 404 for one of those entry names.
That, to me, suggests that some other user deleted an entry from vault (or whatever backend) while we were traversing the directory.
There's two things we can do here:
- ignore this entry that has been concurrently deleted
- throw and crash
(2) would imply that coffer can only traverse a vault directory if no one else is messing with it. I don't think we want to do this, we want to allow multiple people to interact with vault at the same time. So ignoring the entry is the sensible thing to do here, IMO.
There was a problem hiding this comment.
I do think we should add a comment explaining what this branch means.
Nothing ->
-- This entry has been concurrently deleted (e.g. by some other user) _while_ we're traversing the directory.
-- We should just ignore it.
pure ()
| @@ -30,17 +30,10 @@ class (Buildable err) => BackendError err | |||
|
|
|||
| -- | Internal errors that can be thrown if backend is in illegal state. | |||
| data InternalCommandsError | |||
| = InvalidEntry Text | |||
| | EntryPathDoesntHavePrefix EntryPath Path | |||
| = EntryPathDoesntHavePrefix EntryPath Path | |||
|
|
|||
| instance Buildable InternalCommandsError where | |||
| build = \case | |||
There was a problem hiding this comment.
Seems we can get rid of \case, because InternalCommandError have only one way now.
But it will add work to person how will add new InternalCommandError and make code more inconsistent(look at CofferError instance).
Problem: at this moment the `listDirectoryContents` function returns `[Text]` and in `getEntryOrDir` we rely on the fact that `vault` adds a '/' to the directory. But other backends might not behave like this. Solution: created new data `DirectoryContents` which stores 2 lists: 1. list with directories 2. list with entries and changed return type from `Maybe [Text]` to `Maybe DirectoryContents` in `listDirectoryContents` function.
DK318
force-pushed
the
dk318/#106-backend-agnostic-listsecrets
branch
from
634bfc2
to
c2afa9d
Compare
Description
Problem
At this moment the
listDirectoryContentsfunction returns[Text]and in
getEntryOrDirwe rely on the fact thatvaultadds a '/'to the directory. But other backends might not behave like this.
Solution
Created new data
DirectoryContentswhich stores 2 lists:and changed return type from
Maybe [Text]toMaybe DirectoryContentsin
listDirectoryContentsfunction.Related issue(s)
listSecrets's implementation details #106Fixed #
✅ Checklist for your Pull Request
Related changes (conditional)
Tests
silently reappearing again.
Documentation
Stylistic guide (mandatory)