Boolean input without `feel` property in a template throws an error

barmac commented 1 month ago

Describe the bug

Applying this template causes an error to be thrown:

{
  "$schema": "https://unpkg.com/@camunda/zeebe-element-templates-json-schema/resources/schema.json",
  "name": "Boolean input without feel",
  "id": "io.booleanWithoutFeel.test",
  "version": 1,
  "appliesTo": [
    "bpmn:Task"
  ],
  "elementType": {
    "value": "bpmn:ServiceTask"
  },
  "properties": [{
    "label" : "Analyze tables",
    "optional" : false,
    "value" : true,
    "constraints" : {
      "notEmpty" : true
    },
    "binding" : {
      "name" : "input.analyzeTables",
      "type" : "zeebe:input"
    },
    "type" : "Boolean"
  }, {
    "label" : "Dummy required for test failure",
    "value" : "3",
    "feel" : "optional",
    "binding" : {
      "name" : "retries",
      "type" : "zeebe:input"
    },
    "type" : "String"
  }]
}

Error:

[/tmp/test/test.bpmn] r.source.substring is not a function
    at file:///Users/maciej/.asdf/installs/camunda-modeler/5.28.0.app/Contents/Resources/app.asar/public/303.js:1:6429
    at file:///Users/maciej/.asdf/installs/camunda-modeler/5.28.0.app/Contents/Resources/app.asar/public/303.js:1:6482
    at Array.forEach (<anonymous>)
    at file:///Users/maciej/.asdf/installs/camunda-modeler/5.28.0.app/Contents/Resources/app.asar/public/303.js:1:6197
    at Array.forEach (<anonymous>)
    at forEach (webpack://camundanode_modules/@bpmn-io/variable-resolver/lib/zeebe/util/feelUtility.js:17:12)
    at parseIoMappings (webpack://camundanode_modules/@bpmn-io/variable-resolver/lib/zeebe/VariableResolver.js:111:27)
    at file:///Users/maciej/.asdf/installs/camunda-modeler/5.28.0.app/Contents/Resources/app.asar/public/427.js:5:98136
    at listener (webpack://camundanode_modules/diagram-js/lib/core/EventBus.js:457:33)
    at _invokeListener (webpack://camundanode_modules/diagram-js/lib/core/EventBus.js:431:23) [ error ]
This error may be the result of a plug-in compatibility issue. [ info ]
Disable plug-ins (restarts the app) [ info ]

Steps to reproduce

Download the template
Create an element from this
now this happens

Expected behavior

No error is thrown. Two options:

Template can be applied and it works.
Template is rejected by JSON schema.

Environment

OS: [e.g. MacOS 10.2, Windows 10]
Camunda Modeler Version: 5.28.0 and older
Execution Platform: [e.g. Camunda 7, Camunda 8]
Installed plug-ins: [...]

Additional context

Related to https://github.com/camunda/camunda-modeler/issues/4517, but it's a new issue.

barmac commented 1 month ago

This problem can be mitigated by adding feel field to the property.

barmac commented 1 month ago

I think we should just make "feel": "static" the default way of handling boolean inputs. We know that this needs to be an expression to apply boolean value.

philippfromme commented 1 month ago

I had a look with @marstamm. The root cause is that certain properties must be FEEL expressions from the engine's point of view (e.g., input and output source). The variable resolver is built on that assumption which results in an error when the source isn't a FEEL expression (and therefore not a string) but a boolean or number. The information what properties must be FEEL expressions is not part of the meta model but hard-coded in the properties panel. Unless you're using an element template you cannot set the source to a boolean or number which is probably why we haven't seen this issue pop up earlier. We could now hard-code this information in bpmn-js-element-templates, too. But this would mean there is no single source of truth. Ideally, this information should be read from the meta model using the meta field which is already used for allowedIn. This is what it would look like:

{
  "name": "InputOutputParameter",
  "properties": [
    {
      "name": "source",
      "isAttr": true,
      "type": "String"
    },
    {
      "name": "target",
      "isAttr": true,
      "type": "String"
    }
  ],
  "meta": {
    "feel": "required"
  }
}

Both bpmn-js-properties-panel and bpmn-js-element-templates would then read this information from the meta model. When adding a new property to the meta model the information that it must be a FEEL expression could be added without having to adjust the bpmn-js-properties-panel and bpmn-js-element-templates which could easily be forgotten.

@nikku @barmac Open for suggestions.

barmac commented 1 week ago

Sorry for a way too late feedback. So essentially you're proposing to shift the information on whether a property can/must be FEEL to the metamodel. At first sight, this sounds correct. We need to carefully consider whether it blocks any existing behaviors though. What if a property can be a FEEL expression in the engine, but an element template author wants it to be provided as plain text only? Is this a use case already?

philippfromme commented 1 day ago

What if a property can be a FEEL expression in the engine, but an element template author wants it to be provided as plain text only? Is this a use case already?

I did some digging and this is something that doesn't work at the moment. The "feel": "static" option doesn't work with a "type": "text". There's a test to verify this: https://github.com/camunda/element-templates-json-schema/blob/4216f8d28c64b95457935aeaed3c8ecdf618074b/packages/zeebe-element-templates-json-schema/test/fixtures/feel-type-mismatch.js#L9 But interestingly for other binding types there's no error at all but the template doesn't show up. Example:

{
  "$schema": "https://unpkg.com/@camunda/zeebe-element-templates-json-schema/resources/schema.json",
  "name": "FEEL static",
  "id": "feel.static",
  "appliesTo": [
    "bpmn:ServiceTask"
  ],
  "properties": [
    {
      "label": "Static",
      "type": "Text",
      "feel": "static",
      "binding": {
        "type": "zeebe:property",
        "name": "Property"
      }
    }
  ]
}

No error, but template doesn't show up:

Since the template defines what the UI looks like we could convert whenever we find that the meta model requires a FEEL expression. We currently do this only when dealing with boolean and number values (cf. https://github.com/bpmn-io/bpmn-js-element-templates/blob/main/src/cloud-element-templates/util/FeelUtil.js#L1).

camunda / camunda-modeler