Skip to content
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

max_score and _score are required (and can be null) #1144

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

swallez
Copy link
Member

@swallez swallez commented Dec 16, 2021

Follow-up to #1079

The _score and max_score can be null, but are always present. This PR makes them required.

Copy link
Member

@delvedor delvedor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking as we have discovered that many yaml tests suggest that this property should be optional.

@swallez swallez marked this pull request as draft December 16, 2021 17:34
@swallez
Copy link
Member Author

swallez commented Dec 16, 2021

It's weird to not see these fields in the yaml tests as they are always sent by the ES server.

This could be related to some lenient behavior of the yaml test engine. Putting this PR on hold for now.

@swallez swallez changed the title max_score and _score are required (and can be null) [on hold] max_score and _score are required (and can be null) Dec 16, 2021
Copy link
Contributor

Following you can find the validation results for the API you have changed.

API Status Request Response
search 🔴 2026/2107 2041/2089

You can validate this API yourself by using the make validate target.

Copy link
Member

@pquentin pquentin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nearly three years later, YAML test validation no longer suggests those properties should be optional, as 2041/2089 is the current response score.

@pquentin pquentin dismissed delvedor’s stale review June 20, 2024 13:54

This no longer holds true

@swallez swallez marked this pull request as ready for review June 20, 2024 14:06
@swallez swallez changed the title [on hold] max_score and _score are required (and can be null) max_score and _score are required (and can be null) Jun 20, 2024
@swallez
Copy link
Member Author

swallez commented Jun 20, 2024

max_score and _score can be null (see here and here) when the score is NaN. This is semantically equivalent to a missing value (there's no known score) and is not a "meaningful null", and so should be represented as an optional value in strongly typed languages.

@Anaethelion @l-trotta @flobernd can you make sure your respective code generators correctly interpret a required double | null as an optional double?

@swallez swallez requested a review from l-trotta June 20, 2024 14:25
@swallez swallez marked this pull request as draft June 20, 2024 14:26
@flobernd
Copy link
Member

@swallez I can confirm that the .NET generator converts X | null unions to nullable (optional) values (X?).

@pquentin
Copy link
Member

There are few existing mandatory | null in the specification, one example being IndicatorNode in the Health Report API.

In any case, since this already exists, I would be in favor of merging anyway.

@l-trotta
Copy link
Contributor

that's a no for java, the result here is just double, which is not nullable

@l-trotta
Copy link
Contributor

if java is the only problem I'll assign this one to myself and merge it once I have a fix for it

@Anaethelion
Copy link
Contributor

Go is in the same state as Java, for this particular case it translates to a Float64.

@pquentin
Copy link
Member

pquentin commented Sep 2, 2024

I believe Java is the only language left here since https://github.com/elastic/elastic-client-generator-go/pull/39 was merged.

@l-trotta
Copy link
Contributor

l-trotta commented Sep 3, 2024

I am still working on this yes

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

Successfully merging this pull request may close these issues.

8 participants