Skip to content

Update to param validator + test file #7194

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Aug 30, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 24 additions & 2 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 6 additions & 3 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@
"gifenc": "^1.0.3",
"libtess": "^1.2.2",
"omggif": "^1.0.10",
"opentype.js": "^1.3.1"
"opentype.js": "^1.3.1",
"zod-validation-error": "^3.3.1"
},
"devDependencies": {
"@rollup/plugin-commonjs": "^25.0.7",
Expand All @@ -47,11 +48,13 @@
"rollup": "^4.9.6",
"rollup-plugin-string": "^3.0.0",
"rollup-plugin-visualizer": "^5.12.0",
"typescript": "^5.5.4",
"unplugin-swc": "^1.4.2",
"vite": "^5.0.2",
"vite-plugin-string": "^1.2.2",
"vitest": "^2.0.4",
"webdriverio": "^8.38.2"
"webdriverio": "^8.38.2",
"zod": "^3.23.8"
},
"license": "LGPL-2.1",
"main": "./lib/p5.min.js",
Expand All @@ -78,4 +81,4 @@
"pre-commit": "lint-staged"
}
}
}
}
196 changes: 153 additions & 43 deletions src/core/friendly_errors/param_validator.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,57 @@
* @for p5
* @requires core
*/
// import p5 from '../main';
import dataDoc from '../../../docs/parameterData.json';
import p5 from '../main.js';
import * as constants from '../constants.js';
import { z } from 'zod';
import { fromError } from 'zod-validation-error';
import dataDoc from '../../../docs/parameterData.json' assert { type: 'json' };

// Cache for Zod schemas
let schemaRegistry = new Map();
const arrDoc = JSON.parse(JSON.stringify(dataDoc));

// Mapping names of p5 types to their constructor functions.
// p5Constructors:
// - Color: f()
// - Graphics: f()
// - Vector: f()
// and so on.
const p5Constructors = {};
// For speedup over many runs. `funcSpecificConstructors[func]` only has the
// constructors for types which were seen earlier as args of `func`.
const funcSpecificConstructors = {};

for (let [key, value] of Object.entries(p5)) {
p5Constructors[key] = value;
}

// window.addEventListener('load', () => {
// // Make a list of all p5 classes to be used for argument validation
// // This must be done only when everything has loaded otherwise we get
// // an empty array.
// for (let key of Object.keys(p5)) {
// // Get a list of all constructors in p5. They are functions whose names
// // start with a capital letter.
// if (typeof p5[key] === 'function' && key[0] !== key[0].toLowerCase()) {
// p5Constructors[key] = p5[key];
// }
// }
// });

// `constantsMap` maps constants to their values, e.g.
// {
// ADD: 'lighter',
// ALT: 18,
// ARROW: 'default',
// AUTO: 'auto',
// ...
// }
const constantsMap = {};
for (const [key, value] of Object.entries(constants)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a thought for our module restructuring effort, @limzykenneth when we're thinking of moving constants into their relevant modules instead of one big file, will we have to do anything special to continue supporting getting a big list of all of them?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm...that's a good question. It might be that module need to specifically register things as constants but I want to avoid that where possible to simplify things.

Copy link
Member

@limzykenneth limzykenneth Aug 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to solely rely on the documentation parsed data to get this list? We can possibly add extra tags or into into the JSDoc comments if needed to mark something as a constant.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might not be possibly to get it from the parameter data because the types only include the names of the constants, and we won't know from that what value they correspond to (like what specific symbol to check against)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we switch back to using strings, I think it could be doable though?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can possibly switch back to strings, the symbol stuff I'm not 100% sold on yet as I'm also not sure how or if it should work across multiple instances.

constantsMap[key] = value;
}

