elastic / elasticsearch

Free and Open, Distributed, RESTful Search Engine
https://www.elastic.co/products/elasticsearch
Other
69.41k stars 24.56k forks source link

Painless: Improve Error Messages for methods missing in painless but existing in underlying classes #79062

Open spinscale opened 2 years ago

spinscale commented 2 years ago

Sometimes painless decides to implement a subset of methods to not implement slow or dangerous methods - which is good.

I just ran into a tricky one from a debugging perspective. Java's List supports a remove(Object) method, in addition to List.remove(index). Painless does not. However the error messages are somewhat cryptic and even differ if I try to call remove using params or not. See this example

PUT test/_doc/1
{
  "data" : ["1", "2", "3"]
}

GET test/_doc/1

POST test/_update/1
{
  "script": {
    "source": """
    ctx._source.data.remove("2");
    """,
    "lang": "painless"
  }
}

The above returns a wrong_method_type_exception stating

cannot convert MethodHandle(List,int)Object to (Object,String)Object

This one has the same intent, but returns a different error message

POST test/_update/1
{
  "script": {
    "source": "ctx._source.data.remove(params.id);",
    "lang": "painless",
    "params" : {
      "id":"2"
    }
  }
}

returns

class java.lang.String cannot be cast to class java.lang.Number (java.lang.String and java.lang.Number are in module java.base of loader 'bootstrap'

If you know a little java, you can figure out, that the wrong remove method is called resulting in a class cast exception.

Long story short: In addition to unify the error message, we should probably try to have more descriptive error messages stating that this method does not exist or suggest an alternative, especially because Debug.explain() returns ArrayList despite painless not supporting all methods - which of course is listed in the docs correctly.

elasticmachine commented 2 years ago

Pinging @elastic/es-core-infra (Team:Core/Infra)

rjernst commented 2 years ago

I thought we already had an issue for this, I know we have discussed it in the last few months. https://github.com/elastic/elasticsearch/pull/77044 should make this possible. Similarly, we've discussed having a "did you mean" when a method isn't found at all, but is similar to an existing method name.