search

camunda / camunda-bpm-platform

Flexible framework for workflow and decision automation with BPMN and DMN. Integration with Quarkus, Spring, Spring Boot, CDI.
https://camunda.com/
Apache License 2.0
4.03k stars 1.53k forks source link

NullPointerException calling getStartFormKey for multi-start event processes #2654

Open ThorbenLindhauer opened 2 years ago

ThorbenLindhauer commented 2 years ago

This issue was imported from JIRA:

Field Value
JIRA Link CAM-14533
Reporter VvQox5
What is this name? This pseudonym name was generated based on the user name in JIRA to protect the personal data of our JIRA users. You can use this identifier to search for issues by the same reporter.
Has restricted visibility comments true

Environment (Required on creation):

Camunda 7.17.0, all distros

Description (Required on creation; please attach any relevant screenshots, stacktraces, log files, etc. to the ticket):

Calling getFormService().getStartFormKey(processDefinitionId) for a process definition with multiple start events leads to (on JDK 17):

Caused by: java.lang.NullPointerException: Cannot invoke "org.camunda.bpm.engine.impl.form.FormDefinition.getFormKey()" because "formDefinition" is null
    at org.camunda.bpm.engine.impl.cmd.GetFormKeyCmd.execute(GetFormKeyCmd.java:84)
    at org.camunda.bpm.engine.impl.cmd.GetFormKeyCmd.execute(GetFormKeyCmd.java:40)
    at org.camunda.bpm.engine.impl.interceptor.CommandExecutorImpl.execute(CommandExecutorImpl.java:28)
    at org.camunda.bpm.engine.impl.interceptor.CommandContextInterceptor.execute(CommandContextInterceptor.java:110)
    at org.camunda.bpm.engine.impl.interceptor.ProcessApplicationContextInterceptor.execute(ProcessApplicationContextInterceptor.java:70)
    at org.camunda.bpm.engine.impl.interceptor.CommandCounterInterceptor.execute(CommandCounterInterceptor.java:35)
    at org.camunda.bpm.engine.impl.interceptor.LogInterceptor.execute(LogInterceptor.java:33)
    at org.camunda.bpm.engine.impl.FormServiceImpl.getStartFormKey(FormServiceImpl.java:99)

    ... user code```
### Steps to reproduce (Required on creation):

1. Deploy a process definition with multiple start events
1. Call `getFormService().getStartFormKey(processDefinitionId)` for that process definition

### Observed Behavior (Required on creation):

NPE as described above

### Expected behavior (Required on creation):

A meaningful `NullValueException` as thrown in `GetStartFormCmd` for example in the same setting

### Root Cause (Required on prioritization):

* In [GetFormKeyCmd lines 83 and 84](https://github.com/camunda/camunda-bpm-platform/blob/master/engine/src/main/java/org/camunda/bpm/engine/impl/cmd/GetFormKeyCmd.java#L83-L84), we expect there always to be a FormDefinition, which is not the case in 7.17.0 given the preconditions above.
```java
FormDefinition formDefinition = processDefinition.getStartFormDefinition();
formKeyExpression = formDefinition.getFormKey();

Solution Ideas (Optional):

  1. Throw a NullValueException stating the absence of a form definition for the start event

Hints (optional):

Links:

ThorbenLindhauer commented 2 years ago

This comment was imported from JIRA and written by user @tmetzke

Hey VvQox5,

thanks for creating this ticket, looking into it now.

Best, Tobias

ThorbenLindhauer commented 2 years ago

This comment was imported from JIRA and written by user @tmetzke

Hey VvQox5,

I can reproduce this and can track down the change that introduced this. We'll keep you in the loop on how we will progress here.

One detail here: can you confirm that this only occurs with Process Definitions that have multiple start events?

Thanks again so far.

Best, Tobias

ThorbenLindhauer commented 2 years ago

This comment was imported from JIRA and written by user VvQox5

What is this name? This pseudonym name was generated based on the user name in JIRA to protect the personal data of our JIRA users. You can use this identifier to search for issues by the same reporter.

Hi,

Great that you can reproduce! I can't confirm in the sense that I can't be sure, but my tests seem to show that.

As a sidenote, for many, many releases Camunda has been throwing NullValueException when I call getFormService().getStartFormVariables(processDefinitionId) for the same kind of definitions - ones with multiple signal start events. Not sure if it's related (or even if that's not "normal") - I just wanted to mention it.

Regards, Boris

P.S. I think getStartFormData also throws the same NullValueException.

ThorbenLindhauer commented 2 years ago

