Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a custom validation flow for A2UI version 0.9, refactoring the validate method to support version-specific logic and adding detailed error reporting for component and data model updates. The review feedback highlights several areas for improvement: optimizing performance by caching Draft202012Validator instances, refactoring duplicated integrity check logic into a shared helper, ensuring safe access to schema definitions to prevent potential KeyError exceptions, and adopting more robust string parsing for validation error messages.
| temp_schema = { | ||
| "$schema": "https://json-schema.org/draft/2020-12/schema", | ||
| "$ref": f"catalog.json#/components/{comp_type}" | ||
| } | ||
|
|
||
| validator = Draft202012Validator(temp_schema, registry=self._validator._registry) |
There was a problem hiding this comment.
Creating a new Draft202012Validator instance for every component in a loop is inefficient and will cause significant performance degradation for large payloads. These validators should be cached by comp_type.
cache = self.__dict__.setdefault("_comp_validator_cache", {})
if comp_type not in cache:
temp_schema = {
"$schema": "https://json-schema.org/draft/2020-12/schema",
"$ref": f"catalog.json#/components/{comp_type}"
}
cache[comp_type] = Draft202012Validator(
temp_schema, registry=self._validator._registry
)
validator = cache[comp_type]| for message in messages: | ||
| if not isinstance(message, dict): | ||
| continue | ||
|
|
||
| components = None | ||
| surface_id = None | ||
| if "surfaceUpdate" in message: # v0.8 | ||
| components = message["surfaceUpdate"].get(COMPONENTS) | ||
| surface_id = message["surfaceUpdate"].get("surfaceId") | ||
| elif "updateComponents" in message and isinstance( | ||
| if "updateComponents" in message and isinstance( | ||
| message["updateComponents"], dict | ||
| ): # v0.9 | ||
| ): | ||
| components = message["updateComponents"].get(COMPONENTS) | ||
| surface_id = message["updateComponents"].get("surfaceId") | ||
|
|
||
| if components: | ||
| ref_map = extract_component_ref_fields(self._catalog) | ||
| root_id = _find_root_id(messages, surface_id) | ||
| # Always check for basic integrity (duplicates) | ||
| _validate_component_integrity( | ||
| root_id, components, ref_map, skip_root_check=not strict_integrity | ||
| ) | ||
| # Always check topology (cycles), but only raise on orphans if strict_integrity is True | ||
| analyze_topology( | ||
| root_id, components, ref_map, raise_on_orphans=strict_integrity | ||
| ) | ||
|
|
||
| _validate_recursion_and_paths(message) |
There was a problem hiding this comment.
This block for performing integrity and topology checks is nearly identical to the logic in the validate method (lines 292-312). To improve maintainability and reduce duplication, consider refactoring this into a shared helper method that handles component extraction and validation for both protocol versions.
| if ("Unevaluated properties are not allowed" in message or "Additional properties are not allowed" in message) and "(" in message and ")" in message: | ||
| message = message[message.find("(")+1 : message.rfind(")")] |
There was a problem hiding this comment.
String-based parsing of error messages to extract property names is brittle and depends on the internal implementation details of the jsonschema library's error formatting. Consider using a regular expression or inspecting the ValidationError object's attributes for a more robust solution.
| if ("Unevaluated properties are not allowed" in message or "Additional properties are not allowed" in message) and "(" in message and ")" in message: | |
| message = message[message.find("(")+1 : message.rfind(")")] | |
| if err.validator in ("additionalProperties", "unevaluatedProperties"): | |
| match = re.search(r"\(([^)]+)\)", message) | |
| if match: | |
| message = match.group(1) |
|
There're also two build failures. Please fix them. |
2 participants
Running validate on the mcp apps recipe json.
BEFORE:
ValueError: Validation failed: {'version': 'v0.9', 'updateComponents': {'surfaceId': 'recipe-card', 'components': [{'id': 'root', 'component': 'Card', 'child': 'main-column'}, {'id': 'main-column', 'component': 'Column', 'children': ['recipe-image', 'content'], 'gap': 'small'}, {'id': 'recipe-image', 'component': 'Image', 'url': {'path': '/image'}, 'altText': {'path': '/title'}, 'fit': 'cover'}, {'id': 'content', 'component': 'Column', 'children': ['title', 'rating-row', 'times-row', 'servings'], 'gap': 'small'}, {'id': 'title', 'component': 'Text', 'text': {'path': '/title'}, 'usageHint': 'h3'}, {'id': 'rating-row', 'component': 'Row', 'children': ['star-icon', 'rating', 'review-count'], 'gap': 'small', 'alignment': 'center'}, {'id': 'star-icon', 'component': 'Icon', 'name': {'literalString': 'star'}}, {'id': 'rating', 'component': 'Text', 'text': {'path': '/rating'}, 'usageHint': 'body'}, {'id': 'review-count', 'component': 'Text', 'text': {'path': '/reviewCount'}, 'usageHint': 'caption'}, {'id': 'times-row', 'component': 'Row', 'children': ['prep-time', 'cook-time'], 'gap': 'medium'}, {'id': 'prep-time', 'component': 'Row', 'children': ['prep-icon', 'prep-text'], 'gap': 'small', 'alignment': 'center'}, {'id': 'prep-icon', 'component': 'Icon', 'name': {'literalString': 'timer'}}, {'id': 'prep-text', 'component': 'Text', 'text': {'path': '/prepTime'}, 'usageHint': 'caption'}, {'id': 'cook-time', 'component': 'Row', 'children': ['cook-icon', 'cook-text'], 'gap': 'small', 'alignment': 'center'}, {'id': 'cook-icon', 'component': 'Icon', 'name': {'literalString': 'shoppingCart'}}, {'id': 'cook-text', 'component': 'Text', 'text': {'path': '/cookTime'}, 'usageHint': 'caption'}, {'id': 'servings', 'component': 'Text', 'text': {'path': '/servings'}, 'usageHint': 'caption'}]}} is not valid under any of the given schemas
E Context failures:
E - 'createSurface' is a required property
E - Additional properties are not allowed ('updateComponents' was unexpected)
E - {'id': 'main-column', 'component': 'Column', 'children': ['recipe-image', 'content'], 'gap': 'small'} is not valid under any of the given schemas
E - {'id': 'recipe-image', 'component': 'Image', 'url': {'path': '/image'}, 'altText': {'path': '/title'}, 'fit': 'cover'} is not valid under any of the given schemas
E - {'id': 'content', 'component': 'Column', 'children': ['title', 'rating-row', 'times-row', 'servings'], 'gap': 'small'} is not valid under any of the given schemas
E - {'id': 'title', 'component': 'Text', 'text': {'path': '/title'}, 'usageHint': 'h3'} is not valid under any of the given schemas
E - {'id': 'rating-row', 'component': 'Row', 'children': ['star-icon', 'rating', 'review-count'], 'gap': 'small', 'alignment': 'center'} is not valid under any of the given schemas
E - {'id': 'star-icon', 'component': 'Icon', 'name': {'literalString': 'star'}} is not valid under any of the given schemas
E - {'id': 'rating', 'component': 'Text', 'text': {'path': '/rating'}, 'usageHint': 'body'} is not valid under any of the given schemas
E - {'id': 'review-count', 'component': 'Text', 'text': {'path': '/reviewCount'}, 'usageHint': 'caption'} is not valid under any of the given schemas
E - {'id': 'times-row', 'component': 'Row', 'children': ['prep-time', 'cook-time'], 'gap': 'medium'} is not valid under any of the given schemas
E - {'id': 'prep-time', 'component': 'Row', 'children': ['prep-icon', 'prep-text'], 'gap': 'small', 'alignment': 'center'} is not valid under any of the given schemas
E - {'id': 'prep-icon', 'component': 'Icon', 'name': {'literalString': 'timer'}} is not valid under any of the given schemas
E - {'id': 'prep-text', 'component': 'Text', 'text': {'path': '/prepTime'}, 'usageHint': 'caption'} is not valid under any of the given schemas
E - {'id': 'cook-time', 'component': 'Row', 'children': ['cook-icon', 'cook-text'], 'gap': 'small', 'alignment': 'center'} is not valid under any of the given schemas
E - {'id': 'cook-icon', 'component': 'Icon', 'name': {'literalString': 'shoppingCart'}} is not valid under any of the given schemas
E - {'id': 'cook-text', 'component': 'Text', 'text': {'path': '/cookTime'}, 'usageHint': 'caption'} is not valid under any of the given schemas
E - {'id': 'servings', 'component': 'Text', 'text': {'path': '/servings'}, 'usageHint': 'caption'} is not valid under any of the given schemas
E - 'updateDataModel' is a required property
E - Additional properties are not allowed ('updateComponents' was unexpected)
AFTER:
E ValueError: Validation failed:
E - messages[1].updateComponents.components[id='main-column']: 'gap' was unexpected
E - messages[1].updateComponents.components[id='recipe-image'].url: {'path': '/image'} is not of type 'string'
E - messages[1].updateComponents.components[id='recipe-image']: 'altText', 'fit' were unexpected
E - messages[1].updateComponents.components[id='content']: 'gap' was unexpected
E - messages[1].updateComponents.components[id='title']: 'usageHint' was unexpected
E - messages[1].updateComponents.components[id='rating-row']: Unknown component: Row
E - messages[1].updateComponents.components[id='star-icon'].name: {'literalString': 'star'} is not of type 'string'
E - messages[1].updateComponents.components[id='rating']: 'usageHint' was unexpected
E - messages[1].updateComponents.components[id='review-count']: 'usageHint' was unexpected
E - messages[1].updateComponents.components[id='times-row']: Unknown component: Row
E - messages[1].updateComponents.components[id='prep-time']: Unknown component: Row
E - messages[1].updateComponents.components[id='prep-icon'].name: {'literalString': 'timer'} is not of type 'string'
E - messages[1].updateComponents.components[id='prep-text']: 'usageHint' was unexpected
E - messages[1].updateComponents.components[id='cook-time']: Unknown component: Row
E - messages[1].updateComponents.components[id='cook-icon'].name: {'literalString': 'shoppingCart'} is not of type 'string'
E - messages[1].updateComponents.components[id='cook-text']: 'usageHint' was unexpected
E - messages[1].updateComponents.components[id='servings']: 'usageHint' was unexpected