Skip to content
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

kie-issue#1149: DMN Editor produces invalid typeRef attributes #2316

Merged
merged 5 commits into from
May 15, 2024

Conversation

danielzhe
Copy link
Contributor

Closes: apache/incubator-kie-issues#1149

Some places related to the user interface are using "".
The places where the data is marshaled are using undefined.

The "" value is still present in DmnBuiltInDataType because it is still a "valid value".

@danielzhe danielzhe added pr: DO NOT MERGE Draft PR, not ready for merging area:dmn labels May 10, 2024
@danielzhe danielzhe marked this pull request as ready for review May 10, 2024 23:16
@danielzhe danielzhe added pr: waiting-for-review Waiting for peer reviews and removed pr: DO NOT MERGE Draft PR, not ready for merging labels May 10, 2024
Copy link
Contributor

@ljmotta ljmotta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danielzhe Thanks for this change! I've added some comments inline

allTopLevelDataTypesByFeelName: DataTypeIndex;
getInputs?: () => { name: string; typeRef: string | undefined }[] | undefined;
getDefaultColumnWidth?: (args: { name: string; typeRef: string | undefined }) => number | undefined;
widthsById: Map<string, number[]>;
}): BoxedExpression {
const dataType = allTopLevelDataTypesByFeelName.get(typeRef);
const dataType = allTopLevelDataTypesByFeelName.get(typeRef ?? "<Undefined>");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"<Undefined>" is value used in a few places. Maybe extract to a variable? WDYT?

Copy link
Contributor Author

@danielzhe danielzhe May 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. I'm going to use the DmnBuiltInDataType.Undefined for this.

@@ -98,7 +98,7 @@ const BoxedExpressionEditorWrapper: React.FunctionComponent<BoxedExpressionEdito
const beeGwtService: BeeGwtService = {
getDefaultExpressionDefinition(logicType, dataType, isRoot) {
const defaultExpression = gwtExpressionToDmnExpression(
window.beeApiWrapper?.getDefaultExpressionDefinition(gwtLogicType(logicType), dataType)
window.beeApiWrapper?.getDefaultExpressionDefinition(gwtLogicType(logicType), dataType ?? "<Undefined>")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dataType parameter of getDefaultExpressionDefinition accepts an undefined value, why fallback to "<Undefined>"? Maybe should we fallback inside the method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because this is to the GWT layer and since it will be removed in the future I think we should avoid spending time on it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not for display only, though, right? I mean, this is getting into the XML.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we talked in pvt, I changed it to the proposed change by @ljmotta :)

@danielzhe
Copy link
Contributor Author

Changes applied! Ready for your review @ljmotta @tiagobento

Copy link
Contributor

@tiagobento tiagobento left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually... Just checked an there are a lot of references to DmnBuiltInDataType.Undefined in the stunner-editors-dmn-loader component. Those are used in the beeToGwt function, and certainly are leaking to the XML. Please check that too...

@tiagobento tiagobento merged commit 1e82db8 into apache:main May 15, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:dmn pr: waiting-for-review Waiting for peer reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DMN Editor produces invalid typeRef attributes
3 participants