This comment was imported from JIRA and written by user @tmetzke

Hi VvQox5,

thanks for that input. Regarding "confirmation", I rather meant if you are only experiencing this for multi-start event processes or also for the ones with single start events. That would be enough confirmation for my findings.

One note for start form variables/data/key in general. You can only get those for single start event process models. It just doesn't make sense otherwise. Therefore, we are throwing the NullValueException for the "getStartFormVariables" method in that case. This is actually expected because calling this method on a multi-start event model does not make sense. This is also what we would probably do for the "getFormKey" method here to make it consistent and not run into an NPE that is considered as a rather "uncontrolled" exception.

I am not sure about your use case here for calling those methods on all process definitions, no matter how many start events there are. Maybe you can explain that a bit more. But we will most probably not make the exception disappear here but rather make it more explicit and controlled.

Hope that is a reasonable answer for you here.

Best, Tobias

ThorbenLindhauer commented 2 years ago

This comment was imported from JIRA and written by user VvQox5

What is this name? This pseudonym name was generated based on the user name in JIRA to protect the personal data of our JIRA users. You can use this identifier to search for issues by the same reporter.

Hi,

Yes, I only experience this for multi-start event processes.

As for the other exception - yes, I agree it doesn't make sense to get start-form-anything for a process with multiple start events - my point was just that an exception was thrown instead of, for example, returning null (as was the case with getStartFormKey). Making the methods consistent would be the best solution.

My use case is not that special - I just have the same code path for all definitions - and I handled the null/NullValueException cases.

A couple of questions:

1) I'm using the community version of Camunda which means that I'll get the fix in 7.18.0, is that correct?

2) Is there a "proper" way to workaround that issue? If I try-catch the NPE, my code works, but Camunda logs the exception. I seem to be able to suppress that log by using setEnableCmdExceptionLogging(false), however the documentation suggests that's not a very good idea (which I agree with). Any other ideas?

Regards, Boris

ThorbenLindhauer commented 2 years ago

This comment was imported from JIRA and written by user @tmetzke

Hi VvQox5,

thanks for those insights, that's very helpful to understand what you're doing. In my regard, it's actually better if an API throws a "controlled" and meaningful exception in such a case instead of just returning "null" which is - if you ask me - used way too often in Java out of convenience. The "null" return value might actually mean different things which is why I think an API should avoid returning this as much as possible.

Regarding your use case: Maybe you could rather go for a more "proactive" way of figuring out if you should actually call the start form methods for a process or not. More specifically, this would mean checking if there is only a single start event or not. There are multiple ways of doing this, depending on which objects you already have in your code for example. The process definition entity that you receive when you use the ProcessDefinitionQuery API has a "getInitial" method that is only non-null for single start event definitions, as far as I know.

Hope that helps in moving forward with this.

Best, Tobias

ThorbenLindhauer commented 2 years ago

This comment was imported from JIRA and written by user VvQox5

What is this name? This pseudonym name was generated based on the user name in JIRA to protect the personal data of our JIRA users. You can use this identifier to search for issues by the same reporter.

Hi,

I absolutely agree that exceptions are mostly better than returning null - especially in this case!

Thanks for the ideas, I'll perhaps try to do as you suggest.

And thank you very much for the great support!

Regards, Boris

ThorbenLindhauer commented 2 years ago

This comment was imported from JIRA and written by user @tmetzke

Thanks for the constructive discussion, VvQox5. Happy to help, let us know if there are any further questions regarding this in the future.

Best, Tobias

ThorbenLindhauer commented 1 year ago

This comment was imported from JIRA and written by user VvQox5

What is this name? This pseudonym name was generated based on the user name in JIRA to protect the personal data of our JIRA users. You can use this identifier to search for issues by the same reporter.

Just to follow up on this issue - the same problem still happens on 7.18.0.

ThorbenLindhauer commented 1 year ago

This comment was imported from JIRA and written by user @tmetzke

Hi VvQox5,

this is still in our backlog.

Due to other priorities, we didn't work on this yet. We also did not schedule this yet for any specific version. If you would like to speed up the process of fixing this, we are happy to receive a contribution to our GitHub repository.

Best, Tobias

ThorbenLindhauer commented 1 year ago

This comment was imported from JIRA and written by user VvQox5

What is this name? This pseudonym name was generated based on the user name in JIRA to protect the personal data of our JIRA users. You can use this identifier to search for issues by the same reporter.

Hi, yes, no worries, I just wanted to mention it for completeness' sake. It's not an urgent issue so no problem! :)