feat(QTDI-1291): support guess schema as service#1220
feat(QTDI-1291): support guess schema as service#1220ozhelezniak-talend wants to merge 3 commits into
Conversation
dc85e41 to
32db9eb
Compare
c5ddc35 to
c983d31
Compare
undx
left a comment
There was a problem hiding this comment.
Please add tests in ActionResourceImplTest for server side changes.
| private String subCode; | ||
|
|
There was a problem hiding this comment.
We've a model change, we need to specify it in jira issue (pinging @acatoire) but, apparently, it doesn't affect TCOMP-api-test.
There was a problem hiding this comment.
Pull request overview
This PR updates schema discovery to better support “guess schema as a service”, shifting DI-side schema guessing away from action-based invocation, and enhancing component-server error and metadata handling to better distinguish schema parameters and expose richer schema-guess failure info to clients.
Changes:
- Simplify
TaCoKitGuessSchema(DI) to focus on schema discovery through component execution results/lifecycle, and update associated tests. - Add a
definition::parameter::schemametadata marker inPropertiesServiceto identifySchema-typed action parameters. - Extend server error payloads with an optional
subCodeand attempt to surfaceDiscoverSchemaExceptionhandling details to clients.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| component-studio/component-runtime-di/src/main/java/org/talend/sdk/component/runtime/di/schema/TaCoKitGuessSchema.java | Removes action-based guessing entry points; adds lifecycle-based schema discovery method and simplifies flow. |
| component-studio/component-runtime-di/src/test/java/org/talend/sdk/component/runtime/di/schema/TaCoKitGuessSchemaTest.java | Updates tests to match the new guessing entry points and expected behavior. |
| component-studio/component-runtime-di/src/test/java/org/talend/sdk/component/runtime/di/schema/TypeConversionTest.java | Updates constructor usage after TaCoKitGuessSchema signature changes. |
| component-server-parent/component-server/src/main/java/org/talend/sdk/component/server/service/PropertiesService.java | Refactors property-building logic and adds schema-parameter metadata marker. |
| component-server-parent/component-server/src/main/java/org/talend/sdk/component/server/front/ActionResourceImpl.java | Adds DiscoverSchemaException handling and attempts to return richer error info (subCode). |
| component-server-parent/component-server-model/src/main/java/org/talend/sdk/component/server/front/model/error/ErrorPayload.java | Adds subCode field and a compatibility constructor for existing call sites. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (re instanceof final DiscoverSchemaException eSchema) { | ||
| // we send reason to recognize the error on client side | ||
| final String subCode = ofNullable(eSchema.getPossibleHandleErrorWith()) | ||
| .orElse(HandleErrorWith.EXCEPTION) | ||
| .toString(); | ||
| throw new WebApplicationException(Response | ||
| .status(ce.getErrorOrigin() == ComponentException.ErrorOrigin.USER ? 400 | ||
| : ce.getErrorOrigin() == ComponentException.ErrorOrigin.BACKEND ? 456 : 520, | ||
| "Unexpected callback error") | ||
| .entity(new ErrorPayload(ErrorDictionary.ACTION_ERROR, | ||
| "Action execution failed with: " + ofNullable(re.getMessage()) | ||
| .orElseGet(() -> re instanceof NullPointerException ? "unexpected null" | ||
| : "no error message"))) | ||
| .status(400, subCode) | ||
| .entity(new ErrorPayload(ErrorDictionary.ACTION_ERROR, subCode, description)) | ||
| .build()); |
There was a problem hiding this comment.
@undx Do you know why we throw an exception instead just return the Result?
| throw new WebApplicationException(Response | ||
| .status(evaluateStatusCodeForException(eComponent), "Unexpected callback error") | ||
| .entity(new ErrorPayload(ErrorDictionary.ACTION_ERROR, description)) | ||
| .build()); |
| throw new WebApplicationException(Response | ||
| .status(520, "Unexpected callback error") | ||
| .entity(new ErrorPayload(ErrorDictionary.ACTION_ERROR, | ||
| "Action execution failed with: " + ofNullable(re.getMessage()) | ||
| .orElseGet(() -> re instanceof NullPointerException ? "unexpected null" | ||
| : "no error message"))) | ||
| .entity(new ErrorPayload(ErrorDictionary.ACTION_ERROR, description)) | ||
| .build()); |
| if (re instanceof final DiscoverSchemaException eSchema) { | ||
| // we send reason to recognize the error on client side | ||
| final String subCode = ofNullable(eSchema.getPossibleHandleErrorWith()) | ||
| .orElse(HandleErrorWith.EXCEPTION) | ||
| .toString(); | ||
| throw new WebApplicationException(Response | ||
| .status(ce.getErrorOrigin() == ComponentException.ErrorOrigin.USER ? 400 | ||
| : ce.getErrorOrigin() == ComponentException.ErrorOrigin.BACKEND ? 456 : 520, | ||
| "Unexpected callback error") | ||
| .entity(new ErrorPayload(ErrorDictionary.ACTION_ERROR, | ||
| "Action execution failed with: " + ofNullable(re.getMessage()) | ||
| .orElseGet(() -> re instanceof NullPointerException ? "unexpected null" | ||
| : "no error message"))) | ||
| .status(400, subCode) | ||
| .entity(new ErrorPayload(ErrorDictionary.ACTION_ERROR, subCode, description)) | ||
| .build()); |
| if (re instanceof final DiscoverSchemaException eSchema) { | ||
| // we send reason to recognize the error on client side | ||
| final String subCode = ofNullable(eSchema.getPossibleHandleErrorWith()) | ||
| .orElse(HandleErrorWith.EXCEPTION) | ||
| .toString(); | ||
| throw new WebApplicationException(Response | ||
| .status(ce.getErrorOrigin() == ComponentException.ErrorOrigin.USER ? 400 | ||
| : ce.getErrorOrigin() == ComponentException.ErrorOrigin.BACKEND ? 456 : 520, | ||
| "Unexpected callback error") | ||
| .entity(new ErrorPayload(ErrorDictionary.ACTION_ERROR, | ||
| "Action execution failed with: " + ofNullable(re.getMessage()) | ||
| .orElseGet(() -> re instanceof NullPointerException ? "unexpected null" | ||
| : "no error message"))) | ||
| .status(400, subCode) | ||
| .entity(new ErrorPayload(ErrorDictionary.ACTION_ERROR, subCode, description)) | ||
| .build()); |
| final Map<String, String> metadata = ofNullable(sanitized).orElseGet(HashMap::new); | ||
| metadata.put("definition::parameter::index", String.valueOf(siblings.indexOf(p))); | ||
| // this one to mark the Schema parameter somehow to differentiate it from the branch name | ||
| if (p.getJavaType() instanceof Class<?> clazzType && Schema.class.isAssignableFrom(clazzType)) { | ||
| metadata.put("definition::parameter::schema", ""); | ||
| } | ||
| return metadata; |
| // guess schema action will fail and as start of job is true, it should use processor lifecycle | ||
| redirectStdout(out); |
c983d31 to
f15c17d
Compare

Requirements
Why this PR is needed?
What does this PR adds (design/code thoughts)?
AI generated code
https://internal.qlik.dev/general/ways-of-working/code-reviews/#guidelines-for-ai-generated-code