Property value is lost after template version upgrade if condition was changed

[chillleader]

Describe the Bug

When updating to a new element template version, the property value will be lost if the property condition was changed.

Steps to Reproduce

Consider the following simple element template:

Sample element template

``` { "$schema": "https://unpkg.com/@camunda/zeebe-element-templates-json-schema/resources/schema.json", "name": "New Connector Template", "id": "2dfeb14b-b0d9-452b-b385-387d9060552c", "version": 1700747991582, "appliesTo": [ "bpmn:Task" ], "properties": [ { "id": "myConditionProp", "type": "Hidden", "binding": { "type": "zeebe:input", "name": "myConditionProp" }, "value": "a" }, { "type": "String", "binding": { "type": "zeebe:input", "name": "myProp" }, "label": "My prop", "condition": { "type": "simple", "property": "myConditionProp", "oneOf": [ "a", "b" ] } } ] } ```

1. Publish the element template above
2. Create a simple BPMN process that uses this template
3. Enter any data in My prop
4. Edit the element template: change the condition of myProp in any way, e.g. add a new entry to the oneOf list
5. Publish an updated version of the template
6. Update the template version in the BPMN process
7. Observe that My prop value is lost

Expected Behavior

Property value should not be lost if condition is changed.

Environment

Reproduced in the following environments:

• Web Modeler SaaS, as of 23.11.2023
• Desktop Modeler 5.16.0

[marstamm]

Root-Cause

This is happening because in the condition checker, the condition counts towards the "uniqueness" of the property. https://github.com/bpmn-io/bpmn-js-element-templates/blob/6b231453b28170a78018807777f81925273ef201/src/cloud-element-templates/ElementTemplatesConditionChecker.js#L108-L113

We mark the property as "new" and create/update the property according to the template, setting the default property.value (which is undefined).

https://github.com/bpmn-io/bpmn-js-element-templates/blob/6b231453b28170a78018807777f81925273ef201/src/cloud-element-templates/ElementTemplatesConditionChecker.js#L72

This leads to the value being overwritten

[chillleader]

Thanks for the analysis @marstamm. I wonder if it makes sense to consider property ID as the only "uniqueness indicator" of the property, as we can have multiple properties with the equal bindings?

[marstamm]

How to fix it

Checking if the property condition has changed is important, as the new condition might hide/show the property after the update. setPropertyValue should consider the existing data/type of the property, similar to the logic we have for normal updates.

Is it possible to reuse this logic? Seems like we had solved this issue before, when updating to hidden fields.

[marstamm]

@smbea do you remember why we decided to not reuse the logic from ChangeElementTemplateHandler for conditions? I vaguely remember a discussion but not the details

[marstamm]

I wonder if it makes sense to consider property ID as the only "uniqueness indicator" of the property, as we can have multiple properties with the equal bindings?

Cf. https://github.com/bpmn-io/bpmn-js-element-templates/issues/32#issuecomment-1824627231 We also use this check to validate that the condition did not change/is still valid. So we can't take the condition out of the equation, instead the update function for the value should be smarter

[barmac]

I had a look at this and was able to reproduce the issue. Thanks @marstamm for looking into this as well. Ideally, we should be able to tell whether the property is an updated old one or a completely new one. However, the current mechanism does not meet that requirement.

[smbea]

I wonder if it makes sense to consider property ID as the only "uniqueness indicator" of the property

The ID is only required for the property we want to refer to in a condition. So - as it is - we can't assume the conditional property has an id.

@smbea do you remember why we decided to not reuse the logic from ChangeElementTemplateHandler for conditions? I vaguely remember a discussion but not the details

To be honest I'm not quite sure. I can have a look at it since it's been a while since I looked at it.

[nikku]

@marstamm Assigned you as discussed in the weekly. Let's find out if there is a simple fix.

[marstamm]

I am writing this to understand how to best solve this issue.

My assumption

Both Template updates as well as changes to visible properties should be handled with the same mechanism. It should not matter if a property is added/removed due to an update of the Element template or through conditional rendering.

However, some of our current assumptions differ when updating vs conditional rendering. For Conditional rendering, if a property is hidden, the value is ALWAYS removed.

For updates, we have the special cases of

