Hotchocolate: Optional bool is still required in schema (code-first)

[huysentruitw]

Describe the bug
When declaring a bool as Optional<bool>, the type still shows up as Boolean! in the schema.

To Reproduce
Create a mutation method in a code-first model that takes an Optional<bool> state = default as argument.

Expected behavior
The state argument should appear as optional in the schema.

Desktop (please complete the following information):

• OS: Windows 10

[PascalSenn]

Hi @huysentruitw
This is currently the intended behaviour

But it gets commonly confused.

What is your final take on this @michaelstaib

cc @mvestergaard

You can read about it there:
https://hotchocolategraphql.slack.com/archives/CD9TNKT8T/p1587598221363300?thread_ts=1587593198.359100&cid=CD9TNKT8T

Mathias Vestergaard 4 days ago
Any chance you could look into making the resulting type of Optional behave properly in 10.4.3 too?
For an input type, Optional becomes String which is what i'd expect, but Optional becomes Boolean! I'd expect Boolean

Michael:hot_pepper: 4 days ago
actually Optional should become Boolean

Michael:hot_pepper: 4 days ago
Optional is supposed to be a helper that detects when a prop is not set

Michael:hot_pepper: 4 days ago
for instance

Michael:hot_pepper: 4 days ago
you can have an input like the following

Michael:hot_pepper: 4 days ago
input foo {
bar : String! = "bar"
}

Michael:hot_pepper: 4 days ago
in this example you have a required property with a default

Michael:hot_pepper: 4 days ago
meaning

Michael:hot_pepper: 4 days ago
that bar is required but can be omitted by the client since it has a default

Michael:hot_pepper: 4 days ago
with optional you can detect if the field was provided or not

Michael:hot_pepper: 4 days ago
Optional is not meant to interfere with the type itself

Michael:hot_pepper: 4 days ago
so the schema builder strips away optional and inferrs the nullability from the inner type

Mathias Vestergaard 3 days ago
Michael Ok, what we're trying to use Optional for, is for patching behavior on updates. So for an input type, with no default, i would argue that Optional should be inferred to Boolean, otherwise Optional is pointless, since I can never not give a value, and while Optional works, it's wrong in relation to what I'm trying to do. If the value is set i want it to be a bool and not bool?. Make sense? (edited)

Mathias Vestergaard 3 days ago
As mentioned, it also doesn't work consistently atm. Optional becomes String while Optional becomes Int!

Mathias Vestergaard 3 days ago
So any value type, becomes non-nullable, while reference types don't (edited)

Mathias Vestergaard 2 days ago
Michael If nothing else, could you maybe add the IOptional interface to 10.4.3? It'll make some mapping logic a whole lot easier

[PascalSenn]

As said above, when you use nullable reference types you can define Optional<TRef?>. This should be handled correctly

[PascalSenn]

@huysentruitw BTW I've seen your issue on the graphql-client repo. You may be interested in this Strawberry Shake. It is a graphql client that generates code for your queries so you have a fully typesafe experience.
It will be released in May. You can already look at a preview here:
https://chillicream.com/blog/2019/11/25/strawberry-shake_2

[mvestergaard]

I definitely think the logic should be changed to only make the type non-nullable when there's a default value, otherwise it should be implicitly nullable.

[michaelstaib]

Again, optional has nothing to do with nullability. It is totally valid to not send a required field in GraphQL if it has a default value. I do not want to recreate C# built in types like Nullable<bool> and Optional<Nullable<bool>> is really something that is valid.

This is also a focus point for 11. Lets say you have the following input:

input Foo {
  b: Boolean! = false
}

In the above case if b is not send in which is again valid then Optional<bool> b will be HasValue = false and Value = false since the default value is false but it is still not set.

Optional would become useless if we also bind nullability to it. How would you even check nullability?

Optional => Was something send in.
Nullable => Something is nullable.

Same goes for reference types btw.

[mvestergaard]

But if there's no default value, I can never _NOT_ assign the value, making Optional pointless

[michaelstaib]

