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

Headers / inputs are not cleaned when switching between element templates #653

Closed
nikku opened this issue Apr 4, 2022 · 3 comments · Fixed by #659
Closed

Headers / inputs are not cleaned when switching between element templates #653

nikku opened this issue Apr 4, 2022 · 3 comments · Fixed by #659
Assignees
Labels
bug Something isn't working

Comments

@nikku
Copy link
Member

nikku commented Apr 4, 2022

Describe the bug

The overall "check if properties are still valid during replace" interaction has a bug that affects changing from one element template to another where the first and second are distinct in their binding types (i.e. the first defines only inputs, the second one only headers).

In that case the properties are not cleaned up (inputs stay configured) despite the second template not having input bindings. Breaks the other way around, too.

capture ZuLYdR_optimized

Steps to reproduce

You can find two example templates to reproduce this here.

  • Given I have a template that defines a task header <a>
  • If I change to another template that does not define task headers
  • Then the task header <a> binding is not cleaned up

I believe this is due to our C7 legacy of "only touching what has been defined"; it does not seem to be the appropriate behavior for C8 anymore.

Expected Behavior

Bindings (inputs, headers) are cleaned up completely, despite the new template not using them.


Originally posted by @nikku in #648 (comment)

@MaxTru
Copy link
Contributor

MaxTru commented Apr 5, 2022

@marstamm considering you investigated the replace behavior in general, could you please have a look?

@marstamm
Copy link
Contributor

marstamm commented Apr 5, 2022

I checked it and can confirm that the old Input Mappings & Headers are only removed when the new Template declares Input Mappings of their own.

This seems to be done on purpose:

// (1) do not override old inputs and outputs if no new inputs and outputs specified
if (!newProperties.length) {
return;
}

But I could not find the reasoning in the PR or Issue that contributed it. I think we are safe to remove this restriction

@marstamm marstamm added the ready Ready to be worked on label Apr 5, 2022
@nikku
Copy link
Member Author

nikku commented Apr 5, 2022

This is legacy (C7 2016) behavior.

Our assumption in the past was that element templates work along with existing configuration.

Since then we've made it impossible to show specific existing bindings next to the element templates (entriesVisible).

We could keep this behavior for C7, if we desire; we should move to a more consistent replace/clean anything for C8 templates.

CC @MaxTru

@bpmn-io-tasks bpmn-io-tasks bot added in progress Currently worked on and removed ready Ready to be worked on labels Apr 5, 2022
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Apr 5, 2022
@fake-join fake-join bot closed this as completed in #659 Apr 5, 2022
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label Apr 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants