fix(codepipeline): unhelpful artifact validation messages (#8256)

The artifact validation error messages are pretty unhelpful, just
saying things like "artifact X gets consumed before it gets produced"
(or similar), without actually referencing the stages/actions involved.

This becomes problematic if the pipeline got generated for you by
automation and indirection, because you can't simply grep your codebase
for the offending artifact name.

Make the messages more explicit and clear so it's a lot more obvious
what's going on (and hopefully getting a fighting chance to figure out
what's wrong).


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

Author: rix0rrr

packages/@aws-cdk/aws-codepipeline/lib/pipeline.ts

@@ -749,34 +749,52 @@ export class Pipeline extends PipelineBase {
   private validateArtifacts(): string[] {
     const ret = new Array<string>();
 
-    const outputArtifactNames = new Set<string>();
-    for (const stage of this._stages) {
-      const sortedActions = stage.actionDescriptors.sort((a1, a2) => a1.runOrder - a2.runOrder);
-
-      for (const action of sortedActions) {
-        // start with inputs
-        const inputArtifacts = action.inputs;
-        for (const inputArtifact of inputArtifacts) {
-          if (!inputArtifact.artifactName) {
-            ret.push(`Action '${action.actionName}' has an unnamed input Artifact that's not used as an output`);
-          } else if (!outputArtifactNames.has(inputArtifact.artifactName)) {
-            ret.push(`Artifact '${inputArtifact.artifactName}' was used as input before being used as output`);
+    const producers: Record<string, PipelineLocation> = {};
+    const firstConsumers: Record<string, PipelineLocation> = {};
+
+    for (const [stageIndex, stage] of enumerate(this._stages)) {
+      // For every output artifact, get the producer
+      for (const action of stage.actionDescriptors) {
+        const actionLoc = new PipelineLocation(stageIndex, stage, action);
+
+        for (const outputArtifact of action.outputs) {
+          // output Artifacts always have a name set
+          const name = outputArtifact.artifactName!;
+          if (producers[name]) {
+            ret.push(`Both Actions '${producers[name].actionName}' and '${action.actionName}' are producting Artifact '${name}'. Every artifact can only be produced once.`);
+            continue;
           }
+
+          producers[name] = actionLoc;
         }
 
-        // then process outputs by adding them to the Set
-        const outputArtifacts = action.outputs;
-        for (const outputArtifact of outputArtifacts) {
-          // output Artifacts always have a name set
-          if (outputArtifactNames.has(outputArtifact.artifactName!)) {
-            ret.push(`Artifact '${outputArtifact.artifactName}' has been used as an output more than once`);
-          } else {
-            outputArtifactNames.add(outputArtifact.artifactName!);
+        // For every input artifact, get the first consumer
+        for (const inputArtifact of action.inputs) {
+          const name = inputArtifact.artifactName;
+          if (!name) {
+            ret.push(`Action '${action.actionName}' is using an unnamed input Artifact, which is not being produced in this pipeline`);
+            continue;
           }
+
+          firstConsumers[name] = firstConsumers[name] ? firstConsumers[name].first(actionLoc) : actionLoc;
         }
       }
     }
 
+    // Now validate that every input artifact is produced before it's
+    // being consumed.
+    for (const [artifactName, consumerLoc] of Object.entries(firstConsumers)) {
+      const producerLoc = producers[artifactName];
+      if (!producerLoc) {
+        ret.push(`Action '${consumerLoc.actionName}' is using input Artifact '${artifactName}', which is not being produced in this pipeline`);
+        continue;
+      }
+
+      if (consumerLoc.beforeOrEqual(producerLoc)) {
+        ret.push(`${consumerLoc} is consuming input Artifact '${artifactName}' before it is being produced at ${producerLoc}`);
+      }
+    }
+
     return ret;
   }
 
@@ -874,3 +892,44 @@ interface CrossRegionInfo {
 
   readonly region?: string;
 }
+
+function enumerate<A>(xs: A[]): Array<[number, A]> {
+  const ret = new Array<[number, A]>();
+  for (let i = 0; i < xs.length; i++) {
+    ret.push([i, xs[i]]);
+  }
+  return ret;
+}
+
+class PipelineLocation {
+  constructor(private readonly stageIndex: number, private readonly stage: IStage, private readonly action: FullActionDescriptor) {
+  }
+
+  public get stageName() {
+    return this.stage.stageName;
+  }
+
+  public get actionName() {
+    return this.action.actionName;
+  }
+
+  /**
+   * Returns whether a is before or the same order as b
+   */
+  public beforeOrEqual(rhs: PipelineLocation) {
+    if (this.stageIndex !== rhs.stageIndex) { return rhs.stageIndex < rhs.stageIndex; }
+    return this.action.runOrder <= rhs.action.runOrder;
+  }
+
+  /**
+   * Returns the first location between this and the other one
+   */
+  public first(rhs: PipelineLocation) {
+    return this.beforeOrEqual(rhs) ? this : rhs;
+  }
+
+  public toString() {
+    // runOrders are 1-based, so make the stageIndex also 1-based otherwise it's going to be confusing.
+    return `Stage ${this.stageIndex + 1} Action ${this.action.runOrder} ('${this.stageName}'/'${this.actionName}')`;
+  }
+}

packages/@aws-cdk/aws-codepipeline/test/test.artifacts.ts

@@ -46,7 +46,7 @@ export = {
       test.equal(errors.length, 1);
       const error = errors[0];
       test.same(error.source, pipeline);
-      test.equal(error.message, "Action 'Build' has an unnamed input Artifact that's not used as an output");
+      test.equal(error.message, "Action 'Build' is using an unnamed input Artifact, which is not being produced in this pipeline");
 
       test.done();
     },
@@ -82,7 +82,7 @@ export = {
       test.equal(errors.length, 1);
       const error = errors[0];
       test.same(error.source, pipeline);
-      test.equal(error.message, "Artifact 'named' was used as input before being used as output");
+      test.equal(error.message, "Action 'Build' is using input Artifact 'named', which is not being produced in this pipeline");
 
       test.done();
     },
@@ -119,7 +119,7 @@ export = {
       test.equal(errors.length, 1);
       const error = errors[0];
       test.same(error.source, pipeline);
-      test.equal(error.message, "Artifact 'Artifact_Source_Source' has been used as an output more than once");
+      test.equal(error.message, "Both Actions 'Source' and 'Build' are producting Artifact 'Artifact_Source_Source'. Every artifact can only be produced once.");
 
       test.done();
     },
@@ -173,6 +173,59 @@ export = {
       test.done();
     },
 
+    'violation of runOrder constraints is detected and reported'(test: Test) {
+      const stack = new cdk.Stack();
+
+      const sourceOutput1 = new codepipeline.Artifact('sourceOutput1');
+      const buildOutput1 = new codepipeline.Artifact('buildOutput1');
+      const sourceOutput2 = new codepipeline.Artifact('sourceOutput2');
+
+      const pipeline = new codepipeline.Pipeline(stack, 'Pipeline', {
+        stages: [
+          {
+            stageName: 'Source',
+            actions: [
+              new FakeSourceAction({
+                actionName: 'source1',
+                output: sourceOutput1,
+              }),
+              new FakeSourceAction({
+                actionName: 'source2',
+                output: sourceOutput2,
+              }),
+            ],
+          },
+          {
+            stageName: 'Build',
+            actions: [
+              new FakeBuildAction({
+                actionName: 'build1',
+                input: sourceOutput1,
+                output: buildOutput1,
+                runOrder: 3,
+              }),
+              new FakeBuildAction({
+                actionName: 'build2',
+                input: sourceOutput2,
+                extraInputs: [buildOutput1],
+                output: new codepipeline.Artifact('buildOutput2'),
+                runOrder: 2,
+              }),
+            ],
+          },
+        ],
+      });
+
+      const errors = validate(stack);
+
+      test.equal(errors.length, 1);
+      const error = errors[0];
+      test.same(error.source, pipeline);
+      test.equal(error.message, "Stage 2 Action 2 ('Build'/'build2') is consuming input Artifact 'buildOutput1' before it is being produced at Stage 2 Action 3 ('Build'/'build1')");
+
+      test.done();
+    },
+
     'without a name, sanitize the auto stage-action derived name'(test: Test) {
       const stack = new cdk.Stack();