ChilliCream / graphql-platform

Welcome to the home of the Hot Chocolate GraphQL server for .NET, the Strawberry Shake GraphQL client for .NET and Banana Cake Pop the awesome Monaco based GraphQL IDE.
https://chillicream.com
MIT License
5.27k stars 748 forks source link

JsonElement should map to `JsonType` not `JsonType!` #6671

Closed onionhammer closed 4 months ago

onionhammer commented 1 year ago

Is there an existing issue for this?

Product

Hot Chocolate

Describe the bug

JsonElement and other types that utilize it allow for null values on the .NET side;

for example:

using System.Text.Json;

var elem = JsonSerializer.Deserialize<JsonElement>("null");
elem // elem is NOT null, its `ValueKind` is `Null`

However, in HotChocolate by default, JsonElement becomes a non-null JsonType scalar.

using System.Text.Json;
using System.Text.Json.Serialization;

namespace Test;

[MutationType]
public class TestMutation
{
    public MyInputType TakeFlexibleInput(MyInputType test)
    {
        return test;
    }
}

public class MyInputType
{
    [JsonExtensionData]
    public Dictionary<string, JsonElement>? ExtensionData { get; set; }
}

yields a schema like

type Mutation {
  takeFlexibleInput(input: TakeFlexibleInputInput!): TakeFlexibleInputPayload!
}

type MyInputType {
  extensionData: [KeyValuePairOfStringAndJsonElement!]
}

input KeyValuePairOfStringAndJsonElementInput {
  key: String!
  value: JSON!
}

Which does not allow for null values within the extension data, meaning any valid null JSON value will turn into a hot chocolate validation error.

Steps to reproduce

  1. Utilize above code
  2. Post a mutation with a null in the list of values, i.e. { "myField": null }

Relevant log output

No response

Additional Context?

Other things I tried:

  1. Decorating ExtensionData with JsonElement? fails because .NET's JsonSerializer doesnt accept that, it gives: "".ExtensionData' is invalid. It must implement 'IDictionary<string, JsonElement>' or 'IDictionary<string, object>', or be 'JsonObject'.""

  2. .BindRuntimeType<IDictionary<string, JsonElement>, JsonType>() gives an array of non-null JSON values, which will not serialize well to a dictionary, also it is still non-null only

Workaround:

Perhaps create a custom json 'dictionary' scalar?

Version

13.5.1

glen-84 commented 4 months ago

I'm not able to reproduce this issue if I change Dictionary<string, JsonElement>? to Dictionary<string, JsonElement?>? and post the following mutation:

mutation {
  takeFlexibleInput(test: { extensionData: [{ key: "x", value: null }] }) {
    extensionData {
      key
      value
    }
  }
}

Am I missing something?

onionhammer commented 4 months ago

@glen-84 JsonElement can represent null JSON as a valid value, so just because JsonElement is NOT null, doesnt mean that the JSON is NOT null.

In other words, Dictionary<string, JsonElement> (non-null JsonElement) should have no trouble binding to { key: "x", value: null }; null would just be the equivalent of a JsonElement with ValueKind of Null

michaelstaib commented 4 months ago

There is no map semantic in GraphQL ... the dictionary is just a list to Hot Chocolate. Also I would not expose dictionaries like that... rather expose the map itself as JsonType and get a cleaner schema. I am closing this issue.

onionhammer commented 4 months ago

@michaelstaib it's not me that's choosing JsonExtensionData as a dictionary. Not sure if I understand your explanation... also the whole dictionary/list/map thing is completely besides the point; the point is that a non-null JsonElement can result a null value on the client. If you declare it not null by giving it a bang(!) you're essentially lying to the typescript compiler