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

Expression for Context's Get entry/path with optional entries. #540

Closed
pme123 opened this issue Oct 25, 2022 · 14 comments
Closed

Expression for Context's Get entry/path with optional entries. #540

pme123 opened this issue Oct 25, 2022 · 14 comments
Labels
scope: Camunda 8 Required in the context of Camunda 8 type: enhancement

Comments

@pme123
Copy link

pme123 commented Oct 25, 2022

Is your feature request related to a problem? Please describe.
In contexts we have often the two cases a path / entry exists or it doesn't. With the construct (if then else) we have now, it is hard to work with.

Example:

{
  a: 1,
  b: {
    c: {
      d: 2
    }
  }
}

If b, c, and d is required the path is easy: b.c.d

But if these values are optional there is no simple way to get d if it exists.

Describe the solution you'd like
For Scripting like in Groovy, you can do b?.c?.d, which will return null if one of these entries are missing.

Supporting this would help a lot working with data structures in Camunda 8 processes.

@saig0 saig0 added the scope: Camunda 8 Required in the context of Camunda 8 label Oct 26, 2022
@saig0
Copy link
Member

saig0 commented Oct 26, 2022

@pme123 thank you for raising this up. 👍

As a workaround, you could write the following expression:

if b.c.d != null then b.c.d else null

However, the null check is not very elegant.

The proposal by using a new operator ? may conflict with the DMN spec because b? could be a name. Maybe, we could introduce a new built-in function instead.

@pme123
Copy link
Author

pme123 commented Oct 26, 2022

if b.c.d != null then b.c.d else null works - also if c does not exist! I haven't expected that:).

Maybe, we could introduce a new built-in function instead.

That would work as well. Maybe also more readable for a non-programmer;). We could extend the existing one get value(), for Example:

get value({b: {}}, "b?.c?.d") // -> null

@saig0
Copy link
Member

saig0 commented Oct 26, 2022

We could already use the get value() function in the following way:

get value(b.c, "d")
// returns either the value of D if B, C and D exists, or null 

@pme123
Copy link
Author

pme123 commented Oct 26, 2022

This is great.
So if I have only required fields I just use the path, like b.c.d, and if some of the path is optional I use get value(b.c, "d").

@pme123
Copy link
Author

pme123 commented Oct 30, 2022

@saig0 I checked the code and added some Test cases.

So what you propose is working already with the following warning:

WARN org.camunda.feel.FeelEngine - Failed to invoke function: context contains no entry with key 'c'

The function would IMHO look a bit nicer if we only had to have the path as parameter. So I tried that and this worked exactly the same with the above warning.

So what you think - shall I make a pull request for: get value(b.c.d)?

And maybe we need to change the Warning to an Info.

@saig0 saig0 removed their assignment Nov 21, 2022
@saig0
Copy link
Member

saig0 commented Dec 22, 2022

I thought about the issue and different solutions. Currently, my favorite solution would be:

Modify the get value() function and introduce a new parameter list.

In addition to get value(context, key), we would add the parameter list get value(context, keys). The parameter keys must be a list of strings and is interpreted as the path.

get value({x: 1}, ["x"])
// 1

get value({x: {y: 2}}, ["x", "y"])
// 2

get value({}, ["x"])
// null

get value({}, ["x", "y"])
// null

If one of the keys is not part of the context then the function returns null and it doesn't print any warning.

The context put function has also a parameter keys that works in a similar way.

@pme123 would this solution fit your case?

@pme123
Copy link
Author

pme123 commented Dec 22, 2022

Hi @saig0
It sure would work, but it does not look really nice - I could live with it though;).
What do you think is the problem with get value(b.c.d)?

@saig0
Copy link
Member

saig0 commented Dec 23, 2022

What do you think is the problem with get value(b.c.d)?

The semantics. The get value() function is used to extract a key from a given context.

The function get value(b.c.d) would be static context access with the intention to ignore non-existing context entries and return null instead. So, a different intention.

Alternatively, we could introduce a new function for this case. For example, optional(b.c.d) or get or null(b.c.d).

Additional context for the get value(context, keys) function: camunda/camunda#11293

@pme123
Copy link
Author

pme123 commented Dec 24, 2022

Ok, in this case my favourite would be
optional(b.c.d) or optional value(b.c.d)

@nikku
Copy link
Member

nikku commented Jan 16, 2023

Came up in the context of "null" safety discussion internally.

Up for clarification: What is the intention of the DMN spec.

// strict handling
{
  a,
  c
}.b // BOOM "b is not a member"

// lax handling
// non existing member access coherses to <null>
{
  a,
  c
}.b = null

@saig0
Copy link
Member

saig0 commented Jun 1, 2023

Update: the get value(context, keys) function is available since version 1.16.0.

@saig0
Copy link
Member

saig0 commented Jun 1, 2023

Based on the discussions (#582, #572), we came to the conclusion that the FEEL engine should handle non-existing variables (and properties) gracefully and return null. Together with this change, we want to improve error handling (#260) and introduce a way to express required variables (and properties).

There is an open issue for clarifying the behavior in the DMN-TCK: dmn-tck/tck#537.

@saig0
Copy link
Member

saig0 commented Jun 12, 2023

The expected behavior was clarified by merging the new TCK test case.

@saig0
Copy link
Member

saig0 commented Jul 6, 2023

I'm closing this issue in favor of #674. It sums up the finding in this issue but as a bug report.

@saig0 saig0 closed this as completed Jul 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: Camunda 8 Required in the context of Camunda 8 type: enhancement
Projects
None yet
Development

No branches or pull requests

3 participants