fix(QTDI-2215): Add schema/Entry pojo. #1146
Conversation
ypiel-talend
left a comment
ypiel-talend
left a comment
There was a problem hiding this comment.
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.
Why this configuration has been added ?
There was a problem hiding this comment.
because Entry need more than 10 parameters.
There was a problem hiding this comment.
Ok, usually we can do some exception by configuration but it seems not possible for number of parameter as you probably already checked. I had the same issue few days ago, and instead of duplicate all the configuration from the parent I have simply deactivate checkstyle on the methode. In the Entry class you can do:
@ConstructorProperties({ "name", "rawName", "type", "nullable", "metadata", "errorCapable",
"valid", "elementSchema", "comment", "props", "internalDefaultValue" })
// Checkstyle off to let have 11 parameters to this constructor (normally 10 max)
// CHECKSTYLE:OFF
public Entry(
final String name,
final String rawName,
final Schema.Type type,
final boolean nullable,
final boolean metadata,
final boolean errorCapable,
final boolean valid,
final Schema elementSchema,
final String comment,
final Map<String, String> props,
final Object internalDefaultValue) {
// CHECKSTYLE:ON
It is to authorize an exception, the issue with copying the whole configuration is that if we change something the parent, it will not be reflected in this module.
.../component-server-model/src/main/java/org/talend/sdk/component/server/front/model/Entry.java
Outdated
Show resolved
Hide resolved
| if (this.name.equals(newValue)) { | ||
| return this; | ||
| } | ||
| return new Entry( |
There was a problem hiding this comment.
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.
Is that really needed ? Maybe just simple setter ? what do you think ?
| if (this.type == newValue) { | ||
| return this; | ||
| } | ||
| return new Entry( |
| } | ||
|
|
||
| @Override | ||
| public String toString() { |
There was a problem hiding this comment.
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.
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.
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.
…ent-runtime into ypiel/QTDI-2215-JsonSchema # Conflicts: # component-server-parent/component-server/src/test/java/org/talend/sdk/component/server/front/SchemaTest.java
ypiel-talend
left a comment
ypiel-talend
left a comment
There was a problem hiding this comment.
I add some comment and did few commit mainly to improve unit tests
|
|
||
| <build> | ||
| <plugins> | ||
| <plugin> |
There was a problem hiding this comment.
Ok, usually we can do some exception by configuration but it seems not possible for number of parameter as you probably already checked. I had the same issue few days ago, and instead of duplicate all the configuration from the parent I have simply deactivate checkstyle on the methode. In the Entry class you can do:
@ConstructorProperties({ "name", "rawName", "type", "nullable", "metadata", "errorCapable",
"valid", "elementSchema", "comment", "props", "internalDefaultValue" })
// Checkstyle off to let have 11 parameters to this constructor (normally 10 max)
// CHECKSTYLE:OFF
public Entry(
final String name,
final String rawName,
final Schema.Type type,
final boolean nullable,
final boolean metadata,
final boolean errorCapable,
final boolean valid,
final Schema elementSchema,
final String comment,
final Map<String, String> props,
final Object internalDefaultValue) {
// CHECKSTYLE:ON
It is to authorize an exception, the issue with copying the whole configuration is that if we change something the parent, it will not be reflected in this module.
| private final Map<String, String> props; | ||
|
|
||
| @ConstructorProperties({"type", "elementSchema", "entries", "metadata", "props"}) | ||
| public Schema( |
There was a problem hiding this comment.
Maybe add a java doc to explain that this class is only for deserialization usage and should be used as a DTO not as a real TCK schema since it doesn't implement SChama interface and doesn't have some business rule as the SchemaImpl from component-runtime-impl has.
|
Requirements
Why this PR is needed?
What does this PR adds (design/code thoughts)?