[io_file] Add the ability to get file metadata on Windows. #202

brianquinlan · 2025-03-17T17:42:44Z

Implements metadata fetching for Windows (equivalent to dart:io FileStat.stat)
I will reorganize the code and tests so that common concepts are in class Metadata when I implement the POSIX version.
Documentation is incomplete because I may move attributes around.
I'm not confident in the API design - in dart:io FileStat is not constructible but I'm making the Metadata constructor public, which means that it will be harder to change. But I was imagining that making it constructible would be useful in mocks.

TL;DR: The implementation should be correct but the API will change.

I’ve reviewed the contributor guide and applied the relevant portions to this PR.

Contribution guidelines:

See our contributor guide for general expectations for PRs.
Larger or significant changes should be discussed in an issue before creating a PR.
Contributions to our repos should follow the Dart style guide and use dart format.
Most changes should add an entry to the changelog and may need to rev the pubspec package version.
Changes to packages require corresponding tests.

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

brianquinlan · 2025-03-18T03:10:45Z

@halildurmus Ready for review!

halildurmus

LGTM

pkgs/io_file/lib/src/vm_windows_file_system.dart

brianquinlan · 2025-03-18T20:11:16Z

LGTM

Thank you!

lrhn

Looks good. My comments are mainly nits, "improvement suggestions", optimization suggestions (which can definitely be delayed until the API is complete) and some expected Windows quirkiness.

lrhn · 2025-03-19T10:13:10Z

pkgs/io_file/example/io_file_example.dart