I still really think Optional<T>should only do one thing otherwise it becomes complicated. When do you use what and when does it what. Until know we are not taking into account things like default value when inferring types.

Thoughts @PascalSenn @rstaib

[mvestergaard]

For patching behavior, default values cannot be used, because only assigned values are relevant, but when they are assigned, they should be not null.

[michaelstaib]

The problem here is that from a spec standpoint there is nothing like patching. The spec will state that there is something like the argument coercion where we would need to assign a definite value.

So in order to extract that something was send in we have to keep track of what is send in... and this we actually do with optional. That is why we created it.

[mvestergaard]

Example:

public class UserPatch
{
    public Optional<bool> IsActive { get; set; }
}

I only want to use the value of IsActive if it has been assigned, and when it has I want the value to be bool, not bool?. But atm I have to define it as Optional<bool?> which requires extra logic to check first whether it has been assigned, and then whether the value is valid

[michaelstaib]

http://spec.graphql.org/draft/#sec-Coercing-Variable-Values

[mvestergaard]

Fair, but isn't Optional<T> more of an internal HotChocolate utility than anything?

[michaelstaib]

@mvestergaard I understand. So do not think I want to just block this off.

But I think that we would mix things up and writing it more explicit is actually better.

public class UserPatch
{
    public Optional<bool?> IsActive { get; set; }
}

[michaelstaib]

But I am open to discuss this so this. For me personal things should be simple and explainable. But I also get your point.

[michaelstaib]

maybe the the name of optional is not the right one.

[mvestergaard]

We map it internally to an equivalent type called Maybe<T>. Also because we do mapping using AutoMapper, it's not that huge a problem, since it'll map Optional<bool?> to Maybe<bool> just fine. It's, more of a thing that our developers need to be aware of. They need to remember to define it nullable in one place, and not in the other. It feels weird.

[michaelstaib]

As mentioned, it also doesn't work consistently atm. Optional becomes String while Optional becomes Int!

So with nullable ref types this is not working correctly?

[mvestergaard]

As mentioned, it also doesn't work consistently atm. Optional becomes String while Optional becomes Int!

So with nullable ref types this is not working correctly?

Yea Optional<string> becomes String, but Optional<int> becomes Int!. So it's not consistent.

[michaelstaib]

With nullable ref types activated?

[michaelstaib]

If so this is a bug.

[mvestergaard]

Yep

[PascalSenn]

well, the real issue is the name in this case.

Because Optional<T> implies that this field is _optional_ to set which then implies that it should be nullable.

I see that we should not recreate the type system like Nullable<T>..
But the main issue with Nullable is that it does not work for reference types.

Also, it would be a nice tool to use Optional like described above. I ran into this issue too when Optional was introduced.

I think all of your points are valid @michaelstaib. I mean, It also gets hairy when you want to make a property non nullable when it is set with Optional in this case. What would the API look like if someone wants to do what you descirbed?

public class UserPatch
{
    [GraphQLNonNull]
    public Optional<bool> IsActive { get; set; }
}

Not really, right? :D

But I still think we need something like this.

[michaelstaib]

OK, we need to fix that ... I will write some tests, it needs to be consistent. This might be the case since we are removing Optional internally and with that the nullable array does not match anymore.

[mvestergaard]

Another thing. I notice that in the master branch there's an IOptional interface. Would it be possible to have that introduced in a 10.4 update? It'll make mapping a whole lot easier.

[michaelstaib]

yes, if we go with this design, then we could port it back. But if we now decide we need to change the name for instance then not. Before we port anything back we should get consensus on what Optional is and what it should be called :)

[mvestergaard]

@PascalSenn
I guess it could be

public class UserPatch
{
    public Optional<bool> IsActive { get; set; } = false;
}

If you were to go the route of the default value carrying nullability meaning.

[michaelstaib]

I would also like to hear @rstaib thoughts on this and also @tunurgitr thoughts.

[michaelstaib]

public class UserPatch
{
    public Optional<bool> IsActive { get; set; } = false;
}

the default value in this case cannot be inferred.

[michaelstaib]

