-
Notifications
You must be signed in to change notification settings - Fork 51
fix(QTDI-2215): Add schema/Entry pojo. #1146
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: yyin/QTDI-2215-JsonSchema
Are you sure you want to change the base?
fix(QTDI-2215): Add schema/Entry pojo. #1146
Conversation
ypiel-talend
left a comment
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 may have misled you with the code generation for Schema/ Entry. We should simplify hem more. I think we need a builder etc... Simply need to be able to deserialize json->instance.
|
|
||
| <build> | ||
| <plugins> | ||
| <plugin> |
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.
Why this configuration has been added ?
| @Getter | ||
| private final Object internalDefaultValue; | ||
|
|
||
| @ConstructorProperties({"name", "rawName", "type", "nullable", "metadata", "errorCapable", |
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.
Is that needed ?
| if (this.name.equals(newValue)) { | ||
| return this; | ||
| } | ||
| return new Entry( |
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.
Is that really needed ? Maybe just simple setter ? what do you think ?
| if (this.rawName.equals(newValue)) { | ||
| return this; | ||
| } | ||
| return new Entry( |
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.
Is that really needed ? Maybe just simple setter ? what do you think ?
| if (this.type == newValue) { | ||
| return this; | ||
| } | ||
| return new Entry( |
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.
Idem...
| } | ||
|
|
||
| @Override | ||
| public String toString() { |
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 think you can use lombok @DaTa it will automaticall y getter/setter/toString/hashCode et...
| .build(); | ||
| } | ||
|
|
||
| public static Builder builder() { |
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 don't think it is need. The goal is just to have a simple java POJO to desrialize a json to a java instance
|
|
||
| @Test | ||
| void deserializeEntryFromJson() throws Exception { | ||
| String json = """ |
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.
Maybe add component-runtime-impl in scope test, create entry/schema instances, then serialize them to json, and deserialize to the new class. We have to check it is compatible with Entry/Schema defined in component-runtime-impl at test test.

Requirements
Why this PR is needed?
What does this PR adds (design/code thoughts)?