-
Notifications
You must be signed in to change notification settings - Fork 809
Refactor FunctionCall to enable full validation #551
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
base: main
Are you sure you want to change the base?
Conversation
Each function in the `functions` object fully specifies the function, including all validation requirements. Prior to this change, the `length` function (for example) could have specified an invalid type for the min or max (e.g., "five" instead of "5"), or a negative value for `min` or `max`. It is now possible to fully validate the parameters and return type of each function. A new set of tests has been created for all the existing functions. Third parties can add their own functions by following the steps in the a2ui_custom_functions.md document.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a significant improvement by refactoring FunctionCall to enable full schema validation, which enhances the robustness and extensibility of the A2UI protocol. The addition of a2ui_custom_functions.md provides clear guidance for developers looking to add their own functions. The new test suite for function validation is also a great addition.
I have a few minor suggestions to improve consistency in the schema definitions and tests.
Additionally, as per the repository's style guide (lines 18-20), please ensure the pull request description is filled out and the pre-review checklist is completed. This helps with tracking and context for future reference.
| ] | ||
| } | ||
| }, | ||
| "required": ["call"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency with other function definitions in this document and in standard_catalog.json, the getScreenResolution function example should require the args property. Even when a function has optional or no arguments, requiring an args property (which can be an empty array) makes the function call structure uniform and more predictable for implementers.
| "required": ["call"] | |
| "required": ["call", "args"] |
| "args": { | ||
| "type": "array", | ||
| "minItems": 2, | ||
| "maxItems": 2, | ||
| "prefixItems": [ | ||
| { | ||
| "$ref": "common_types.json#/$defs/DynamicNumber", | ||
| "description": "The numeric value used to determine the plural category." | ||
| }, | ||
| "required": [ | ||
| "other" | ||
| ], | ||
| "additionalProperties": false | ||
| } | ||
| ], | ||
| "minItems": 2, | ||
| "maxItems": 2 | ||
| } | ||
| { | ||
| "type": "object", | ||
| "description": "A map of CLDR plural categories to their corresponding strings.", | ||
| "properties": { | ||
| "zero": { | ||
| "$ref": "common_types.json#/$defs/DynamicString", | ||
| "description": "String for the 'zero' category (e.g., 0 items)." | ||
| }, | ||
| "one": { | ||
| "$ref": "common_types.json#/$defs/DynamicString", | ||
| "description": "String for the 'one' category (e.g., 1 item)." | ||
| }, | ||
| "two": { | ||
| "$ref": "common_types.json#/$defs/DynamicString", | ||
| "description": "String for the 'two' category (used in Arabic, Welsh, etc.)." | ||
| }, | ||
| "few": { | ||
| "$ref": "common_types.json#/$defs/DynamicString", | ||
| "description": "String for the 'few' category (e.g., small groups in Slavic languages)." | ||
| }, | ||
| "many": { | ||
| "$ref": "common_types.json#/$defs/DynamicString", | ||
| "description": "String for the 'many' category (e.g., large groups in various languages)." | ||
| }, | ||
| "other": { | ||
| "$ref": "common_types.json#/$defs/DynamicString", | ||
| "description": "The default/fallback string (used for general plural cases)." | ||
| } | ||
| }, | ||
| "required": [ | ||
| "other" | ||
| ], | ||
| "additionalProperties": false | ||
| } | ||
| ] | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency with other function definitions that take a fixed number of arguments (like regex, length, numeric, etc.), please add "items": false to the args schema for the pluralize function. While maxItems: 2 already prevents additional arguments, using items: false is more explicit and aligns with the pattern used elsewhere in this file.
"args": {
"type": "array",
"minItems": 2,
"maxItems": 2,
"prefixItems": [
{
"$ref": "common_types.json#/$defs/DynamicNumber",
"description": "The numeric value used to determine the plural category."
},
{
"type": "object",
"description": "A map of CLDR plural categories to their corresponding strings.",
"properties": {
"zero": {
"$ref": "common_types.json#/$defs/DynamicString",
"description": "String for the 'zero' category (e.g., 0 items)."
},
"one": {
"$ref": "common_types.json#/$defs/DynamicString",
"description": "String for the 'one' category (e.g., 1 item)."
},
"two": {
"$ref": "common_types.json#/$defs/DynamicString",
"description": "String for the 'two' category (used in Arabic, Welsh, etc.)."
},
"few": {
"$ref": "common_types.json#/$defs/DynamicString",
"description": "String for the 'few' category (e.g., small groups in Slavic languages)."
},
"many": {
"$ref": "common_types.json#/$defs/DynamicString",
"description": "String for the 'many' category (e.g., large groups in various languages)."
},
"other": {
"$ref": "common_types.json#/$defs/DynamicString",
"description": "The default/fallback string (used for general plural cases)."
}
},
"required": [
"other"
],
"additionalProperties": false
}
],
"items": false
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| "description": "pluralize: Invalid dictionary (missing 'other')", | ||
| "valid": false, | ||
| "data": { | ||
| "updateComponents": { | ||
| "surfaceId": "test", | ||
| "components": [ | ||
| { | ||
| "id": "txt1", | ||
| "component": "Text", | ||
| "text": { | ||
| "call": "pluralize", | ||
| "args": [5, { "one": "item" }], | ||
| "returnType": "string" | ||
| } | ||
| } | ||
| ] | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| }, | ||
| // 2. Allow the custom functions | ||
| { | ||
| "$ref": "https://example.com/schemas/custom_catalog.json#/$defs/CustomFunctions" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just inline your functions here, no need for another file
here is an example
| "oneOf": [ | ||
| // 1. Allow all standard functions | ||
| { | ||
| "$ref": "https://a2ui.dev/specification/v0_9/standard_catalog.json#/$defs/StandardFunctions" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its just functions in the standard catalog
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$defs/StandardFunctions provides some abstraction. If someone (for whatever reason) wants to only include a subset of the functions referenced there, they could do something like:
"oneOf": [
{ "$ref": "https://a2ui.dev/specification/v0_9/standard_catalog.json#/functions/required" },
{ "$ref": "https://a2ui.dev/specification/v0_9/standard_catalog.json#/functions/regex" },
...,
{ "$ref": "#/functions/trim" },
{ "$ref": "#/functions/getScreenResolution" }
]
@wrenj -- I could add this as an alternative in the document if that would make sense?
| "propertyName": "component" | ||
| } | ||
| }, | ||
| "StandardFunctions": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we just call these functions
I'm trying to get away from standard SDK vs customizations thinking, since the standard componnts are just a starter set and we want to encourage people to override them or do their own thing, lmk if that makes sense
| "$ref": "https://example.com/schemas/custom_catalog.json#/$defs/CustomFunctions" | ||
| } | ||
|
|
||
| // 3. (Optional) Allow fallback for other functions if strict validation isn't desired |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets get rid of this
the idea with a2ui is everything is strongly typed through json schema and the custom catalog, so we dont need a generic function, since you just extend the standard catalog with your custom functionality
| "functions": [ | ||
| { | ||
| "name": "required", | ||
| "functions": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't we just put these directly in functioncall now?
I'm a little foncused it seems like we have
Common->FunctionCall
Standard Catalog -> functions
ApplicationCatalog -> FunctionCall redefinition
Can we just do
StandardCatalog->FunctionCall
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue with putting them inline in FunctionCall is that we would lose the extensibility. E.g., right now, we define FunctionCall and we tell it that these are the options (via the "oneOf": [ { "$ref": "standard_catalog.json#/$defs/StandardFunctions" }]. So a 3rd party can redefine FunctionCall and add more functions (as described in the a2ui_custom_functions.md file).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our approach to custom catalog requires that all our generic schemas are not specific to any catalog, and they refer to the specifics of the current catalog via some placeholder name. E.g. catalog.json#/$defs/anyComponent or maybe catalog.json#/$defs/anyFunction. I realised I think we made a mistake here currently because server_to_client.json currently refers to standard_catalog.json#/$defs/anyComponent which isn't quite right I think. Sending #563 to propose fixing this.
By using anyComponent, we get a bit of JSON-level validation because we force the name to be valid, and the set of params to be valid.
I don't think we get the same value in this PR, because functions are intended to be more flexible in how they are instantiated, e.g. all the parameters can be dynamic, they're positional parameters instead of named parameters etc, so there just isn't as much validation that is possible. So given v0.9 is already agreed upon, I'd prefer we keep things as-is.
But if we do go in this direction, then I'd love to try to make functions work as similar to components, as James suggested. So perhaps this might involve jumping to named parameters etc.
| { | ||
| "id": "txt1", | ||
| "component": "Text", | ||
| "text": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do these work on standard catalog or just the application specific catalog?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests work against the functions defined in the standard catalog.
If a 3rd party wanted to add their own functions, and write their own tests, they would have to make some changes to the run_tests.py, but it should all work.
| "minimum": 0, | ||
| "description": "The minimum allowed length." | ||
| "properties": { | ||
| "call": { "const": "length" }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency can we model these the exact same way as components? with the function name as the key instead of using "call"? Maybe dont need to do now, greg might have looked at this approach earlier not sure why he didn't use the same pattern.
"components": {
"Text": { ... }
"Icon": { ... }
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it's hard to see in this diff, but that is how the functions are modeled:
"functions": {
"regex": {
"type": "object",
"description": "Checks that the value matches a regular expression string.",
. . .
},
}
`StandardFunctions` --> `Functions` Remove duplicate test Modify docs
| }, | ||
| "required": ["call"] | ||
| "required": ["call"], | ||
| "oneOf": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we just leave this oneOf out of common_types.json, then step 2 in the documentation is entirely unnecessary. Couldn't you instead add a required block at the end of the list of functions in the catalog that requires at least one of the the functions, so you don't need the separate Functions definition? I'd like to find a way where the catalog contains all of the validation needed, and we don't need to customize the common types for each client.
Vastly simplifies how new functions are added as valid for the `FunctionCall`, removing the need to copy/paste `FunctionCall` in the user's schema.
Each function in the
functionsobject fully specifies the function, including all validation requirements.Prior to this change, the
lengthfunction (for example) could have specified an invalid type for the min or max (e.g., "five" instead of "5"), or a negative value forminormax.It is now possible to fully validate the parameters and return type of each function. A new set of tests has been created for all the existing functions.
Third parties can add their own functions by following the steps in the a2ui_custom_functions.md document.
Description
Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.
List which issues are fixed by this PR. For larger changes, raising an issue first helps reduce redundant work.
Pre-launch Checklist
If you need help, consider asking for advice on the discussion board.