you can do it with an attribute.

[mvestergaard]

Ah, it can only be inferred on method parameters i suppose?

[michaelstaib]

We also could discuss this in the standup.

[huysentruitw]

Intuitively, I've tried bool? and Optional<bool> to get me an optional Boolean in the schema. I would have never thought it was Optional<bool?>, am I then supposed to write if (field.HasValue && field.Value.HasValue) and field.Value.Value? This also complicates unit-testing branch coverage.

Anyway, let me know the outcome of the standup 😎

[michaelstaib]

Why Optional<T>is correctly implemented.

So, what is Optional<T> actually, and I think this is important to understand before we have a look at how it works.

In GraphQL we have two kinds of values in the coercion. We have explicitly provided values and we have implicitly provided values.

We do have the same in java script, we can have properties that are intentionally null, we can have properties that are intentionally some value and we can have unspecified values.

In C# we are missing that and this is why we introduced Optional<T> in the first place.

Case 1:

input Foo {
  bar: Int
}

The above input Foo has a nullable field bar which is an int. If we were to use Optional<int?> in order to implement patch we can have the following states.

{
}

We provide an empty object which in our case means that bar was not provided and is implicitly null. Meaning the user did not set this field.

bar.HasValue = false;
bar.Value = null;

{
  bar: null
}

In the above example we now provided null as value which means that bar is intentionally / explicitly null.

bar.HasValue = true;
bar.Value = null;

The last case in this example is where we explicitly provide a value (intentionally).

{
  bar: 1
}

bar.HasValue = true;
bar.Value = 1;

in the explicitly provided cases we do want to apply a patch for instance. The same goes for filters, in the case of filters we honor explicitly set fields but ignore implicitly set fields.

Case 2:

input Foo {
  bar: Int = 5
}

The above input Foo has a nullable field bar which is an int. If we were to use Optional<int?> in order to implement patch we can have the following states.

{
}

We provide an empty object which in our case means that bar was not provided and is implicitly 5. Meaning the user did not set this field.

bar.HasValue = false;
bar.Value = 5;

{
  bar: 5
}

In the above example we now provided 5 as value which means that bar is intentionally / explicitly 5.

bar.HasValue = true;
bar.Value = 5;

The last case in this example is where we explicitly provide null (intentionally).

{
  bar: null
}

bar.HasValue = true;
bar.Value = null;

Case 3

input Foo {
  bar: Int! = 5
}

The above input Foo has a required field bar which is an int. If we were to use Optional<int> in order to implement patch we can have the following states.

{
}

We provide an empty object which in our case means that bar was not provided and is implicitly 5. Meaning the user did not set this field.

bar.HasValue = false;
bar.Value = 5;

In the case of a required field with default we cannot provide null since null is not allowed by the type. But we can provide any value including the value 5 intentionally.

{
  bar: 5
}

bar.HasValue = true;
bar.Value = 5;

This means inferring Optional<int> as nullable by default is not possible since Optional<int> cannot hold null as implicit value. It would basically become Nullable<int>. Which is already supported.

Optional<T> is meant for those cases where it matters if something is implicitly default / explicitly default / explicitly other values. So, basically we are preserving with this information that we have while performing the variable-/argument-coercion algorithms specified in the GraphQL spec for tools so that we can build on top of this solutions like filters or patch.

In this sense we having the ability to ask for bar.Value.HasValue is actually what is wanted. Why else have optional in the first place.

[michaelstaib]

I will write some tests for nullable ref types however so that we can see what is wrong with that.

[huysentruitw]

Thank you for this detailed explanation, I appreciate the time you're putting in this.

When I read your explanation as a clean code enthusiast, it becomes clear that the terminology you're using doesn't match the implementation. To me, HasValue means there is a Value, no matter if that was provided by the caller or if it was a default value.

F.e. try to get your head around this, without knowing this entire background:

bar.HasValue = false;
bar.Value = 5;

Also I'd expect HasValue to be false when a null is provided which is probably because of the .NET Nullable type we all know.

