From 044c0ed6fdecdd7dcb90287a539206f68297660e Mon Sep 17 00:00:00 2001 From: John Keiser Date: Thu, 13 Feb 2025 13:35:14 -0800 Subject: [PATCH] Add validation for required props - Also makes sure top level props are processed the same way as deep props --- bin/clover/src/cfDb.ts | 16 +-- bin/clover/src/spec/pkgs.ts | 2 +- bin/clover/src/spec/props.ts | 233 +++++++++++++++++++-------------- bin/clover/src/specPipeline.ts | 69 ++-------- 4 files changed, 161 insertions(+), 159 deletions(-) diff --git a/bin/clover/src/cfDb.ts b/bin/clover/src/cfDb.ts index 05165337a3..42ddeb3b11 100644 --- a/bin/clover/src/cfDb.ts +++ b/bin/clover/src/cfDb.ts @@ -39,19 +39,19 @@ export type CfProperty = ]; }>; -type CfBooleanProperty = JSONSchema.Boolean; +export type CfBooleanProperty = JSONSchema.Boolean; -type CfStringProperty = JSONSchema.String; +export type CfStringProperty = JSONSchema.String; -type CfNumberProperty = JSONSchema.Number & { +export type CfNumberProperty = JSONSchema.Number & { format?: string; }; -type CfIntegerProperty = JSONSchema.Integer & { +export type CfIntegerProperty = JSONSchema.Integer & { format?: string; }; -type CfArrayProperty = Extend; -type CfObjectProperty = Extend; // e.g. patternProperties: { "^[a-z]+": { type: "string" } } patternProperties?: Record; @@ -261,7 +261,7 @@ function isCfObjectProperty(prop: CfProperty): prop is CfObjectProperty { "patternProperties" in prop; } -export interface CfSchema extends JSONSchema.Interface { +export type CfSchema = Extend; }; propertyTransform?: Record; -} +}>; type CfDb = Record; const DB: CfDb = {}; diff --git a/bin/clover/src/spec/pkgs.ts b/bin/clover/src/spec/pkgs.ts index 3adac2c18d..a112d7c5b9 100644 --- a/bin/clover/src/spec/pkgs.ts +++ b/bin/clover/src/spec/pkgs.ts @@ -18,7 +18,7 @@ export type ExpandedSchemaVariantSpec = Extend; sockets: ExpandedSocketSpec[]; domain: ExpandedPropSpecFor["object"]; - secrets: ExpandedPropSpec; + secrets: ExpandedPropSpecFor["object"]; secretDefinition: ExpandedPropSpec | null; resourceValue: ExpandedPropSpecFor["object"]; }>; diff --git a/bin/clover/src/spec/props.ts b/bin/clover/src/spec/props.ts index 652d5bfb0a..f7d5dfd70f 100644 --- a/bin/clover/src/spec/props.ts +++ b/bin/clover/src/spec/props.ts @@ -6,6 +6,7 @@ import { PropSpecWidgetKind } from "../bindings/PropSpecWidgetKind.ts"; import _ from "npm:lodash"; import ImportedJoi from "joi"; import { Extend } from "../extend.ts"; +import { CfSchema } from "../cfDb.ts"; const { createHash } = await import("node:crypto"); export const CREATE_ONLY_PROP_LABEL = "si_create_only_prop"; @@ -64,83 +65,96 @@ interface PropSpecOverrides { joiValidation?: string; } -type CreatePropQueue = { - addTo: null | ((data: ExpandedPropSpec) => undefined); - name: string; - cfProp: CfProperty; +type CreatePropArgs = { propPath: string[]; -}[]; - -export function createPropFromCf( - name: string, - cfProp: CfProperty, - onlyProperties: OnlyProperties, - typeName: string, - propPath: string[], -): ExpandedPropSpec | undefined { - if (!cfProp.type) { - return undefined; - } + cfProp: CfProperty; + required?: boolean; + addTo?: (data: ExpandedPropSpec) => undefined; +}; +export type CreatePropQueue = { + cfSchema: CfSchema; + onlyProperties: OnlyProperties; + queue: CreatePropArgs[]; +}; - const queue: CreatePropQueue = [ - { - name, - cfProp, - addTo: null, - propPath, - }, - ]; +const MAX_PROP_DEPTH = 30; - let rootProp = undefined; +// Create top-level prop such as domain, resource_value, secrets, etc. +export function createDefaultPropFromCf( + name: DefaultPropType, + properties: Record, + cfSchema: CfSchema, + onlyProperties: OnlyProperties, +): ExpandedPropSpecFor["object"] { + // Enqueue the root prop only, and then iterate over its children + let rootProp: ExpandedPropSpecFor["object"] | undefined; + const queue: CreatePropQueue = { + cfSchema, + onlyProperties, + queue: [{ + propPath: ["root", name], + // Pretend the prop only has the specified properties (since we split it up) + cfProp: { ...cfSchema, properties }, + required: true, + addTo: (prop: ExpandedPropSpec) => { + if (prop.kind !== "object") { + throw new Error(`${name} prop is not an object`); + } + // Set "rootProp" before returning it + rootProp = prop; + }, + }], + }; - while (queue.length > 0) { - if (propPath.length > 10) { + while (queue.queue.length > 0) { + const propArgs = queue.queue.shift()!; + if (propArgs.propPath.length > MAX_PROP_DEPTH) { throw new Error( - `Prop tree loop detected: Tried creating prop more than 10 levels deep in the prop tree: ${propPath}`, + `Prop tree loop detected: Tried creating prop more than 10 levels deep in the prop tree: ${propArgs.propPath}`, ); } - const data = queue.shift(); - if (!data) break; - - const prop = createPropFromCfInner( - data.name, - data.cfProp, - onlyProperties, - typeName, - data.propPath, - queue, - ); + const prop = createPropFromCf(propArgs, queue); if (!prop) continue; - - if (!data.addTo) { - rootProp = prop; - } else { - data.addTo(prop); - } + if (propArgs.addTo) propArgs.addTo(prop); } if (!rootProp) { - console.log(cfProp); - throw new Error(`createProp for ${name} did not generate a prop`); + throw new Error( + `createProp for ${cfSchema.typeName} did not generate a ${name} prop`, + ); + } + + // TEMP uncomment this to ensure diffs stay the same before checkin + { + const { kind, data, name, entries, uniqueId, metadata } = rootProp; + data.hidden = null; + rootProp = { kind, data, name, entries, uniqueId, metadata }; } + // Top level prop doesn't actually get the generated doc; we add that to the schema instead + rootProp.data.docLink = null; + rootProp.data.documentation = null; + return rootProp; } +export function createDefaultProp( + name: DefaultPropType, +): ExpandedPropSpecFor["object"] { + return createObjectProp(name, ["root"]); +} + function createDocLink(typeName: string, propName: string): string | null { const typeNameSnake = typeName.split("::").slice(1).join("-").toLowerCase(); return `https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-${typeNameSnake}.html#cfn-${typeNameSnake}-${propName.toLowerCase()}`; } -function createPropFromCfInner( - name: string, - cfProp: CfProperty, - onlyProperties: OnlyProperties, - typeName: string, - propPath: string[], +function createPropFromCf( + { propPath, cfProp, required }: CreatePropArgs, queue: CreatePropQueue, ): ExpandedPropSpec | undefined { + const name = propPath[propPath.length - 1]; const propUniqueId = ulid(); const data: ExpandedPropSpec["data"] = { name, @@ -151,20 +165,19 @@ function createPropFromCfInner( widgetKind: null, widgetOptions: [], hidden: false, - docLink: createDocLink(typeName, name), + docLink: createDocLink(queue.cfSchema.typeName, name), documentation: cfProp.description ?? null, }; - propPath.push(name); const partialProp: Partial = { name, data, uniqueId: propUniqueId, metadata: { // cfProp, - createOnly: onlyProperties.createOnly.includes(name), - readOnly: onlyProperties.readOnly.includes(name), - writeOnly: onlyProperties.writeOnly.includes(name), - primaryIdentifier: onlyProperties.primaryIdentifier.includes(name), + createOnly: queue.onlyProperties.createOnly.includes(name), + readOnly: queue.onlyProperties.readOnly.includes(name), + writeOnly: queue.onlyProperties.writeOnly.includes(name), + primaryIdentifier: queue.onlyProperties.primaryIdentifier.includes(name), propPath, }, }; @@ -229,19 +242,31 @@ function createPropFromCfInner( `Unsupported number format: ${normalizedCfProp.format}`, ); } + if (required) { + validation += ".required()"; + } if (validation) { setJoiValidation(prop, `Joi.number()${validation}`); } return prop; } else if (normalizedCfProp.type === "boolean") { - const prop = partialProp as Extract; + const prop = partialProp as ExpandedPropSpecFor["boolean"]; prop.kind = "boolean"; prop.data.widgetKind = "Checkbox"; + // Add validation + let validation = ""; + if (required) { + validation += ".required()"; + } + if (validation) { + setJoiValidation(prop, `Joi.boolean()${validation}`); + } + return prop; } else if (normalizedCfProp.type === "string") { - const prop = partialProp as Extract; + const prop = partialProp as ExpandedPropSpecFor["string"]; prop.kind = "string"; if (normalizedCfProp.enum) { prop.data.widgetKind = "ComboBox"; @@ -300,13 +325,19 @@ function createPropFromCfInner( } if (normalizedCfProp.pattern !== undefined) { if ( - !shouldIgnorePattern(typeName, normalizedCfProp.pattern) + !shouldIgnorePattern( + queue.cfSchema.typeName, + normalizedCfProp.pattern, + ) ) { validation += `.pattern(new RegExp(${ JSON.stringify(normalizedCfProp.pattern) }))`; } } + if (required) { + validation += ".required()"; + } if (validation) { try { setJoiValidation(prop, `Joi.string()${validation}`); @@ -314,7 +345,7 @@ function createPropFromCfInner( if (normalizedCfProp.pattern !== undefined) { console.log( `If this is a regex syntax error, add this to IGNORE_PATTERNS: - ${JSON.stringify(typeName)}: [ ${ + ${JSON.stringify(queue.cfSchema.typeName)}: [ ${ JSON.stringify(normalizedCfProp.pattern) }, ], `, @@ -327,29 +358,38 @@ function createPropFromCfInner( return prop; } else if (normalizedCfProp.type === "json") { // TODO if this is gonna be json we should really check that it's valid json ... - const prop = partialProp as Extract; + const prop = partialProp as ExpandedPropSpecFor["string"]; prop.kind = "string"; prop.data.widgetKind = "TextArea"; + // Add validation + let validation = ""; + if (required) { + validation += ".required()"; + } + if (validation) { + setJoiValidation(prop, `Joi.string()${validation}`); + } + return prop; } else if (normalizedCfProp.type === "array") { - const prop = partialProp as Extract; + const prop = partialProp as ExpandedPropSpecFor["array"]; prop.kind = "array"; prop.data.widgetKind = "Array"; - queue.push({ + queue.queue.push({ + propPath: [...propPath, `${name}Item`], + cfProp: normalizedCfProp.items, + required: true, // If you *do* have an item which is an object, this enables validation for required fields addTo: (data: ExpandedPropSpec) => { prop.typeProp = data; }, - name: `${name}Item`, - cfProp: normalizedCfProp.items, - propPath: _.clone(propPath), }); return prop; } else if (normalizedCfProp.type === "object") { if (normalizedCfProp.patternProperties) { - const prop = partialProp as Extract; + const prop = partialProp as ExpandedPropSpecFor["map"]; prop.kind = "map"; prop.data.widgetKind = "Map"; @@ -373,40 +413,49 @@ function createPropFromCfInner( throw new Error("could not extract type from pattern prop"); } - queue.push({ + queue.queue.push({ + cfProp, + propPath: [...propPath, `${name}Item`], + required: true, // If you *do* have an item which is an object, this enables validation for required fields addTo: (data: ExpandedPropSpec) => { prop.typeProp = data; }, - name: `${name}Item`, - cfProp, - propPath: _.clone(propPath), }); return prop; } else if (normalizedCfProp.properties) { - const prop = partialProp as Extract; + const prop = partialProp as ExpandedPropSpecFor["object"]; prop.kind = "object"; prop.data.widgetKind = "Header"; prop.entries = []; - - Object.entries(normalizedCfProp.properties).forEach( - ([objName, objProp]) => { - queue.push({ - addTo: (data: ExpandedPropSpec) => { - prop.entries.push(data); - }, - name: objName, - cfProp: objProp, - propPath: _.clone(propPath), - }); - }, - ); + for ( + const [name, childCfProp] of Object.entries(normalizedCfProp.properties) + ) { + queue.queue.push({ + cfProp: childCfProp, + propPath: [...propPath, name], + // Object fields are only required if the object itself is required + required: required && normalizedCfProp.required?.includes(name), + addTo: (childProp: ExpandedPropSpec) => { + prop.entries.push(childProp); + }, + }); + } return prop; } else { - const prop = partialProp as Extract; + const prop = partialProp as ExpandedPropSpecFor["string"]; prop.kind = "string"; prop.data.widgetKind = "Text"; + // Add validation + let validation = ""; + if (required) { + validation += ".required()"; + } + if (validation) { + setJoiValidation(prop, `Joi.string()${validation}`); + } + return prop; } } @@ -485,12 +534,6 @@ function setCreateOnlyProp(data: ExpandedPropSpec["data"]) { export type DefaultPropType = "domain" | "secrets" | "resource_value"; -export function createDefaultProp( - type: DefaultPropType, -): Extract { - return createObjectProp(type, ["root"]); -} - export function createObjectProp( name: string, parentPath: string[], diff --git a/bin/clover/src/specPipeline.ts b/bin/clover/src/specPipeline.ts index ed566da063..650142971c 100644 --- a/bin/clover/src/specPipeline.ts +++ b/bin/clover/src/specPipeline.ts @@ -1,11 +1,6 @@ import { CfProperty, CfSchema } from "./cfDb.ts"; import { ulid } from "https://deno.land/x/ulid@v0.3.0/mod.ts"; -import { - createDefaultProp, - createPropFromCf, - DefaultPropType, - OnlyProperties, -} from "./spec/props.ts"; +import { createDefaultPropFromCf, OnlyProperties } from "./spec/props.ts"; import { ExpandedPkgSpec, ExpandedSchemaSpec, @@ -33,9 +28,16 @@ export function pkgSpecFromCf(src: CfSchema): ExpandedPkgSpec { primaryIdentifier: normalizeOnlyProperties(src.primaryIdentifier), }; - const domain = createDomainFromSrc(src, onlyProperties); + const domain = createDefaultPropFromCf( + "domain", + pruneDomainValues(src.properties, onlyProperties), + src, + onlyProperties, + ); - const resourceValue = createResourceValueFromSrc( + const resourceValue = createDefaultPropFromCf( + "resource_value", + pruneResourceValues(src.properties, onlyProperties), src, onlyProperties, ); @@ -61,7 +63,7 @@ export function pkgSpecFromCf(src: CfSchema): ExpandedPkgSpec { siPropFuncs: [], managementFuncs: [], domain, - secrets: createDefaultProp("secrets"), + secrets: createDefaultPropFromCf("secrets", {}, src, onlyProperties), secretDefinition: null, resourceValue, rootPropFuncs: [], @@ -102,50 +104,6 @@ function versionFromDate(): string { return new Date().toISOString().replace(/[-:T.Z]/g, "").slice(0, 14); } -function createDomainFromSrc( - src: CfSchema, - onlyProperties: OnlyProperties, -) { - return createRootFromProperties( - "domain", - pruneDomainValues(src.properties, onlyProperties), - onlyProperties, - src.typeName, - ); -} - -function createResourceValueFromSrc( - src: CfSchema, - onlyProperties: OnlyProperties, -) { - return createRootFromProperties( - "resource_value", - pruneResourceValues(src.properties, onlyProperties), - onlyProperties, - src.typeName, - ); -} - -function createRootFromProperties( - root_name: DefaultPropType, - properties: Record, - onlyProperties: OnlyProperties, - typeName: string, -) { - const root = createDefaultProp(root_name); - Object.entries(properties).forEach(([name, cfData]) => { - const newProp = createPropFromCf(name, cfData, onlyProperties, typeName, [ - ...root.metadata.propPath, - ]); - - if (!newProp) return; - - root.entries.push(newProp); - }); - - return root; -} - // Remove all read only props from this list, since readonly props go on the // resource value tree function pruneDomainValues( @@ -159,7 +117,8 @@ function pruneDomainValues( const readOnlySet = new Set(onlyProperties.readOnly); return Object.fromEntries( Object.entries(properties) - .filter(([name]) => !readOnlySet.has(name)), + // TODO we shouldn't be ignoring things just because "type" isn't set + .filter(([name, prop]) => prop.type && !readOnlySet.has(name)), ); } @@ -174,7 +133,7 @@ function pruneResourceValues( const readOnlySet = new Set(onlyProperties.readOnly); return Object.fromEntries( Object.entries(properties) - .filter(([name]) => readOnlySet.has(name)), + .filter(([name, prop]) => prop.type && readOnlySet.has(name)), ); }