camunda / feel-scala

FEEL parser and interpreter written in Scala
https://camunda.github.io/feel-scala/
Apache License 2.0
124 stars 50 forks source link

Implicit cast of number to string when trying to concatenate a number to a string #510

Open christian-konrad opened 2 years ago

christian-konrad commented 2 years ago

Describe the bug

When I try to concatenate a number to a string, the whole expression evaluates to null. I must explicitly cast the number to a string. Even as a professional developer, I have no clue to do so.

To Reproduce Steps to reproduce the behavior:

  1. Instantiate and assign two vars, one with a number, one with a string.
  2. Write an expression to concatenate those to a string (such as testString + " " + testNumber)
  3. Run the expression -> it yields null

Or use the BPMN process in the comment below.

Expected behavior

Either throw an exception, or better implicitly cast the number to a string.

(I know the documentation mentions that you need to cast explicitly, but returning null instead of an error makes this really hard to debug and understand, especially when you don't exactly know the type of the data to concatenate, e.g. when returned from an API that you evaluate)

Environment

christian-konrad commented 2 years ago

BPMN process for testing:

<?xml version="1.0" encoding="UTF-8"?>
<bpmn:definitions xmlns:bpmn="http://www.omg.org/spec/BPMN/20100524/MODEL" xmlns:bpmndi="http://www.omg.org/spec/BPMN/20100524/DI" xmlns:dc="http://www.omg.org/spec/DD/20100524/DC" xmlns:zeebe="http://camunda.org/schema/zeebe/1.0" xmlns:modeler="http://camunda.org/schema/modeler/1.0" xmlns:di="http://www.omg.org/spec/DD/20100524/DI" xmlns:camunda="http://camunda.org/schema/1.0/bpmn" id="Definitions_1" targetNamespace="http://bpmn.io/schema/bpmn" exporter="Camunda Web Modeler" exporterVersion="a1ecf89" modeler:executionPlatform="Camunda Cloud" modeler:executionPlatformVersion="8.0.0" camunda:diagramRelationId="83118ed1-3ca1-427d-9fd6-d3eb3e89f18a">
  <bpmn:process id="Process_0ad7120c-6815-43f4-b585-62bb2b03fd13" isExecutable="true">
    <bpmn:startEvent id="StartEvent_1" name="Set variables">
      <bpmn:extensionElements>
        <zeebe:ioMapping>
          <zeebe:output source="=&#34;abc&#34;" target="testString" />
          <zeebe:output source="=123" target="testNumber" />
        </zeebe:ioMapping>
      </bpmn:extensionElements>
      <bpmn:outgoing>Flow_11hyyfb</bpmn:outgoing>
    </bpmn:startEvent>
    <bpmn:sequenceFlow id="Flow_11hyyfb" sourceRef="StartEvent_1" targetRef="Activity_0vm464j" />
    <bpmn:userTask id="Activity_0vm464j" name="Transform and evaluate variables">
      <bpmn:extensionElements>
        <zeebe:ioMapping>
          <zeebe:input source="=testString + &#34; &#34; + testNumber" target="testExpression" />
        </zeebe:ioMapping>
      </bpmn:extensionElements>
      <bpmn:incoming>Flow_11hyyfb</bpmn:incoming>
      <bpmn:outgoing>Flow_1voaj36</bpmn:outgoing>
    </bpmn:userTask>
    <bpmn:endEvent id="Event_19i1pnf">
      <bpmn:incoming>Flow_1voaj36</bpmn:incoming>
    </bpmn:endEvent>
    <bpmn:sequenceFlow id="Flow_1voaj36" sourceRef="Activity_0vm464j" targetRef="Event_19i1pnf" />
  </bpmn:process>
  <bpmndi:BPMNDiagram id="BPMNDiagram_1">
    <bpmndi:BPMNPlane id="BPMNPlane_1" bpmnElement="Process_0ad7120c-6815-43f4-b585-62bb2b03fd13">
      <bpmndi:BPMNShape id="_BPMNShape_StartEvent_2" bpmnElement="StartEvent_1">
        <dc:Bounds x="150" y="100" width="36" height="36" />
        <bpmndi:BPMNLabel>
          <dc:Bounds x="136" y="143" width="64" height="14" />
        </bpmndi:BPMNLabel>
      </bpmndi:BPMNShape>
      <bpmndi:BPMNShape id="Activity_1rem74c_di" bpmnElement="Activity_0vm464j">
        <dc:Bounds x="240" y="78" width="100" height="80" />
        <bpmndi:BPMNLabel />
      </bpmndi:BPMNShape>
      <bpmndi:BPMNShape id="Event_19i1pnf_di" bpmnElement="Event_19i1pnf">
        <dc:Bounds x="402" y="100" width="36" height="36" />
      </bpmndi:BPMNShape>
      <bpmndi:BPMNEdge id="Flow_11hyyfb_di" bpmnElement="Flow_11hyyfb">
        <di:waypoint x="186" y="118" />
        <di:waypoint x="240" y="118" />
      </bpmndi:BPMNEdge>
      <bpmndi:BPMNEdge id="Flow_1voaj36_di" bpmnElement="Flow_1voaj36">
        <di:waypoint x="340" y="118" />
        <di:waypoint x="402" y="118" />
      </bpmndi:BPMNEdge>
    </bpmndi:BPMNPlane>
  </bpmndi:BPMNDiagram>
</bpmn:definitions>
saig0 commented 2 years ago

@christian-konrad thank you for raising this up. :+1:

As a developer, I see your point and agree with it. However, according to the DMN specification (version 1.4, chapter 10.3.2.15, page 127), this is not possible. The concatenation is only defined for two string values. Other values should return in null.

Since the FEEL engine behaves as defined in the spec, I change the issue type to enhancement.

We will discuss if we want to change the behavior and not follow the spec here.

christian-konrad commented 1 year ago

I ran into that issue again, it is so inconvenient and users may never be able to resolve it themselves:

Bildschirmfoto 2022-11-17 um 15 26 55

Evaluated to null, my connector threw an incident, and there was no hint why. Until I remembered that string + integer = null and investigated that the IDs returned are int, not string...

menski commented 1 year ago

@felix-mueller please give us input if you think this is something we should build

christian-konrad commented 1 year ago

Btw. in case this is not default FEEL, we should propose it to the OMG first, and mitigate it meanwhile via linting @nikku

nikku commented 1 year ago

In standard FEEL we need to wrap non strings into string function, indeed.

It violates the "(business) user friendliness" claim. If we have type inference we would indeed be able to detect + potentially fix this in our modeling tooling. We'd need to address this issue one way or the other. Make our FEEL version less FEEL, but more friendly enough, or add enhanced validation / linting support on the modeling side.

nikku commented 1 year ago

Created https://github.com/camunda/camunda-modeler/issues/3312 to track the growing list of "semantic quirks" to be validated on the modeling side.

saig0 commented 1 year ago

The issue was reported to the OMG. Currently, it is targeted for DMN 1.6.

https://issues.omg.org/browse/DMN16-71