You keep talking about 'provided' and in your head HasValue clearly means 'was provided', so why not rename that property to ValueWasProvided or ValueIsProvided instead? I think that would clear some things up.

[huysentruitw]

"Explicitly set" is also terminology that could be used in the naming.

[michaelstaib]

That is why I said it might be the wrong name. But, we followed the naming of Roslyn which also uses an optional for the same use-case in their compiler API. So, as I reflect we decided to follow their API.

[michaelstaib]

https://github.com/dotnet/roslyn/blob/master/src/Compilers/Core/Portable/Optional.cs

[michaelstaib]

They call it a meaningful value. Which also fits the bill here. If it is explicitly provided by the user / request it basically is a meaningful value. Since Optional was already established for the same use-case in roslyn we followed the naming.

I think we have to provide more documentation around this to remove confusion. But after looking at our old discussions on this all of the behavior was deliberately chosen to help people basically implement patch solutions.

GraphQL does not explicitly allow patching and this state would not be provided to the user in a simple manner. With Optional<T> we allow this.

But I know that optional sounds kind of wrong :) Nevertheless, it is now established and we should fix confusion around this with documentation rather than with breaking changes. I think a breaking changes should always provide a distinct value to the user that is worth breaking APIs.

thoughts @rstaib @PascalSenn @huysentruitw @mvestergaard

[mvestergaard]

@michaelstaib I realise after all this that what I may be missing on my end is to use default values.

public class UserPatch
{
    [DefaultValue(false)]
    public Optional<bool> IsActive { get; set; }
}

This is probably the behavior i want.
The only thing with that is that it's somewhat verbose to set the default values. I don't know if any other solution could be made for that. Perhaps some type that can have the default value be configured elsewhere. OptionalWithDefault<bool> kind of thing. Not sure how clean that would be.

Also, as you've hinted towards, Optional may not be a good name for it, as it leads to thinking the field will be Optional in the graphql schema. Names to consider Maybe<T>, MaybeAssigned<T>.

[michaelstaib]

The default value here is really a pity. Since we can get it if it is a parameter. But prop default assignments do not exist. We have planned a lot more refinements around this so we will look at the default values.

[huysentruitw]

@michaelstaib I don't think a lot of HotChocolate users will know the internals of Roslyn, so to me that's not an excuse not to make things clear, you talk about 'confusion' and 'needs documentation', which is a clear sign the API is wrong. Anyway, I'm just a random Internet stranger so I'm ending this discussion here on my end.

[michaelstaib]

@huysentruitw this is a valid criticism. The question is and I think I wait for more opinions to come in on this issue if it is worth breaking peoples projects tp rename this. I mean we do breaking changes but this always has to be a collective decision.

Also, if we decide to change that we also need to find a good name that we use instead :)

I so far have not found a good none.

[PascalSenn]

@michaelstaib
Maybe<T>: Is maybe set, maybe not, can but not has to be optional.
Optional<T>: Is maybe set, maybe not but is optional (Nullable)

[michaelstaib]

But does maybe convey that the value is implicitly created like in the spec text mentioned?

[huysentruitw]

I can think of ValueIsSetExplicitly instead of HasValue, but yeah, naming is hard.

[mvestergaard]

My 2 cents: MaybeAssigned<T> or MaybeSet<T> although a little more verbose, could be more communicative, as that is what it's actually there for.
And the HasValue property renamed to HasBeenAssigned/IsAssigned or HasBeenSet/IsSet

[michaelstaib]

Would also value your input @tunurgitr.

[tunurgitr]

So, trying to follow this thread....I think the remaining concern is how Optional occurs when T is non-nullable and the parameter has a default value.

After typing out a few different arguments, I think I want to start off with.....I don't think it's a good idea to use Optional<T> here.

First, a caveat, it seems like this statement is broad enough that I may be missing something important.

That being said, my main point is: I think it's confusing for the backend to operate _differently_ when the value is explicitly provided vs. when it's left for the default to be provided. I don't think that would be what a client would logically expect?

