CAM-10771 Improve BPMN Parser Exceptions

[falko]

Currently, the BPMN parser throws exceptions with an error code, a more or less helpful message and line number in the XML file.

In order to locate the problem in a BPMN model, users (and tools) require BPMN element IDs in the exceptions. This pull request add those. I've also started working on getting multiple element IDs included because some errors are related to multiple elements and it's not always possible to derive that information outside the parser. The message format is parsable (pipe-separated values)

Another issue addressed here, is that the current parser requires multiple runs to find all errors or warnings, i.e. after fixing the first errors, more will surface during the next deployment.

Some error messages are wrong or at least misleading. So it would be helping to correct and improve the exception texts.

Ideally, there is a REST endpoint for just parsing without deploying.

This work started during Camunda Hackdays 2015 where we already demonstrated the value of this improvement by visualizing parser errors in Camunda Workbench.

[yanavasileva]

Hi @falko ,

Thanks for raising this. I created CAM-10245 for it.
Is the pull request a work in progress? Do you want to continue with the mentioned points for which you didn't commit anything so far:

• multiple runs to find all errors or warnings
• wrong or at least misleading
• REST endpoint for just parsing without deploying

Best regards,
Yana

[falko]

Hi Yana,

After @nikku already confirmed that this would help him for future modeler features, I would like to get your opinion if this way of implementing it is acceptable for you. If so, I would at least finish the handling of multiple element ids and depending on my time also others features. If it's getting to big, we can also split it up in multiple issues/PRs.

Cheers,
Falko

[ThorbenLindhauer]

Hi Falko,

I like the idea and goal of this pull request. From a quick glance at the code, your contribution encodes the element ids into the exception message, which is then thrown as a ProcessEngineException. I'd like to propose the following alternative:

• We extract an API interface out of the Problem class. This interface will expose all the problem's properties, i.e. line number, error message, bpmn element ids
• We create a new subclass of ProcessEngineException (or reuse BpmnParseException)
• This subclass will have a list of problems Problem instances (or maybe two lists, one for errors and one for warnings)
• In the REST API, we will have an exception mapper that serializes our exception and the problems to proper JSON objects

That way, clients don't have to parse the exception message, but can work with JSON to access the parsing errors.

What do you think?

Cheers,
Thorben

PS: Timeline-wise, I think we won't be able to include this in 7.11.0.

[nikku]

@falko @ThorbenLindhauer Sounds good to me, either way.

[nikku]

As discussed with @ThorbenLindhauer I'll take this PR over and do the following things:

• Expose errors and warnings in a structured manner via Java and REST API
• Add context information to the structured information being returned

The plan is to keep the existing error message as is.

[falko]

@nikku thank you for taking over!

What do you mean by "keep the existing error message as is"?

Will they still not contain information about the affected BPMN elements?
Will one still need multiple parser runs to find all errors if one is not using your new API?

[nikku]

Our solution sketch is two-fold:

(1) Provide additional getErrors() and getWarnings() interfaces for the ParseException. This allows you to catch the exception from Java Code and see what's up:

try {
  // deploy
} catch (ParseException ex) {
  ex.getErrors();
  ex.getWarnings();
}

(2) Expose the errors and warnings via REST API on deployment

POST /engine-rest/deployment/create

=> HTTP 400
{
  "type" : "ParseException",
  "message" : "existing, unchanged message",
  "errors": [ ... ],
  "warnings": [ ... ]
}

[falko]

Sounds like a good starting point.

[CLAassistant]

All committers have signed the CLA.

[ThorbenLindhauer]

@nikku we may be able to implement this with 7.12.

For designing this feature, I have the following question: Would you prefer if an error/warning is assigned to exactly one BPMN element or to a list of involved elements?

Example: An exclusive gateway has a non-default outgoing flow without condition. The error could be assigned to both the flow and the gateway or only one of them.

My impression is that a single element will be easier to use, because each error is then clearly assigned to one element. The referenced element should be the element that needs to be primarily changed, i.e. in the example it should be the sequence flow.

[falko]

In my experience, there are errors that cannot be assigned to one exact element. Also, it might be helpful if related elements are listed for visualizations or explanations to users.

[nikku]

TLDR: If you ask me, my gut feeling is that:

• One primary shape is broken
• A number of elements are related because they span the context of the error

Read details / rationale below.

---

@nikku we may be able to implement this with 7.12.

That is great news.

In my experience, there are errors that cannot be assigned to one exact element.

I'd like to validate @falko's statement. Which errors do we capture? Can each error be pinned to a primary shape?

For designing this feature, I have the following question: Would you prefer if an error/warning is assigned to exactly one BPMN element or to a list of involved elements?

I agree with @falko that we should include additional, related elements that help to better understand the issue / setup the relevant context.

Example: Missing condition on a sequence flow is a problem on the sequence flow. It is only a problem though, because it originates from a conditional forking gateway though (that would be the related element).

When parsing the error consumers have the choice how to visualize and/or list it. On the diagram they probably would like to visualize both elements, on a "diagram errors" list, they may show the actual sequence flow only.

[ThorbenLindhauer]

Thanks for the feedback. We will then probably have both: A primary element id and a list of involved elements. My gut feeling is that we can always declare one element to be the primary, so that in combination with the error message the mistake is clear. We'll validate that during implementation.

[falko]

Just as an example the exception "messageEventDefinition only allowed on start event if subprocess is an event subprocess" could mean that the event should be a none event or the sub-process should be an event sub-process. It is hard to guess the original intent of the user.

[ThorbenLindhauer]

FYI @yanavasileva has implemented this via #528. See the staged docs here: https://stage.docs.camunda.org/manual/develop/reference/rest/deployment/post-deployment/#response-on-parse-errors. Please let us know if you have any feedback. I am closing this PR accordingly.

[nikku]

This is a major improvement that we'll be able to work with.

Thanks for implementing it.