[incubator-kie-issues#1150] Improve Import Resolver error messages to be more user friendly

[yesamer]

Closes apache/incubator-kie-issues#1150

As agreed, this PR will improve the Error messages only to be more user-friendly.
That means there are no changes in the logic.

[gitgabrio]

Thx @yesamer - there are just few details that maybe could be changed

[kie-ci3]

PR job #5 was: UNSTABLE
Possible explanation: This should be test failures

Reproducer

build-chain build full_downstream -f 'https://raw.githubusercontent.com/${AUTHOR:apache}/incubator-kie-kogito-pipelines/${BRANCH:main}/.ci/buildchain-config-pr-cdb.yaml' -o 'bc' -p apache/incubator-kie-drools -u #6014 --skipParallelCheckout

NOTE: To install the build-chain tool, please refer to https://github.com/kiegroup/github-action-build-chain#local-execution

Please look here: https://ci-builds.apache.org/job/KIE/job/drools/job/main/job/pullrequest_jobs/job/drools-pr/job/PR-6014/5/display/redirect

Test results:

• PASSED: 22157
• FAILED: 12

Those are the test failures:

org.kie.dmn.core.compiler.ImportDMNResolverUtilTest.locateInNSunexistent

Cannot invoke "org.kie.dmn.model.api.Definitions.getNamespace()" because the return value of "org.kie.dmn.model.api.Import.getParent()" is null

org.kie.dmn.core.compiler.ImportDMNResolverUtilTest.locateInNSAliasedBadScenario

Cannot invoke "org.kie.dmn.model.api.Definitions.getNamespace()" because the return value of "org.kie.dmn.model.api.Import.getParent()" is null

org.kie.dmn.core.compiler.ImportDMNResolverUtilTest.locateInNSnoModelNameWithAlias

Cannot invoke "org.kie.dmn.model.api.Definitions.getNamespace()" because the return value of "org.kie.dmn.model.api.Import.getParent()" is null

org.kie.dmn.core.compiler.ImportDMNResolverUtilTest.nSnoModelNameWithAlias

Cannot invoke "org.kie.dmn.model.api.Definitions.getNamespace()" because the return value of "org.kie.dmn.model.api.Import.getParent()" is null

org.kie.dmn.core.compiler.ImportDMNResolverUtilTest.nSandUnexistentModelName

Cannot invoke "org.kie.dmn.model.api.Definitions.getNamespace()" because the return value of "org.kie.dmn.model.api.Import.getParent()" is null

org.kie.dmn.core.compiler.ImportDMNResolverUtilTest.locateInNSnoModelNameWithAlias2

Cannot invoke "org.kie.dmn.model.api.Definitions.getNamespace()" because the return value of "org.kie.dmn.model.api.Import.getParent()" is null

org.kie.dmn.core.compiler.ImportDMNResolverUtilTest.nSandModelName

Cannot invoke "org.kie.dmn.model.api.Definitions.getNamespace()" because the return value of "org.kie.dmn.model.api.Import.getParent()" is null

org.kie.dmn.core.compiler.ImportDMNResolverUtilTest.locateInNSAliased

Cannot invoke "org.kie.dmn.model.api.Definitions.getNamespace()" because the return value of "org.kie.dmn.model.api.Import.getParent()" is null

org.kie.dmn.core.compiler.ImportDMNResolverUtilTest.nSonly

Cannot invoke "org.kie.dmn.model.api.Definitions.getNamespace()" because the return value of "org.kie.dmn.model.api.Import.getParent()" is null

org.kie.dmn.core.compiler.ImportDMNResolverUtilTest.nSandModelNameWithAlias

Cannot invoke "org.kie.dmn.model.api.Definitions.getNamespace()" because the return value of "org.kie.dmn.model.api.Import.getParent()" is null

org.kie.dmn.core.compiler.ImportDMNResolverUtilTest.locateInNS

Cannot invoke "org.kie.dmn.model.api.Definitions.getNamespace()" because the return value of "org.kie.dmn.model.api.Import.getParent()" is null

org.kie.dmn.core.compiler.ImportDMNResolverUtilTest.nSnoModelNameDefaultWithAlias2

Cannot invoke "org.kie.dmn.model.api.Definitions.getNamespace()" because the return value of "org.kie.dmn.model.api.Import.getParent()" is null

[gitgabrio]

Thx @yesamer LGTM!

[yesamer]

@mariofusco Can you please review it? Thank you!

[jomarko]

Hi @yesamer and team, sorry to be late to the discussion, but got these questions:

1. We introduce this snippet ((Definitions) importElement.getParent()).getNamespace(), are we sure the getParent() returns non null value? Asking because of the change in the test mocks. To me it seems not guaranteed the getparent() will return non null.
2. We introduce also few checks in the form a.equals(b) that potentially may throw NPE if a or b is null, what is your opinion about using Objects.equals(a, b) that never throws NPE?
3. The log messages we introduce are quite long. My question is where user can see them? (In terminal when running project via maven? in browser console log? in DMN Runner pannel? ) We delimit the sentences with a white space ' '. For such a long messages, should we use '\n' to delimit sentences? Really do not know, just asking.

[yesamer]

@jomarko Thank you for your questions.

1. The import's parent is the Definition element. If that is missed, the DMN file is invalid, so I guess it's safe to don't perform a null check
2. The equals were already present in the original code IINW, anyway in this case, I guess it's correct using .equals(). For Instance, if the namespace is null, I prefer to have a NPE than a false, because namespace = null is an invalid status that should result in an Exception
3. For the LOGGER messages, I tried to use very concise messages. For error messages, I wrote more verbose messages, because they will be consumed by the final user (in the terminal or UI, eg. Business Central). I guess using a return line could be a nice improvement in case of very long messages, yeah

[gitgabrio]

@jomarko
As general rule, logging should be the first resource for remote/async debugging.
If there is a problem on some production server, log files are the first resource to be consumed, and that's why it is important to put there informations usfeull to identify the problems without the need of a reproducer, or manual debug, or similar.