Maybe I'm missing something, but I think if your contract is showing there's a default value, it's not expected that the backend will handle things differently based on whether you _actually passed in_ a value vs. whether you left it at the default.

Doesn't "default value" mean, "if you don't include this value, we'll treat it as if you passed X"?

Looking at your example @mvestergaard,

I only want to use the value of IsActive if it has been assigned, and when it has I want the value to be bool, not bool?

if I were a client, looking at this schema:

input UserPatch {
  isActive: Bool! = false
}

I would think that if I sent:

{ }

or:

{ isActive: false }

these would do the same thing? If you're only going to use the value if it's been assigned, why provide a default? Wouldn't it be more clear to have the schema:

input UserPatch {
  isActive: Bool
}

with the C# class:

public class UserPatch
{
  public Optional<bool> IsActive { get; set; }
}

Maybe I'm missing a use-case here, but if the backend only takes an action if a value is provided.....why do you need a default value in the first place?

I was typing out some more thoughts on this (other non-patching usages of Optional<T>), and I think the same principle applies...

But before I went into all that I first I wanted to see if anyone has any feedback on whether I've missed something important here? @michaelstaib @PascalSenn @mvestergaard ?

[mvestergaard]

@tunurgitr Well honestly, it's a choice between two evils.

Either it's isActive: Bool! = false where as you stated the system could behave differently than what a user may expect.

The alternative is isActive: Bool where i potentially allow the user to pass in null explicitly. You can kinda get around issues with this by calling .GetValueOrDefault() on bool?, but for strings (that can be null) that could lead to bugs.

After discussing with a team mate I think we feel better about the approach with default values.
It's not a public API, so the issues you describe, we can document our way out of.

[tunurgitr]

@mvestergaard okay, I think I'm getting it more....to make sure I understand: you're not actually using the default value, so the behavior of the system is the same regardless of what you're specifying as the default value of isActive?

This is just a marker to be able to have a spec that's clear that the value of isActive cannot be null?

And this is all because of essentially a limitation in graphql....there's no way of defining a field that must be non-null if you define it, but can also be omitted.

And just to make sure I'm following, the bugs you're talking about are the different treatment of null?

Let me setup a trivial example just so I'm sure I've got it:

type User {
  isActive: Bool!
  username: String!
  comment: String
}

The way I'd think to do it is:

input UserPatch {
  isActive: Bool
  username: String
  comment: String
}

Where the side-effect is that this operation:

{
  username: null
  comment: null
}

is allowed and means "do not change the username, but DO set the comment to null". Which is potentially confusing.

The way your team is handling would be something like:

input UserPatch {
  isActive: Bool! = false
  username: String! = ""
  comment: String
}

Which means the following operation isn't actually allowed:

{
  username: null
  comment: null
}

With the side-effect being that the default values aren't actually used by the backend for anything (and in-fact "" may be invalid as a username value). This also potentially confusing, but you're seeing that as the least-worst-solution because the nullability of all fields are clear when patching?

I'm not sure how this would work for any non-scalar fields, but leaving that aside...I'm still pondering this, but I want to make sure I'm understanding what you're doing here.

[mvestergaard]

Yep, you've got the picture.

I don't think non-scalar fields are a concern from our usage. If we're referencing an existing object, we do it by Id, and if we need to patch a nested object (if ever), that field would not use Optional<T>, it would just be a nullable NestedObjectPatch, which is then applied if not null.

[tunurgitr]

@mvestergaard crap. Well now I don't like either solution. Either way the schema is communicating information that isn't true.

Although I realize you guys aren't encountering this....one follow-up on nested-object-patching -- how would you apply NestedObjectPatch on nullable non-scalar fields? Is it handled differently if it's null than if it's not provided? How would you set the entire nested field to null?

Example scenario where a user's "profile" is optional, but a user's "address" is required and cannot be null:

input UserPatch {
  isActive: Bool! = false
  profile: UserProfilePatch # ???
  address: UserAddressPatch # ???
}

Isn't this the same issue we encountered earlier where it's unclear from the spec whether or not passing null is a no-op vs. set-to-null?

