feat(QTDI-2215):deserialize a json to a Schema instance #1145
Conversation
* fix(QTDI-2215): Add schema/Entry pojo. --------- Co-authored-by: yyin-talend <yueyan.yin@qlik.com>
|
There was a problem hiding this comment.
Pull request overview
This PR adds JSON deserialization capability for Schema and Entry models, enabling the component server to parse JSON representations into Schema instances. The changes support comprehensive schema definition including all data types, nested structures, metadata entries, and logical types.
Changes:
- Added Schema and Entry model classes with Jackson deserialization support
- Implemented comprehensive test coverage for all schema types and field parameters
- Moved component-server-api dependency to test scope and added jackson-databind for testing
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| Schema.java | New model class representing schema structure with type, entries, metadata, and properties |
| Entry.java | New model class representing schema entry with support for nested schemas and typed default values |
| SchemaTest.java | Comprehensive test suite covering serialization/deserialization of all schema types and configurations |
| EntryTest.java | Unit tests for Entry class including JSON deserialization and accessor methods |
| pom.xml | Dependency reorganization moving component-server-api to test scope and adding jackson-databind |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public String getProp(final String key) { | ||
| return this.props.get(key); |
There was a problem hiding this comment.
The getProp method will throw NullPointerException if props is null. Add a null check before accessing props, or ensure props is never null in the constructor.
| this.valid = valid; | ||
| this.elementSchema = elementSchema; | ||
| this.comment = comment; | ||
| this.props.putAll(props); |
There was a problem hiding this comment.
The constructor will throw NullPointerException if the props parameter is null. Add a null check before calling putAll, e.g., if (props != null) { this.props.putAll(props); }
| this.props.putAll(props); | |
| if (props != null) { | |
| this.props.putAll(props); | |
| } |
| public String getOriginalFieldName() { | ||
| return rawName != null ? rawName : name; | ||
| } | ||
|
|
There was a problem hiding this comment.
Consider documenting that getProp returns null when the key doesn't exist. This is standard Map behavior but worth clarifying for API consumers.
| /** | |
| * Returns the property value associated with the given key. | |
| * | |
| * @param key the property key | |
| * @return the property value, or {@code null} if there is no mapping for the given key | |
| */ |
| private Object getInternalDefaultValue() { | ||
| return defaultValue; | ||
| } |
There was a problem hiding this comment.
The getInternalDefaultValue method appears to be a simple getter for the defaultValue field. Consider removing this method and accessing the field directly in getDefaultValue() to reduce unnecessary indirection.
ypiel-talend
left a comment
ypiel-talend
left a comment
There was a problem hiding this comment.
Some minor changes requested
| </dependency> | ||
| <dependency> | ||
| <groupId>org.talend.sdk.component</groupId> | ||
| <artifactId>component-server-api</artifactId> |
There was a problem hiding this comment.
Why it has been moved ?
| </dependency> | ||
|
|
||
| <dependency> | ||
| <groupId>com.fasterxml.jackson.core</groupId> |
There was a problem hiding this comment.
You can remove this dependency
| * | ||
| * @return true if input is null or ok. | ||
| */ | ||
| public boolean isCompatible(final Object input) { |
There was a problem hiding this comment.
Remove this method, it is not used.
Requirements
Why this PR is needed?
What does this PR adds (design/code thoughts)?