elastic / elasticsearch-net

This strongly-typed, client library enables working with Elasticsearch. It is the official client maintained and supported by Elastic.
https://www.elastic.co/guide/en/elasticsearch/client/net-api/current/index.html
Apache License 2.0
11 stars 1.15k forks source link

8.15.6 - Term/Terms Filtering feedback #8344

Open niemyjski opened 2 months ago

niemyjski commented 2 months ago

Elastic.Clients.Elasticsearch version: 8.15.6

Elasticsearch version: 8.15.1

.NET runtime version: 8.x

Operating system version: Any

Description of the problem including expected versus actual behavior:

I noticed a lot of inconsistencies / verbosity when converting to the new api's

Previous

The existing TermQueryDescriptor is more aligned with the new mapping api where you select the field with an expression. The new one is very verbose

Filter(f => f.Term(m => m.Field1, "value1") 

New

Filter(f => f.Term(m => m.Field(tf => tf.Field1).Value("value1")))

Expected behavior

New filtering is really verbose and and doesn't feel like other areas of the api.

Reference: https://github.com/FoundatioFx/Foundatio.Parsers/pull/84

flobernd commented 2 months ago

Hi @niemyjski ,

I think this boils down to a code generator optimization which I've already planned in the corresponding (private) repository.

A Term query, for example, requires 2 mandatory properties Field and Value. For the object initializer syntax (TermQuery) we already generate a custom constructor that enforces the Field to get initialized. Value is currently missing here and for the fluent syntax (TermQueryDescriptor) no custom constructor is generated at all.

This feature requires quite some effort to implement in the code generator, but in the end we should be able to generate an API overload that looks exactly like the one from NEST.

I still have to think about some details; for example:

Term(Action<T, object> field, FieldValue value)

does not allow to further mutate the query (aka setting optional properties like Boost).

My current plan is to keep the original overload (marked as deprecated):

Term(Action<TermQueryDescriptor<T>> configure)

and to add another one that looks like this:

Term(Action<T, object> field, FieldValue value, Action<TermQueryDescriptor<T>> configure)