-
Notifications
You must be signed in to change notification settings - Fork 721
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
New File type that tracks whether the file is an input or output file or both #5017
Conversation
cardano-api/src/Cardano/Api/IO.hs
Outdated
data Out | ||
|
||
-- | Phantom type to indicate a file for reading or writing. | ||
data InOut |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Introduce phantom types to track what direction the file will be processed. An In
file is only read. An Out
file is only written. An InOut
file is both read and written.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if maybe DataKinds
would be helpful here? For example:
data FileDirection = In | Out | InOut
then you could use the new kind in the File
phantom type:
newtype File (a :: FileDirection) = File { unFile :: FilePath }
The end result would be pretty much the same, maybe a bit more readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I support the use of DataKinds
but will wait for more feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree let's use DataKinds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Converted to use DataKinds
. This means we now use 'In/'Out/'InOut
instead of In/Out/InOut
.
286e1b1
to
60f33a0
Compare
93b974f
to
64a749b
Compare
cardano-api/src/Cardano/Api/IO.hs
Outdated
-- be used for reading or writing. | ||
newtype File direction = File { unFile :: FilePath } | ||
deriving (Eq, Ord) | ||
deriving newtype (Show, IsString, FromJSON, ToJSON) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrapper around FilePath
to track direction the file should be used.
64a749b
to
f8db1d7
Compare
@@ -636,7 +636,7 @@ parseK = | |||
<$> parseIntegral "k" "The security parameter of the Ouroboros protocol." | |||
|
|||
parseNewDirectory :: String -> String -> Parser NewDirectory | |||
parseNewDirectory opt desc = NewDirectory <$> parseFilePath opt desc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This used parseFilePath
which uses "file"
as the bash completer, which is wrong.
14373f6
to
fbfa29a
Compare
= SocketPath { unSocketPath :: FilePath } | ||
deriving (FromJSON, Show, Eq, Ord) | ||
= SocketPath { unSocketPath :: File InOut } | ||
deriving newtype (FromJSON, Show, Eq, Ord) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use sockets bi-directionally, so this is marked as InOut
.
| NodeKeyGenVRF (VerificationKeyFile Out) (SigningKeyFile Out) | ||
| NodeKeyHashVRF (VerificationKeyOrFile VrfKey) (Maybe (File Out)) | ||
| NodeNewCounter ColdVerificationKeyOrFile Word (OpCertCounterFile Out) | ||
| NodeIssueOpCert (VerificationKeyOrFile KesKey) (SigningKeyFile In) (OpCertCounterFile InOut) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An example of where we read and write to a file.
@@ -996,11 +992,11 @@ getCurrentTimePlus30 = | |||
-- | Attempts to read Shelley genesis from disk | |||
-- and if not found creates a default Shelley genesis. | |||
readShelleyGenesisWithDefault | |||
:: FilePath | |||
:: File InOut |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an example of a file that reads from a file also writing to that same file.
deriving Show | ||
|
||
data ProtocolParamsSourceSpec | ||
= ParamsFromGenesis !GenesisFile | ||
= ParamsFromGenesis !(GenesisFile InOut) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without the InOut
annotation, it's not obvious that this file is both read from and written to.
8a7b3c6
to
0485568
Compare
@@ -70,7 +71,7 @@ instance AdjustFilePaths NixServiceOptions where | |||
adjustFilePaths f opts | |||
= opts { | |||
_nix_nodeConfigFile = f <$> _nix_nodeConfigFile opts | |||
, _nix_sigKey = SigningKeyFile . f . unSigningKeyFile $ _nix_sigKey opts | |||
, _nix_sigKey = mapFile f $ _nix_sigKey opts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An example of how mapFile
is used.
Without the MapFile
type class, this expression becomes:
SigningKeyFile . File . f . unFile . unSigningKeyFile $ _nix_sigKey opts
deriving (Eq, Ord, Show, IsString) | ||
newtype NewVerificationKeyFile (direction :: FileDirection) = NewVerificationKeyFile | ||
{ unNewVerificationKeyFile :: File direction | ||
} deriving newtype (Eq, Ord, Show, IsString, HasFileMode, MapFile) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deriving an instance of HasFileMode
and MapFile
for a newtype
is very easy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The MapFile
typeclass is unnecessary when we remove the direction
variable from NodeConfig
0485568
to
3e5df85
Compare
toJSON (SigningKeyFile (File a)) = toJSON a | ||
|
||
instance FromJSON (SigningKeyFile direction) where | ||
parseJSON a = SigningKeyFile . File <$> parseJSON a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By defining our own Aeson instances, we can eliminate some orphans.
@@ -1444,7 +1444,7 @@ pGenesisCmd = | |||
-- Shelley CLI flag parsers | |||
-- | |||
|
|||
data FileDirection | |||
data ParserFileDirection |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a localised rename to a name avoid conflict with the new FileDirection
type that is being introduced.
deriving newtype (Eq, Ord, Show, IsString, ToJSON, FromJSON) | ||
|
||
writeByteStringFile :: MonadIO m => FilePath -> ByteString -> m (Either (FileError ()) ()) | ||
writeByteStringFile :: MonadIO m => File 'Out -> ByteString -> m (Either (FileError ()) ()) | ||
writeByteStringFile fp bs = runExceptT $ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of these functions now take the new file type as the argument. This reduces the opportunity for introducing bugs for instance writing to a file that was annotated as In
or reading from a file that was annotated as Out
.
f7a5d49
to
892b87b
Compare
(SigningKeyFile $ File $ dir </> "kes" ++ strIndex ++ ".skey") | ||
runNodeKeyGenVRF | ||
(VerificationKeyFile $ File $ dir </> "vrf" ++ strIndex ++ ".vkey") | ||
(SigningKeyFile $ File $ dir </> "vrf" ++ strIndex ++ ".skey") | ||
runNodeKeyGenCold | ||
(VerificationKeyFile $ File $ dir </> "cold" ++ strIndex ++ ".vkey") | ||
(usingOut coldSK) | ||
(usingOut opCertCtr) | ||
(usingOut @SigningKeyFile coldSK) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've type annotated usingOut
to show everywhere where the type isn't File
. This is because we sometimes need to constraint the direction from InOut
to either In
or Out
for File
wrappers too like SigningKeyFile
.
If I remove HasFileMode
, then I would have to create a lot of functions like toSigningKeyFileIn
and toSigningKeyFileOut
, one for each wrapper type, which introduces a lot of extra functions to the API.
These functions have been added to the next commit for illustrations purposes only.
Eventually I want to expand File direction
to File content direction
. As part of that change all the wrapper types will be deleted as well.
This will allow me to remove HasFileMode
without having to introduce lots of conversion functions.
I recommend keeping HasFileMode
for now.
If we are okay with this, I will undo the last two commits including this one.
toNetworkConfigFileIn, | ||
toNetworkConfigFileOut, | ||
toNodeConfigIn, | ||
toNodeConfigOut, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are the functions that have been added in order to delete HasFileMode
.
Nothing -> liftIO $ Text.putStr t | ||
|
||
class MapFile a where |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another option would be to use MonoTraversable
instead - but I like your solution with specialized typeclass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the point of updating NodeConfig
and GenesisConfig
with direction
because there isn't a single instance of NodeConfig 'Out
or GenesisConfig 'Out
. Whatever filepaths are part of these types we can just default to In'
.
fileOption :: forall direction. Mod OptionFields FilePath -> Parser (File direction) | ||
fileOption = fmap File . Opt.strOption | ||
|
||
inFileOption :: forall. Mod OptionFields FilePath -> Parser (File 'In) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can ditch the forall
in inFileOption
and outFileOption
, Opt.help desc | ||
, Opt.completer (Opt.bashCompleter "file") | ||
] | ||
-- {-# DEPRECATED parseFilePath "Use parseFile instead" #-} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left over comment?
} | ||
|
||
data NodeConfig = NodeConfig | ||
data NodeConfig (direction :: FileDirection) = NodeConfig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There isn't a single instance of NodeConfig 'Out
which makes me wonder is this change necessary/worth it?
@@ -954,9 +958,9 @@ toLedgerStateEvents lr = (ledgerState, ledgerEvents) | |||
|
|||
|
|||
-- Usually only one constructor, but may have two when we are preparing for a HFC event. | |||
data GenesisConfig | |||
data GenesisConfig direction |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likewise here, not a single instance of GenesisConfig 'Out
@@ -24,6 +26,7 @@ import qualified Options.Applicative.Help as OptI | |||
import System.Posix.Types (Fd (..)) | |||
import Text.Read (readMaybe) | |||
|
|||
import Cardano.Api (fileOption) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be with the Prelude
import
|
||
|
||
-- | Make sure the VRF private key file is readable only | ||
-- by the current process owner the node is running under. | ||
checkVRFFilePermissions :: FilePath -> ExceptT VRFPrivateKeyFilePermissionError IO () | ||
checkVRFFilePermissions :: File direction -> ExceptT VRFPrivateKeyFilePermissionError IO () |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can default to 'In
here
@@ -6,14 +7,15 @@ | |||
{-# LANGUAGE UndecidableInstances #-} | |||
|
|||
{-# OPTIONS_GHC -Wno-name-shadowing -Wno-orphans #-} | |||
{-# LANGUAGE DataKinds #-} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be with the other LANGAUGE
pragmas
@@ -808,6 +810,13 @@ module Cardano.Api ( | |||
parseFilePath, | |||
parseFileOut, | |||
parseDirectory, | |||
|
|||
toGenesisFileIn, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These functions aren't used anywhere. Why implement them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For completeness so that if someone wanted to use them, they'd be available.
@@ -46,12 +46,19 @@ module Cardano.CLI.Shelley.Commands | |||
, BlockId (..) | |||
, WitnessSigningData (..) | |||
, ColdVerificationKeyOrFile (..) | |||
|
|||
, toOpCertCounterFileIn | |||
, toOpCertCounterFileOut |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same with these with the exception of toOpCertCounterFileOut
892b87b
to
8846dbc
Compare
Superseded by #5105 |
Introduce a
newtype File direction
to track how files are used to replace use ofFilePath
.The type
File In
indicates that the file is for reading,File Out
for writing andFile InOut
for both.Annotating the type of file paths in this way makes it clear how files are used. The most notable improvements are found where the
InOut
annotation is used. In these cases, the file path is used to both open the file for reading and writing and in some cases, the fact that the file is written to is non-obvious.New functions have been added to parse CLI arguments for files in this new typesafe scheme.
The process of making the code more type-safe around files also helped identify a bug where we used a
file
bash completer for a directory when we should have useddirectory
.This is a large PR and it isn't necessary to merge it all at once. The PR is mostly to demonstrate that this is possible and the first commit serves as a good first step in this direction.
Future work
This PR lays the ground work for:
File
type to support special pseudo-files like"-"
forstdin
andstdout
described in [FR] - All command line options that take a filename should also take "-" #5016.content
for describing the contents of the file. For example CBOR vs JSON.File
andunFile
that is currently used in the code as often.