devtools_shared: Use the file package for a file system abstraction for testing (#9872)
diff --git a/packages/devtools_app_shared/pubspec.yaml b/packages/devtools_app_shared/pubspec.yaml
index 15b3067..4ed27ca 100644
--- a/packages/devtools_app_shared/pubspec.yaml
+++ b/packages/devtools_app_shared/pubspec.yaml
@@ -15,7 +15,7 @@
dependencies:
collection: ^1.15.0
dds_service_extensions: ^2.0.0
- devtools_shared: ^13.0.0
+ devtools_shared: ^14.0.0-wip
dtd: ^4.0.0
flutter:
sdk: flutter
diff --git a/packages/devtools_extensions/pubspec.yaml b/packages/devtools_extensions/pubspec.yaml
index 7135b64..a23c14d 100644
--- a/packages/devtools_extensions/pubspec.yaml
+++ b/packages/devtools_extensions/pubspec.yaml
@@ -18,7 +18,7 @@
dependencies:
args: ^2.4.2
- devtools_shared: ^13.0.0
+ devtools_shared: ^14.0.0-wip
devtools_app_shared: ^0.5.1
flutter:
sdk: flutter
diff --git a/packages/devtools_shared/CHANGELOG.md b/packages/devtools_shared/CHANGELOG.md
index eaf9a53..a3c2483 100644
--- a/packages/devtools_shared/CHANGELOG.md
+++ b/packages/devtools_shared/CHANGELOG.md
@@ -3,8 +3,33 @@
Use of this source code is governed by a BSD-style license that can be
found in the LICENSE file or at https://developers.google.com/open-source/licenses/bsd.
-->
+# 14.0.0-wip
+
+* **Breaking changes**: `LocalFileSystem`, an extension which provided some handy
+ helpers, has been refactored into an extension on the `file` package's
+ `FileSystem` abstraction. In detail:
+ * `fileSystem` is a new top-level constant which represents the local (real)
+ file system.
+ * `LocalFileSystem.devToolsDir()` is now a static getter,
+ `FileSystemExtension.devToolsDir`.
+ * `LocalFileSystem.maybeMoveLegacyDevToolsStore()` is now an instance method,
+ `FileSystemExtension.maybeMoveLegacyDevToolsStore()`.
+ * `LocalFileSystem.devToolsStoreLocation()` is now a static getter,
+ `FileSystemExtension.devToolsStoreLocation`.
+ * `LocalFileSystem.ensureDevToolsDirectory()` is now private.
+ * `LocalFileSystem.devToolsFileFromPath()` is now an instance method,
+ `FileSystemExtension.devToolsFileFromPath()`.
+ * `LocalFileSystem.devToolsFileAsJson()` is now an instance method,
+ `FileSystemExtension.devToolsFileAsJson()`.
+ * `LocalFileSystem.flutterStoreExists()` is now an instance getter,
+ `FileSystemExtension.flutterStoreExists`.
+ * `IOPersistentProperties.new` accepts a new optional `FileSystem fs`
+ argument.
+* Update `LocalFileSystem` and `IOPersistentProperties` to use `package:file`
+ instead of `dart:io` to allow mocking the file system.
# 13.0.2
+
* Validate the `devtoolsOptionsUri` query parameter in the extension enabled
state handler so it must be a `file:` URI named `devtools_options.yaml`,
preventing arbitrary file writes by the DevTools server process.
diff --git a/packages/devtools_shared/lib/src/server/devtools_store.dart b/packages/devtools_shared/lib/src/server/devtools_store.dart
index 5efdf5d..5df2442 100644
--- a/packages/devtools_shared/lib/src/server/devtools_store.dart
+++ b/packages/devtools_shared/lib/src/server/devtools_store.dart
@@ -21,10 +21,11 @@
/// Provides access to the local DevTools store (~/.flutter-devtools/.devtools).
class DevToolsUsage {
DevToolsUsage() {
- LocalFileSystem.maybeMoveLegacyDevToolsStore();
+ // TODO(srawlins): Accept a FileSystem parameter during tests.
+ fileSystem.maybeMoveLegacyDevToolsStore();
properties = IOPersistentProperties(
storeName,
- documentDirPath: LocalFileSystem.devToolsDir(),
+ documentDirPath: FileSystemExtension.devToolsDir,
);
_removeLegacyKeys();
}
diff --git a/packages/devtools_shared/lib/src/server/file_system.dart b/packages/devtools_shared/lib/src/server/file_system.dart
index 6a2036f..450790b 100644
--- a/packages/devtools_shared/lib/src/server/file_system.dart
+++ b/packages/devtools_shared/lib/src/server/file_system.dart
@@ -3,108 +3,117 @@
// found in the LICENSE file or at https://developers.google.com/open-source/licenses/bsd.
import 'dart:convert';
-import 'dart:io';
+import 'dart:io' as io show Platform, File;
+import 'package:file/file.dart';
+import 'package:file/local.dart';
import 'package:path/path.dart' as path;
import 'devtools_store.dart';
-/// A namespace local file system utlities.
-extension LocalFileSystem on Never {
- static String _userHomeDir() {
- final envKey = Platform.operatingSystem == 'windows' ? 'APPDATA' : 'HOME';
- return Platform.environment[envKey] ?? '.';
+/// The real, local file system, which can be avoided in tests.
+const FileSystem fileSystem = LocalFileSystem();
+
+extension FileSystemExtension on FileSystem {
+ static String get _userHomeDir {
+ final envKey = io.Platform.operatingSystem == 'windows'
+ ? 'APPDATA'
+ : 'HOME';
+ return io.Platform.environment[envKey] ?? '.';
}
- /// Returns the path to the DevTools storage directory.
- static String devToolsDir() {
- return path.join(_userHomeDir(), '.flutter-devtools');
+ /// The path to the DevTools storage directory.
+ static String get devToolsDir {
+ return path.join(_userHomeDir, '.flutter-devtools');
}
- /// Moves the .devtools file to ~/.flutter-devtools/.devtools if the .devtools
- /// file exists in the user's home directory.
- static void maybeMoveLegacyDevToolsStore() {
- final file = File(path.join(_userHomeDir(), DevToolsUsage.storeName));
- if (file.existsSync()) {
- ensureDevToolsDirectory();
- file.copySync(devToolsStoreLocation());
- file.deleteSync();
+ /// Moves the `.devtools` file to `~/.flutter-devtools/.devtools` if the
+ /// `.devtools` file exists in the user's home directory.
+ void maybeMoveLegacyDevToolsStore() {
+ final storeFile = file(path.join(_userHomeDir, DevToolsUsage.storeName));
+ if (storeFile.existsSync()) {
+ _ensureDevToolsDirectory();
+ storeFile.copySync(devToolsStoreLocation);
+ storeFile.deleteSync();
}
}
- static String devToolsStoreLocation() {
- return path.join(devToolsDir(), DevToolsUsage.storeName);
+ static String get devToolsStoreLocation {
+ return path.join(devToolsDir, DevToolsUsage.storeName);
}
- /// Creates the ~/.flutter-devtools directory if it does not already exist.
- static void ensureDevToolsDirectory() {
- Directory(devToolsDir()).createSync();
+ /// Creates the `~/.flutter-devtools` directory if it does not already exist.
+ void _ensureDevToolsDirectory() {
+ directory(devToolsDir).createSync();
}
/// Returns a DevTools file from the given path.
///
/// Only files within ~/.flutter-devtools/ can be accessed.
- static File? devToolsFileFromPath(String pathFromDevToolsDir) {
- if (pathFromDevToolsDir.contains('..') ||
- path.isAbsolute(pathFromDevToolsDir)) {
+ File? devToolsFileFromPath(String relativePath) {
+ if (relativePath.contains('..') || path.isAbsolute(relativePath)) {
// The passed in path should not be able to walk up the directory tree
- // outside of the ~/.flutter-devtools/ directory. It must also not be an
- // absolute path: path.join() discards the base directory when its second
- // argument is absolute, which would otherwise allow reading an arbitrary
- // file on disk (e.g. an absolute path to a credentials .json file).
+ // outside of the `~/.flutter-devtools/` directory. It must also not be an
+ // absolute path: `path.join()` discards the base directory when its
+ // second argument is absolute, which would otherwise allow reading an
+ // arbitrary file on disk (e.g. an absolute path to a credentials `.json`
+ // file).
return null;
}
- ensureDevToolsDirectory();
- final devToolsDirPath = devToolsDir();
- final file = File(path.join(devToolsDirPath, pathFromDevToolsDir));
+ _ensureDevToolsDirectory();
+ final targetFile = file(path.join(devToolsDir, relativePath));
// Defense in depth: ensure the resolved path is actually contained within
// the DevTools directory.
- if (!path.isWithin(devToolsDirPath, file.path)) {
- return null;
- }
- if (!file.existsSync()) {
- return null;
- }
- return file;
+ if (!path.isWithin(devToolsDir, targetFile.path)) return null;
+ if (!targetFile.existsSync()) return null;
+ return targetFile;
}
- /// Returns a DevTools file from the given path as encoded json.
+ /// Returns a DevTools file from the given path as encoded JSON.
///
- /// Only files within ~/.flutter-devtools/ can be accessed.
- static String? devToolsFileAsJson(String pathFromDevToolsDir) {
- final file = devToolsFileFromPath(pathFromDevToolsDir);
- if (file == null) return null;
+ /// Only files within `~/.flutter-devtools/` can be accessed.
+ String? devToolsFileAsJson(String relativePath) {
+ final targetFile = devToolsFileFromPath(relativePath);
+ if (targetFile == null) return null;
- final fileName = path.basename(file.path);
+ final fileName = path.basename(targetFile.path);
if (!fileName.endsWith('.json')) return null;
- final content = file.readAsStringSync();
+ final content = targetFile.readAsStringSync();
final json = jsonDecode(content) as Map;
- json['lastModifiedTime'] = file.lastModifiedSync().toString();
+ json['lastModifiedTime'] = targetFile.lastModifiedSync().toString();
return jsonEncode(json);
}
/// Whether the flutter store file exists.
- static bool flutterStoreExists() {
- final flutterStore = File(path.join(_userHomeDir(), '.flutter'));
+ bool get flutterStoreExists {
+ final flutterStore = file(path.join(_userHomeDir, '.flutter'));
return flutterStore.existsSync();
}
}
class IOPersistentProperties {
- IOPersistentProperties(this.name, {String? documentDirPath}) {
+ IOPersistentProperties(
+ this.name, {
+ String? documentDirPath,
+ FileSystem fs = fileSystem,
+ }) : _fs = fs {
final fileName = name.replaceAll(' ', '_');
- documentDirPath ??= LocalFileSystem._userHomeDir();
- _file = File(path.join(documentDirPath, fileName));
+ documentDirPath ??= FileSystemExtension._userHomeDir;
+ _file = _fs.file(path.join(documentDirPath, fileName));
if (!_file.existsSync()) {
_file.createSync(recursive: true);
}
syncSettings();
}
- IOPersistentProperties.fromFile(File file) : name = path.basename(file.path) {
- _file = file;
+ // TODO(srawlins): This is unused in any devtools code. Even in tests. Either
+ // test it, if it is needed by users, or remove it.
+ IOPersistentProperties.fromFile(io.File file)
+ : name = path.basename(file.path),
+ _fs = file is File ? file.fileSystem : fileSystem {
+ _file = file is File ? file : _fs.file(file.path);
if (!_file.existsSync()) {
_file.createSync(recursive: true);
}
@@ -113,7 +122,9 @@
final String name;
- late File _file;
+ final FileSystem _fs;
+
+ late final File _file;
late Map<String, Object?> _map;
diff --git a/packages/devtools_shared/lib/src/server/handlers/_app_size.dart b/packages/devtools_shared/lib/src/server/handlers/_app_size.dart
index f7f31a5..f6e61dc 100644
--- a/packages/devtools_shared/lib/src/server/handlers/_app_size.dart
+++ b/packages/devtools_shared/lib/src/server/handlers/_app_size.dart
@@ -19,7 +19,7 @@
if (missingRequiredParams != null) return missingRequiredParams;
final filePath = queryParams[AppSizeApi.baseAppSizeFilePropertyName]!;
- final fileJson = LocalFileSystem.devToolsFileAsJson(filePath);
+ final fileJson = fileSystem.devToolsFileAsJson(filePath);
if (fileJson == null) {
return api.badRequest('No JSON file available at $filePath.');
}
@@ -39,7 +39,7 @@
if (missingRequiredParams != null) return missingRequiredParams;
final filePath = queryParams[AppSizeApi.testAppSizeFilePropertyName]!;
- final fileJson = LocalFileSystem.devToolsFileAsJson(filePath);
+ final fileJson = fileSystem.devToolsFileAsJson(filePath);
if (fileJson == null) {
return api.badRequest('No JSON file available at $filePath.');
}
diff --git a/packages/devtools_shared/lib/src/server/server_api.dart b/packages/devtools_shared/lib/src/server/server_api.dart
index cc96620..b4d6b7e 100644
--- a/packages/devtools_shared/lib/src/server/server_api.dart
+++ b/packages/devtools_shared/lib/src/server/server_api.dart
@@ -72,9 +72,7 @@
// ----- Flutter Tool GA store. -----
case apiGetFlutterGAClientId:
return _encodeResponse(
- LocalFileSystem.flutterStoreExists()
- ? _flutterStore.flutterClientId
- : '',
+ fileSystem.flutterStoreExists ? _flutterStore.flutterClientId : '',
api: api,
);
@@ -223,16 +221,16 @@
: null;
}
- /// Accessing DevTools store file e.g., ~/.flutter-devtools/.devtools
+ /// Accessing DevTools store file e.g., `~/.flutter-devtools/.devtools`.
static final _devToolsStore = DevToolsUsage();
- /// Accessing Flutter store file e.g., ~/.flutter
+ /// Accessing Flutter store file e.g., `~/.flutter`.
static final _flutterStore = FlutterStore();
static DevToolsUsage get devToolsPreferences => _devToolsStore;
/// Provides read and write access to DevTools options files
- /// (e.g. path/to/app/root/devtools_options.yaml).
+ /// (e.g. `path/to/app/root/devtools_options.yaml`).
static final _devToolsOptions = DevToolsOptions();
/// Logs a page view in the DevTools server.
diff --git a/packages/devtools_shared/pubspec.yaml b/packages/devtools_shared/pubspec.yaml
index 332adaa..781cf03 100644
--- a/packages/devtools_shared/pubspec.yaml
+++ b/packages/devtools_shared/pubspec.yaml
@@ -4,7 +4,7 @@
name: devtools_shared
description: Package of shared Dart structures between devtools_app, dds, and other tools.
-version: 13.0.2
+version: 14.0.0-wip
repository: https://github.com/flutter/devtools/tree/master/packages/devtools_shared
@@ -18,6 +18,7 @@
collection: ^1.15.0
dtd: ^4.0.0
extension_discovery: ^2.1.0
+ file: ^7.0.0
meta: ^1.9.1
path: ^1.8.0
shelf: ^1.1.0
diff --git a/packages/devtools_shared/test/server/file_system_test.dart b/packages/devtools_shared/test/server/file_system_test.dart
index 78069ec..b7c2a0d 100644
--- a/packages/devtools_shared/test/server/file_system_test.dart
+++ b/packages/devtools_shared/test/server/file_system_test.dart
@@ -2,35 +2,110 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file or at https://developers.google.com/open-source/licenses/bsd.
-import 'package:devtools_shared/src/server/file_system.dart';
+import 'dart:convert';
+
+import 'package:devtools_shared/src/server/file_system.dart' hide fileSystem;
+import 'package:file/memory.dart';
+import 'package:path/path.dart' as path;
import 'package:test/test.dart';
void main() {
- group('LocalFileSystem.devToolsFileFromPath path validation', () {
- // These inputs must be rejected before any filesystem access so that reads
- // stay confined to the ~/.flutter-devtools/ directory.
+ group('LocalFileSystem.devToolsFileFromPath', () {
+ late MemoryFileSystem fs;
- test('rejects absolute paths', () {
- // path.join() discards the base directory when its second argument is
- // absolute, so an absolute path would otherwise escape the DevTools
- // directory and read an arbitrary file on disk.
- expect(LocalFileSystem.devToolsFileFromPath('/etc/passwd'), isNull);
- expect(
- LocalFileSystem.devToolsFileFromPath('/absolute/path/to/file.json'),
- isNull,
- );
+ setUp(() {
+ fs = MemoryFileSystem();
+ fs
+ .directory(FileSystemExtension.devToolsDir)
+ .parent
+ .createSync(recursive: true);
});
- test('rejects paths containing ".."', () {
- expect(LocalFileSystem.devToolsFileFromPath('..'), isNull);
- expect(
- LocalFileSystem.devToolsFileFromPath('../../../etc/passwd'),
- isNull,
+ group('path validation', () {
+ // These inputs must be rejected before any filesystem access so that reads
+ // stay confined to the ~/.flutter-devtools/ directory.
+
+ test('rejects absolute paths', () {
+ final root = path.style == .windows ? 'C:\\' : '/';
+ // `path.join()` discards the base directory when its second argument is
+ // absolute, so an absolute path would otherwise escape the DevTools
+ // directory and read an arbitrary file on disk.
+ expect(
+ fs.devToolsFileFromPath(path.join('${root}etc', 'passwd')),
+ isNull,
+ );
+ expect(
+ fs.devToolsFileFromPath(
+ path.join('${root}absolute', 'path', 'to', 'file.json'),
+ ),
+ isNull,
+ );
+ });
+
+ test('rejects paths containing ".."', () {
+ expect(fs.devToolsFileFromPath('..'), isNull);
+ expect(
+ fs.devToolsFileFromPath(path.join('..', '..', '..', 'etc', 'passwd')),
+ isNull,
+ );
+ expect(
+ fs.devToolsFileFromPath(
+ path.join('subdir', '..', '..', 'escape.json'),
+ ),
+ isNull,
+ );
+ });
+ });
+
+ test('returns file when path is valid and file exists', () {
+ final testFile = fs.file(
+ path.join(FileSystemExtension.devToolsDir, 'sub', 'file.json'),
+ )..createSync(recursive: true);
+
+ final file = fs.devToolsFileFromPath(path.join('sub', 'file.json'))!;
+ expect(file.path, testFile.path);
+ });
+
+ test('returns null when path is valid but file does not exist', () {
+ final file = fs.devToolsFileFromPath(
+ path.join('sub', 'non_existent.json'),
);
- expect(
- LocalFileSystem.devToolsFileFromPath('subdir/../../escape.json'),
- isNull,
- );
+ expect(file, isNull);
+ });
+ });
+
+ group('LocalFileSystem.devToolsFileAsJson', () {
+ late MemoryFileSystem fs;
+
+ setUp(() {
+ fs = MemoryFileSystem();
+ fs
+ .directory(FileSystemExtension.devToolsDir)
+ .parent
+ .createSync(recursive: true);
+ });
+
+ test('returns null if file does not exist', () {
+ expect(fs.devToolsFileAsJson('test.json'), isNull);
+ });
+
+ test('returns null if file is not json', () {
+ fs.file(path.join(FileSystemExtension.devToolsDir, 'test.txt'))
+ ..createSync(recursive: true)
+ ..writeAsStringSync('hello');
+ expect(fs.devToolsFileAsJson('test.txt'), isNull);
+ });
+
+ test('returns json content with lastModifiedTime', () {
+ final file = fs.file(
+ path.join(FileSystemExtension.devToolsDir, 'test.json'),
+ )..createSync(recursive: true);
+ file.writeAsStringSync('{"key": "value"}');
+
+ final jsonStr = fs.devToolsFileAsJson('test.json')!;
+ final json = jsonDecode(jsonStr) as Map;
+ expect(json['key'], 'value');
+ expect(json['lastModifiedTime'], file.lastModifiedSync().toString());
});
});
}
diff --git a/packages/devtools_shared/test/server/persistent_properties_test.dart b/packages/devtools_shared/test/server/persistent_properties_test.dart
index 4f78b19..fd337ef 100644
--- a/packages/devtools_shared/test/server/persistent_properties_test.dart
+++ b/packages/devtools_shared/test/server/persistent_properties_test.dart
@@ -3,36 +3,37 @@
// found in the LICENSE file or at https://developers.google.com/open-source/licenses/bsd.
import 'dart:convert';
-import 'dart:io';
import 'package:devtools_shared/src/server/file_system.dart';
+import 'package:file/file.dart';
+import 'package:file/memory.dart';
+import 'package:path/path.dart' as path;
import 'package:test/test.dart';
void main() {
group('IOPersistentProperties', () {
+ late MemoryFileSystem fs;
late Directory tempDir;
late IOPersistentProperties properties;
const storeName = 'test_store';
setUp(() {
- tempDir = Directory.systemTemp.createTempSync(
+ fs = MemoryFileSystem();
+ tempDir = fs.systemTempDirectory.createTempSync(
'persistent_properties_test',
);
properties = IOPersistentProperties(
storeName,
documentDirPath: tempDir.path,
+ fs: fs,
);
});
- tearDown(() {
- tempDir.deleteSync(recursive: true);
- });
-
test('remove persists changes to disk', () {
properties['key1'] = 'value1';
properties['key2'] = 'value2';
- final file = File('${tempDir.path}/$storeName');
+ final file = fs.file(path.join(tempDir.path, storeName));
expect(file.existsSync(), isTrue);
var content = file.readAsStringSync();
diff --git a/pubspec.lock b/pubspec.lock
index cc8b58e..78ac48a 100644
--- a/pubspec.lock
+++ b/pubspec.lock
@@ -181,10 +181,10 @@
dependency: transitive
description:
name: cross_file
- sha256: "28bb3ae56f117b5aec029d702a90f57d285cd975c3c5c281eaca38dbc47c5937"
+ sha256: d687bec93342bf6a764a116d15c8694ebeff10e633dc28a39dd3144f7195024e
url: "https://pub.dev"
source: hosted
- version: "0.3.5+2"
+ version: "0.3.5+3"
crypto:
dependency: transitive
description:
