From 8e5f8837bb0b67213fadc7dbbc75f04990bf79bb Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Mon, 13 May 2024 11:29:44 -0700 Subject: [PATCH 1/8] Use extension types to parse JSON sanely in et. --- tools/engine_tool/lib/src/gn_utils.dart | 9 +- tools/engine_tool/lib/src/json_utils.dart | 119 -------- tools/engine_tool/lib/src/proc_utils.dart | 43 ++- tools/engine_tool/lib/src/run_utils.dart | 17 +- tools/engine_tool/lib/src/typed_json.dart | 299 ++++++++++++++++++++ tools/engine_tool/pubspec.yaml | 3 +- tools/engine_tool/test/typed_json_test.dart | 216 ++++++++++++++ 7 files changed, 546 insertions(+), 160 deletions(-) delete mode 100644 tools/engine_tool/lib/src/json_utils.dart create mode 100644 tools/engine_tool/lib/src/typed_json.dart create mode 100644 tools/engine_tool/test/typed_json_test.dart diff --git a/tools/engine_tool/lib/src/gn_utils.dart b/tools/engine_tool/lib/src/gn_utils.dart index 65da125ca4069..d767af29cf4c7 100644 --- a/tools/engine_tool/lib/src/gn_utils.dart +++ b/tools/engine_tool/lib/src/gn_utils.dart @@ -11,8 +11,8 @@ import 'package:process_runner/process_runner.dart'; import 'build_utils.dart'; import 'environment.dart'; -import 'json_utils.dart'; import 'proc_utils.dart'; +import 'typed_json.dart'; /// Canonicalized build targets start with this prefix. const String buildTargetPrefix = '//'; @@ -86,9 +86,8 @@ Future> findTargets( environment.logger .fatal('gn desc output is malformed $label has no value.'); } - final Map properties = - targetEntry.value! as Map; - final String? typeString = getString(properties, 'type'); + final JsonObject properties = JsonObject(targetEntry.value! as Map); + final String? typeString = properties.stringOrNull('type'); if (typeString == null) { environment.logger.fatal('gn desc is missing target type: $properties'); } @@ -97,7 +96,7 @@ Future> findTargets( // Target is a type that we don't support. continue; } - final bool testOnly = getBool(properties, 'testonly'); + final bool testOnly = properties.boolean('testonly'); final List outputs = await _runGnOutputs(buildDir.path, label, environment); File? executable; diff --git a/tools/engine_tool/lib/src/json_utils.dart b/tools/engine_tool/lib/src/json_utils.dart deleted file mode 100644 index e813512e7cd02..0000000000000 --- a/tools/engine_tool/lib/src/json_utils.dart +++ /dev/null @@ -1,119 +0,0 @@ -// Copyright 2013 The Flutter Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -import 'dart:convert'; - -void _appendTypeError( - Map map, - String field, - String expected, - List errors, { - Object? element, -}) { - if (element == null) { - final Type actual = map[field]!.runtimeType; - errors.add( - 'For field "$field", expected type: $expected, actual type: $actual.', - ); - } else { - final Type actual = element.runtimeType; - errors.add( - 'For element "$element" of "$field", ' - 'expected type: $expected, actual type: $actual', - ); - } -} - -/// Type safe getter of a List field from map. -List? stringListOfJson( - Map map, - String field, - List errors, -) { - if (map[field] == null) { - return []; - } - if (map[field]! is! List) { - _appendTypeError(map, field, 'list', errors); - return null; - } - for (final Object? obj in map[field]! as List) { - if (obj is! String) { - _appendTypeError(map, field, element: obj, 'string', errors); - return null; - } - } - return (map[field]! as List).cast(); -} - -/// Type safe getter of a String field from map. -String? stringOfJson( - Map map, - String field, - List errors, -) { - if (map[field] == null) { - return ''; - } - if (map[field]! is! String) { - _appendTypeError(map, field, 'string', errors); - return null; - } - return map[field]! as String; -} - -/// Type safe getter of an int field from map. -int? intOfJson( - Map map, - String field, - List errors, { - int fallback = 0, -}) { - if (map[field] == null) { - return fallback; - } - if (map[field]! is! int) { - _appendTypeError(map, field, 'int', errors); - return null; - } - return map[field]! as int; -} - -const JsonEncoder _jsonEncoder = JsonEncoder.withIndent(' '); - -/// Same as [jsonEncode] but is formatted to be human readable. -String jsonEncodePretty(Object? object) => _jsonEncoder.convert(object); - -/// Returns the value in map[field] iff it is a String. null otherwise. -String? getString(Map map, String field) { - if (map[field] case final String value) { - return value; - } - return null; -} - -/// Returns the value in map[field] iff it is a bool. missingValue otherwise. -bool getBool(Map map, String field, - [bool missingValue = false]) { - if (map[field] case final bool value) { - return value; - } - return missingValue; -} - -/// Returns the value in map[field] iff it is a List. null otherwise. -List? getListOfString(Map map, String field) { - if (map[field] == null) { - return null; - } - if (map[field]! is! List) { - return null; - } - for (final Object? obj in map[field]! as List) { - if (obj is! String) { - return null; - } - } - return (map[field]! as List).cast(); -} diff --git a/tools/engine_tool/lib/src/proc_utils.dart b/tools/engine_tool/lib/src/proc_utils.dart index fa43c80c4c30d..a4343e4bc2570 100644 --- a/tools/engine_tool/lib/src/proc_utils.dart +++ b/tools/engine_tool/lib/src/proc_utils.dart @@ -3,7 +3,6 @@ // found in the LICENSE file. import 'dart:async'; -import 'dart:convert'; import 'dart:io'; import 'dart:math'; @@ -11,8 +10,8 @@ import 'package:path/path.dart' as p; import 'package:process_runner/process_runner.dart'; import 'environment.dart'; -import 'json_utils.dart'; import 'logger.dart'; +import 'typed_json.dart'; import 'worker_pool.dart'; /// Artifacts from an exited sub-process. @@ -33,18 +32,16 @@ final class ProcessArtifacts { /// Constructs an instance of ProcessArtifacts from serialized JSON text. factory ProcessArtifacts.fromJson(String serialized) { - final Map artifact = - jsonDecode(serialized) as Map; - final List errors = []; - final Directory cwd = Directory(stringOfJson(artifact, 'cwd', errors)!); - final List commandLine = - stringListOfJson(artifact, 'commandLine', errors)!; - final int exitCode = intOfJson(artifact, 'exitCode', errors)!; - final String stdout = stringOfJson(artifact, 'stdout', errors)!; - final String stderr = stringOfJson(artifact, 'stderr', errors)!; - final int? pid = intOfJson(artifact, 'pid', errors); - return ProcessArtifacts(cwd, commandLine, exitCode, stdout, stderr, - pid: pid); + final JsonObject artifact = JsonObject.parse(serialized); + return artifact.map((JsonObject json) => ProcessArtifacts( + Directory(json.string('cwd')), + json.stringList('commandLine'), + json.integer('exitCode'), + json.string('stdout'), + json.string('stderr'), + pid: json.integer('pid'), + ) + ); } /// Constructs an instance of ProcessArtifacts from a file containing JSON. @@ -54,16 +51,14 @@ final class ProcessArtifacts { /// Saves ProcessArtifacts into file. void save(File file) { - final Map data = {}; - if (pid != null) { - data['pid'] = pid!; - } - data['exitCode'] = exitCode; - data['stdout'] = stdout; - data['stderr'] = stderr; - data['cwd'] = cwd.absolute.path; - data['commandLine'] = commandLine; - file.writeAsStringSync(jsonEncodePretty(data)); + file.writeAsStringSync(JsonObject({ + if (pid != null) 'pid': pid, + 'exitCode': exitCode, + 'stdout': stdout, + 'stderr': stderr, + 'cwd': cwd.absolute.path, + 'commandLine': commandLine, + }).toPrettyString()); } /// Creates a temporary file and saves the artifacts into it. diff --git a/tools/engine_tool/lib/src/run_utils.dart b/tools/engine_tool/lib/src/run_utils.dart index 5c2f720879c00..e8640049a82b3 100644 --- a/tools/engine_tool/lib/src/run_utils.dart +++ b/tools/engine_tool/lib/src/run_utils.dart @@ -7,7 +7,7 @@ import 'dart:convert'; import 'package:process_runner/process_runner.dart'; import 'environment.dart'; -import 'json_utils.dart'; +import 'typed_json.dart'; const String _targetPlatformKey = 'targetPlatform'; const String _nameKey = 'name'; @@ -17,16 +17,11 @@ const String _idKey = 'id'; class RunTarget { /// Construct a RunTarget from a JSON map. factory RunTarget.fromJson(Map map) { - final List errors = []; - final String name = stringOfJson(map, _nameKey, errors)!; - final String id = stringOfJson(map, _idKey, errors)!; - final String targetPlatform = - stringOfJson(map, _targetPlatformKey, errors)!; - - if (errors.isNotEmpty) { - throw FormatException('Failed to parse RunTarget: ${errors.join('\n')}'); - } - return RunTarget._(name, id, targetPlatform); + return JsonObject(map).map((JsonObject json) => RunTarget._( + json.string(_nameKey), + json.string(_idKey), + json.string(_targetPlatformKey), + )); } RunTarget._(this.name, this.id, this.targetPlatform); diff --git a/tools/engine_tool/lib/src/typed_json.dart b/tools/engine_tool/lib/src/typed_json.dart new file mode 100644 index 0000000000000..aeb63de2f9dcb --- /dev/null +++ b/tools/engine_tool/lib/src/typed_json.dart @@ -0,0 +1,299 @@ +import 'dart:convert' show JsonEncoder, jsonDecode, jsonEncode; + +import 'package:meta/meta.dart'; + +/// A JSON object that contains key-value pairs of other JSON values. +/// +/// This extension type provides type-safe access to the values in the +/// underlying JSON object without the need for manual type casting and with +/// helpful APIs for common operations. +/// +/// ## Examples +/// +/// ### Basic Usage +/// +/// ```dart +/// const json = JsonObject({ +/// 'name': 'John Doe', +/// 'age': 30, +/// 'isEmployed': true, +/// 'pets': ['Fido', 'Whiskers'], +/// }); +/// +/// final name = json.string('name'); +/// final age = json.integer('age'); +/// final isEmployed = json.boolean('isEmployed'); +/// final pets = json.stringList('pets'); +/// ``` +/// +/// ### Null Safety +/// +/// In the above example, we assumed that all keys exist in the JSON object. To +/// gracefully handle cases where a key may not exist, you can use the `OrNull` +/// variants of the methods: +/// +/// ```dart +/// final name = json.stringOrNull('name'); +/// ``` +/// +/// This works nicely with null-aware operators: +/// +/// ```dart +/// final name = json.stringOrNull('name') ?? 'Unknown'; +/// ``` +/// +/// ### Bulk Operations +/// +/// By default, a [JsonReadException] is thrown if a key is not found or if the +/// value cannot be cast to the expected type, which can be handled using a +/// `try-catch` block: +/// +/// ```dart +/// try { +/// final name = json.string('name'); +/// } on FormatException catch (e) { +/// print('Error: $e'); +/// } +/// ``` +/// +/// However, it is cumbersome to wrap every access in a `try-catch` block, and +/// sometimes you will want to collect all errors at once. For this, you can use +/// [map] to read multiple values at once and convert them to a custom object: +/// +/// ```dart +/// try { +/// final person = json.map( +/// (json) => Person( +/// name: json.string('name'), +/// age: json.integer('age'), +/// isEmployed: json.boolean('isEmployed'), +/// pets: json.stringList('pets'), +/// ), +/// ); +/// } on JsonMapException catch (e) { +/// print('Errors: ${e.exceptions}'); +/// } +/// ``` +extension type const JsonObject(Map _object) { + /// Parses a JSON string into a [JsonObject]. + static JsonObject parse(String jsonString) { + return JsonObject(jsonDecode(jsonString) as Map); + } + + // If non-null, errors caught during [map] are stored here. + static List? _mapErrors; + + // If in the context of a [map] operation, collects exceptions. + // Otherwise, throws the exception. + static void _error(JsonReadException e) { + if (_mapErrors case final List list) { + list.add(e); + } else { + throw e; + } + } + + T _get(String key, T defaultTo) { + final T? result = _getOrNull(key); + if (result == null) { + _error(MissingKeyJsonReadException(this, key)); + return defaultTo; + } + return result; + } + + /// Returns the value at the given [key] as a [String]. + /// + /// Throws a [JsonReadException] if the value is not found or cannot be cast. + String string(String key) => _get(key, ''); + + /// Returns the value at the given [key] as an [int]. + /// + /// Throws a [JsonReadException] if the value is not found or cannot be cast. + int integer(String key) => _get(key, 0); + + /// Returns the value at the given [key] as a [bool]. + /// + /// Throws a [JsonReadException] if the value is not found or cannot be cast. + bool boolean(String key) => _get(key, false); + + List _getList(String key) { + final List? result = _getListOrNull(key); + if (result == null) { + _error(MissingKeyJsonReadException(this, key)); + return []; + } + return result; + } + + /// Returns the value at the given [key] as a [List] of [String]s. + /// + /// Throws a [JsonReadException] if the value is not found or cannot be cast. + List stringList(String key) => _getList(key); + + T? _getOrNull(String key) { + final Object? value = _object[key]; + if (value == null && !_object.containsKey(key)) { + return null; + } else if (value is! T) { + _error(InvalidTypeJsonReadException( + this, + key, + expected: T, + actual: value.runtimeType, + )); + return null; + } else { + return value; + } + } + + /// Returns the value at the given [key] as a [String]. + /// + /// If the value is not found, returns `null`. + /// + /// Throws a [JsonReadException] if the value cannot be cast. + String? stringOrNull(String key) => _getOrNull(key); + + /// Returns the value at the given [key] as an [int]. + /// + /// If the value is not found, returns `null`. + /// + /// Throws a [JsonReadException] if the value cannot be cast. + int? integerOrNull(String key) => _getOrNull(key); + + /// Returns the value at the given [key] as a [bool]. + /// + /// If the value is not found, returns `null`. + /// + /// Throws a [JsonReadException] if the value cannot be cast. + bool? booleanOrNull(String key) => _getOrNull(key); + + List? _getListOrNull(String key) { + final Object? value = _object[key]; + if (value == null && !_object.containsKey(key)) { + return null; + } else if (value is! List) { + _error(InvalidTypeJsonReadException( + this, + key, + expected: List, + actual: value.runtimeType, + )); + return []; + } else { + final List result = []; + for (final Object? element in value) { + if (element is! T) { + _error(InvalidTypeJsonReadException( + this, + key, + expected: T, + actual: element.runtimeType, + )); + return []; + } + result.add(element); + } + return result; + } + } + + /// Returns the value at the given [key] as a [List] of [String]s. + /// + /// If the value is not found, returns `null`. + /// + /// Throws a [JsonReadException] if the value cannot be cast. + List? stringListOrNull(String key) => _getListOrNull(key); + + /// Returns the result of applying the given [mapper] to this JSON object. + /// + /// Any exception that otherwise would be thrown by the mapper is caught + /// internally and rethrown as a [JsonMapException] with all of the original + /// exceptions added to its [exceptions] property. + T map(T Function(JsonObject) mapper) { + final List mapErrors = []; + _mapErrors = mapErrors; + try { + return mapper(this); + } finally { + _mapErrors = null; + if (mapErrors.isNotEmpty) { + final JsonMapException exception = JsonMapException(mapErrors); + // ignore: throw_in_finally + throw exception; + } + } + } + + /// Returns a "pretty" JSON representation of this object. + String toPrettyString() { + return const JsonEncoder.withIndent(' ').convert(_object); + } +} + +/// An exception thrown when read exceptions occur during [JsonObject.map]. +final class JsonMapException implements Exception { + /// Creates an exception for multiple JSON read exceptions. + /// + /// If [exceptions] is empty, a [StateError] is thrown. + JsonMapException(Iterable exceptions) + : exceptions = List.unmodifiable(exceptions) { + if (exceptions.isEmpty) { + throw StateError('Must provide at least one exception.'); + } + } + + /// The list of exceptions that occurred while mapping the JSON object. + final List exceptions; + + @override + String toString() { + return 'Failed to read object:\n\n${exceptions.join('\n')}'; + } +} + +/// An exception thrown when a JSON value cannot be read as the expected type. +sealed class JsonReadException implements Exception { + const JsonReadException(this.object, this.key); + + /// The JSON object that was being read. + final JsonObject object; + + /// The key that was being read. + final String key; + + @override + @mustBeOverridden + String toString(); +} + +/// An exception thrown when a JSON [key] is not found in the JSON object. +final class MissingKeyJsonReadException extends JsonReadException { + /// Creates an exception for a missing key in a JSON object. + const MissingKeyJsonReadException(super.object, super.key); + + @override + String toString() => 'Key "$key" not found.'; +} + +/// An exception thrown when a JSON [key] is found but the value cannot be cast. +final class InvalidTypeJsonReadException extends JsonReadException { + /// Creates an exception for an invalid type in a JSON object. + const InvalidTypeJsonReadException( + super.object, + super.key, { + required this.expected, + required this.actual, + }); + + /// The expected type of the value. + final Type expected; + + /// The actual type of the value. + final Type actual; + + @override + String toString() => + 'Key "$key" expected type: $expected, actual type: $actual.'; +} diff --git a/tools/engine_tool/pubspec.yaml b/tools/engine_tool/pubspec.yaml index c9ff25fe9743b..0e0e14e159c96 100644 --- a/tools/engine_tool/pubspec.yaml +++ b/tools/engine_tool/pubspec.yaml @@ -5,7 +5,8 @@ name: engine_tool publish_to: none environment: - sdk: '>=3.2.0-0 <4.0.0' + # Required for extension types. + sdk: ^3.3.0 # Do not add any dependencies that require more than what is provided in # //third_party/pkg, //third_party/dart/pkg, or diff --git a/tools/engine_tool/test/typed_json_test.dart b/tools/engine_tool/test/typed_json_test.dart new file mode 100644 index 0000000000000..95e73d8522b08 --- /dev/null +++ b/tools/engine_tool/test/typed_json_test.dart @@ -0,0 +1,216 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'package:engine_tool/src/typed_json.dart'; +import 'package:litetest/litetest.dart'; + +void main() { + group('JsonObject.string', () { + test('returns string value', () { + const JsonObject jsonObject = JsonObject({'key': 'value'}); + expect(jsonObject.string('key'), 'value'); + }); + + test('throws due to missing key', () { + const JsonObject jsonObject = JsonObject({'key': 'value'}); + expect(() => jsonObject.string('missing'), throwsA(isInstanceOf())); + }); + + test('throws due to wrong type', () { + const JsonObject jsonObject = JsonObject({'key': 42}); + expect(() => jsonObject.string('key'), throwsA(isInstanceOf())); + }); + }); + + group('JsonObject.integer', () { + test('returns integer value', () { + const JsonObject jsonObject = JsonObject({'key': 42}); + expect(jsonObject.integer('key'), 42); + }); + + test('throws due to missing key', () { + const JsonObject jsonObject = JsonObject({'key': 42}); + expect(() => jsonObject.integer('missing'), throwsA(isInstanceOf())); + }); + + test('throws due to wrong type', () { + const JsonObject jsonObject = JsonObject({'key': 'value'}); + expect(() => jsonObject.integer('key'), throwsA(isInstanceOf())); + }); + }); + + group('JsonObject.boolean', () { + test('returns boolean value', () { + const JsonObject jsonObject = JsonObject({'key': true}); + expect(jsonObject.boolean('key'), true); + }); + + test('throws due to missing key', () { + const JsonObject jsonObject = JsonObject({'key': true}); + expect(() => jsonObject.boolean('missing'), throwsA(isInstanceOf())); + }); + + test('throws due to wrong type', () { + const JsonObject jsonObject = JsonObject({'key': 'value'}); + expect(() => jsonObject.boolean('key'), throwsA(isInstanceOf())); + }); + }); + + group('JsonObject.stringList', () { + test('returns string list value', () { + const JsonObject jsonObject = JsonObject({'key': ['value1', 'value2']}); + expect(jsonObject.stringList('key'), ['value1', 'value2']); + }); + + test('throws due to missing key', () { + const JsonObject jsonObject = JsonObject({'key': ['value1', 'value2']}); + expect(() => jsonObject.stringList('missing'), throwsA(isInstanceOf())); + }); + + test('throws due to wrong type', () { + const JsonObject jsonObject = JsonObject({'key': 'value'}); + expect(() => jsonObject.stringList('key'), throwsA(isInstanceOf())); + }); + + test('throws due to wrong element type', () { + const JsonObject jsonObject = JsonObject({'key': ['value1', 42]}); + expect(() => jsonObject.stringList('key'), throwsA(isInstanceOf())); + }); + }); + + group('JsonObject.stringOrNull', () { + test('returns string value', () { + const JsonObject jsonObject = JsonObject({'key': 'value'}); + expect(jsonObject.stringOrNull('key'), 'value'); + }); + + test('returns null due to missing key', () { + const JsonObject jsonObject = JsonObject({'key': 'value'}); + expect(jsonObject.stringOrNull('missing'), isNull); + }); + + test('throws due to wrong type', () { + const JsonObject jsonObject = JsonObject({'key': 42}); + expect(() => jsonObject.stringOrNull('key'), throwsA(isInstanceOf())); + }); + }); + + group('JsonObject.integerOrNull', () { + test('returns integer value', () { + const JsonObject jsonObject = JsonObject({'key': 42}); + expect(jsonObject.integerOrNull('key'), 42); + }); + + test('returns null due to missing key', () { + const JsonObject jsonObject = JsonObject({'key': 42}); + expect(jsonObject.integerOrNull('missing'), isNull); + }); + + test('throws due to wrong type', () { + const JsonObject jsonObject = JsonObject({'key': 'value'}); + expect(() => jsonObject.integerOrNull('key'), throwsA(isInstanceOf())); + }); + }); + + group('JsonObject.booleanOrNull', () { + test('returns boolean value', () { + const JsonObject jsonObject = JsonObject({'key': true}); + expect(jsonObject.booleanOrNull('key'), true); + }); + + test('returns null due to missing key', () { + const JsonObject jsonObject = JsonObject({'key': true}); + expect(jsonObject.booleanOrNull('missing'), isNull); + }); + + test('throws due to wrong type', () { + const JsonObject jsonObject = JsonObject({'key': 'value'}); + expect(() => jsonObject.booleanOrNull('key'), throwsA(isInstanceOf())); + }); + }); + + group('JsonObject.stringListOrNull', () { + test('returns string list value', () { + const JsonObject jsonObject = JsonObject({'key': ['value1', 'value2']}); + expect(jsonObject.stringListOrNull('key'), ['value1', 'value2']); + }); + + test('returns null due to missing key', () { + const JsonObject jsonObject = JsonObject({'key': ['value1', 'value2']}); + expect(jsonObject.stringListOrNull('missing'), isNull); + }); + + test('throws due to wrong type', () { + const JsonObject jsonObject = JsonObject({'key': 'value'}); + expect(() => jsonObject.stringListOrNull('key'), throwsA(isInstanceOf())); + }); + + test('throws due to wrong element type', () { + const JsonObject jsonObject = JsonObject({'key': ['value1', 42]}); + expect(() => jsonObject.stringListOrNull('key'), throwsA(isInstanceOf())); + }); + }); + + group('JsonObject.map', () { + test('returns multiple fields', () { + final (String name, int age, bool isStudent) = const JsonObject({ + 'name': 'Alice', + 'age': 42, + 'isStudent': true, + }).map((JsonObject json) { + return ( + json.string('name'), + json.integer('age'), + json.boolean('isStudent'), + ); + }); + + expect(name, 'Alice'); + expect(age, 42); + expect(isStudent, true); + }); + + test('throws due to missing keys', () { + try { + const JsonObject({ + 'age': 42, + }).map((JsonObject json) { + return ( + json.string('name'), + json.integer('age'), + json.boolean('isStudent'), + ); + }); + fail('Expected JsonMapException'); + } on JsonMapException catch (e) { + expect( + e.exceptions.map((JsonReadException e) => e.key).toList(), + containsStringsInOrder(['name', 'isStudent'], + )); + } + }); + + test('throws due to wrong types', () { + try { + const JsonObject({ + 'name': 42, + 'age': '42', + 'isStudent': false, + }).map((JsonObject json) { + return ( + json.string('name'), + json.integer('age'), + json.boolean('isStudent'), + ); + }); + fail('Expected JsonMapException'); + } on JsonMapException catch (e) { + expect( + e.exceptions.map((JsonReadException e) => e.key).toList(), + containsStringsInOrder(['name', 'age'], + )); + } + }); + }); +} From 21dd40f8193f52da89b080c399f0d45a9152c6b5 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Mon, 13 May 2024 11:30:02 -0700 Subject: [PATCH 2/8] ++ --- tools/engine_tool/test/typed_json_test.dart | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/engine_tool/test/typed_json_test.dart b/tools/engine_tool/test/typed_json_test.dart index 95e73d8522b08..3a90500698b26 100644 --- a/tools/engine_tool/test/typed_json_test.dart +++ b/tools/engine_tool/test/typed_json_test.dart @@ -185,7 +185,7 @@ void main() { fail('Expected JsonMapException'); } on JsonMapException catch (e) { expect( - e.exceptions.map((JsonReadException e) => e.key).toList(), + e.exceptions.map((JsonReadException e) => e.key).toList(), containsStringsInOrder(['name', 'isStudent'], )); } @@ -207,7 +207,7 @@ void main() { fail('Expected JsonMapException'); } on JsonMapException catch (e) { expect( - e.exceptions.map((JsonReadException e) => e.key).toList(), + e.exceptions.map((JsonReadException e) => e.key).toList(), containsStringsInOrder(['name', 'age'], )); } From 466bb3ce892664e9dddeedbcfd001cd12ae4d1b0 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Mon, 13 May 2024 11:38:14 -0700 Subject: [PATCH 3/8] ++ --- tools/engine_tool/lib/src/typed_json.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/engine_tool/lib/src/typed_json.dart b/tools/engine_tool/lib/src/typed_json.dart index aeb63de2f9dcb..0dc126f05c4ad 100644 --- a/tools/engine_tool/lib/src/typed_json.dart +++ b/tools/engine_tool/lib/src/typed_json.dart @@ -1,4 +1,4 @@ -import 'dart:convert' show JsonEncoder, jsonDecode, jsonEncode; +import 'dart:convert' show JsonEncoder, jsonDecode; import 'package:meta/meta.dart'; From 8d89979ec2df320d07f9b898705b3118b0b01c2b Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Mon, 13 May 2024 15:21:28 -0700 Subject: [PATCH 4/8] ++ --- tools/engine_tool/lib/src/typed_json.dart | 111 ++++++++++++++------ tools/engine_tool/test/typed_json_test.dart | 36 ++++++- 2 files changed, 113 insertions(+), 34 deletions(-) diff --git a/tools/engine_tool/lib/src/typed_json.dart b/tools/engine_tool/lib/src/typed_json.dart index 0dc126f05c4ad..75c17f8bf93a4 100644 --- a/tools/engine_tool/lib/src/typed_json.dart +++ b/tools/engine_tool/lib/src/typed_json.dart @@ -74,6 +74,16 @@ import 'package:meta/meta.dart'; /// print('Errors: ${e.exceptions}'); /// } /// ``` +/// +/// Or, provide an `onError` handler to handle the exception (either to log and +/// provide a default value, or to rethrow a different exception): +/// +/// ```dart +/// final person = json.map( +/// (json) => Person(name: json.string('name')), +/// onError: (json, e) => throw FormatException('Failed to read person: $e'), +/// ); +/// ``` extension type const JsonObject(Map _object) { /// Parses a JSON string into a [JsonObject]. static JsonObject parse(String jsonString) { @@ -96,7 +106,9 @@ extension type const JsonObject(Map _object) { T _get(String key, T defaultTo) { final T? result = _getOrNull(key); if (result == null) { - _error(MissingKeyJsonReadException(this, key)); + if (!_object.containsKey(key)) { + _error(MissingKeyJsonReadException(this, key)); + } return defaultTo; } return result; @@ -206,23 +218,54 @@ extension type const JsonObject(Map _object) { /// Throws a [JsonReadException] if the value cannot be cast. List? stringListOrNull(String key) => _getListOrNull(key); + static Never _onErrorThrow(JsonObject object, JsonMapException e) { + throw e; + } + /// Returns the result of applying the given [mapper] to this JSON object. /// /// Any exception that otherwise would be thrown by the mapper is caught /// internally and rethrown as a [JsonMapException] with all of the original /// exceptions added to its [exceptions] property. - T map(T Function(JsonObject) mapper) { - final List mapErrors = []; - _mapErrors = mapErrors; + /// + /// If [onError] is provided, it is called with this object and the caught + /// [JsonMapException] to handle the error. If not provided, the exception is + /// rethrown. + /// + /// ## Example + /// + /// ```dart + /// final person = json.map( + /// (json) => Person( + /// name: json.string('name'), + /// age: json.integer('age'), + /// isEmployed: json.boolean('isEmployed'), + /// ), onError: (json, e) { + /// throw FormatException('Failed to read person: $e'); + /// }, + /// ); + /// ``` + T map( + T Function(JsonObject) mapper, { + T Function(JsonObject, JsonMapException) onError = _onErrorThrow, + }) { + // Store the previous errors, if any. + final List? previousErrors = _mapErrors; + + // Start collecting errors for this map operation. + final List currentErrors = []; + _mapErrors = currentErrors; + try { - return mapper(this); - } finally { - _mapErrors = null; - if (mapErrors.isNotEmpty) { - final JsonMapException exception = JsonMapException(mapErrors); - // ignore: throw_in_finally - throw exception; + final T result = mapper(this); + if (currentErrors.isEmpty) { + return result; + } else { + return onError(this, JsonMapException(this, currentErrors)); } + } finally { + // Restore the previous errors. + _mapErrors = previousErrors; } } @@ -232,12 +275,24 @@ extension type const JsonObject(Map _object) { } } +/// An exception thrown when a JSON value cannot be read as the expected type. +sealed class JsonReadException implements Exception { + const JsonReadException(this.object); + + /// The JSON object that was being read. + final JsonObject object; + + @override + @mustBeOverridden + String toString(); +} + /// An exception thrown when read exceptions occur during [JsonObject.map]. -final class JsonMapException implements Exception { +final class JsonMapException extends JsonReadException { /// Creates an exception for multiple JSON read exceptions. /// /// If [exceptions] is empty, a [StateError] is thrown. - JsonMapException(Iterable exceptions) + JsonMapException(super.object, Iterable exceptions) : exceptions = List.unmodifiable(exceptions) { if (exceptions.isEmpty) { throw StateError('Must provide at least one exception.'); @@ -253,25 +308,13 @@ final class JsonMapException implements Exception { } } -/// An exception thrown when a JSON value cannot be read as the expected type. -sealed class JsonReadException implements Exception { - const JsonReadException(this.object, this.key); - - /// The JSON object that was being read. - final JsonObject object; - - /// The key that was being read. - final String key; - - @override - @mustBeOverridden - String toString(); -} - /// An exception thrown when a JSON [key] is not found in the JSON object. final class MissingKeyJsonReadException extends JsonReadException { /// Creates an exception for a missing key in a JSON object. - const MissingKeyJsonReadException(super.object, super.key); + const MissingKeyJsonReadException(super.object, this.key); + + /// The key that was being read. + final String key; @override String toString() => 'Key "$key" not found.'; @@ -282,11 +325,14 @@ final class InvalidTypeJsonReadException extends JsonReadException { /// Creates an exception for an invalid type in a JSON object. const InvalidTypeJsonReadException( super.object, - super.key, { + this.key, { required this.expected, required this.actual, }); + /// The key that was being read. + final String key; + /// The expected type of the value. final Type expected; @@ -294,6 +340,7 @@ final class InvalidTypeJsonReadException extends JsonReadException { final Type actual; @override - String toString() => - 'Key "$key" expected type: $expected, actual type: $actual.'; + String toString() { + return 'Key "$key" expected type: $expected, actual type: $actual.'; + } } diff --git a/tools/engine_tool/test/typed_json_test.dart b/tools/engine_tool/test/typed_json_test.dart index 3a90500698b26..96ec2f2bdc6f2 100644 --- a/tools/engine_tool/test/typed_json_test.dart +++ b/tools/engine_tool/test/typed_json_test.dart @@ -185,7 +185,7 @@ void main() { fail('Expected JsonMapException'); } on JsonMapException catch (e) { expect( - e.exceptions.map((JsonReadException e) => e.key).toList(), + e.exceptions.map((JsonReadException e) => (e as MissingKeyJsonReadException).key).toList(), containsStringsInOrder(['name', 'isStudent'], )); } @@ -207,10 +207,42 @@ void main() { fail('Expected JsonMapException'); } on JsonMapException catch (e) { expect( - e.exceptions.map((JsonReadException e) => e.key).toList(), + e.exceptions.map((JsonReadException e) => switch (e) { + final InvalidTypeJsonReadException e => e.key, + final MissingKeyJsonReadException e => e.key, + _ => throw StateError('Unexpected exception type: $e'), + }).toList(), containsStringsInOrder(['name', 'age'], )); } }); + + test('allows a default with onError', () { + final (String name, int age, bool isStudent) = const JsonObject({ + 'name': 'Alice', + 'age': 42, + 'isStudent': 'true', + }).map((JsonObject json) { + return ( + json.string('name'), + json.integer('age'), + json.boolean('isStudent'), + ); + }, onError: expectAsync2((_, JsonMapException e) { + expect( + e.exceptions.map((JsonReadException e) => switch (e) { + final InvalidTypeJsonReadException e => e.key, + final MissingKeyJsonReadException e => e.key, + _ => throw StateError('Unexpected exception type: $e'), + }).toList(), + ['isStudent'], + ); + return ('Bob', 0, false); + })); + + expect(name, 'Bob'); + expect(age, 0); + expect(isStudent, false); + }); }); } From f0ee9274dc5bbdda3cb933937a6fa1f1ba5967c1 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Mon, 13 May 2024 15:22:53 -0700 Subject: [PATCH 5/8] ++ --- tools/engine_tool/lib/src/proc_utils.dart | 4 +++- tools/engine_tool/lib/src/run_utils.dart | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/tools/engine_tool/lib/src/proc_utils.dart b/tools/engine_tool/lib/src/proc_utils.dart index a4343e4bc2570..7270b80d2a290 100644 --- a/tools/engine_tool/lib/src/proc_utils.dart +++ b/tools/engine_tool/lib/src/proc_utils.dart @@ -40,7 +40,9 @@ final class ProcessArtifacts { json.string('stdout'), json.string('stderr'), pid: json.integer('pid'), - ) + ), onError: (JsonObject source, JsonMapException e) { + throw FormatException('Failed to parse ProcessArtifacts: $e', source.toPrettyString()); + }, ); } diff --git a/tools/engine_tool/lib/src/run_utils.dart b/tools/engine_tool/lib/src/run_utils.dart index e8640049a82b3..7f0b34f3ce64b 100644 --- a/tools/engine_tool/lib/src/run_utils.dart +++ b/tools/engine_tool/lib/src/run_utils.dart @@ -21,7 +21,9 @@ class RunTarget { json.string(_nameKey), json.string(_idKey), json.string(_targetPlatformKey), - )); + ), onError: (JsonObject source, JsonMapException e) { + throw FormatException('Failed to parse RunTarget: $e', source.toPrettyString()); + }); } RunTarget._(this.name, this.id, this.targetPlatform); From e0d0fd09185cb065e517b056e26ff3d217961a02 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Mon, 13 May 2024 15:24:55 -0700 Subject: [PATCH 6/8] ++ --- tools/engine_tool/test/typed_json_test.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/engine_tool/test/typed_json_test.dart b/tools/engine_tool/test/typed_json_test.dart index 96ec2f2bdc6f2..f86e167057932 100644 --- a/tools/engine_tool/test/typed_json_test.dart +++ b/tools/engine_tool/test/typed_json_test.dart @@ -234,7 +234,7 @@ void main() { final InvalidTypeJsonReadException e => e.key, final MissingKeyJsonReadException e => e.key, _ => throw StateError('Unexpected exception type: $e'), - }).toList(), + }).toList(), ['isStudent'], ); return ('Bob', 0, false); From ecd890f806e8ac2d96b096e6550e08494eae4d1b Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Mon, 13 May 2024 15:48:58 -0700 Subject: [PATCH 7/8] ++ --- tools/engine_tool/lib/src/typed_json.dart | 20 +++++++++++++++++++- tools/engine_tool/test/typed_json_test.dart | 16 ++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/tools/engine_tool/lib/src/typed_json.dart b/tools/engine_tool/lib/src/typed_json.dart index 75c17f8bf93a4..b58f732ef8917 100644 --- a/tools/engine_tool/lib/src/typed_json.dart +++ b/tools/engine_tool/lib/src/typed_json.dart @@ -1,3 +1,7 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + import 'dart:convert' show JsonEncoder, jsonDecode; import 'package:meta/meta.dart'; @@ -231,7 +235,9 @@ extension type const JsonObject(Map _object) { /// If [onError] is provided, it is called with this object and the caught /// [JsonMapException] to handle the error. If not provided, the exception is /// rethrown. - /// + /// + /// **Note:** The [mapper] function _must_ be synchronous. + /// /// ## Example /// /// ```dart @@ -249,6 +255,18 @@ extension type const JsonObject(Map _object) { T Function(JsonObject) mapper, { T Function(JsonObject, JsonMapException) onError = _onErrorThrow, }) { + // Refuse asynchronous mappers to avoid reentrancy issues. + // + // Could be replaced with a static check in the future: + // https://github.com/dart-lang/sdk/issues/35024 + if (mapper is Future Function(JsonObject)) { + throw ArgumentError.value( + mapper, + 'mapper', + 'must be synchronous', + ); + } + // Store the previous errors, if any. final List? previousErrors = _mapErrors; diff --git a/tools/engine_tool/test/typed_json_test.dart b/tools/engine_tool/test/typed_json_test.dart index f86e167057932..c23df2b7940cc 100644 --- a/tools/engine_tool/test/typed_json_test.dart +++ b/tools/engine_tool/test/typed_json_test.dart @@ -244,5 +244,21 @@ void main() { expect(age, 0); expect(isStudent, false); }); + + test('disallows a return type of Future<*>', () { + expect(() { + const JsonObject({ + 'name': 'Alice', + 'age': 42, + 'isStudent': true, + }).map((JsonObject json) async { + return ( + json.string('name'), + json.integer('age'), + json.boolean('isStudent'), + ); + }); + }, throwsA(isInstanceOf())); + }); }); } From 2b12cfa81a0bbb7ba8c2cc8365f92e85f630e60a Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Mon, 13 May 2024 15:50:15 -0700 Subject: [PATCH 8/8] ++ --- tools/engine_tool/lib/src/typed_json.dart | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/engine_tool/lib/src/typed_json.dart b/tools/engine_tool/lib/src/typed_json.dart index b58f732ef8917..690c6a498123b 100644 --- a/tools/engine_tool/lib/src/typed_json.dart +++ b/tools/engine_tool/lib/src/typed_json.dart @@ -235,9 +235,9 @@ extension type const JsonObject(Map _object) { /// If [onError] is provided, it is called with this object and the caught /// [JsonMapException] to handle the error. If not provided, the exception is /// rethrown. - /// + /// /// **Note:** The [mapper] function _must_ be synchronous. - /// + /// /// ## Example /// /// ```dart