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<bool?> 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<bool?> 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.