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

Recasting "date and time" results in null #697

Closed
abbasadel opened this issue Aug 1, 2023 · 7 comments
Closed

Recasting "date and time" results in null #697

abbasadel opened this issue Aug 1, 2023 · 7 comments
Assignees
Labels
support relates to a support case type: enhancement

Comments

@abbasadel
Copy link

abbasadel commented Aug 1, 2023

Describe the bug
Evaluate the following FEEL expression

{   builtin_now: now(),   builtin_now_again: date and time(builtin_now) }

built_now_again comes out null

To Reproduce
Steps to reproduce the behavior:

Expression with redundant 'date and time' casting:
{ builtin_now: now(), builtin_now_again: date and time(builtin_now) }

Results in:
{"builtin_now":"2023-07-31T18:39:50.009723879Z[Etc/UTC]","builtin_now_again":null}

Expression without redundant casting:
{ builtin_now: now(), builtin_now_again: builtin_now }

Results correctly:
{"builtin_now":"2023-07-31T18:42:39.022504381Z[Etc/UTC]","builtin_now_again":"2023-07-31T18:42:39.022504381Z[Etc/UTC]"}

DMN example
<?xml version="1.0" encoding="UTF-8"?>
<definitions xmlns="https://www.omg.org/spec/DMN/20191111/MODEL/" xmlns:dmndi="https://www.omg.org/spec/DMN/20191111/DMNDI/" xmlns:dc="http://www.omg.org/spec/DMN/20180521/DC/" xmlns:modeler="http://camunda.org/schema/modeler/1.0" id="Definitions_1dyiz3a" name="DRD" namespace="http://camunda.org/schema/1.0/dmn" exporter="Camunda Modeler" exporterVersion="5.12.0" modeler:executionPlatform="Camunda Cloud" modeler:executionPlatformVersion="8.2.0">
  <decision id="Decision_0kdaoiu" name="Decision 1">
    <variable id="InformationItem_1ely7et" name="test" />
    <literalExpression id="LiteralExpression_08ot0qg">
      <text>

{   builtin_now: now(),   builtin_now_again: date and time(builtin_now) }
</text>
    </literalExpression>
  </decision>
  <dmndi:DMNDI>
    <dmndi:DMNDiagram>
      <dmndi:DMNShape id="DMNShape_0u9e1z6" dmnElementRef="Decision_0kdaoiu">
        <dc:Bounds height="80" width="180" x="160" y="100" />
      </dmndi:DMNShape>
    </dmndi:DMNDiagram>
  </dmndi:DMNDI>
</definitions>

Expected behavior
builtin_now_again should equal builtin_now

Workaround
Avoid redundant casting

Hint
Seems like there's a case missing in datetime function in ConversionBuiltinFunctions.
The warning output we get is:

15:14:10.596 [main] WARN org.camunda.feel.FeelEngine – Suppressed failure: illegal arguments: List(ValDateTime(2023-07-28T15:14:10.593081-04:00[America/New_York]))

Environment

  • FEEL engine version: 1.16.0

Support: https://jira.camunda.com/browse/SUPPORT-17817

@abbasadel abbasadel added type: bug support relates to a support case labels Aug 1, 2023
@korthout
Copy link
Member

korthout commented Aug 2, 2023

ZPA triage:

  • severity: unable to use conversion function from date and time to date and time
  • bug seems reasonable to resolve
  • expected size: small
  • marking as later
  • might be interesting as a mob programming topic

@nicpuppa
Copy link
Contributor

nicpuppa commented Aug 3, 2023

Is this really a bug ? Looking at the documentation the date and time function accept only string object. The behaviour of the function is correct from my perspective.

@abbasadel
Copy link
Author

Very valid point @nicpuppa. I think we can safely assume this is a feature request and not a bug.
@saig0 since you are the stakeholder/codeowner, kindly have a look on this feature request.

@abbasadel
Copy link
Author

Moving this issue to blocked since we are waiting for stakeholders' feedback

@saig0
Copy link
Member

saig0 commented Aug 28, 2023

@abbasadel I tend to reject this feature request.

Reasoning:

  • I see no relevant use case for extending the built-in function date and time() to accept a date-and-time value and return it as it is. If there is a use case, please share it to demonstrate the need.
  • Without a good use case, it is unlike that the DMN spec will adopt this change. As a result, this function will end up as a Camunda extension. We prefer to stay close to the DMN spec.
  • There are no other built-in functions that behave like the expected behavior of this request. We should aim for a consistent behavior of all built-in functions to keep FEEL "friendly" (i.e. easy to use).

I recommend rethinking how the expressions are modeled.

@abbasadel
Copy link
Author

abbasadel commented Aug 28, 2023

Thanks @saig0 for your feedback. What you said makes sense to me.

@saig0
Copy link
Member

saig0 commented Sep 25, 2023

It seems that we have an agreement that we don't want to add the requested function. Let's close the issue.

@saig0 saig0 closed this as completed Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
support relates to a support case type: enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants