fix(sam): CfnFunction events are not rendered (#26679)

Because of a mistake introduced into the SAM schema, the `AlexaSkill` event type doesn't have any required properties anymore.

When the `CfnFunction` is trying all the different event types in the type union that it supports, it will go through every type in alphabetical order and pick the first type that doesn't fail its validation.

After the schema change, the first type (`Alexa` which starts with an `A`) would therefore accept all types: no required fields, and for JavaScript compatibility purposes we allow superfluous fields, and so we pick a type that doesn't render anything.

This change reorders the alternatives in the union such that stronger types are tried first.

`HttpApiEvent` and `AlexaSkillEvent` both have no required properties, and this now reverses the problem: `AlexaSkillEvent` can no longer be specified because `HttpApiEvent` will match first.

But that's the more common use case, so better for now, while we wait for the spec fix to come in, to prefer the HTTP API.

Relates to #26637.

----

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

packages/aws-cdk-lib/aws-sam/test/l1/sam.test.ts

@@ -0,0 +1,34 @@
+import * as assertions from '../../../assertions';
+import * as cdk from '../../../core';
+import * as sam from '../../lib';
+
+test('generation of alts from CfnFunction', () => {
+  const app = new cdk.App();
+  const stack = new cdk.Stack(app, 'Stack');
+  new sam.CfnFunction(stack, 'MyAPI', {
+    codeUri: 'build/',
+    events: {
+      GetResource: {
+        type: 'Api',
+        properties: {
+          restApiId: '12345',
+          path: '/myPath',
+          method: 'POST',
+        },
+      },
+    },
+  });
+
+  const template = assertions.Template.fromStack(stack);
+  template.hasResourceProperties('AWS::Serverless::Function', {
+    Events: {
+      GetResource: {
+        Properties: {
+          Method: 'POST',
+          Path: '/myPath',
+          RestApiId: '12345',
+        },
+      },
+    },
+  });
+});

tools/@aws-cdk/spec2cdk/jest.config.js

@@ -9,7 +9,7 @@ module.exports = {
   coverageThreshold: {
     global: {
       // Pretty bad but we disabled snapshots
-      branches: 40,
+      branches: 30,
     },
   },
 };

tools/@aws-cdk/spec2cdk/lib/cdk/cloudformation-mapping.ts

@@ -15,6 +15,8 @@ import {
 } from '@cdklabs/typewriter';
 import { CDK_CORE } from './cdk';
 import { PropertyValidator } from './property-validator';
+import { TypeConverter } from './type-converter';
+import { UnionOrdering } from './union-ordering';
 import { cfnParserNameFromType, cfnProducerNameFromType, cfnPropsValidatorNameFromType } from '../naming';
 
 export interface PropertyMapping {
@@ -34,7 +36,7 @@ export class CloudFormationMapping {
   private readonly cfn2ts: Record<string, string> = {};
   private readonly cfn2Prop: Record<string, PropertyMapping> = {};
 
-  constructor(private readonly mapperFunctionsScope: IScope) {}
+  constructor(private readonly mapperFunctionsScope: IScope, private readonly converter: TypeConverter) {}
 
   public add(mapping: PropertyMapping) {
     this.cfn2ts[mapping.cfnName] = mapping.propName;
@@ -181,7 +183,10 @@ export class CloudFormationMapping {
     }
 
     if (type.unionOfTypes) {
-      const innerProducers = type.unionOfTypes.map((t) => this.typeHandlers(t));
+      // Need access to the PropertyTypes to order these
+      const originalTypes = type.unionOfTypes.map((t) => this.converter.originalType(t));
+      const orderedTypes = new UnionOrdering(this.converter.db).orderTypewriterTypes(type.unionOfTypes, originalTypes);
+      const innerProducers = orderedTypes.map((t) => this.typeHandlers(t));
       const validators = innerProducers.map((p) => p.validate);
 
       return {

tools/@aws-cdk/spec2cdk/lib/cdk/resource-class.ts

@@ -97,7 +97,7 @@ export class ResourceClass extends ClassType {
    */
   public build() {
     // Build the props type
-    const cfnMapping = new CloudFormationMapping(this.module);
+    const cfnMapping = new CloudFormationMapping(this.module, this.converter);
 
     for (const prop of this.decider.propsProperties) {
       this.propsType.addProperty(prop.propertySpec);

tools/@aws-cdk/spec2cdk/lib/cdk/type-converter.ts

@@ -79,6 +79,9 @@ export class TypeConverter {
   private readonly typeDefinitionConverter: TypeDefinitionConverter;
   private readonly typeDefCache = new Map<TypeDefinition, StructType>();
 
+  /** Reverse mapping so we can find the original type back for every generated Type */
+  private readonly originalTypes = new WeakMap<Type, PropertyType>();
+
   constructor(options: TypeConverterOptions) {
     this.db = options.db;
     this.typeDefinitionConverter = options.typeDefinitionConverter;
@@ -101,35 +104,50 @@ export class TypeConverter {
     return new RichProperty(property).types();
   }
 
+  /**
+   * Convert a spec Type to a typewriter Type
+   */
   public typeFromSpecType(type: PropertyType): Type {
-    switch (type?.type) {
-      case 'string':
-        return Type.STRING;
-      case 'number':
-      case 'integer':
-        return Type.NUMBER;
-      case 'boolean':
-        return Type.BOOLEAN;
-      case 'date-time':
-        return Type.DATE_TIME;
-      case 'array':
-        return Type.arrayOf(this.typeFromSpecType(type.element));
-      case 'map':
-        return Type.mapOf(this.typeFromSpecType(type.element));
-      case 'ref':
-        const ref = this.db.get('typeDefinition', type.reference.$ref);
-        return this.convertTypeDefinitionType(ref).type;
-      case 'tag':
-        return CDK_CORE.CfnTag;
-      case 'union':
-        return Type.unionOf(...type.types.map((t) => this.typeFromSpecType(t)));
-      case 'null':
-        return Type.UNDEFINED;
-      case 'tag':
-        return CDK_CORE.CfnTag;
-      case 'json':
-        return Type.ANY;
+    const converted = ((): Type => {
+      switch (type?.type) {
+        case 'string':
+          return Type.STRING;
+        case 'number':
+        case 'integer':
+          return Type.NUMBER;
+        case 'boolean':
+          return Type.BOOLEAN;
+        case 'date-time':
+          return Type.DATE_TIME;
+        case 'array':
+          return Type.arrayOf(this.typeFromSpecType(type.element));
+        case 'map':
+          return Type.mapOf(this.typeFromSpecType(type.element));
+        case 'ref':
+          const ref = this.db.get('typeDefinition', type.reference.$ref);
+          return this.convertTypeDefinitionType(ref).type;
+        case 'tag':
+          return CDK_CORE.CfnTag;
+        case 'union':
+          return Type.unionOf(...type.types.map((t) => this.typeFromSpecType(t)));
+        case 'null':
+          return Type.UNDEFINED;
+        case 'tag':
+          return CDK_CORE.CfnTag;
+        case 'json':
+          return Type.ANY;
+      }
+    })();
+    this.originalTypes.set(converted, type);
+    return converted;
+  }
+
+  public originalType(type: Type): PropertyType {
+    const ret = this.originalTypes.get(type);
+    if (!ret) {
+      throw new Error(`Don't know original type for ${type}`);
     }
+    return ret;
   }
 
   public convertTypeDefinitionType(ref: TypeDefinition): TypeDeclaration {

tools/@aws-cdk/spec2cdk/lib/cdk/typedefinition-struct.ts

@@ -45,7 +45,7 @@ export class TypeDefinitionStruct extends StructType {
   }
 
   public build() {
-    const cfnMapping = new CloudFormationMapping(this.module);
+    const cfnMapping = new CloudFormationMapping(this.module, this.converter);
 
     const decider = new TypeDefinitionDecider(this.resource, this.typeDefinition, this.converter);
 

tools/@aws-cdk/spec2cdk/lib/cdk/union-ordering.ts

@@ -0,0 +1,120 @@
+import { PropertyType, RichPropertyType, SpecDatabase, TypeDefinition } from '@aws-cdk/service-spec-types';
+import { Type } from '@cdklabs/typewriter';
+import { isSubsetOf } from '../util/sets';
+import { topologicalSort } from '../util/toposort';
+
+/**
+ * Order types for use in a union
+ * Order the types such that the types with the most strength (i.e., excluding the most values from the type) are checked first
+ *
+ * This is necessary because at runtime, the union checker will iterate
+ * through the types one-by-one to check whether a value inhabits  a type, and
+ * it will stop at the first one that matches.
+ *
+ * We therefore shouldn't have the weakest type up front, because we'd pick the wrong type.
+ */
+export class UnionOrdering {
+  constructor(private readonly db: SpecDatabase) {}
+
+  /**
+   * Order typewriter Types based on the strength of the associated PropertyTypes
+   */
+  public orderTypewriterTypes(writerTypes: Type[], propTypes: PropertyType[]): Type[] {
+    if (writerTypes.length !== propTypes.length) {
+      throw new Error('Arrays need to be the same length');
+    }
+
+    const correspondence = new Map<PropertyType, Type>();
+    for (let i = 0; i < writerTypes.length; i++) {
+      correspondence.set(propTypes[i], writerTypes[i]);
+    }
+
+    return this.orderPropertyTypes(propTypes).map((t) => assertTruthy(correspondence.get(t)));
+  }
+
+  /**
+   * Order PropertyTypes, strongest first
+   */
+  public orderPropertyTypes(types: PropertyType[]): PropertyType[] {
+    // Map { X -> [Y] }, indicating that X is weaker than each of Y
+    const afterMap = new Map<PropertyType, PropertyType[]>(types.map((type) => [
+      type,
+      types.filter((other) => !new RichPropertyType(type).equals(other) && this.strongerThan(other, type)),
+    ]));
+    return topologicalSort(types, (t) => t, (t) => afterMap.get(t) ?? []);
+  }
+
+  /**
+   * Whether type A is strictly stronger than type B (and hence should be tried before B)
+   *
+   * Currently only specialized if both types are type declarations, otherwise we default
+   * to the general kind of the type.
+   */
+  private strongerThan(a: PropertyType, b: PropertyType) {
+    const aType = a.type === 'ref' ? this.db.get('typeDefinition', a.reference.$ref) : undefined;
+    const bType = b.type === 'ref' ? this.db.get('typeDefinition', b.reference.$ref) : undefined;
+    if (aType && bType) {
+      const aReq = requiredPropertyNames(aType);
+      const bReq = requiredPropertyNames(bType);
+
+      // If the required properties of A are a proper supserset of B, A goes first (== B is a proper subset of A)
+      const [aSubB, bSubA] = [isSubsetOf(aReq, bReq), isSubsetOf(bReq, aReq)];
+      if (aSubB !== bSubA) {
+        return bSubA;
+      }
+
+      // Otherwise, the one with more required properties goes first
+      if (aReq.size !== bReq.size) {
+        return aReq.size > bReq.size;
+      }
+
+      // Otherwise the one with the most total properties goes first
+      return Object.keys(aType.properties).length > Object.keys(bType.properties).length;
+    }
+    return basicKindStrength(a) < basicKindStrength(b);
+
+    /**
+     * Return an order for the kind of the type, lower is stronger.
+     */
+    function basicKindStrength(x: PropertyType): number {
+      switch (x.type) {
+        case 'array':
+          return 0;
+        case 'date-time':
+          return 1;
+        case 'string':
+          return 2;
+        case 'number':
+        case 'integer':
+          return 3;
+        case 'null':
+          return 4;
+        case 'boolean':
+          return 5;
+        case 'ref':
+          return 6;
+        case 'tag':
+          return 7;
+        case 'map':
+          // Must be higher than type declaration, because they will look the same in JS
+          return 8;
+        case 'union':
+          return 9;
+        case 'json':
+          // Must have the highest number of all
+          return 100;
+      }
+    }
+  }
+}
+
+function requiredPropertyNames(t: TypeDefinition): Set<string> {
+  return new Set(Object.entries(t.properties).filter(([_, p]) => p.required).map(([n, _]) => n));
+}
+
+function assertTruthy<T>(x: T): NonNullable<T> {
+  if (x == null) {
+    throw new Error('Expected truhty value');
+  }
+  return x;
+}

tools/@aws-cdk/spec2cdk/lib/util/sets.ts

@@ -0,0 +1,11 @@
+/**
+ * Whether A is a subset of B
+ */
+export function isSubsetOf<T>(as: Set<T>, bs: Set<T>) {
+  for (const a of as) {
+    if (!bs.has(a)) {
+      return false;
+    }
+  }
+  return true;
+}

tools/@aws-cdk/spec2cdk/lib/util/toposort.ts

@@ -0,0 +1,44 @@
+export type KeyFunc<T, K> = (x: T) => K;
+export type DepFunc<T, K> = (x: T) => K[];
+
+/**
+ * Return a topological sort of all elements of xs, according to the given dependency functions
+ *
+ * Dependencies outside the referenced set are ignored.
+ *
+ * Not a stable sort, but in order to keep the order as stable as possible, we'll sort by key
+ * among elements of equal precedence.
+ */
+export function topologicalSort<T, K=string>(xs: Iterable<T>, keyFn: KeyFunc<T, K>, depFn: DepFunc<T, K>): T[] {
+  const remaining = new Map<K, TopoElement<T, K>>();
+  for (const element of xs) {
+    const key = keyFn(element);
+    remaining.set(key, { key, element, dependencies: depFn(element) });
+  }
+
+  const ret = new Array<T>();
+  while (remaining.size > 0) {
+    // All elements with no more deps in the set can be ordered
+    const selectable = Array.from(remaining.values()).filter(e => e.dependencies.every(d => !remaining.has(d)));
+
+    selectable.sort((a, b) => a.key < b.key ? -1 : b.key < a.key ? 1 : 0);
+
+    for (const selected of selectable) {
+      ret.push(selected.element);
+      remaining.delete(selected.key);
+    }
+
+    // If we didn't make any progress, we got stuck
+    if (selectable.length === 0) {
+      throw new Error(`Could not determine ordering between: ${Array.from(remaining.keys()).join(', ')}`);
+    }
+  }
+
+  return ret;
+}
+
+interface TopoElement<T, K> {
+  key: K;
+  dependencies: K[];
+  element: T;
+}