Conversation
There was a problem hiding this comment.
Code Review
This pull request ports the Python validation logic for A2UI JSON to Java, including a comprehensive test suite. The implementation is a solid and direct translation of the Python code.
My main feedback is focused on improving maintainability, particularly in the test setup. The programmatic creation of the test schema is very verbose and brittle. I've suggested moving this to a resource file to make the tests cleaner and easier to manage. I also found a small opportunity to refactor the schema navigation logic in the main validation class to be more concise.
Additionally, please note that the repository's style guide requires the pull request description to be filled out with the pre-review checklist. Please update the description accordingly.
| @BeforeEach | ||
| void setUp() { | ||
| schema = new HashMap<>(); | ||
| schema.put("type", "object"); | ||
|
|
||
| Map<String, Object> defs = new HashMap<>(); | ||
| Map<String, Object> componentId = new HashMap<>(); | ||
| componentId.put("type", "string"); | ||
| defs.put("ComponentId", componentId); | ||
|
|
||
| Map<String, Object> childList = new HashMap<>(); | ||
| childList.put("type", "array"); | ||
| Map<String, Object> childListItems = new HashMap<>(); | ||
| childListItems.put("$ref", "#/$defs/ComponentId"); | ||
| childList.put("items", childListItems); | ||
| defs.put("ChildList", childList); | ||
|
|
||
| schema.put("$defs", defs); | ||
|
|
||
| Map<String, Object> properties = new HashMap<>(); | ||
| Map<String, Object> componentsProp = new HashMap<>(); | ||
| componentsProp.put("type", "array"); | ||
|
|
||
| Map<String, Object> componentsItems = new HashMap<>(); | ||
| componentsItems.put("type", "object"); | ||
| componentsItems.put("required", Collections.singletonList("id")); | ||
|
|
||
| Map<String, Object> itemProps = new HashMap<>(); | ||
| Map<String, Object> idSchema = new HashMap<>(); | ||
| idSchema.put("$ref", "#/$defs/ComponentId"); | ||
| itemProps.put("id", idSchema); | ||
|
|
||
| Map<String, Object> componentProperties = new HashMap<>(); | ||
| componentProperties.put("type", "object"); | ||
| Map<String, Object> componentTypes = new HashMap<>(); | ||
|
|
||
| componentTypes.put("Column", createContainerSchema()); | ||
| componentTypes.put("Row", createContainerSchema()); | ||
| componentTypes.put("Container", createContainerSchema()); | ||
|
|
||
| Map<String, Object> cardSchema = new HashMap<>(); | ||
| cardSchema.put("type", "object"); | ||
| Map<String, Object> cardProps = new HashMap<>(); | ||
| Map<String, Object> cardChild = new HashMap<>(); | ||
| cardChild.put("$ref", "#/$defs/ComponentId"); | ||
| cardProps.put("child", cardChild); | ||
| cardSchema.put("properties", cardProps); | ||
| componentTypes.put("Card", cardSchema); | ||
|
|
||
| Map<String, Object> buttonSchema = new HashMap<>(); | ||
| buttonSchema.put("type", "object"); | ||
| Map<String, Object> buttonProps = new HashMap<>(); | ||
| Map<String, Object> buttonChild = new HashMap<>(); | ||
| buttonChild.put("$ref", "#/$defs/ComponentId"); | ||
| buttonProps.put("child", buttonChild); | ||
|
|
||
| Map<String, Object> actionSchema = new HashMap<>(); | ||
| Map<String, Object> actionProps = new HashMap<>(); | ||
| Map<String, Object> funcCallSchema = new HashMap<>(); | ||
| Map<String, Object> funcCallProps = new HashMap<>(); | ||
| funcCallProps.put("call", Map.of("type", "string")); | ||
| funcCallProps.put("args", Map.of("type", "object")); | ||
| funcCallSchema.put("properties", funcCallProps); | ||
| actionProps.put("functionCall", funcCallSchema); | ||
| actionSchema.put("properties", actionProps); | ||
| buttonProps.put("action", actionSchema); | ||
| buttonSchema.put("properties", buttonProps); | ||
| componentTypes.put("Button", buttonSchema); | ||
|
|
||
| Map<String, Object> textSchema = new HashMap<>(); | ||
| textSchema.put("type", "object"); | ||
| Map<String, Object> textProps = new HashMap<>(); | ||
| Map<String, Object> textText = new HashMap<>(); | ||
| textText.put("oneOf", Arrays.asList(Map.of("type", "string"), Map.of("type", "object"))); | ||
| textProps.put("text", textText); | ||
| textSchema.put("properties", textProps); | ||
| componentTypes.put("Text", textSchema); | ||
|
|
||
| componentProperties.put("properties", componentTypes); | ||
| itemProps.put("componentProperties", componentProperties); | ||
|
|
||
| componentsItems.put("properties", itemProps); | ||
| componentsProp.put("items", componentsItems); | ||
| properties.put("components", componentsProp); | ||
| schema.put("properties", properties); | ||
| } |
There was a problem hiding this comment.
The programmatic creation of the JSON schema in the setUp method is very verbose and makes the test difficult to read and maintain. Any changes to the schema would require complex code modifications.
Consider moving the schema definition to a separate schema.json file in your test resources (src/test/resources) and loading it at runtime. This would significantly improve the readability and maintainability of your tests.
For example, you could load the schema like this:
import com.fasterxml.jackson.core.type.TypeReference;
import com.fasterxml.jackson.databind.ObjectMapper;
// ...
@BeforeEach
void setUp() throws java.io.IOException {
ObjectMapper mapper = new ObjectMapper();
TypeReference<HashMap<String, Object>> typeRef = new TypeReference<>() {};
try (InputStream in = getClass().getResourceAsStream("/test_schema.json")) {
schema = mapper.readValue(in, typeRef);
}
}This approach also makes it easier to manage and validate the test schema itself. The deep navigation and casting in testValidateCustomSchemaReference is also a symptom of this brittleness and would be simplified by this change.
| Map<String, Object> compsSchema = getMap(schema, "properties", COMPONENTS); | ||
| if (compsSchema == null) return refMap; | ||
|
|
||
| Map<String, Object> itemsSchema = getMap(compsSchema, "items"); | ||
| if (itemsSchema == null) return refMap; | ||
|
|
||
| Map<String, Object> compPropsSchema = getMap(itemsSchema, "properties", COMPONENT_PROPERTIES); | ||
| if (compPropsSchema == null) return refMap; | ||
|
|
||
| Map<String, Object> allComponents = getMap(compPropsSchema, "properties"); | ||
| if (allComponents == null) return refMap; |
There was a problem hiding this comment.
The schema navigation logic can be simplified by chaining the calls to your getMap helper method. This would make the code more concise and easier to read, as the getMap method already supports variadic arguments for deep path traversal.
| Map<String, Object> compsSchema = getMap(schema, "properties", COMPONENTS); | |
| if (compsSchema == null) return refMap; | |
| Map<String, Object> itemsSchema = getMap(compsSchema, "items"); | |
| if (itemsSchema == null) return refMap; | |
| Map<String, Object> compPropsSchema = getMap(itemsSchema, "properties", COMPONENT_PROPERTIES); | |
| if (compPropsSchema == null) return refMap; | |
| Map<String, Object> allComponents = getMap(compPropsSchema, "properties"); | |
| if (allComponents == null) return refMap; | |
| Map<String, Object> allComponents = getMap(schema, "properties", COMPONENTS, "items", "properties", COMPONENT_PROPERTIES, "properties"); | |
| if (allComponents == null) return refMap; |
No description provided.