Implement webfs file/directory locator property (#4532)

jasnell · joshthoward · commit 7b1af5623ffa · 2025-08-07T13:38:16.000Z
diff --git a/src/workerd/api/filesystem.c++ b/src/workerd/api/filesystem.c++
@@ -2022,22 +2022,15 @@ jsg::Ref<jsg::DOMException> fsErrorToDomException(jsg::Lock& js, workerd::FsErro
 jsg::Promise<jsg::Ref<FileSystemDirectoryHandle>> StorageManager::getDirectory(
     jsg::Lock& js, const jsg::TypeHandler<jsg::Ref<jsg::DOMException>>& exception) {
   auto& vfs = workerd::VirtualFileSystem::current(js);
-  return js.resolvedPromise(js.alloc<FileSystemDirectoryHandle>(jsg::USVString(), vfs.getRoot(js)));
+  return js.resolvedPromise(js.alloc<FileSystemDirectoryHandle>(
+      KJ_ASSERT_NONNULL(jsg::Url::tryParse("file:///"_kj)), jsg::USVString(), vfs.getRoot(js)));
 }
 
 FileSystemDirectoryHandle::FileSystemDirectoryHandle(
-    jsg::USVString name, kj::Rc<workerd::Directory> inner)
-    : FileSystemHandle(kj::mv(name)),
+    jsg::Url locator, jsg::USVString name, kj::Rc<workerd::Directory> inner)
+    : FileSystemHandle(kj::mv(locator), kj::mv(name)),
       inner(kj::mv(inner)) {}
 
-jsg::Promise<bool> FileSystemDirectoryHandle::isSameEntry(
-    jsg::Lock& js, jsg::Ref<FileSystemHandle> other) {
-  KJ_IF_SOME(other, kj::dynamicDowncastIfAvailable<FileSystemDirectoryHandle>(*other)) {
-    return js.resolvedPromise(other.inner.get() == inner.get());
-  }
-  return js.resolvedPromise(false);
-}
-
 jsg::Promise<jsg::Ref<FileSystemFileHandle>> FileSystemDirectoryHandle::getFileHandle(jsg::Lock& js,
     jsg::USVString name,
     jsg::Optional<FileSystemGetFileOptions> options,
@@ -2057,7 +2050,9 @@ jsg::Promise<jsg::Ref<FileSystemFileHandle>> FileSystemDirectoryHandle::getFileH
           })) {
     KJ_SWITCH_ONEOF(node) {
       KJ_CASE_ONEOF(file, kj::Rc<workerd::File>) {
-        return js.resolvedPromise(js.alloc<FileSystemFileHandle>(kj::mv(name), kj::mv(file)));
+        auto locator = KJ_ASSERT_NONNULL(getLocator().tryResolve(name));
+        return js.resolvedPromise(
+            js.alloc<FileSystemFileHandle>(kj::mv(locator), kj::mv(name), kj::mv(file)));
       }
       KJ_CASE_ONEOF(dir, kj::Rc<workerd::Directory>) {
         auto ex =
@@ -2100,7 +2095,9 @@ jsg::Promise<jsg::Ref<FileSystemDirectoryHandle>> FileSystemDirectoryHandle::get
           })) {
     KJ_SWITCH_ONEOF(node) {
       KJ_CASE_ONEOF(dir, kj::Rc<workerd::Directory>) {
-        return js.resolvedPromise(js.alloc<FileSystemDirectoryHandle>(kj::mv(name), kj::mv(dir)));
+        auto locator = KJ_ASSERT_NONNULL(getLocator().tryResolve(kj::str(name, "/")));
+        return js.resolvedPromise(
+            js.alloc<FileSystemDirectoryHandle>(kj::mv(locator), kj::mv(name), kj::mv(dir)));
       }
       KJ_CASE_ONEOF(dir, kj::Rc<workerd::File>) {
         auto ex = js.domException(kj::str("TypeMismatchError"), kj::str("File name is a file"));
@@ -2158,17 +2155,19 @@ jsg::Promise<kj::Array<jsg::USVString>> FileSystemDirectoryHandle::resolve(
 
 namespace {
 kj::Array<jsg::Ref<FileSystemHandle>> collectEntries(
-    jsg::Lock& js, kj::Rc<workerd::Directory>& inner) {
+    jsg::Lock& js, kj::Rc<workerd::Directory>& inner, const jsg::Url& parentLocator) {
   kj::Vector<jsg::Ref<FileSystemHandle>> entries;
   for (auto& entry: *inner.get()) {
     KJ_SWITCH_ONEOF(entry.value) {
       KJ_CASE_ONEOF(file, kj::Rc<workerd::File>) {
-        entries.add(
-            js.alloc<FileSystemFileHandle>(js.accountedUSVString(entry.key), file.addRef()));
+        auto locator = KJ_ASSERT_NONNULL(parentLocator.tryResolve(entry.key));
+        entries.add(js.alloc<FileSystemFileHandle>(
+            kj::mv(locator), js.accountedUSVString(entry.key), file.addRef()));
       }
       KJ_CASE_ONEOF(dir, kj::Rc<workerd::Directory>) {
-        entries.add(
-            js.alloc<FileSystemDirectoryHandle>(js.accountedUSVString(entry.key), dir.addRef()));
+        auto locator = KJ_ASSERT_NONNULL(parentLocator.tryResolve(kj::str(entry.key, "/")));
+        entries.add(js.alloc<FileSystemDirectoryHandle>(
+            kj::mv(locator), js.accountedUSVString(entry.key), dir.addRef()));
       }
       KJ_CASE_ONEOF(link, kj::Rc<workerd::SymbolicLink>) {
         SymbolicLinkRecursionGuardScope guardScope;
@@ -2179,12 +2178,14 @@ kj::Array<jsg::Ref<FileSystemHandle>> collectEntries(
         KJ_IF_SOME(res, link->resolve(js)) {
           KJ_SWITCH_ONEOF(res) {
             KJ_CASE_ONEOF(file, kj::Rc<workerd::File>) {
-              entries.add(
-                  js.alloc<FileSystemFileHandle>(js.accountedUSVString(entry.key), file.addRef()));
+              auto locator = KJ_ASSERT_NONNULL(parentLocator.tryResolve(entry.key));
+              entries.add(js.alloc<FileSystemFileHandle>(
+                  kj::mv(locator), js.accountedUSVString(entry.key), file.addRef()));
             }
             KJ_CASE_ONEOF(dir, kj::Rc<workerd::Directory>) {
+              auto locator = KJ_ASSERT_NONNULL(parentLocator.tryResolve(kj::str(entry.key, "/")));
               entries.add(js.alloc<FileSystemDirectoryHandle>(
-                  js.accountedUSVString(entry.key), dir.addRef()));
+                  kj::mv(locator), js.accountedUSVString(entry.key), dir.addRef()));
             }
             KJ_CASE_ONEOF(_, workerd::FsError) {
               JSG_FAIL_REQUIRE(DOMOperationError, "Symbolic link recursion detected"_kj);
@@ -2200,16 +2201,16 @@ kj::Array<jsg::Ref<FileSystemHandle>> collectEntries(
 
 jsg::Ref<FileSystemDirectoryHandle::EntryIterator> FileSystemDirectoryHandle::entries(
     jsg::Lock& js) {
-  return js.alloc<EntryIterator>(IteratorState(JSG_THIS, collectEntries(js, inner)));
+  return js.alloc<EntryIterator>(IteratorState(JSG_THIS, collectEntries(js, inner, getLocator())));
 }
 
 jsg::Ref<FileSystemDirectoryHandle::KeyIterator> FileSystemDirectoryHandle::keys(jsg::Lock& js) {
-  return js.alloc<KeyIterator>(IteratorState(JSG_THIS, collectEntries(js, inner)));
+  return js.alloc<KeyIterator>(IteratorState(JSG_THIS, collectEntries(js, inner, getLocator())));
 }
 
 jsg::Ref<FileSystemDirectoryHandle::ValueIterator> FileSystemDirectoryHandle::values(
     jsg::Lock& js) {
-  return js.alloc<ValueIterator>(IteratorState(JSG_THIS, collectEntries(js, inner)));
+  return js.alloc<ValueIterator>(IteratorState(JSG_THIS, collectEntries(js, inner, getLocator())));
 }
 
 void FileSystemDirectoryHandle::forEach(jsg::Lock& js,
@@ -2225,23 +2226,16 @@ void FileSystemDirectoryHandle::forEach(jsg::Lock& js,
   }
   callback.setReceiver(js.v8Ref(receiver));
 
-  for (auto& entry: collectEntries(js, inner)) {
+  for (auto& entry: collectEntries(js, inner, getLocator())) {
     callback(js, js.accountedUSVString(entry->getName(js)), entry.addRef(), JSG_THIS);
   }
 }
 
-FileSystemFileHandle::FileSystemFileHandle(jsg::USVString name, kj::Rc<workerd::File> inner)
-    : FileSystemHandle(kj::mv(name)),
+FileSystemFileHandle::FileSystemFileHandle(
+    jsg::Url locator, jsg::USVString name, kj::Rc<workerd::File> inner)
+    : FileSystemHandle(kj::mv(locator), kj::mv(name)),
       inner(kj::mv(inner)) {}
 
-jsg::Promise<bool> FileSystemFileHandle::isSameEntry(
-    jsg::Lock& js, jsg::Ref<FileSystemHandle> other) {
-  KJ_IF_SOME(other, kj::dynamicDowncastIfAvailable<FileSystemFileHandle>(*other)) {
-    return js.resolvedPromise(other.inner.get() == inner.get());
-  }
-  return js.resolvedPromise(false);
-}
-
 jsg::Promise<jsg::Ref<File>> FileSystemFileHandle::getFile(
     jsg::Lock& js, const jsg::TypeHandler<jsg::Ref<jsg::DOMException>>& deHandler) {
   // TODO(node-fs): Currently this copies the file data into the new File object.
diff --git a/src/workerd/api/filesystem.h b/src/workerd/api/filesystem.h
@@ -270,7 +270,10 @@ class FileSystemHandle: public jsg::Object {
   // file handle. It would still likely be useful tho. As a follow up
   // step, we will implement serialization/deserialization of these objects.
  public:
-  FileSystemHandle(jsg::USVString name): name(kj::mv(name)) {}
+  FileSystemHandle(jsg::Url&& locator, jsg::USVString name)
+      : locator(kj::mv(locator)),
+        name(kj::mv(name)) {}
+
   const jsg::USVString& getName(jsg::Lock& js) {
     return name;
   }
@@ -279,9 +282,16 @@ class FileSystemHandle: public jsg::Object {
     // Implemented by subclasses.
     KJ_UNIMPLEMENTED("getKind() not implemented");
   }
-  virtual jsg::Promise<bool> isSameEntry(jsg::Lock& js, jsg::Ref<FileSystemHandle> other) {
-    // Implemented by subclasses.
-    KJ_UNIMPLEMENTED("isSameEntry() not implemented");
+
+  jsg::Promise<bool> isSameEntry(jsg::Lock& js, jsg::Ref<FileSystemHandle> other) {
+    // Per the spec, two handles are the same if they refer to the same entry (that is,
+    // have the same locator). It does not matter if they are different actual entries.
+    return locator.equal(other->getLocator(),
+               jsg::Url::EquivalenceOption::IGNORE_FRAGMENTS |
+                   jsg::Url::EquivalenceOption::IGNORE_SEARCH |
+                   jsg::Url::EquivalenceOption::NORMALIZE_PATH)
+        ? js.resolvedPromise(true)
+        : js.resolvedPromise(false);
   }
 
   JSG_RESOURCE_TYPE(FileSystemHandle) {
@@ -290,20 +300,23 @@ class FileSystemHandle: public jsg::Object {
     JSG_METHOD(isSameEntry);
   }
 
+  const jsg::Url& getLocator() const {
+    return locator;
+  }
+
  private:
+  const jsg::Url locator;
   jsg::USVString name;
 };
 
 class FileSystemFileHandle: public FileSystemHandle {
  public:
-  FileSystemFileHandle(jsg::USVString name, kj::Rc<workerd::File> inner);
+  FileSystemFileHandle(jsg::Url locator, jsg::USVString name, kj::Rc<workerd::File> inner);
 
   kj::StringPtr getKind(jsg::Lock& js) override {
     return "file"_kj;
   }
 
-  jsg::Promise<bool> isSameEntry(jsg::Lock& js, jsg::Ref<FileSystemHandle> other) override;
-
   struct FileSystemCreateWritableOptions {
     jsg::Optional<bool> keepExistingData;
     JSG_STRUCT(keepExistingData);
@@ -379,14 +392,13 @@ class FileSystemDirectoryHandle final: public FileSystemHandle {
   };
 
  public:
-  FileSystemDirectoryHandle(jsg::USVString name, kj::Rc<workerd::Directory> inner);
+  FileSystemDirectoryHandle(
+      jsg::Url locator, jsg::USVString name, kj::Rc<workerd::Directory> inner);
 
   kj::StringPtr getKind(jsg::Lock& js) override {
     return "directory"_kj;
   }
 
-  jsg::Promise<bool> isSameEntry(jsg::Lock& js, jsg::Ref<FileSystemHandle> other) override;
-
   struct FileSystemGetFileOptions {
     bool create = false;
     JSG_STRUCT(create);
diff --git a/src/wpt/fs-test.ts b/src/wpt/fs-test.ts
@@ -6,7 +6,10 @@ const root = await navigator.storage.getDirectory();
 const tmp = await root.getDirectoryHandle('tmp');
 // The root is read-only, so we need to use a writable subdirectory
 // to actually run the tests.
-navigator.storage.getDirectory = async () => tmp;
+navigator.storage.getDirectory = async () => {
+  // Let's create a new random tmp subdirectory for each test run to avoid interference
+  return tmp.getDirectoryHandle(crypto.randomUUID(), { create: true });
+};
 
 export default {
   'FileSystemBaseHandle-buckets.https.any.js': {
@@ -23,12 +26,7 @@ export default {
   },
   'FileSystemBaseHandle-isSameEntry.https.any.js': {
     comment: '...',
-    expectedFailures: [
-      // TODO(node-fs): Fix these tests
-      'isSameEntry comparing two files pointing to the same path returns true',
-      'isSameEntry comparing two directories pointing to the same path returns true',
-      'isSameEntry comparing a file to a directory of the same path returns false',
-    ],
+    expectedFailures: [],
   },
   'FileSystemBaseHandle-postMessage-BroadcastChannel.https.window.js': {
     comment: 'BroadcastChannel is not implemented in workers',
@@ -81,7 +79,6 @@ export default {
     comment: '...',
     expectedFailures: [
       // TODO(node-fs): Fix these tests
-      'getDirectoryHandle(create=true) creates an empty directory',
       'getDirectoryHandle() with empty name',
       'getDirectoryHandle() with "." name',
       'getDirectoryHandle() with ".." name',
@@ -115,9 +112,7 @@ export default {
       // TODO(node-fs): Fix these tests
       'removeEntry() to remove a file',
       'removeEntry() on an already removed file should fail',
-      'removeEntry() to remove an empty directory',
       'removeEntry() on a non-empty directory should fail',
-      'removeEntry() on a directory recursively should delete all sub-items',
       'removeEntry() with empty name should fail',
       'removeEntry() with "." name should fail',
       'removeEntry() with ".." name should fail',