1. bpmn properties - we do not remove old values, maybe to not override names? This seems like an oversight in the current implementation, as we know which properties are defined in the old template and could only remove defined properties. https://github.com/bpmn-io/bpmn-js-element-templates/blob/aa00312bd1184fb06e7f99d95c0d2166e89d38e0/src/cloud-element-templates/cmd/ChangeElementTemplateHandler.js#L164-L166
2. Task definitions - this is explicitly mentioned in a comment. I don't see the value in it (see proposal below) and I can't get more information on WHY this is necessary: https://github.com/bpmn-io/bpmn-js-element-templates/blob/aa00312bd1184fb06e7f99d95c0d2166e89d38e0/src/cloud-element-templates/cmd/ChangeElementTemplateHandler.js#L212
3. All message properties - probably only a case of copy/pasting the bpmn-properties code in the initial implementation and should be considered a bug: https://github.com/bpmn-io/bpmn-js-element-templates/blob/aa00312bd1184fb06e7f99d95c0d2166e89d38e0/src/cloud-element-templates/cmd/ChangeElementTemplateHandler.js#L650-L652

Proposed way forward

I would consider all these cases bugs in the update logic. If I remove a binding from a new version of a template, I do not expect it to be present in the business object after the update. It is also confusing that the behavior for input/output and other properties differs from the cases above.

Therefore, let's align the update behavior to make it consistent. Once we have done this, we can then use it for applying conditions as well, getting rid of our duplicated update behavior.

Note: We will use the update mechanism for the property value here, so the value is persisted if it was changed, but will get the new default if the old value was never changed from the old default value.

[nikku]

For Conditional rendering, if a property is hidden, the value is ALWAYS removed.

What you refer to as hidden is not a type=Hidden property, but one that is currently not active, right? Every property that is active shall be bound, every property that is inactive shall not be bound.

bpmn properties - we do not remove old values, maybe to not override names?

I think that may be the case, yes. Name and other BPMN properties are special cases, and unless bound explicitly in a template (so the template controls them) they are not touched by upgrading logic. What we want to prevent is surprises as the user applies a template.

[nikku]

Task definitions

This prevents us from properly removing the definition from one template to the other, right? I agree this may be an oversight, but let's test this thoroughly (document current bugs!) and move from there.

[nikku]

I would consider all these cases bugs in the update logic.

Agreed with the exception of BPMN properties, where we need to ensure we don't break "taking over names" as well as interrupting vs. non-interrupting and other core properties that the editor relies upon.

[marstamm]

with the exception of BPMN properties

As we only remove properties that were present in the old template, this would only be relevant if the property was templated. Otherwise, the modeler config will be unaffected. I checked the connector templates to see if this is a concern in a realworld use-case, and there is no instance where the name or interrupting state is templated.

For simplicity, I would propose to make the behavior consistent and ignore (but document in the code) these edge cases for now. If we get bug reports for it, we can implement it as follow-ups.

Even in the case that special properties are templated, it does not make the modeler unusable. You might have to toggle interrupting and add a name again.

[nikku]

One thing to account for is what happens when we move from a non-templated element, with properties set, to a templated one. That can happen if you decide to replace a service task with a REST task. Also in such case I expect valid data to be kept, and what is not defined in the template to be removed.

Example

• Service task has a taskDefinition:retries configured, but REST task does not allow me to set it. I expect it to be removed.

[nikku]

@chillleader We'll proceed and fix the bug that you raised. Regarding your additional comment I'd love you to open a follow-up issue in case you think there is something that we could do to substantially improve element template definition.

[marstamm]

Service task has a taskDefinition:retries configured, but REST task does not allow me to set it. I expect it to be removed.

This is a separate issue. Let's not bloat the scope of this ticket.

What we will handle in this ticket:

• Align update mechanisms for upgrading a Template with the mechanism for conditional rendering. This requires us to align the logic for properties/messages/... with how we handle it for inputs/zeebe:properties/...
• Remove logic duplication, reuse existing behavior

What we will not handle:

• Fix existing bugs/edge-cases in update handeling, e.g. applying a new template

[marstamm]

Happy to open a follow-up issue, as this is currently not handled as expected

[nikku]

What we will not handle:

100% agreed. Thanks @marstamm.