Repository navigation
[flutter_tools] Report malformed plugin platform entries instead of crashing - #193151
auto-submit[bot] merged 5 commits into
Conversation
…rashing A platform entry under `platforms:` that declares `default_package`, `dartPluginClass` or `dartFileName` with a non-string value skipped validation and reached a `String` cast while the plugin was built, so a malformed pubspec crashed the tool with a type error. Reject those entries in `_validateMultiPlatformYaml` so the existing invalid specification tool exit is reported instead.
github-actions
Bot
added
the
tool
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds validation to reject non-string values for default_package, dartPluginClass, and dartFileName in plugin specifications, preventing tool crashes due to type errors. It also includes regression tests for default_package and dartPluginClass. The feedback suggests adding a test case for dartFileName to ensure complete test coverage of the new validation logic.
| testUsingContext('Plugin.fromYaml rejects a non-string dartPluginClass', () async { | ||
| const malformedYaml = ''' | ||
| platforms: | ||
| linux: | ||
| pluginClass: SamplePlugin | ||
| dartPluginClass: | ||
| '''; | ||
| expect( | ||
| () => Plugin.fromYaml( | ||
| 'malformed_plugin', | ||
| '', | ||
| loadYaml(malformedYaml) as YamlMap, | ||
| null, | ||
| const <String>[], | ||
| fileSystem: globals.fs, | ||
| isDevDependency: false, | ||
| ), | ||
| throwsToolExit(message: 'Invalid "linux" plugin specification.'), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
While default_package and dartPluginClass are covered by the new regression tests, dartFileName is also checked by _hasNonStringValue but lacks a corresponding test. Adding a test case for dartFileName ensures complete coverage of the new validation logic.
testUsingContext('Plugin.fromYaml rejects a non-string dartPluginClass', () async {
const malformedYaml = '''
platforms:
linux:
pluginClass: SamplePlugin
dartPluginClass:
''';
expect(
() => Plugin.fromYaml(
'malformed_plugin',
'',
loadYaml(malformedYaml) as YamlMap,
null,
const <String>[],
fileSystem: globals.fs,
isDevDependency: false,
),
throwsToolExit(message: 'Invalid "linux" plugin specification.'),
);
});
testUsingContext('Plugin.fromYaml rejects a non-string dartFileName', () async {
const malformedYaml = '''
platforms:
windows:
pluginClass: SamplePlugin
dartFileName:
''';
expect(
() => Plugin.fromYaml(
'malformed_plugin',
'',
loadYaml(malformedYaml) as YamlMap,
null,
const <String>[],
fileSystem: globals.fs,
isDevDependency: false,
),
throwsToolExit(message: 'Invalid "windows" plugin specification.'),
);
});References
- Code should be tested and follow the guidance described in the writing effective tests guide. (link)
There was a problem hiding this comment.
Added a dartFileName test, though not the empty one. An empty dartFileName: never crashed: that key is read with as String? and falls back to <plugin>.dart, so rejecting it would have broken plugins that work today. What does crash is a value of the wrong type (dartFileName: 123), since that still reaches the cast. The check now leaves null alone for this key and rejects only that, and there are two tests: the wrong type is reported, and the empty entry still resolves to the default file name 👌
The non-string check was too broad for dartFileName. Unlike default_package and dartPluginClass, which are read with a non-nullable `as String` cast and so crash on an empty entry, dartFileName is read with `as String?` and falls back to `<plugin>.dart` when null. An empty `dartFileName:` has therefore always been valid, and rejecting it would have broken working plugins. Only a wrong-typed value (`dartFileName: 123`) reaches the cast and crashes, so that is what is rejected now. Tests cover both: the wrong type is reported, and the empty entry still resolves to the default file name.
bkonyi
left a comment
bkonyi
left a comment
There was a problem hiding this comment.
LGTM! We'll just need one more review before this can land.
auto-submit
Bot
removed
the
autosubmit
|
autosubmit label was removed for flutter/flutter/193151, because - The status or check suite Dashboard Checks has failed. Please fix the issues identified (or deflake) before re-applying this label. |
|
Hi @ishaquehassan, it looks like there's a formatting failure. Can you run |
auto-submit
Bot
removed
the
autosubmit
|
autosubmit label was removed for flutter/flutter/193151, because The base commit of the PR is older than 7 days and can not be merged. Please merge the latest changes from the main into this branch and resubmit the PR. |
bkonyi
added
autosubmit
flutter-dashboard
Bot
removed
the
autosubmit
3 participants
A
platforms:entry that declaresdefault_package:with no value makes the tool crash with_TypeError: type 'Null' is not a subtype of type 'String' in type castout ofPlugin._getDefaultPackageForPlatform, rather than telling the user which part of the pubspec is wrong.Plugin.fromYamlvalidates before constructing, and the private constructors document that they rely on that, so_getDefaultPackageForPlatformand_getPluginDartClassForPlatformcast straight toString. The gap is in_validateMultiPlatformYaml: itsisInvalidhelper returns early as soon as a platform entry containsdefault_package, so neither that key's type nor the rest of the entry is ever checked. The per-platformvalidatemethods do testyaml[kDefaultPackage] is String, but those are exactly the ones being skipped.isInvalidnow rejects a platform entry that declaresdefault_package,dartPluginClassordartFileNamewith a non-string value, the three per-platform keys that are cast toStringwhile the plugin is built. A malformed pubspec gets the existingInvalid "android" plugin specification.tool exit instead of a crash. Entries that declare those keys as strings, or leave them out, take the same path as before, and no platform validator changed.Two regression tests in
packages/flutter_tools/test/general.shard/plugins_test.dartcover one cast site each, alongside the otherPlugin.fromYaml rejects ...cases. Both fail without the change with the type error above.Fixes #162150
Pre-launch Checklist
///).