From df6770e16a111ee993893a7234bf743c366b2d28 Mon Sep 17 00:00:00 2001 From: Dillon Nys <24740863+dnys1@users.noreply.github.com> Date: Tue, 12 Sep 2023 12:34:06 -0700 Subject: [PATCH] fix(aft): Constraints edge cases (#3732) Fixes a couple bugs noticed in the publish this morning and updates constraints in the repo. --- packages/aft/lib/src/constraints_checker.dart | 215 ++++++++++++------ .../aft/test/constraints_checker_test.dart | 67 +++++- .../aft/test/helpers/package_descriptor.dart | 32 +-- .../auth/amplify_auth_cognito/pubspec.yaml | 3 +- .../amplify_auth_cognito_test/pubspec.yaml | 8 +- 5 files changed, 235 insertions(+), 90 deletions(-) diff --git a/packages/aft/lib/src/constraints_checker.dart b/packages/aft/lib/src/constraints_checker.dart index 2a7019a7e2..9cae1a9118 100644 --- a/packages/aft/lib/src/constraints_checker.dart +++ b/packages/aft/lib/src/constraints_checker.dart @@ -4,8 +4,10 @@ import 'package:aft/aft.dart'; import 'package:aws_common/aws_common.dart'; import 'package:collection/collection.dart'; +import 'package:path/path.dart' as p; import 'package:pub_semver/pub_semver.dart'; import 'package:pubspec_parse/pubspec_parse.dart'; +import 'package:yaml/yaml.dart'; typedef MismatchedDependency = ({ PackageInfo package, @@ -26,7 +28,7 @@ sealed class ConstraintsChecker { bool checkConstraints(PackageInfo package); /// Populated by [checkConstraints] if a constraint check fails. - List get mismatchedDependencies; + final Set mismatchedDependencies = {}; /// Processes the [expectedConstraint] for [dependencyPath] in [package]. /// @@ -36,7 +38,7 @@ sealed class ConstraintsChecker { bool _processConstraint({ required PackageInfo package, required List dependencyPath, - required VersionConstraint expectedConstraint, + required Dependency expectedConstraint, required String errorMessage, }) { switch (action) { @@ -53,7 +55,7 @@ sealed class ConstraintsChecker { case ConstraintsAction.update: package.pubspecInfo.pubspecYamlEditor.update( dependencyPath, - expectedConstraint.toString(), + expectedConstraint.toYaml(), ); return true; } @@ -70,9 +72,6 @@ final class GlobalConstraintChecker extends ConstraintsChecker { final Map globalConstraints; final Environment globalEnvironment; - @override - final List mismatchedDependencies = []; - /// Checks the package's dependency constraints against the global config. bool _checkDependency( PackageInfo package, @@ -99,7 +98,7 @@ final class GlobalConstraintChecker extends ConstraintsChecker { return _processConstraint( package: package, dependencyPath: [dependencyType.key, dependencyName], - expectedConstraint: globalConstraint, + expectedConstraint: HostedDependency(version: globalConstraint), errorMessage: 'Expected $globalConstraint\n' 'Found $currentConstraint', ); @@ -126,7 +125,7 @@ final class GlobalConstraintChecker extends ConstraintsChecker { _processConstraint( package: package, dependencyPath: ['environment', 'sdk'], - expectedConstraint: globalSdkConstraint, + expectedConstraint: HostedDependency(version: globalSdkConstraint), errorMessage: 'Expected $globalSdkConstraint\n' 'Found $localSdkConstraint', ); @@ -143,7 +142,9 @@ final class GlobalConstraintChecker extends ConstraintsChecker { _processConstraint( package: package, dependencyPath: ['environment', 'flutter'], - expectedConstraint: globalFlutterConstraint, + expectedConstraint: HostedDependency( + version: globalFlutterConstraint, + ), errorMessage: 'Expected $globalFlutterConstraint\n' 'Found $localFlutterConstraint', ); @@ -210,9 +211,6 @@ final class PublishConstraintsChecker extends ConstraintsChecker { final Map> repoGraph; - @override - final List mismatchedDependencies = []; - /// Returns the intersection of all [constraints]. VersionConstraint _intersection(Iterable constraints) { var constraint = VersionConstraint.any; @@ -222,6 +220,112 @@ final class PublishConstraintsChecker extends ConstraintsChecker { return constraint; } + /// Checks the [constraint] on [dependency] in [package] given its + /// [dependencyType]. + void _checkConstraint( + PackageInfo package, + PackageInfo dependency, + DependencyType dependencyType, + Dependency constraint, + _DependencyConstraintMap allConstraints, + ) { + switch (( + packageIsPublished: package.isPublishable, + dependencyIsPublished: dependency.isPublishable, + type: dependencyType, + )) { + /// Publishable packages listed in the `dependencies` block must have a hosted constraint. + case ( + dependencyIsPublished: true, + packageIsPublished: _, + type: DependencyType.dependency, + ) || + + /// Publishable packages must list other publishable packages with a hosted constraint. + ( + dependencyIsPublished: true, + packageIsPublished: true, + type: _, + ): + switch (constraint) { + case HostedDependency(:final version): + allConstraints.recordConstraint( + dependency: dependency, + package: package, + constraint: version, + ); + default: + _processConstraint( + package: package, + dependencyPath: [dependencyType.key, dependency.name], + expectedConstraint: HostedDependency( + version: package.isPublishable + ? dependency.currentConstraint + : VersionConstraint.any, + ), + errorMessage: + 'Invalid constraint type: ${constraint.runtimeType}. ' + 'A hosted dependency is required when listing any publishable ' + 'package in the `dependencies` block.', + ); + } + + /// Published packages must list unpublished packages with a path constraint. + case ( + packageIsPublished: true, + dependencyIsPublished: false, + type: + DependencyType.devDependency || DependencyType.dependencyOverride, + ): + switch (constraint) { + case PathDependency _: + return; + default: + _processConstraint( + package: package, + dependencyPath: [dependencyType.key, dependency.name], + expectedConstraint: PathDependency( + p.relative(dependency.path, from: package.path), + ), + errorMessage: 'Invalid constraint on unpublished package. ' + 'A path dependency is required when listing any unpublished ' + 'package in the `dev_dependencies` block of a published package.', + ); + } + + /// A published package cannot take a dependency on an unpublished package + /// anywhere but the `dev_dependencies` block. + case ( + packageIsPublished: true, + dependencyIsPublished: false, + type: DependencyType.dependency, + ): + throw AssertionError( + 'Non-publishable package (${dependency.name}) found in ' + 'the `dependencies` block of ${package.name}.', + ); + + /// Unpublished packages can depend on other unpublished packages in + /// any way they like without it affecting the pub validator. + case ( + packageIsPublished: false, + dependencyIsPublished: false, + type: _, + ): + return; + + /// Unpublished packages' `dev_dependencies` and `dependency_overrides` + /// blocks are not checked by the pub validator. + case ( + dependencyIsPublished: true, + packageIsPublished: false, + type: + DependencyType.devDependency || DependencyType.dependencyOverride, + ): + return; + } + } + @override bool checkConstraints(PackageInfo package) { if (!package.isPublishable) { @@ -249,56 +353,21 @@ final class PublishConstraintsChecker extends ConstraintsChecker { }; for (final MapEntry( key: dependencyName, - value: (dependencyType, dependency) + value: (dependencyType, constraint) ) in dependencies.entries) { - final repoDependency = repoGraph.keys.singleWhereOrNull( + final dependency = repoGraph.keys.singleWhereOrNull( (pkg) => pkg.name == dependencyName, ); - if (repoDependency == null) { + if (dependency == null) { continue; } - switch (dependency) { - case HostedDependency(version: final constraint): - allConstraints.recordConstraint( - repoDependency: repoDependency, - inPackage: package, - constraint: constraint, - ); - - // Do not verify the constraints in the `dev_dependencies` - // block since this only affects the `pub` algorithm transitively. - case _ when dependencyType == DependencyType.devDependency: - return; - - // Do not verify the constraint of non-publishable packages listed - // in the `dependencies` block of other non-publishable packages. - // We only care about publishable dependencies listed in the `dependencies` - // block of non-publishable packages. - case _ when !repoDependency.isPublishable: - if (package.isPublishable) { - throw AssertionError( - 'Non-publishable package ($dependencyName) found in ' - 'the `dependencies` block of ${package.name}.', - ); - } - return; - - // Otherwise, we have a constraint which might cause an error. This - // is most often caused by path dependency on a publishable package - // in a test package's `dependencies` block. - case _: - _processConstraint( - package: package, - dependencyPath: [dependencyType.key, dependencyName], - expectedConstraint: package.isPublishable - ? repoDependency.currentConstraint - : VersionConstraint.any, - errorMessage: - 'Invalid constraint type: ${dependency.runtimeType}. ' - 'A hosted dependency is required when listing any publishable ' - 'package in the `dependencies` block.', - ); - } + _checkConstraint( + package, + dependency, + dependencyType, + constraint, + allConstraints, + ); } }, ); @@ -320,9 +389,11 @@ final class PublishConstraintsChecker extends ConstraintsChecker { package.dependencyType(repoDependency)!.key, repoDependency.name, ], - expectedConstraint: package.isPublishable - ? repoDependency.currentConstraint - : VersionConstraint.any, + expectedConstraint: HostedDependency( + version: package.isPublishable + ? repoDependency.currentConstraint + : VersionConstraint.any, + ), errorMessage: 'Constraint for dependency causes an empty intersection ' 'for ${rootPackage.name}: $constraint', @@ -339,13 +410,13 @@ final class _DependencyConstraintMap with AWSDebuggable, AWSSerializable> { _DependencyConstraintMap() : super({}); - /// Records the [constraint] for [repoDependency] found in [inPackage]. + /// Records the [constraint] for [dependency] found in [package]. void recordConstraint({ - required PackageInfo repoDependency, - required PackageInfo inPackage, + required PackageInfo package, + required PackageInfo dependency, required VersionConstraint constraint, }) { - (this[repoDependency] ??= _ConstraintMap())[inPackage] = constraint; + (this[dependency] ??= _ConstraintMap())[package] = constraint; } @override @@ -379,3 +450,19 @@ extension on PackageInfo { max: version.nextMinor, ); } + +extension DependencyToYaml on Dependency { + YamlNode toYaml() => switch (this) { + HostedDependency(:final version) => YamlScalar.wrap(version.toString()), + PathDependency(:final path) => YamlMap.wrap({'path': path}), + SdkDependency(:final sdk) => YamlMap.wrap({'sdk': sdk}), + GitDependency(:final url, :final ref, :final path) => YamlMap.wrap({ + 'git': { + 'url': url.toString(), + if (ref != null) 'ref': ref, + if (path != null) 'path': path, + }, + }), + _ => throw StateError('Invalid dependency: $this'), + }; +} diff --git a/packages/aft/test/constraints_checker_test.dart b/packages/aft/test/constraints_checker_test.dart index eb5e7864f4..f8baf2915a 100644 --- a/packages/aft/test/constraints_checker_test.dart +++ b/packages/aft/test/constraints_checker_test.dart @@ -132,7 +132,9 @@ void main() { 'amplify_core': '>=1.0.0 <1.1.0', }, devDependencies: { - 'amplify_test': 'any', + 'amplify_test': { + 'path': '../amplify_test', + }, }, ), ]).create(); @@ -204,6 +206,69 @@ void main() { } }, ); + + test( + '$result when a published package lists an unpublished package ' + 'as a hosted dependency', () async { + final repo = await d.repo([ + d.package('amplify_test', publishable: false), + d.package( + 'amplify_core', + version: '1.0.0', + devDependencies: { + 'amplify_test': 'any', + }, + ), + ]).create(); + + final constraintsChecker = PublishConstraintsChecker( + action, + repo.getPackageGraph(includeDevDependencies: true), + ); + + switch (action) { + case ConstraintsAction.apply || ConstraintsAction.update: + expect( + constraintsChecker.checkConstraints(repo.amplifyCore), + isTrue, + ); + expect( + repo.amplifyCore.pubspecInfo.pubspecYamlEditor.edits.single, + isA().having( + (edit) => edit.replacement, + 'replacement', + '{path: ../amplify_test}', + ), + ); + expect(constraintsChecker.mismatchedDependencies, isEmpty); + case ConstraintsAction.check: + expect( + constraintsChecker.checkConstraints(repo.amplifyCore), + isFalse, + reason: 'The constraint amplify_core has on amplify_test would ' + 'cause a publish error since amplify_test cannot be retrieved ' + "from pub.dev (it's unpublished)", + ); + expect( + constraintsChecker.mismatchedDependencies.single, + isA() + .having( + (err) => err.package.name, + 'packageName', + 'amplify_core', + ) + .having( + (err) => err.dependencyName, + 'dependencyName', + 'amplify_test', + ), + ); + expect( + repo.amplifyTest.pubspecInfo.pubspecYamlEditor.edits, + isEmpty, + ); + } + }); } }); } diff --git a/packages/aft/test/helpers/package_descriptor.dart b/packages/aft/test/helpers/package_descriptor.dart index 5f6ef3f5fe..c6a94e3973 100644 --- a/packages/aft/test/helpers/package_descriptor.dart +++ b/packages/aft/test/helpers/package_descriptor.dart @@ -2,8 +2,10 @@ // SPDX-License-Identifier: Apache-2.0 import 'package:aft/aft.dart'; +import 'package:aft/src/constraints_checker.dart'; import 'package:collection/collection.dart'; import 'package:pub_semver/pub_semver.dart'; +import 'package:pubspec_parse/src/dependency.dart'; import 'package:test/test.dart'; import 'package:test_descriptor/test_descriptor.dart' as d; import 'package:yaml_edit/yaml_edit.dart'; @@ -14,8 +16,8 @@ PackageDescriptor package( String? version, bool publishable = true, String? sdkConstraint, - Map dependencies = const {}, - Map devDependencies = const {}, + Map dependencies = const {}, + Map devDependencies = const {}, List contents = const [], }) => PackageDescriptor( @@ -41,8 +43,8 @@ final class PackageDescriptor extends d.Descriptor { String? version, bool publishable = true, String? sdkConstraint, - Map dependencies = const {}, - Map devDependencies = const {}, + Map dependencies = const {}, + Map devDependencies = const {}, List contents = const [], }) { return PackageDescriptor._( @@ -56,18 +58,8 @@ final class PackageDescriptor extends d.Descriptor { final sdkConstraint? => VersionConstraint.parse(sdkConstraint), _ => null, }, - dependencies: dependencies.map( - (name, constraint) => MapEntry( - name, - VersionConstraint.parse(constraint), - ), - ), - devDependencies: devDependencies.map( - (name, constraint) => MapEntry( - name, - VersionConstraint.parse(constraint), - ), - ), + dependencies: parseDeps(dependencies), + devDependencies: parseDeps(devDependencies), contents: contents, ); } @@ -85,8 +77,8 @@ final class PackageDescriptor extends d.Descriptor { final Version? version; final bool publishable; final VersionConstraint? sdkConstraint; - final Map dependencies; - final Map devDependencies; + final Map dependencies; + final Map devDependencies; /// The contents of the package directory. /// @@ -106,7 +98,7 @@ environment: '''); void addConstraints( - Map constraints, + Map constraints, DependencyType type, ) { if (constraints.isNotEmpty) { @@ -114,7 +106,7 @@ environment: } for (final MapEntry(key: dep, value: constraint) in constraints.entries) { final path = [type.key, dep]; - pubspecEditor.update(path, constraint.toString()); + pubspecEditor.update(path, constraint.toYaml()); } } diff --git a/packages/auth/amplify_auth_cognito/pubspec.yaml b/packages/auth/amplify_auth_cognito/pubspec.yaml index 2aea374432..62ff765af1 100644 --- a/packages/auth/amplify_auth_cognito/pubspec.yaml +++ b/packages/auth/amplify_auth_cognito/pubspec.yaml @@ -33,7 +33,8 @@ dependencies: plugin_platform_interface: ^2.0.0 dev_dependencies: - amplify_auth_cognito_test: any + amplify_auth_cognito_test: + path: ../amplify_auth_cognito_test amplify_lints: ">=3.0.0 <3.1.0" flutter_test: sdk: flutter diff --git a/packages/auth/amplify_auth_cognito_test/pubspec.yaml b/packages/auth/amplify_auth_cognito_test/pubspec.yaml index 1457023afa..6608e869f5 100644 --- a/packages/auth/amplify_auth_cognito_test/pubspec.yaml +++ b/packages/auth/amplify_auth_cognito_test/pubspec.yaml @@ -11,7 +11,7 @@ dependencies: amplify_core: any amplify_secure_storage_dart: any async: ^2.10.0 - aws_common: ">=0.4.0 <0.5.0" + aws_common: any built_collection: ^5.0.0 built_value: ">=8.6.0 <8.7.0" collection: ^1.15.0 @@ -21,14 +21,14 @@ dependencies: json_rpc_2: ^3.0.0 shelf: ^1.4.0 shelf_web_socket: ^1.0.0 - smithy: ">=0.4.0+1 <0.5.0" - smithy_aws: ">=0.4.0+1 <0.5.0" + smithy: any + smithy_aws: any stream_channel: ^2.0.0 stream_transform: ^2.0.0 test: ^1.22.1 web_socket_channel: ^2.3.0 webdriver: ^3.0.0 - worker_bee: ">=0.1.3+2 <0.2.0" + worker_bee: any dev_dependencies: amplify_lints: