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

Add OptionalNullable<T>? #6308

Open
antkmsft opened this issue Dec 18, 2024 · 5 comments
Open

Add OptionalNullable<T>? #6308

antkmsft opened this issue Dec 18, 2024 · 5 comments
Labels
needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team.

Comments

@antkmsft
Copy link
Member

Non-nullable optionals should be Nullable<> - to distinguish between "not set" vs "" (strings) / 0 (ints), [] (arrays).
That distinction allows customer to express any value, without sacrificing anything. When !HasValue(), we don't send the value.

With non-optional nullables, everything is also Nullable<>, the only difference is that when nullable has no value, we serialize it as null (at least, in JSON body context; I don't remember exactly how paths/query params/headers behave - that's ok, that's beyond the point).

And finally, if there is an optional nullable, then we have two options:

  1. Nullable<T>, "not set" = do not send, "has value" = send value. This won't allow users to send null values, but maybe it is practical enough.
  2. Nullable<T>, "not set" = send null -- I don't think it is the right approach to optionals.
  3. Nullable<Nullable<T>>, options.Property.HasValue() == false = do not send, options.Property.HasValue() == true && options.Property.Value().HasValue() == false = send null; options.Property.HasValue() == true && options.Property.Value().HasValue() == true = send options.Value().Value(). This would be an implementation that allows to accurately represent any value, but it is most likely too complex to be used by humans. I think we should implement option 1, and if we ever encounter a need for the service to distinguish between null and lack of value sent in this case, we work with the service team, or we should put some other solution (i.e. something like item 4 below).
  4. Develop something like OptionalNullable<T>, which has methods for both HasNonNullValue() and IsNull() (and does not have HasValue() to avoid confusion with Nulable<T>). To set null values, we add operator=(std::nullptr_t)/OptionalNullable<T>(std::nullptr_t).
@github-actions github-actions bot added the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Dec 18, 2024
@antkmsft
Copy link
Member Author

antkmsft commented Jan 8, 2025

Better approach:

add Nullable<T>::operator=(std::nullptr_t), Nullable<T>::operator==(std::nullptr_t), and m_isLiterallyNull.
if m_isLiterallyNull == true, hasValue() returns false. To check for null value, you use == nullptr.
I

@LarryOsterman
Copy link
Member

Do we know of any Azure services which send "Value": null as opposed to simply not sending the field?

@antkmsft
Copy link
Member Author

antkmsft commented Jan 8, 2025

Here is what I am thinking about: #6322. If we find out that we need this, we can implement it as in that PR, and it should be a change that is easy to make and not break anyone.

@antkmsft
Copy link
Member Author

antkmsft commented Jan 8, 2025

Knowing that we can do that is probably good enough to close this work item. So, if we find out that we need this, we'll implement it as in that PR and will update the codegen.

@LarryOsterman
Copy link
Member

Oh, that's rather clever. I like it. And it's good to have this in our back pocket.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team.
Projects
None yet
2 participants