Skip to content
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

Closed
wants to merge 8 commits into from

Conversation

newhoggy
Copy link
Contributor

@newhoggy newhoggy commented Mar 24, 2023

Introduce a newtype File direction to track how files are used to replace use of FilePath.

The type File In indicates that the file is for reading, File Out for writing and File 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 used directory.

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:

  • Expanding the File type to support special pseudo-files like "-" for stdin and stdout described in [FR] - All command line options that take a filename should also take "-" #5016.
  • Supporting a second type parameter content for describing the contents of the file. For example CBOR vs JSON.
  • Tightening type-safety by providing more typesafe versions of read/write functions and not use the escape hatch File and unFile that is currently used in the code as often.
  • Using the annotations to improve the documentation making it clear which CLI options are inputs and which are outputs.

data Out

-- | Phantom type to indicate a file for reading or writing.
data InOut
Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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

Copy link
Contributor Author

@newhoggy newhoggy Mar 28, 2023

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.

@newhoggy newhoggy changed the title New File type that tracks whether the file is an input or output file… New File type that tracks whether the file is an input or output file or both Mar 26, 2023
@newhoggy newhoggy force-pushed the newhoggy/file-types branch from 286e1b1 to 60f33a0 Compare March 26, 2023 22:40
@newhoggy newhoggy force-pushed the newhoggy/file-types branch 6 times, most recently from 93b974f to 64a749b Compare March 27, 2023 05:14
-- be used for reading or writing.
newtype File direction = File { unFile :: FilePath }
deriving (Eq, Ord)
deriving newtype (Show, IsString, FromJSON, ToJSON)
Copy link
Contributor Author

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.

@newhoggy newhoggy force-pushed the newhoggy/file-types branch from 64a749b to f8db1d7 Compare March 27, 2023 05:50
@@ -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
Copy link
Contributor Author

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.

@newhoggy newhoggy force-pushed the newhoggy/file-types branch 2 times, most recently from 14373f6 to fbfa29a Compare March 27, 2023 06:26
= SocketPath { unSocketPath :: FilePath }
deriving (FromJSON, Show, Eq, Ord)
= SocketPath { unSocketPath :: File InOut }
deriving newtype (FromJSON, Show, Eq, Ord)
Copy link
Contributor Author

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)
Copy link
Contributor Author

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
Copy link
Contributor Author

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)
Copy link
Contributor Author

@newhoggy newhoggy Mar 27, 2023

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.

@newhoggy newhoggy marked this pull request as ready for review March 27, 2023 06:45
@newhoggy newhoggy force-pushed the newhoggy/file-types branch 5 times, most recently from 8a7b3c6 to 0485568 Compare April 13, 2023 11:30
@@ -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
Copy link
Contributor Author

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)
Copy link
Contributor Author

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.

Copy link
Contributor

@Jimbo4350 Jimbo4350 Apr 14, 2023

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

@newhoggy newhoggy force-pushed the newhoggy/file-types branch from 0485568 to 3e5df85 Compare April 13, 2023 12:24
toJSON (SigningKeyFile (File a)) = toJSON a

instance FromJSON (SigningKeyFile direction) where
parseJSON a = SigningKeyFile . File <$> parseJSON a
Copy link
Contributor Author

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
Copy link
Contributor Author

@newhoggy newhoggy Apr 13, 2023

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 $
Copy link
Contributor Author

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.

@newhoggy newhoggy force-pushed the newhoggy/file-types branch 2 times, most recently from f7a5d49 to 892b87b Compare April 13, 2023 14:38
(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)
Copy link
Contributor Author

@newhoggy newhoggy Apr 13, 2023

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,
Copy link
Contributor Author

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
Copy link
Contributor

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.

Copy link
Contributor

@Jimbo4350 Jimbo4350 left a 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'.

cardano-api/src/Cardano/Api/IO.hs Show resolved Hide resolved
fileOption :: forall direction. Mod OptionFields FilePath -> Parser (File direction)
fileOption = fmap File . Opt.strOption

inFileOption :: forall. Mod OptionFields FilePath -> Parser (File 'In)
Copy link
Contributor

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" #-}
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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)
Copy link
Contributor

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 ()
Copy link
Contributor

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 #-}
Copy link
Contributor

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,
Copy link
Contributor

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?

Copy link
Contributor Author

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
Copy link
Contributor

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

@newhoggy
Copy link
Contributor Author

Superseded by #5105

@newhoggy newhoggy closed this Apr 19, 2023
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.

3 participants