🌊 Add type safety to Painless conditions

[simianhacker]

🍒 Summary

This PR closes https://github.com/elastic/streams-program/issues/18 by adding some basic type checking to the painless output for the Stream conditions. The new code will check to ensure that none of the fields used in the condition are Map objects. Then it wraps the if statement in a try/catch.

Condition

{
  and: [
    { field: 'log.logger', operator: 'eq' as const, value: 'nginx_proxy' },
    {
      or: [
        { field: 'log.level', operator: 'eq' as const, value: 'error' },
        { field: 'log.level', operator: 'eq' as const, value: 'ERROR' },
      ],
    },
  ],
}

Before

(ctx.log?.logger !== null && ctx.log?.logger == "nginx_proxy") && ((ctx.log?.level !== null && ctx.log?.level == "error") || (ctx.log?.level !== null && ctx.log?.level == "ERROR"))

After

if (ctx.log?.logger instanceof Map || ctx.log?.level instanceof Map) {
  return false;
}
try {
  if ((ctx.log?.logger !== null && ctx.log?.logger == "nginx_proxy") && ((ctx.log?.level !== null && ctx.log?.level == "error") || (ctx.log?.level !== null && ctx.log?.level == "ERROR"))) {
    return true;
  }
  return false;
} catch (Exception e) {
  return false;
}

[flash1293]

Thanks for setting this up, @simianhacker - works really well, the only thing I think would make sense to add is to cast to number types when doing gt, gte and so on both for values and fields.

Otherwise this situation will not work as I would expect:

// condition
{
            "field": "numberfield",
            "operator": "gt",
            "value": "15"
          }

// Doc (will not be rerouted)
POST logs/_doc
{
  "numberfield": "18"
}

// condition
{
            "field": "numberfield",
            "operator": "gt",
            "value": 15
          }

// Doc (will not be rerouted)
POST logs/_doc
{
  "numberfield": "18"
}

// condition
{
            "field": "numberfield",
            "operator": "gt",
            "value": "15"
          }

// Doc (will not be rerouted)
POST logs/_doc
{
  "numberfield": 18
}

[flash1293]

The same thing we should probably do the other way for contains and equals so everything is a string always? The UI only gives one input field for the value, so we can't really tell whether we should do an equal check on strings or numbers.

[simianhacker]

@flash1293 I added some type safety to lt, lte, gt, gte, contains, startsWith, and endsWith. I also added some integration tests. Let me know what you think.

[flash1293]

Thanks for the changes @simianhacker , this works great, I just found one more case we should handle which is eq/neq:

// condition
{
            "field": "numberfield",
            "operator": "eq",
            "value": 15
          }

// Doc (will be rerouted)
POST logs/_doc
{
  "numberfield": 15
}

// Doc (will not be rerouted)
POST logs/_doc
{
  "numberfield": "15"
}

[simianhacker]

@flash1293 I wonder if we shouldn't keep the eq and neq as EXACT match instead of converting it to a string. Maybe add regex and the caveat for using regex is that it's always converted to a string and then does the regex match.

[flash1293]

@simianhacker how would the user use eq/neq in the UI?

This is how it looks today:

Would the user need to enter "15" in order to match 15-the-string? That seems weird to me, especially because it's hard to tell how the data will arrive. Reaching to regex for this also seems weird.

This is not just a theoretical concern, it's pretty common for numbers to be sent as strings instead, but it's also not always the case.

I'm happy to solve this general concern in a different way, but I think we need to solve it somehow - casts just seem like the easiest way to do so. Maybe we need a "like" operator? :)

[simianhacker]

@flash1293 I see you point, that's kind of a limitation of HTML forms is that it's really hard to discern a number vs a string when the API can accept both. Base on our Slack conversation, let's move towards "ease of use". The distinction in the API of "15" vs 15 feels like a "foot gun" for new users when it comes to eq and neq, they probably mean the same thing.

I'll add the same checks/conversion to eq and neq

[flash1293]

The distinction in the API of "15" vs 15 feels like a "foot gun" for new users when it comes to eq and neq, they probably mean the same thing

Should we follow through and only allow strings on the API?

[elasticmachine]

💚 Build Succeeded

• Buildkite Build
• Commit: adf8960

Metrics [docs]

✅ unchanged

History

• 💔 Build #257109 failed 9b65b9e
• 💔 Build #256715 failed e392b14

[flash1293]

LGTM, thanks for all the changes!

[kibanamachine]

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/12181236765

[kibanamachine]

💚 All backports created successfully

Status	Branch	Result
✅	8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation