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

Adding 'MustExist' constructor to assert a file is assumed to exist #85

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 12 additions & 11 deletions fs-api/src-unix/System/FS/IO/Unix.hs
Original file line number Diff line number Diff line change
Expand Up @@ -73,19 +73,16 @@ open fp openMode = Posix.openFd fp posixOpenMode fileFlags
AppendMode ex -> ( Posix.WriteOnly
, defaultFileFlags { Posix.append = True
, Posix.exclusive = isExcl ex
, Posix.creat = Just Posix.stdFileMode }
, Posix.creat = creat ex }
)
ReadWriteMode ex -> ( Posix.ReadWrite
, defaultFileFlags { Posix.exclusive = isExcl ex
, Posix.creat = Just Posix.stdFileMode }
, Posix.creat = creat ex }
)
WriteMode ex -> ( Posix.ReadWrite
, defaultFileFlags { Posix.exclusive = isExcl ex
, Posix.creat = Just Posix.stdFileMode }
, Posix.creat = creat ex }
)

isExcl AllowExisting = False
isExcl MustBeNew = True
# else
open fp openMode = Posix.openFd fp posixOpenMode fileMode fileFlags
where
Expand All @@ -95,22 +92,26 @@ open fp openMode = Posix.openFd fp posixOpenMode fileMode fileFlags
, defaultFileFlags
)
AppendMode ex -> ( Posix.WriteOnly
, Just Posix.stdFileMode
, creat x
, defaultFileFlags { Posix.append = True
, Posix.exclusive = isExcl ex }
)
ReadWriteMode ex -> ( Posix.ReadWrite
, Just Posix.stdFileMode
, creat x
, defaultFileFlags { Posix.exclusive = isExcl ex }
)
WriteMode ex -> ( Posix.ReadWrite
, Just Posix.stdFileMode
, creat x
, defaultFileFlags { Posix.exclusive = isExcl ex }
)

# endif
isExcl AllowExisting = False
isExcl MustBeNew = True
# endif
isExcl MustExist = False

creat AllowExisting = Just Posix.stdFileMode
creat MustBeNew = Just Posix.stdFileMode
creat MustExist = Nothing

-- | Writes the data pointed by the input 'Ptr Word8' into the input 'FHandle'.
write :: FHandle -> Ptr Word8 -> Int64 -> IO Word32
Expand Down
1 change: 1 addition & 0 deletions fs-api/src-win32/System/FS/IO/Windows.hs
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ open filename openMode = do
ReadWriteMode ex -> (gENERIC_READ .|. gENERIC_WRITE, createNew ex)
createNew AllowExisting = oPEN_ALWAYS
createNew MustBeNew = cREATE_NEW
createNew MustExist = oPEN_ALWAYS
Comment on lines 61 to +63
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be wrong: MustExist and AllowExisting get the same file flags


write :: FHandle -> Ptr Word8 -> Int64 -> IO Word32
write fh data' bytes = withOpenHandle "write" fh $ \h ->
Expand Down
6 changes: 5 additions & 1 deletion fs-api/src/System/FS/API/Types.hs
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,11 @@ data AllowExisting
-- ^ The file may already exist. If it does, it is reopened. If it
-- doesn't, it is created.
| MustBeNew
-- ^ The file may not yet exist. If it does, an error
-- ^ The file must not yet exist. If it does, an error
-- ('FsResourceAlreadyExist') is thrown.
| MustExist
-- ^ The file must already exist. If it does not, an error
-- ('FsResourceDoesNotExist') is thrown.
deriving (Eq, Show)

allowExisting :: OpenMode -> AllowExisting
Copy link
Collaborator

@jorisdral jorisdral Dec 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder, should allowExisting ReadMode = MustExist? That seems to be line with the semantics of hOpen on Unix at least.

Let's also take this opportunity to document the semantics of ReadMode. Something like: if opening a file in read mode, then the file must exist or an exception is thrown

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also add a TODO to test that this is the actual behaviour on both Windows and Unix. Hopefully this is already checked by the state machine tests, but it does not hurt to have some more targeted property tests

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there should probably also be an issue to properly document OpenMode. I'll create that

Expand Down Expand Up @@ -460,6 +463,7 @@ instance Condense SeekMode where
instance Condense AllowExisting where
condense AllowExisting = ""
condense MustBeNew = "!"
condense MustExist = "+"

instance Condense OpenMode where
condense ReadMode = "r"
Expand Down
19 changes: 13 additions & 6 deletions fs-sim/src/System/FS/Sim/FsTree.hs
Original file line number Diff line number Diff line change
Expand Up @@ -231,14 +231,21 @@ getDir fp =
Specific file system functions
-------------------------------------------------------------------------------}

-- | Open a file: create it if necessary or throw an error if it existed
-- already wile we were supposed to create it from scratch (when passed
-- 'MustBeNew').
-- | Open a file: create it if necessary or throw an error if either:
-- 1. It existed already while we were supposed to create it from scratch
-- (when passed 'MustBeNew').
-- 2. It did not already exists when we expected to (when passed 'MustExist').
Comment on lines +234 to +237
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's link to AllowExisting instead, which already describes the intended semantics

Suggested change
-- | Open a file: create it if necessary or throw an error if either:
-- 1. It existed already while we were supposed to create it from scratch
-- (when passed 'MustBeNew').
-- 2. It did not already exists when we expected to (when passed 'MustExist').
-- | Open a file based on an 'AllowExisting'.