+  if (fileSystem case final WindowsFileSystem fs) {
+    return fs.metadata(path).isHidden;
+  } else {
+    // On POSIX, convention is that files starting with a period are hidden


Maybe: "files" -> "files and directories"

lrhn · 2025-03-19T10:15:47Z

pkgs/io_file/lib/src/file_system.dart

+    final count = (isDirectory ? 1 : 0) + (isFile ? 1 : 0) + (isLink ? 1 : 0);
+    if (count > 1) {
+      // TODO(brianquinlan): Decide whether a path must be a a file, directory
+      // or link and whether it can be more than one of these at once.


Windows links are either file links or directory links. It makes sense that they can represent the difference.

Do you have to use WindowsMetadata below on Windows?
(That is: What is the use-cases for this class. Is it a default least-common-denominator of the platform specific metadata subclasses? Can users use it as input to any function of the file system, where they can fx. choose between passing in a Metadata or a WindowsMetadata to a WindowsFilesystem? If so, they can use this class on Windows. If not, does it need a public constructor?)

You don't have to use WindowsMetadata on Windows. The usual pattern of calling fileSystem.metadata(path) will return a Metadata, which should contain the metadata available on all platforms. WindowsMetadata is designed to capture the Windows platform-specific attributes.

The constructor is public because I want other people to be able to implement their own file system that return Metadata or a subclass.

Maybe I should make this an abstract interface? But then I won't be able to add to it in the future.

I'm not confident in my API design choices here :-(

pkgs/io_file/lib/src/file_system.dart

lrhn · 2025-03-19T10:18:09Z

pkgs/io_file/lib/src/file_system.dart

+  final bool isFile;
+  final bool isDirectory;
+  final bool isLink;
+  final int size;


(Totally over-optimizing, ... but you can store three bools in one int and save two storage slots. I don't trust our compilers to do that for you.)

I'll consider it ;-)

pkgs/io_file/lib/src/file_system.dart

lrhn · 2025-03-19T10:52:32Z

pkgs/io_file/example/io_file_example.dart

+import 'package:path/path.dart' as p;
+
+bool isHidden(String path) {
+  if (fileSystem case final WindowsFileSystem fs) {


(For future extension: The OSX file system has a hidden flag too, you can set it with chflags hidden /path/to/folder/. Not all tools recognize it - Finder does, ls doesn't. It's from BSD, so other Unixes might have it too. We should probably support it if possible/reasonable.)

Yeah, linux has a hidden flag too - it's just not used in many file systems. I plan on moving some of these attributes around when I do the POSIX stat implementation.

Given that the semantics probably are quite different it makes a lot of sense to not share the hidden flag between platforms though they are named the same...

lrhn · 2025-03-19T12:01:54Z

pkgs/io_file/lib/src/vm_windows_file_system.dart

+
+    final isDirectory = attributes & win32.FILE_ATTRIBUTE_DIRECTORY > 0;
+    final isLink = attributes & win32.FILE_ATTRIBUTE_REPARSE_POINT > 0;
+    final isFile = !(isDirectory || isLink);


If a link is a link to a directory (or to a link to a directory) I expect both of isDirectory and isLink to be true. (Can't check right now, no Windows machine.)

In that case isFile should probably just be !isDirectory (And you don't need to store it, you can just have bool get isFile => !isDiretory;.)

Or you could make isDirectory be (attributes & ...) & !isLink, and then have WindowsMetadata specific getters for isFileLink and/or isDirectoryLink, separate from isFile and isDirectory, so isFile, isDirectory and isLink are mutually exclusive again.

In Windows, the type of link is not indicated in GetFileAttributes

And yeah, there is some tension here. Right now I was going to make one of the is* true and the values would be consistent on POSIX and Windows.

I checked, and I think a reparse point having the DIRECTORY bit set controls whether it's a file or directory reparse point.
(It doesn't say whether it's a junction, symbolic link, or something more esoteric, I think you need a second query to get the reparse-data.)

Every entity in the Windows FS is either a file or a directory, including links, controlled by the DIRECTORY bit.
If it's a reparse-point, then it's also a link, so it can be link-and-file or link-and-directory.

What makes sense from an API POV? I think that no more than one of these should be set. And maybe it makes sense for zero of them to be set e.g. for a unix domain socket, a named pipe, etc.

pkgs/io_file/lib/src/vm_windows_file_system.dart

lrhn · 2025-03-19T13:19:59Z

pkgs/io_file/lib/src/file_system.dart

@@ -23,4 +61,12 @@ base class FileSystem {
  void rename(String oldPath, String newPath) {
    throw UnsupportedError('rename');
  }
+
+  /// Returns metadata for the given path.


The method has a noun-phrased word, so it's a parameterized getter. Document it the same way.
Drop the "Returns". (Never start a DartDoc with the word "Returns". )

Maybe just /// Metadata for the file or directory at [path].

Wrote as:

/// Metadata for the file system object at [path].

brianquinlan · 2025-03-22T00:11:55Z

@lrhn Please take another look when you get a chance.

lrhn · 2025-04-02T11:47:50Z

pkgs/io_file/lib/src/file_system.dart

+  /// If `path` represents a symbolic link then metadata for the link is
+  /// returned.
+  Metadata metadata(String path) {
+    throw UnsupportedError('metadata');


It's a valid design for a growing API, but also a potentially troublesome one.

It can become a problem if people can't trust that methods are implemented.

If anyone ever feels they need to write try { fs.something(); } on UnsupportedError { ... }, I'd say the design has failed.

Maybe some annotations can help ...
Maybe start with @subclassMustOverride ... something() => throw UnsupportedError("something not implemented"); and then, in a later version, change it to ... something();. But that's still a breaking change, so it'd deserve a major version increment.

And then one might as well just add something(); directly, in a new major version. If most users will just use the file systems supprted by default, and only a few rarely-used packages will try to implement FileSystem, then .... well, versioning is still hard.

MAYBE, just for complete pedantic adherence to semver:

Make FileSystem a final class.

Export FileSystemBase which implements FileSystem and is base from a different package intended for file system implementors (with a strict dependence on the FileSystem package it comes from).

Then FileSystem can add a new member in a minor version increment, because to users it's a minor version increment, but the FileSystemBase class package gets a major version increment.

So FS 3.5.0 adds a new member. FSB (base package) releases 35.0.0 supporting (and requiring) that member. If you want to use FS 3.5.0 for that new member, and your custom file system implementation doesn't have a version that works with FSB 35.0.0, it won't solve. You can use FS 3.4.0/FSB 34.0.0, but not the new member, or you can make CustomeFS release a version that implements FSB 35.0.0's FileSystem.

Not sure the complexity is worth the result. It'll still end up with failed resolution. The only benefit is that an incremental addition to the API isn't breaking (and does not require a major version increment, which is hard to push through if multiple packages use the library) for code that only uses the default implementation.

lrhn · 2025-04-02T12:03:48Z

pkgs/io_file/lib/src/vm_windows_file_system.dart

+    this.isReadOnly = false,
+    this.isHidden = false,
+    this.isSystem = false,
+    this.isArchive = false,


Adding is works for most things, but not all. One size doesn't fit all, as usual :)

I can see why "Flag" isn't great in the name. needsArchive, isArchiveReady or isArchiveable might work (if the last one is even a word). So stepping back ...

The read only flag means that the file "is read-only". ✓
The hidden flag means that the file "is hidden". ✓
The system flag means that the file "is (a) system (file)". ✓

The "archive" bit is really a "dirty" bit, ... and isDirty would be fine, it's just not the traditional name.
Phrased in terms of "archiving", it means "needs to be archived", "do archive" (matching the flag name) and "is not archived", which is the opposite of "is archived", and isArchive just doesn't work in that context.
We could use isArchived if we flip the value. It's my best suggestion so far.

if (!file.isArchived) copy(file, archivePathOf(file)); // ✓

lrhn · 2025-04-02T14:09:59Z

pkgs/io_file/lib/src/vm_windows_file_system.dart

+    this.isArchive = false,
+    this.isTemporary = false,
+    this.isOffline = false,
+    this.isContentNotIndexed = false,


I'd go with a good Dart API, with names that are relatable to the windows API, with documentation about the specific relation.

If we were just doing a Windows API, it would be an integer and bit flags.

So something like:

/// Whether the file is considered archived, and not in need of being archived. /// /// The Windows file system ARCHIVE flag (traditionally displayed as an "A" attribute) /// is set when the file needs archiving. /// This value expresses whether the ARCHIVE flag is unset. abstract final bool isArchived;

or

/// Whether the file needs to be archived. /// /// The Windows file system ARCHIVE flag (traditionally displayed as an "A" attribute) /// is set when the file needs archiving. /// This value expresses whether the ARCHIVE flag is set. abstract final bool isNotArchived;

(Heck, have both! It's not unheard of to have both isEmpty and isNotEmpty.)

lrhn · 2025-04-02T14:24:42Z

pkgs/io_file/lib/src/vm_windows_file_system.dart

+  /// Sets metadata for the file system entity.
+  ///
+  /// TODO(brianquinlan): Document the arguments.
+  void setMetadata(


Would it make any sense to take a Metadata argument that can be used as base, instead of fetching it from the file system.

It's not an atomic operation to setMetadata if it starts by reading metadata, does updating, and the writes it back. If it's not an atomic operation, it means the file system can change between fetching the existing attributes and writing them back.
Not being atomic is generally not a good thing for low-level functions.

It also means that if I first call metadata to look at the data, then decide to change the values using setMetadata, then I'll actually read the attributes twice.

If I could pass the WindowsMetadata I got from metadata as a base argument to setMetadata, so it uses that instead of fetching new, then the set operation becomes atomic. (The entire operation doesn't, but that's clear when I call two different methods.)

If the base argument is omitted, it can still fetch the attributes from the file system.

lrhn · 2025-04-02T14:26:55Z

pkgs/io_file/lib/src/vm_windows_file_system.dart

+      // `FILE_ATTRIBUTE_NORMAL` indicates that no other attributes are set and
+      // is valid only when used alone.
+      attributes = win32.FILE_ATTRIBUTE_NORMAL;
+    }


(Could we check here whether the value didn't change, and then not write anything?
So keep a copy of the original, and if (original == attributes) return;.
It's not the same - if the underlying file system has changed in the meantime, it matters whether we write or not. Especially if using a base metadata argument, then you should probably always write.)

I'm supporting base (called original) and always write.

lrhn · 2025-04-02T14:29:05Z

pkgs/io_file/lib/src/vm_windows_file_system.dart

+
+    final isDirectory = attributes & win32.FILE_ATTRIBUTE_DIRECTORY > 0;
+    final isLink = attributes & win32.FILE_ATTRIBUTE_REPARSE_POINT > 0;
+    final isFile = !(isDirectory || isLink);


I checked, and I think a reparse point having the DIRECTORY bit set controls whether it's a file or directory reparse point.
(It doesn't say whether it's a junction, symbolic link, or something more esoteric, I think you need a second query to get the reparse-data.)

Every entity in the Windows FS is either a file or a directory, including links, controlled by the DIRECTORY bit.
If it's a reparse-point, then it's also a link, so it can be link-and-file or link-and-directory.

lrhn · 2025-04-02T18:30:04Z

pkgs/io_file/lib/src/vm_windows_file_system.dart

+      isFile: isFile,
+      isOffline: attributes & win32.FILE_ATTRIBUTE_OFFLINE != 0,
+      isContentNotIndexed:
+          attributes & win32.FILE_ATTRIBUTE_NOT_CONTENT_INDEXED != 0,


(Could just store the attributes and compute the booleans on access.)

brianquinlan · 2025-04-17T00:49:47Z

Changed isArchive to needsArchive.
Changed isContentNotIndexed to isContentIndexed.

brianquinlan added 10 commits March 12, 2025 22:15

Add rename support for Windows

5db08c8

Update rename_test.dart

a6f5c1c

Some fixes

f89d063

Update vm_windows_file_system.dart

0f81c95

Update vm_windows_file_system.dart

ed98ec4

Test utils

2dfc83b

Metadata

8bf09fb

Set metadata

1dcd074

Update vm_windows_file_system.dart

2877b5e

Tests

7e4717d

brianquinlan marked this pull request as draft March 17, 2025 17:42

github-actions bot added the package:io_file label Mar 17, 2025

brianquinlan added 18 commits March 17, 2025 10:44

Merge branch 'main' into metadata

278de74

Fixes

69490d5

Windows metadata tests

9704766

More windows properties

36647c2

Update vm_windows_file_system.dart

a363b49

Test fixes

305655c

Update metadata_windows_test.dart

88dc69e

size

647a6a5

creation time

f7c6d76

not such path

dd67782

GetLastError

9185690

Update metadata_test.dart

a687b84

Times

e3fc41a

More fixes

309c186

More tests

2c51b7d

Update metadata_windows_test.dart

fa3e0a2

Docs

b287897

Update vm_windows_file_system.dart

b88bfca

Update vm_windows_file_system.dart

d14f1a9

brianquinlan marked this pull request as ready for review March 18, 2025 03:10

halildurmus approved these changes Mar 18, 2025

View reviewed changes

pkgs/io_file/lib/src/vm_windows_file_system.dart Outdated Show resolved Hide resolved

pkgs/io_file/lib/src/vm_windows_file_system.dart Show resolved Hide resolved

pkgs/io_file/lib/src/vm_windows_file_system.dart Show resolved Hide resolved

Code review updates

4d8ccfd

lrhn reviewed Mar 19, 2025

View reviewed changes

Review feedback.

1aeb152

brianquinlan added 7 commits March 27, 2025 09:43

Merge branch 'main' into metadata

4fd4da1

Add library

b6ffeff

Update file_system.dart

8c95248

Review comments

18e6f8b

Update file_system.dart

186c228

Update file_system_test.dart

7d11d36

Update vm_windows_file_system.dart

90d4070

lrhn reviewed Apr 3, 2025

View reviewed changes

brianquinlan added 4 commits April 3, 2025 16:12

Merge remote-tracking branch 'upstream/main' into pr/brianquinlan/202

c878448

Allow if original metadata

0188da6

Update vm_windows_file_system.dart

ea0946f

Fixes

e28568d

brianquinlan mentioned this pull request Apr 21, 2025

Add support for same on POSIX #211

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[io_file] Add the ability to get file metadata on Windows. #202

[io_file] Add the ability to get file metadata on Windows. #202

brianquinlan commented Mar 17, 2025 •

edited

Loading

brianquinlan commented Mar 18, 2025

halildurmus left a comment

brianquinlan commented Mar 18, 2025

lrhn left a comment

lrhn Mar 19, 2025

brianquinlan Mar 19, 2025

lrhn Mar 19, 2025

brianquinlan Mar 19, 2025

lrhn Mar 19, 2025

brianquinlan Mar 19, 2025

lrhn Mar 19, 2025

brianquinlan Mar 19, 2025

sigurdm Mar 24, 2025

lrhn Mar 19, 2025

brianquinlan Mar 19, 2025

lrhn Apr 2, 2025

brianquinlan Apr 4, 2025

lrhn Mar 19, 2025

brianquinlan Mar 19, 2025

brianquinlan commented Mar 22, 2025

lrhn Apr 2, 2025

lrhn Apr 2, 2025

lrhn Apr 2, 2025

lrhn Apr 2, 2025

brianquinlan Apr 4, 2025

lrhn Apr 2, 2025

brianquinlan Apr 4, 2025

lrhn Apr 2, 2025

lrhn Apr 2, 2025

brianquinlan Apr 17, 2025

brianquinlan commented Apr 17, 2025

[io_file] Add the ability to get file metadata on Windows. #202

Are you sure you want to change the base?

[io_file] Add the ability to get file metadata on Windows. #202

Conversation

brianquinlan commented Mar 17, 2025 • edited Loading

brianquinlan commented Mar 18, 2025

halildurmus left a comment

Choose a reason for hiding this comment

brianquinlan commented Mar 18, 2025

lrhn left a 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

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

brianquinlan commented Mar 22, 2025

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

Choose a reason for hiding this comment

brianquinlan commented Apr 17, 2025

brianquinlan commented Mar 17, 2025 •

edited

Loading