const schemaMap = {
'Any': z.any(),
'Array': z.array(z.any()),
Expand All @@ -26,6 +68,30 @@ const schemaMap = {
'String[]': z.array(z.string())
};

const webAPIObjects = [
'AudioNode',
'HTMLCanvasElement',
'HTMLElement',
'KeyboardEvent',
'MouseEvent',
'TouchEvent',
'UIEvent',
'WheelEvent'
];

function generateWebAPISchemas(apiObjects) {
return apiObjects.map(obj => {
return {
name: obj,
schema: z.custom((data) => data instanceof globalThis[obj], {
message: `Expected a ${obj}`
})
};
});
}

const webAPISchemas = generateWebAPISchemas(webAPIObjects);

/**
* This is a helper function that generates Zod schemas for a function based on
* the parameter data from `docs/parameterData.json`.
Expand All @@ -46,8 +112,6 @@ const schemaMap = {
*
* TODO:
* - [ ] Support for p5 constructors
* - [ ] Support for p5 constants
* - [ ] Support for generating multiple schemas for optional parameters
* - [ ] Support for more obscure types, such as `lerpPalette` and optional
* objects in `p5.Geometry.computeNormals()`
* (see https://github.com/processing/p5.js/pull/7186#discussion_r1724983249)
Expand All @@ -68,45 +132,90 @@ function generateZodSchemasForFunc(func) {
overloads = funcInfo.overloads;
}

const createParamSchema = param => {
// Returns a schema for a single type, i.e. z.boolean() for `boolean`.
const generateTypeSchema = type => {
// Type only contains uppercase letters and underscores -> type is a
// constant. Note that because we're ultimately interested in the value of
// the constant, mapping constants to their values via `constantsMap` is
// necessary.
if (/^[A-Z_]+$/.test(type)) {
return z.literal(constantsMap[type]);
} else if (schemaMap[type]) {
return schemaMap[type];
} else {
// TODO: Make this throw an error once more types are supported.
console.log(`Warning: Zod schema not found for type '${type}'. Skip mapping`);
return undefined;
}
};

// Generate a schema for a single parameter. In the case where a parameter can
// be of multiple types, `generateTypeSchema` is called for each type.
const generateParamSchema = param => {
const optional = param.endsWith('?');
param = param.replace(/\?$/, '');

let schema;

// Generate a schema for a single parameter that can be of multiple
// types / constants, i.e. `String|Number|Array`.
//
// Here, z.union() is used over z.enum() (which seems more intuitive) for
// constants for the following reasons:
// 1) z.enum() only allows a fixed set of allowable string values. However,
// our constants sometimes have numeric or non-primitive values.
// 2) In some cases, the type can be constants or strings, making z.enum()
// insufficient for the use case.
if (param.includes('|')) {
const types = param.split('|');

/*
* Note that for parameter types that are constants, such as for
* `blendMode`, the parameters are always all caps, sometimes with
* underscores, separated by `|`
* (i.e. "BLEND|DARKEST|LIGHTEST|DIFFERENCE|MULTIPLY|EXCLUSION|SCREEN|
* REPLACE|OVERLAY|HARD_LIGHT|SOFT_LIGHT|DODGE|BURN|ADD|REMOVE|SUBTRACT").
* Use a regex check here to filter them out and distinguish them from
* parameters that allow multiple types.
*/
return types.every(t => /^[A-Z_]+$/.test(t))
? z.enum(types)
: z.union(types
.filter(t => {
if (!schemaMap[t]) {
console.warn(`Warning: Zod schema not found for type '${t}'. Skip mapping`);
return false;
}
return true;
})
.map(t => schemaMap[t]));
schema = z.union(types
.map(t => generateTypeSchema(t))
.filter(s => s !== undefined));
} else {
schema = generateTypeSchema(param);
}

let schema = schemaMap[param];
return optional ? schema.optional() : schema;
};

const overloadSchemas = overloads.map(overload => {
// For now, ignore schemas that cannot be mapped to a defined type
return z.tuple(
overload
.map(p => createParamSchema(p))
.filter(schema => schema !== undefined)
// Note that in Zod, `optional()` only checks for undefined, not the absence
// of value.
//
// Let's say we have a function with 3 parameters, and the last one is
// optional, i.e. func(a, b, c?). If we only have a z.tuple() for the
// parameters, where the third schema is optional, then we will only be able
// to validate func(10, 10, undefined), but not func(10, 10), which is
// a completely valid call.
//
// Therefore, on top of using `optional()`, we also have to generate parameter
// combinations that are valid for all numbers of parameters.
const generateOverloadCombinations = params => {
// No optional parameters, return the original parameter list right away.
if (!params.some(p => p.endsWith('?'))) {
return [params];
}

const requiredParamsCount = params.filter(p => !p.endsWith('?')).length;
const result = [];

for (let i = requiredParamsCount; i <= params.length; i++) {
result.push(params.slice(0, i));
}

return result;
}

// Generate schemas for each function overload and merge them
const overloadSchemas = overloads.flatMap(overload => {
const combinations = generateOverloadCombinations(overload);

return combinations.map(combo =>
z.tuple(
combo
.map(p => generateParamSchema(p))
// For now, ignore schemas that cannot be mapped to a defined type
.filter(schema => schema !== undefined)
)
);
});

Expand Down Expand Up @@ -181,7 +290,7 @@ function findClosestSchema(schema, args) {
return score;
};

// Default to the first schema, so that we will always return something.
// Default to the first schema, so that we are guaranteed to return a result.
let closestSchema = schema._def.options[0];
// We want to return the schema with the lowest score.
let bestScore = Infinity;
Expand All @@ -198,25 +307,21 @@ function findClosestSchema(schema, args) {
return closestSchema;
}


/**
* Runs parameter validation by matching the input parameters to Zod schemas
* generated from the parameter data from `docs/parameterData.json`.
*
* TODO:
* - [ ] Turn it into a private method of `p5`.
*
* @param {String} func - Name of the function.
* @param {Array} args - User input arguments.
* @returns {Object} The validation result.
* @returns {Boolean} result.success - Whether the validation was successful.
* @returns {any} [result.data] - The parsed data if validation was successful.
* @returns {import('zod-validation-error').ZodValidationError} [result.error] - The validation error if validation failed.
*/
export function validateParams(func, args) {
// if (p5.disableFriendlyErrors) {
// return; // skip FES
// }
p5._validateParams = function validateParams(func, args) {
if (p5.disableFriendlyErrors) {
return; // skip FES
}

let funcSchemas = schemaRegistry.get(func);
if (!funcSchemas) {
Expand All @@ -242,7 +347,12 @@ export function validateParams(func, args) {
}
}

const result = validateParams('p5.fill', [1]);
p5.prototype._validateParams = p5._validateParams;
export default p5;

const result = p5._validateParams('arc', [200, 100, 100, 80, 0, Math.PI, 'pie']);
if (!result.success) {
console.log(result.error.toString());
} else {
console.log('Validation successful');
}
Loading
Loading