openFile :: Monoid a
=> FsPath -> AllowExisting -> FsTree a -> Either FsTreeError (FsTree a)
openFile fp ex = alterFile fp Left (Right mempty) $ \a -> case ex of
AllowExisting -> Right a
MustBeNew -> Left (FsExists fp)
openFile fp ex = alterFile fp Left caseDoesNotExist caseAlreadyExist
where
caseAlreadyExist a = case ex of
MustBeNew -> Left (FsExists fp)
_ -> Right a

caseDoesNotExist = case ex of
MustExist -> Left (FsMissing fp (pathLast fp :| []))
_ -> Right mempty
Comment on lines +242 to +248
Copy link
Collaborator

@jorisdral jorisdral Dec 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Preferably, list all constructors, so that we won't forget to update the cases if the AllowExisting type changes in the future


-- | Replace the contents of the specified file (which must exist)
replace :: FsPath -> a -> FsTree a -> Either FsTreeError (FsTree a)
Expand Down
8 changes: 6 additions & 2 deletions fs-sim/src/System/FS/Sim/MockFS.hs
Original file line number Diff line number Diff line change
Expand Up @@ -479,7 +479,11 @@ hOpen fp openMode = do
, fsLimitation = True
}
modifyMockFS $ \fs -> do
let alreadyHasWriter =
let assumedExistance (WriteMode MustExist) = True
assumedExistance (AppendMode MustExist) = True
assumedExistance (ReadWriteMode MustExist) = True
assumedExistance _ = False
Comment on lines +482 to +485
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe name it fileShouldExist?

alreadyHasWriter =
any (\hs -> openFilePath hs == fp && isWriteHandle hs) $
openHandles fs
when (openMode /= ReadMode && alreadyHasWriter) $
Expand All @@ -491,7 +495,7 @@ hOpen fp openMode = do
, fsErrorStack = prettyCallStack
, fsLimitation = True
}
when (openMode == ReadMode) $ void $
when (openMode == ReadMode || assumedExistance openMode) $ void $
checkFsTree $ FS.getFile fp (mockFiles fs)
Comment on lines +498 to 499
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we make the change allowExisting ReadMode = MustExist, then I think this check can go away completely. openFile should then already throw the correct error

files' <- checkFsTree $ FS.openFile fp ex (mockFiles fs)
return $ newHandle (fs { mockFiles = files' })
Expand Down
28 changes: 27 additions & 1 deletion fs-sim/test/Test/System/FS/StateMachine.hs
Original file line number Diff line number Diff line change
Expand Up @@ -676,7 +676,7 @@ generator Model{..} = oneof $ concat [
(rf, wf) = if fileExists then (10,3) else (1,3)

genAllowExisting :: Gen AllowExisting
genAllowExisting = elements [AllowExisting, MustBeNew]
genAllowExisting = elements [AllowExisting, MustBeNew, MustExist]

genSeekMode :: Gen SeekMode
genSeekMode = elements [
Expand Down Expand Up @@ -1055,6 +1055,7 @@ data Tag =
-- > Get ..
| TagPutSeekNegGet


Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

-- Open with MustBeNew (O_EXCL flag), but the file already existed.
--
-- > h <- Open fp (AppendMode _)
Expand All @@ -1063,6 +1064,13 @@ data Tag =
| TagExclusiveFail


-- Open with MustExist, but the file does not exist.
--
-- > DoesFileExist fp
-- > h <- Open fp (AppendMode _)
| TagAssumeExists
Comment on lines +1067 to +1071
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A similar tag for opening a non-existent file in read mode would also be useful



-- Reading returns an empty bytestring when EOF
--
-- > h <- open fp ReadMode
Expand Down Expand Up @@ -1136,6 +1144,7 @@ tag = C.classify [
, tagPutSeekGet Set.empty Set.empty
, tagPutSeekNegGet Set.empty Set.empty
, tagExclusiveFail
-- , tagAssumeExistsFail -- Set.empty
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be updated

, tagReadEOF
, tagPread
, tagPutGetBuf Set.empty
Expand Down Expand Up @@ -1481,6 +1490,23 @@ tag = C.classify [
Left TagExclusiveFail
_otherwise -> Right tagExclusiveFail

tagAssumeExistsFail :: EventPred
tagAssumeExistsFail = C.predicate $ \ev ->
{-
tagClosedTwice closed = successful $ \ev _suc ->
case eventMockCmd ev of
Close (Handle h _) | Set.member h closed -> Left TagClosedTwice
Close (Handle h _) -> Right $ tagClosedTwice $ Set.insert h closed
_otherwise -> Right $ tagClosedTwice closed
(DoesFileExist _, Bool False) -> Left TagDoesFileExistKO
-}
Comment on lines +1495 to +1502
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove commented out code

case (eventMockCmd ev, eventMockResp ev) of
(Open _ mode, Resp (Left fsError))
| MustExist <- allowExisting mode
, fsErrorType fsError == FsResourceDoesNotExist ->
Left TagAssumeExists
_otherwise -> Right tagAssumeExistsFail

tagReadEOF :: EventPred
tagReadEOF = successful $ \ev suc ->
case (eventMockCmd ev, suc) of
Expand Down
Loading