Skip to content

Discrepancy between Query types in Fields property #8081

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

Closed
gpetrou opened this issue Apr 4, 2024 · 1 comment · Fixed by #8084
Closed

Discrepancy between Query types in Fields property #8081

gpetrou opened this issue Apr 4, 2024 · 1 comment · Fixed by #8084

Comments

@gpetrou
Copy link
Contributor

gpetrou commented Apr 4, 2024

Elastic.Clients.Elasticsearch version: 8.13.1

Elasticsearch version: 8.11

.NET runtime version: 8.0

Operating system version:

Description of the problem including expected versus actual behavior:
MultiMatchQuery has a Fields property of Elastic.Clients.Elasticsearch.Fields? type.
QueryStringQuery and SimpleQueryStringQuery have a Fields property of ICollection<Elastic.Clients.Elasticsearch.Field>? type.
Is there a reason for this change in the new version of the library? Shouldn't these be of the same Elastic.Clients.Elasticsearch.Fields? type?

Steps to reproduce:
1.
2.
3.

Expected behavior
A clear and concise description of what you expected to happen.

Provide ConnectionSettings (if relevant):

Provide DebugInformation (if relevant):

@flobernd
Copy link
Member

flobernd commented Apr 4, 2024

Hi @gpetrou,

thanks again for testing the new version and reporting rough edges. That's super helpful 🙂

Semantics for Fields and Field[] are actually different. The specification defines Fields as:

export type Fields = Field | Field[]

We call this a "single-or-many" construct. The ES server accepts the JSON property in a singular form "field" and in an array form ["field", "..."].

Field[] on the other hand, only accepts the array form.

The old generator incorrectly replaced Field[] with Fields which caused an issue with e.g. the stored_fields that Steve tried to fix in #8018.

The current way of generating these properties is semantically correct, but a litte bit cumbersome to use. A proper fix that I've planned involves removing the [JsonConverter(typeof(FieldsConverter))] attribute from the Fields class. Afterwards, the generator will emit Field[] properties using the Fields type again, but dynamically places converter attributes on the properties - depending on the semantics (either FieldsConverter or FieldArrayConverter).

I expect to release this improvement already in the beginning of the next week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants