Integrate jsoncons for robust JSON schema validation#1274
Integrate jsoncons for robust JSON schema validation#1274nan-yu wants to merge 21 commits intogoogle:mainfrom
Conversation
…rdized conformance test data files
…on file and remove legacy parser tests.
…idate utility tests
… schema manager test file
…ate test loader paths
…nt in conformance tests
… data directories
… move validation to anyOf variants
…and enable corresponding conformance tests
…alog structure to use standard $defs.
…chema-based reference field discovery
![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 comprehensive conformance test suite for the A2UI SDKs, covering parsing, validation, and catalog management across Python and C++ implementations. Key changes include the migration of the C++ validator to use the jsoncons library for more robust schema validation and the addition of extensive YAML-based test cases. Feedback focuses on improving the robustness of the C++ implementation, specifically regarding dependency management, error handling for unpredictable LLM outputs, performance optimizations in JSON library conversions, and the fragility of schema-based heuristics.
| GIT_TAG 2.4.0 | ||
| jsoncons | ||
| GIT_REPOSITORY https://github.com/danielaparker/jsoncons.git | ||
| GIT_TAG master |
There was a problem hiding this comment.
Using master as a GIT_TAG for an external dependency is risky as it makes builds non-reproducible and can introduce breaking changes unexpectedly. It is recommended to use a specific version tag or a commit hash.
GIT_TAG v0.176.0
| if (it == component_ref_fields_.end()) { | ||
| throw std::runtime_error("Schema analysis failed or component type unknown: " + comp_type); | ||
| } | ||
|
|
There was a problem hiding this comment.
Throwing a std::runtime_error when an unknown component type is encountered during streaming can cause the entire application to crash. Since LLM output can be unpredictable and may include hallucinated component types, it's safer to handle this gracefully (e.g., by skipping the component or logging a warning) rather than throwing an exception.
| if (it == component_ref_fields_.end()) { | |
| throw std::runtime_error("Schema analysis failed or component type unknown: " + comp_type); | |
| } | |
| auto it = component_ref_fields_.find(comp_type); | |
| if (it == component_ref_fields_.end()) { | |
| continue; | |
| } |
| void inject_additional_properties(jsoncons::json& obj, const jsoncons::json& source_properties) | ||
| { | ||
| if (obj.is_object()) | ||
| { |
There was a problem hiding this comment.
The brace style here (Allman style) is inconsistent with the rest of the file and the repository, which generally follows the K&R style (braces on the same line).
void inject_additional_properties(jsoncons::json& obj, const jsoncons::json& source_properties) {
if (obj.is_object()) {References
- Code should follow the relevant style guides for each language. (link)
| std::string json_str = message.dump(); | ||
| jsoncons::json cons_message = jsoncons::json::parse(json_str); |
There was a problem hiding this comment.
Converting nlohmann::json to jsoncons::json by dumping to a string and re-parsing is inefficient. Since this happens for every message in a potentially long stream, it could impact performance. Consider if jsoncons can be used more directly or if the conversion can be optimized.
| if (ref.rfind("ComponentId") != std::string::npos || | ||
| ref.rfind("child") != std::string::npos || | ||
| ref.find("/child") != std::string::npos) { |
There was a problem hiding this comment.
The heuristics used to identify component references (e.g., checking if a $ref contains "ComponentId" or "child") are fragile. This logic assumes specific naming conventions in the JSON schema which might not hold true if the schema is updated or if a custom catalog uses different names.
| return ss.str(); | ||
| } | ||
|
|
||
| static std::string glob_to_regex(const std::string& glob) { |
There was a problem hiding this comment.
The manual implementation of glob_to_regex for loading examples may not handle all edge cases of glob patterns (like escaping or complex character classes). While sufficient for simple test data, using a standard library or a more robust implementation would be safer for general path matching.
1 participant
Description
Fixes #1268.
Pre-launch Checklist
If you need help, consider asking for advice on the discussion board.