search

elastic / elasticsearch

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

Make TermsQueryBuilders Object ctor safer or deprecate it #71972

Open cbuescher opened 3 years ago

cbuescher commented 3 years ago

We currently have a constructor on TermsQueryBuilder that takes generic Object... values as argument, probably to facilitate creating terms queries for things like Date and GeoPoint. However we need to be able to serialize these objects via our transport protocol (everything that StreamOutput.writeGenericValue seems to work at least on that level), so not all Objects will work. We recently changed the code to use this serialization alread on construction to convert values to a BytesReference. Before that it was possible to use the object ctor for example in the High Level Rest Client for Enums, the json serialization for the request would then convert these to their String representation behind the scenes (see #71388). This doesn't work any longe, which on one hand is a good thing because we fail early on things we later cannot serialize anyway, but also shows how misleading using this constructor can be. I'm opening this issue as a starting point for investigation if we can make this ctor safer, either by replacing it with a stronger typed one (and deprecate and eventually remove the existing one), or by enhancing javadocs on the ctor to avoid its wider usage except for cases where it really makes sense (i.e. Date, GeoPoint etc...).

Relates to #71388

elasticmachine commented 3 years ago

Pinging @elastic/es-search (Team:Search)

cbuescher commented 3 years ago

In #73547 it was pointed out that several other query builders from the "match" family should also be checked if we should make them less lenient with general Object type constructors. In most of the cases I just spot-check we sooner or later convert to a String value down the line somewhere, so it would be good to check this and at least document this upfront or maybe prohibit this early.

lbenedetto commented 3 years ago

I believe this issue is affecting me. In 7.9.3 I was passing a List of whatever custom classes I wanted, but it was working because I had added an XContentBuilderExtension. After updating to 7.12.1 this no longer seems to be working and I now have to manually convert everything to a String before handing it over to elasticsearch which is annoying.

elasticsearchmachine commented 2 months ago

Pinging @elastic/es-search-relevance (Team:Search Relevance)