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

recursion-ninja · 2024-12-21T17:28:32Z

There are times when it is advantageous to assert that a file "must already exist" on the file system and if it does not exist, a FsResourceDoesNotExist exception ought to be thrown. To enable these semantics through the API of the fs-api/fs-sim packages, I have added a MustExist constructor to the AllowExisting type.

jorisdral

Thanks for the feature! Some suggestions below

jorisdral · 2024-12-23T10:48:32Z

fs-sim/src/System/FS/Sim/FsTree.hs

+-- | 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').


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'.

jorisdral · 2024-12-23T10:49:24Z

fs-sim/src/System/FS/Sim/FsTree.hs

+    caseAlreadyExist a = case ex of
+      MustBeNew -> Left (FsExists fp)
+      _         -> Right a
+
+    caseDoesNotExist = case ex of
+      MustExist -> Left (FsMissing fp (pathLast fp :| []))
+      _         -> Right mempty


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

jorisdral · 2024-12-23T10:51:11Z

fs-sim/src/System/FS/Sim/MockFS.hs

+      let assumedExistance (WriteMode MustExist) = True
+          assumedExistance (AppendMode MustExist) = True
+          assumedExistance (ReadWriteMode MustExist) = True
+          assumedExistance _ = False


Maybe name it fileShouldExist?

jorisdral · 2024-12-23T10:54:41Z

fs-api/src/System/FS/API/Types.hs

    -- ('FsResourceAlreadyExist') is thrown.
+  | MustExist
+    -- ^ The file must already exist. If it does not, an error
+    -- ('FsResourceDoesNotExist') is thrown.
  deriving (Eq, Show)

 allowExisting :: OpenMode -> AllowExisting


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

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

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

jorisdral · 2024-12-23T10:55:28Z

fs-api/src-win32/System/FS/IO/Windows.hs

    createNew AllowExisting = oPEN_ALWAYS
    createNew MustBeNew     = cREATE_NEW
+    createNew MustExist     = oPEN_ALWAYS


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

jorisdral · 2024-12-23T11:03:39Z

fs-sim/src/System/FS/Sim/MockFS.hs

+      when (openMode == ReadMode || assumedExistance openMode) $ void $
        checkFsTree $ FS.getFile fp (mockFiles fs)


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

jorisdral · 2024-12-23T11:04:00Z

fs-sim/test/Test/System/FS/StateMachine.hs

@@ -1055,6 +1055,7 @@ data Tag =
  -- > Get ..
  | TagPutSeekNegGet

+


Suggested change

jorisdral · 2024-12-23T11:07:00Z

fs-sim/test/Test/System/FS/StateMachine.hs

+  -- Open with MustExist, but the file does not exist.
+  --
+  -- > DoesFileExist fp
+  -- > h <- Open fp (AppendMode _)
+  | TagAssumeExists


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

jorisdral · 2024-12-23T11:07:26Z

fs-sim/test/Test/System/FS/StateMachine.hs

@@ -1136,6 +1144,7 @@ tag = C.classify [
    , tagPutSeekGet Set.empty Set.empty
    , tagPutSeekNegGet Set.empty Set.empty
    , tagExclusiveFail
+--    , tagAssumeExistsFail -- Set.empty


Should be updated

jorisdral · 2024-12-23T11:07:37Z

fs-sim/test/Test/System/FS/StateMachine.hs

+{-      
+    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
+-}


Remove commented out code

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

c5ffc9c

recursion-ninja requested a review from a team as a code owner December 21, 2024 17:28

jorisdral requested changes Dec 23, 2024

View reviewed changes

jorisdral mentioned this pull request Dec 23, 2024

Document the semantics of OpenMode #86

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

recursion-ninja commented Dec 21, 2024

jorisdral left a comment

jorisdral Dec 23, 2024

jorisdral Dec 23, 2024 •

edited

Loading

jorisdral Dec 23, 2024

jorisdral Dec 23, 2024 •

edited

Loading

jorisdral Dec 23, 2024

jorisdral Dec 23, 2024

jorisdral Dec 23, 2024

jorisdral Dec 23, 2024

jorisdral Dec 23, 2024

jorisdral Dec 23, 2024

jorisdral Dec 23, 2024

jorisdral Dec 23, 2024

		when (openMode == ReadMode \|\| assumedExistance openMode) $ void $
		checkFsTree $ FS.getFile fp (mockFiles fs)

		@@ -1055,6 +1055,7 @@ data Tag =
		-- > Get ..
		\| TagPutSeekNegGet

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

Are you sure you want to change the base?

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

Conversation

recursion-ninja commented Dec 21, 2024

jorisdral left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisdral Dec 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisdral Dec 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorisdral Dec 23, 2024 •

edited

Loading

jorisdral Dec 23, 2024 •

edited

Loading