I'm not saying you shouldn't do it the way you've outlined internally, I'm just trying to think through your approach enough to have an opinion on whether/how Optional<T> should work in this situation. At the moment, it doesn't seem like you can apply your approach universally if you have any non-scalar fields -- you'd have to mix and match the "fake-defaults" with the "nulls have double-meaning" approach. So for me, I think that should mean that the official "suggestion" would be.....try not to do it this way.

And you _can_ use IResolverContext and you don't _have_ to use Optional<T>.

I'm still sorta on-the-fence on whether Optional should support this scenario. Having it "ignore default values" as a sort of "default operation" feels weird.

Specifically, thinking of @michaelstaib 's example above:

bar.HasValue = false;
bar.Value = 5;

This definitely seems....unexpected.

A couple ideas around options:

1. Make "undefined" more explicitly clear.

bar.IsDefined = false;
bar.IsUndefined = true;
bar.ValueOrDefault = 5;

But then it's more an Undefinable<T> than an Optional<T>.

1. Similar to @mvestergaard 's suggestion: add a new OptionalWithDefault<T> that explicitly handles this situation and offers more fields that give more insight into what's going on in these situations.

I'm not sure.

[mvestergaard]

It kinda appears GraphQL doesn't even behave as expected, at least not in the playground tool

input UserPatch {
  isActive: Bool! = false
}

If i don't set the isActive field, it complains about a missing required field.

Which is odd since the spec says this:

Input object fields may be required. Much like a field may have required arguments, an input object may have required fields. An input field is required if it has a non‐null type and does not have a default value. Otherwise, the input object field is optional.

[the-dag]

Which is odd since the spec says this:

Input object fields may be required. Much like a field may have required arguments, an input object may have required fields. An input field is required if it has a non‐null type and does not have a default value. Otherwise, the input object field is optional.

I admittedly haven't read this entire thread, but the _spec_ is a bit closer than HotChocolate to get this right. It makes absolutely no sense to define a field as optional unless it has a default value. And the reverse holds as well: It makes zero sense to define a default value unless the field is optional. A default value for a required field never applies, so it is just confusing.

Why on earth they conflated nullability in there, I have no idea. I certainly agree with the statement put forward early on in this thread that nullability has nothing to do with whether a field is optional or required. Obviously, a non-nullable field may not have null as its default value, but only for the same reason a numerical field cannot have "foobar" as its default value (the default value must be a valid value for the field type).

Now, whether the type descriptors were Optional() or just DefaultValue() is perhaps only a matter of taste. But having two different descriptors, one for optionality, and another for default values, is nonsense. It only makes it harder to get it right and allows us to specify logically impossible things like "this field is required, and has default value 3", or "this field is not required, and has no default value" which presumably means it has a default value of NULL if it is nullable and lord knows what if it is a datetime.

Legacy aside, the way to fix it is to get rid of this conceptual duplicity and have the type system reflect the fact that optional values and default values are two sides of the same coin.

[the-dag]

A small aside: exposing whether a value was explicitly provided by the user or is the schema-defined default is a really bad idea.

The only thing this information can really be used for is to have the system behave differently depending on whether the value was explicitly provided or not, which is just crazy. In order to be able to understand how to query the graph, it must be understandable from the schema information. And that tells us what the default value is, from which we infer that we get the behavior we'd get if we supplied that value.

Why build support to make it easier to make bad APIs?

[mvestergaard]

@the-dag We've quite successfully built an API that uses this functionality to do patches on updates. So however bad of an idea you feel that is, it works great. And your suggestions would completely break that, so no thanks from my end.

[michaelstaib]

@the-dag, optional is not intended to reflect if something is a default value. It is intended to reflect if the user by intent set a value. So if you set something explicitly even if the value that you just set is the default value then optional will be set.

It is so long and we just keep adding things here :)

If you wish to do a proper proposal please open a new issue where we can discuss it or if you want to pre-discuss it with the community join our slack channel.

[michaelstaib]

BTW, you could also do all of this without optional, it is just a small utility to help.