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

Represent "meaningful null" #2049

Closed
swallez opened this issue Mar 31, 2023 · 4 comments · Fixed by #2478
Closed

Represent "meaningful null" #2049

swallez opened this issue Mar 31, 2023 · 4 comments · Fixed by #2478

Comments

@swallez
Copy link
Member

swallez commented Mar 31, 2023

[edit 2023-04-03 - rewritten to explain "semantic null" and provide several proposals]

Some fields in the settings API handle null values differently from missing values: sending null resets the setting to their default value, whereas omitting the field just keeps the current value.

We already use SomeType | null in the spec for fields where ES can return null for missing or unknown values. Semantically these are optional values where the ES server doesn't have a if (value == null) test before serializing the field. We added these | null mainly to allow validation tests to pass, even if semantically speaking they should not be there.

We need to distinguish "semantic null" from "null-as-optional".

There are different ways to represent this in the spec:

  • Follow the example of Stringified to represent non-string values that ES can sometimes serialize as strings, and introduce type OptionalNull<T> = T | null:

    • semantic nulls: use SomeType | null.
    • null as optional: replace existing SomeType | null with OptionalNull<SomeType>.
  • Consider that semantic null is important enough to have its own type defined as type NullValue = null:

    • semantic nulls: use SomeType | NullValue
    • null as optional: leave existing SomeType | null as is.
  • Consider that the null value is an encoding concern and that it's really about default values:

    • semantic nulls: use SomeType | Default where type Default = null
    • null as optional: leave existing SomeType | null as is.

    Note: we can also consider Reset instead of Default but this name implies some action rather than a value or state. It makes sense for a request value but not so much for a response value.

  • Something else?

@Mpdreamz
Copy link
Member

This has come up in the .NET world often too and while we offer workarounds the Developer Experience has never been great. +1 modelling this explicitly.

In .NET I would still model this explicitly as T | Reset | null so folks can explicitly set the property to Reset while still serializing null. While null would retain its unset properties and not serialize the whole property.

This is also an issue with the Update API where people may use null to reset fields.

Since the update doc is userdefined there is nothing we can spec for it but something to consider in this context too.

@Anaethelion
Copy link
Contributor

I think its important we keep the distinction between null as a possible type and null as a possible value, not necessarily default, from an API perspective.

Stringified is not entirely the same since its purpose was the augmentation of a type to a union of said type and a string. Here we are trying to convey not-only a type but probably an API change.

TLDR: I really like the SomeType | NullValue approach.

@stevejgordon
Copy link
Contributor

++ To introduce a way to distinguish this in the spec. I've avoided generating endpoints to update index settings at the moment, as I didn't have a way to identify this nuance without coding something into my generator.

I have a local branch with essentially what @Mpdreamz suggested that I was considering as a way to support source updates with explicit nulls to remove fields. The same type could serve this purpose for "resettable" properties.

I'm also wondering if we need to consider explicitly splitting the type used for, say, index settings, where the type used in requests includes the chosen representation to allow resetting a value by sending null but where the response type uses a more standard type since the properties will either be set or potentially null/optional. For .NET, this would make consumption of GET index settings easier since we'd avoid wrapping properties as a Resettable<T> (name to be determined) type. On the flip side, this would probably make updating the response type properties to send them back into an update more painful without some further magic to introduce conversion operators or some other mechanism to make GET followed by UPDATE easy to work with.

I'm leaning towards SomeType | NullValue as a way to represent these quite cleanly.

@sethmlarson
Copy link
Contributor

Since we know that the server team will be taking over updating the spec in the future, I'd like to go with the explicit approach which forces a choice over how code generation should handle nulls, thus I propose:

  • Add two new type aliases of null: NullAsValue and NullAsMissing which have specific defined meaning for the API and code generators.
    • NullAsValue has the new definition that's defined in this issue.
    • NullAsMissing has the current definition of null where it's synonymous with a missing value.
  • Disallow using an unspecified null as a type in the compiler.

dakrone added a commit to dakrone/elasticsearch-specification that referenced this issue Jun 27, 2023
These keys allow using an explicit `null` as the value to "unset" the configuration when merging multiple component templates. As related to the merging tables seen in elastic/elasticsearch#95979 (comment)

Relates also to the discussion in elastic#2049
dakrone added a commit to dakrone/elasticsearch-specification that referenced this issue Jun 27, 2023
These keys allow using an explicit `null` as the value to "unset" the configuration when merging multiple component templates. As related to the merging tables seen in elastic/elasticsearch#95979 (comment)

Relates also to the discussion in elastic#2049
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.

5 participants