Skip to content

Commit e5dfd38

Browse files
pqcommit-bot@chromium.org
pq
authored and
commit-bot@chromium.org
committed
check for invalid git and path deps in publishable packages
Fixes: #43930 Change-Id: Ia1e7899cf04644387f2686e997e338baa197dac3 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/169246 Reviewed-by: Phil Quitslund <pquitslund@google.com> Reviewed-by: Brian Wilkerson <brianwilkerson@google.com> Commit-Queue: Phil Quitslund <pquitslund@google.com>
1 parent f8fdb4b commit e5dfd38

File tree

4 files changed

+224
-26
lines changed

4 files changed

+224
-26
lines changed

‎pkg/analyzer/lib/error/error.dart‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import 'package:analyzer/src/generated/java_core.dart';
1616
import 'package:analyzer/src/generated/parser.dart' show ParserErrorCode;
1717
import 'package:analyzer/src/generated/source.dart';
1818
import 'package:analyzer/src/manifest/manifest_warning_code.dart';
19+
import 'package:analyzer/src/pubspec/pubspec_warning_code.dart';
1920

2021
export 'package:_fe_analyzer_shared/src/base/errors.dart'
2122
show ErrorCode, ErrorSeverity, ErrorType;
@@ -795,6 +796,9 @@ const List<ErrorCode> errorCodeValues = [
795796
ParserErrorCode.WITH_BEFORE_EXTENDS,
796797
ParserErrorCode.WRONG_SEPARATOR_FOR_POSITIONAL_PARAMETER,
797798
ParserErrorCode.WRONG_TERMINATOR_FOR_PARAMETER_GROUP,
799+
PubspecWarningCode.INVALID_DEPENDENCY,
800+
PubspecWarningCode.PATH_DOES_NOT_EXIST,
801+
PubspecWarningCode.PATH_PUBSPEC_DOES_NOT_EXIST,
798802
ScannerErrorCode.EXPECTED_TOKEN,
799803
ScannerErrorCode.ILLEGAL_CHARACTER,
800804
ScannerErrorCode.MISSING_DIGIT,

‎pkg/analyzer/lib/src/pubspec/pubspec_validator.dart‎

Lines changed: 66 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import 'package:analyzer/file_system/file_system.dart';
88
import 'package:analyzer/src/generated/engine.dart';
99
import 'package:analyzer/src/generated/source.dart';
1010
import 'package:analyzer/src/pubspec/pubspec_warning_code.dart';
11+
import 'package:analyzer/src/util/yaml.dart';
1112
import 'package:path/path.dart' as path;
1213
import 'package:source_span/src/span.dart';
1314
import 'package:yaml/yaml.dart';
@@ -27,12 +28,21 @@ class PubspecValidator {
2728
/// configuration data.
2829
static const String FLUTTER_FIELD = 'flutter';
2930

31+
/// The name of the field whose value is a git dependency.
32+
static const String GIT_FIELD = 'git';
33+
3034
/// The name of the field whose value is the name of the package.
3135
static const String NAME_FIELD = 'name';
3236

3337
/// The name of the field whose value is a path to a package dependency.
3438
static const String PATH_FIELD = 'path';
3539

40+
/// The name of the field whose value is the where to publish the package.
41+
static const String PUBLISH_TO_FIELD = 'publish_to';
42+
43+
/// The name of the field whose value is the version of the package.
44+
static const String VERSION_FIELD = 'version';
45+
3646
/// The resource provider used to access the file system.
3747
final ResourceProvider provider;
3848

@@ -89,6 +99,16 @@ class PubspecValidator {
8999
return false;
90100
}
91101

102+
String _asString(dynamic node) {
103+
if (node is String) {
104+
return node;
105+
}
106+
if (node is YamlScalar && node.value is String) {
107+
return node.value as String;
108+
}
109+
return null;
110+
}
111+
92112
/// Return a map whose keys are the names of declared dependencies and whose
93113
/// values are the specifications of those dependencies. The map is extracted
94114
/// from the given [contents] using the given [key].
@@ -122,8 +142,17 @@ class PubspecValidator {
122142
Map<dynamic, YamlNode> declaredDevDependencies =
123143
_getDeclaredDependencies(reporter, contents, DEV_DEPENDENCIES_FIELD);
124144

145+
bool isPublishablePackage = false;
146+
var version = contents[VERSION_FIELD];
147+
if (version != null) {
148+
var publishTo = _asString(contents[PUBLISH_TO_FIELD]);
149+
if (publishTo != 'none') {
150+
isPublishablePackage = true;
151+
}
152+
}
153+
125154
for (var dependency in declaredDependencies.entries) {
126-
_validatePathEntries(reporter, dependency.value);
155+
_validatePathEntries(reporter, dependency.value, isPublishablePackage);
127156
}
128157

129158
for (var dependency in declaredDevDependencies.entries) {
@@ -132,7 +161,7 @@ class PubspecValidator {
132161
_reportErrorForNode(reporter, packageName,
133162
PubspecWarningCode.UNNECESSARY_DEV_DEPENDENCY, [packageName.value]);
134163
}
135-
_validatePathEntries(reporter, dependency.value);
164+
_validatePathEntries(reporter, dependency.value, false);
136165
}
137166
}
138167

@@ -209,33 +238,44 @@ class PubspecValidator {
209238
/// Valid paths are directories that:
210239
///
211240
/// 1. exist,
212-
/// 2. contain a pubspec.yaml file, and
213-
/// 3. `lib` dir
214-
void _validatePathEntries(ErrorReporter reporter, YamlNode dependency) {
241+
/// 2. contain a pubspec.yaml file
242+
///
243+
/// If [checkForPathAndGitDeps] is true, `git` or `path` dependencies will
244+
/// be marked invalid.
245+
void _validatePathEntries(ErrorReporter reporter, YamlNode dependency,
246+
bool checkForPathAndGitDeps) {
215247
if (dependency is YamlMap) {
216-
for (var node in dependency.nodes.entries) {
217-
var pathEntry = dependency[PATH_FIELD];
218-
if (pathEntry != null) {
219-
var context = provider.pathContext;
220-
var normalizedPath = context.joinAll(path.posix.split(pathEntry));
221-
var packageRoot = context.dirname(source.fullName);
222-
var dependencyPath = context.join(packageRoot, normalizedPath);
223-
dependencyPath = context.absolute(dependencyPath);
224-
dependencyPath = context.normalize(dependencyPath);
225-
var packageFolder = provider.getFolder(dependencyPath);
226-
if (!packageFolder.exists) {
227-
_reportErrorForNode(reporter, node.value,
228-
PubspecWarningCode.PATH_DOES_NOT_EXIST, [pathEntry]);
229-
} else {
230-
if (!packageFolder
231-
.getChild(AnalysisEngine.PUBSPEC_YAML_FILE)
232-
.exists) {
233-
_reportErrorForNode(reporter, node.value,
234-
PubspecWarningCode.PATH_PUBSPEC_DOES_NOT_EXIST, [pathEntry]);
235-
}
236-
// todo (pq): test for presence of a lib dir.
248+
var pathEntry = _asString(dependency[PATH_FIELD]);
249+
if (pathEntry != null) {
250+
YamlNode pathKey() => getKey(dependency, PATH_FIELD);
251+
var context = provider.pathContext;
252+
var normalizedPath = context.joinAll(path.posix.split(pathEntry));
253+
var packageRoot = context.dirname(source.fullName);
254+
var dependencyPath = context.join(packageRoot, normalizedPath);
255+
dependencyPath = context.absolute(dependencyPath);
256+
dependencyPath = context.normalize(dependencyPath);
257+
var packageFolder = provider.getFolder(dependencyPath);
258+
if (!packageFolder.exists) {
259+
_reportErrorForNode(reporter, pathKey(),
260+
PubspecWarningCode.PATH_DOES_NOT_EXIST, [pathEntry]);
261+
} else {
262+
if (!packageFolder
263+
.getChild(AnalysisEngine.PUBSPEC_YAML_FILE)
264+
.exists) {
265+
_reportErrorForNode(reporter, pathKey(),
266+
PubspecWarningCode.PATH_PUBSPEC_DOES_NOT_EXIST, [pathEntry]);
237267
}
238268
}
269+
if (checkForPathAndGitDeps) {
270+
_reportErrorForNode(reporter, pathKey(),
271+
PubspecWarningCode.INVALID_DEPENDENCY, [PATH_FIELD]);
272+
}
273+
}
274+
275+
var gitEntry = dependency[GIT_FIELD];
276+
if (gitEntry != null && checkForPathAndGitDeps) {
277+
_reportErrorForNode(reporter, getKey(dependency, GIT_FIELD),
278+
PubspecWarningCode.INVALID_DEPENDENCY, [GIT_FIELD]);
239279
}
240280
}
241281
}

‎pkg/analyzer/lib/src/pubspec/pubspec_warning_code.dart‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,17 @@ class PubspecWarningCode extends ErrorCode {
5252
"The value of the 'flutter' field is expected to be a map.",
5353
correction: "Try converting the value to be a map.");
5454

55+
/// A code indicating that a versioned package has an invalid dependency (git
56+
/// or path).
57+
///
58+
/// Parameters:
59+
/// 0: the kind of dependency.
60+
static const PubspecWarningCode INVALID_DEPENDENCY = PubspecWarningCode(
61+
'INVALID_DEPENDENCY', "Publishable packages can't have {0} dependencies.",
62+
correction:
63+
"Try adding a 'publish_to: none' entry to mark the package as not "
64+
"for publishing or remove the {0} dependency.");
65+
5566
/// A code indicating that the name field is missing.
5667
static const PubspecWarningCode MISSING_NAME = PubspecWarningCode(
5768
'MISSING_NAME', "The name field is required but missing.",

‎pkg/analyzer/test/src/pubspec/pubspec_validator_test.dart‎

Lines changed: 143 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -196,6 +196,73 @@ dependencies:
196196
''');
197197
}
198198

199+
test_dependencyGit_malformed_empty() {
200+
// todo (pq): consider validating.
201+
assertNoErrors('''
202+
name: sample
203+
dependencies:
204+
foo:
205+
git:
206+
''');
207+
}
208+
209+
test_dependencyGit_malformed_list() {
210+
// todo (pq): consider validating.
211+
assertNoErrors('''
212+
name: sample
213+
dependencies:
214+
foo:
215+
git:
216+
- baz
217+
''');
218+
}
219+
220+
test_dependencyGit_malformed_scalar() {
221+
// todo (pq): consider validating.
222+
assertNoErrors('''
223+
name: sample
224+
dependencies:
225+
foo:
226+
git: baz
227+
''');
228+
}
229+
230+
test_dependencyGit_noVersion_valid() {
231+
assertNoErrors('''
232+
name: sample
233+
dependencies:
234+
foo:
235+
git:
236+
url: git@github.com:foo/foo.git
237+
path: path/to/foo
238+
''');
239+
}
240+
241+
test_dependencyGit_version_error() {
242+
assertErrors('''
243+
name: sample
244+
version: 0.1.0
245+
dependencies:
246+
foo:
247+
git:
248+
url: git@github.com:foo/foo.git
249+
path: path/to/foo
250+
''', [PubspecWarningCode.INVALID_DEPENDENCY]);
251+
}
252+
253+
test_dependencyGit_version_valid() {
254+
assertNoErrors('''
255+
name: sample
256+
version: 0.1.0
257+
publish_to: none
258+
dependencies:
259+
foo:
260+
git:
261+
url: git@github.com:foo/foo.git
262+
path: path/to/foo
263+
''');
264+
}
265+
199266
test_dependencyGitPath() {
200267
// git paths are not validated
201268
assertNoErrors('''
@@ -208,6 +275,40 @@ dependencies:
208275
''');
209276
}
210277

278+
test_dependencyPath_malformed_empty() {
279+
// todo (pq): consider validating.
280+
assertNoErrors('''
281+
name: sample
282+
dependencies:
283+
foo:
284+
path:
285+
''');
286+
}
287+
288+
test_dependencyPath_malformed_list() {
289+
// todo (pq): consider validating.
290+
assertNoErrors('''
291+
name: sample
292+
dependencies:
293+
foo:
294+
path:
295+
- baz
296+
''');
297+
}
298+
299+
test_dependencyPath_noVersion_valid() {
300+
newFolder('/foo');
301+
newFile('/foo/pubspec.yaml', content: '''
302+
name: foo
303+
''');
304+
assertNoErrors('''
305+
name: sample
306+
dependencies:
307+
foo:
308+
path: /foo
309+
''');
310+
}
311+
211312
test_dependencyPath_pubspecDoesNotExist() {
212313
newFolder('/foo');
213314
assertErrors('''
@@ -257,6 +358,35 @@ dependencies:
257358
''');
258359
}
259360

361+
test_dependencyPath_version_error() {
362+
newFolder('/foo');
363+
newFile('/foo/pubspec.yaml', content: '''
364+
name: foo
365+
''');
366+
assertErrors('''
367+
name: sample
368+
version: 0.1.0
369+
dependencies:
370+
foo:
371+
path: /foo
372+
''', [PubspecWarningCode.INVALID_DEPENDENCY]);
373+
}
374+
375+
test_dependencyPath_version_valid() {
376+
newFolder('/foo');
377+
newFile('/foo/pubspec.yaml', content: '''
378+
name: foo
379+
''');
380+
assertNoErrors('''
381+
name: sample
382+
version: 0.1.0
383+
publish_to: none
384+
dependencies:
385+
foo:
386+
path: /foo
387+
''');
388+
}
389+
260390
test_dependencyPathDoesNotExist_path_error() {
261391
assertErrors('''
262392
name: sample
@@ -288,6 +418,19 @@ dev_dependencies:
288418
''');
289419
}
290420

421+
test_devDependencyGit_version_no_error() {
422+
// Git paths are OK in dev_dependencies
423+
assertNoErrors('''
424+
name: sample
425+
version: 0.1.0
426+
dev_dependencies:
427+
foo:
428+
git:
429+
url: git@github.com:foo/foo.git
430+
path: path/to/foo
431+
''');
432+
}
433+
291434
test_devDependencyPathDoesNotExist_path_error() {
292435
assertErrors('''
293436
name: sample

0 commit comments

Comments
 (0)