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

Custom scalar for JObject is not parsed correctly #1051

Closed
stalniy opened this issue Aug 30, 2019 · 21 comments
Closed

Custom scalar for JObject is not parsed correctly #1051

stalniy opened this issue Aug 30, 2019 · 21 comments
Assignees
Milestone

Comments

@stalniy
Copy link

stalniy commented Aug 30, 2019

Describe the bug

I want to write a custom scalar for the JSON object of any shape (I know I know about strong typing and GraphQL but...) and when I do this hotchocolate incorrectly parses the input object.

All works fine if you use this scalar type as mutation argument but doesn't work when it's a field of InputObjectType.

To Reproduce
Steps to reproduce the behavior:

  1. Create custom scalar for JSON object
Scalar Implementation
public class JsonScalarType : HotChocolate.Types.ScalarType
    {
        public JsonScalarType() : base("JsonScalar")
        {
        }

        protected JsonScalarType(string name) : base(name)
        {
        }

        public override Type ClrType => typeof(object);

        public override bool IsInstanceOfType(IValueNode literal)
        {
            if (literal == null) throw new ArgumentNullException(nameof(literal));
            return literal is NullValueNode || literal is ObjectValueNode;
        }

        public override object ParseLiteral(IValueNode literal)
        {
            if (literal is NullValueNode) // NullValueNode is present after TryDeserialize call
                return null;
            throw new NotImplementedException();
        }

        public override IValueNode ParseValue(object value)
        {
            throw new NotImplementedException();
        }

        public override object Serialize(object value)
        {
            if (!(value is JObject jo))
                throw new ArgumentException(nameof(value));
            return jo;
        }

        public override bool TryDeserialize(object serialized, out object value)
        {
            if (serialized is Dictionary<string, object>)
            {
                value = JObject.FromObject(serialized);
                return true;
            }

            if (serialized is JObject)
            {
                value = serialized;
                return true;
            }

            value = null;
            return false;
        }
    }
2. Register it as a field inside input object type and add to mutation
Input Object Type
public class ComplexInput
{
    [GraphQLType(typeof(JsonScalarType))]
    public JObject Inner { get; set; }
}
3. Register `ComplexInput` in mutation
Mutation
public class Mutation {

    public string ProcessJsonScalar([GraphQLType(typeof(JsonScalarType))] JObject input)
    {
        return $"ok: {input}";
    }

    public string ProcessComplexJsonScalar(ComplexInput input)
    {
        return $"ok: {input.Inner}";
    }
}
  1. send mutation
mutation send($input: ComplexInput) {
  processComplexJsonScalar(input: $input) 
}

Variables:

{
  "input": {
    "inner": {
        "someCustomProp": 1,
        "someCustomProp2": 2
    }
  }
}

Expected behavior
Custom scalar is correctly parsed into JObject

Actual behavior:
null is returned

Additional context
.NET Core 2.2
hotchocolate version 11.0.0-preview.2

@stalniy stalniy changed the title Custom scalar for JObject Custom scalar for JObject is not parsed correctly Aug 30, 2019
@michaelstaib michaelstaib self-assigned this Aug 30, 2019
@michaelstaib michaelstaib added the 🔍 investigate Indicates that an issue or pull request needs more information. label Aug 30, 2019
@michaelstaib
Copy link
Member

So, the ComplexInput is a scalar type? If it is a scalar then the spec defines serialization types, it can. serialize to string, bool, number, decimal number. So you cannot hav a json scalar that can be defined with json.

What is possible is to define a json string ....

{
  "input": "{ ... }"
}

Scalar types represent primitive leaf values in a GraphQL type system. GraphQL responses take the form of a hierarchical tree; the leaves on these trees are GraphQL scalars.

GraphQL provides a number of built‐in scalars, but type systems can add additional scalars with semantic meaning. For example, a GraphQL system could define a scalar called Time which, while serialized as a string, promises to conform to ISO‐8601. When querying a field of type Time, you can then rely on the ability to parse the result with an ISO‐8601 parser and use a client‐specific primitive for time. Another example of a potentially useful custom scalar is Url, which serializes as a string, but is guaranteed by the server to be a valid URL.

So while you can have complex scalars they cannot build structured data. This is a bigger problem for output types where you could not properly write selections on those types. There will be some more clarification on this with the future spec that all custom scalars have to serialize to the built-in ones.

But what should work is having a JSON string.

@michaelstaib
Copy link
Member

michaelstaib commented Aug 30, 2019

Ah ... just forgot it... the GraphQL spec can be found here:
https://graphql.github.io/graphql-spec/

@stalniy
Copy link
Author

stalniy commented Aug 30, 2019

Apollo server supports JSON as scalar type. Sometimes it’s a good trade off when you don’t want to bother with types.

For example, GeoJson spec says that Point can be described as:

{
  “type”: “Point”,
  “coordinates”: [lat, lng]
}

So, in this case client doesn’t bother about selecting fields for the Point as the shape of Point is strictly defined and there is no sense to select one field but not another.

There are also other cases like this

@stalniy
Copy link
Author

stalniy commented Aug 30, 2019

Json string looks like a hack. Especially from client side (e.g., js in browser)

update: also it will be hard to specify it fir example in graphql playground

@stalniy
Copy link
Author

stalniy commented Aug 30, 2019

ComplexInput is an input object type with inner property which type is a custom Json scalar

@michaelstaib
Copy link
Member

Let me look into the Apollo implementation. I will also discuss that with some other people to get their thoughts on this one. If there is a way to implement it without violating the spec we will do it.

@michaelstaib
Copy link
Member

Also, as a side note. One thing why it won't work is that you use JObject as a backing type whereas we are not using Json.net for parsing variables and requests. We getting rid of Json.net and have only I think two places where we still use it. The literal would need to be a ObjectValueNode and the internal ClrType would need to be a IReadOnlyDictionary<string, object>. I will try that out later and post the code if it works out.

But in any case, this will make your GraphQL server non-spec compliant and might have an impact on tooling.

I will give you feedback on that too.

@michaelstaib
Copy link
Member

So, we will build in a proper any type with 10.1

@michaelstaib michaelstaib added enhancement and removed 🔍 investigate Indicates that an issue or pull request needs more information. labels Aug 31, 2019
@michaelstaib michaelstaib added this to the 10.1.0 milestone Aug 31, 2019
@michaelstaib
Copy link
Member

I have talked to Ivan about this (the graphql-js maintainer) and it is not forbidding to do that. I had interpreted some sections of the spec differently but I was wrong here.

We will create the new AnyType so that it really can be anything (object, int, float, string, bool).

I will describe the AnyType in more detail in a separate issue that I will link to this issue.

We will put this in 10.1.0 so expect it in the next two weeks.

@stalniy
Copy link
Author

stalniy commented Aug 31, 2019

Awesome! Thanks for the amazing support! Keep on doing this great project!

@michaelstaib
Copy link
Member

#1055

@michaelstaib
Copy link
Member

I Have now implemented the type and will release a first preview ... 10.2.0-preview.1.

I still need to write mire tests for this one.

If you want to have a look how this type works head over to the unit tests here:
https://github.com/ChilliCream/hotchocolate/blob/any_type/src/Core/Types.Tests/Types/Scalars/AnyTypeTests.cs

@michaelstaib
Copy link
Member

I still have to smooth out the implementation so feedback is appreciated.

Important we are not using json.net here. This is done by our own parser. So there are now JObjects... Since GraphQL is transport agnostic there should be no json type. So the any type can be anything.

You can also get as anything. The type is in it serialized state and IReadOnlyDictionary<string, object> or IReadOnlyList or a scalar.

If we cannot serialize the type you can register type converters like for any type and then intercept conversion.

@michaelstaib
Copy link
Member

@michaelstaib
Copy link
Member

#1078

@michaelstaib
Copy link
Member

We have merged it now and will close this issue... the new version is now in preview.2.

@AALGate
Copy link

AALGate commented Oct 21, 2019

@michaelstaib

We have merged it now and will close this issue... the new version is now in preview.2.

Thanks a lot for your help

but how i can define this ANY type in code first ?

there's only an example of "SCHEMA" first : https://hotchocolate.io/docs/custom-scalar-types#any-type

I have read that

The `Any scalar is a special type that can be compared to object in c#

but actually i have an Input object called "GetListQueryInput" and it has a property called "query" of type object

but i am getting an error :
SchemaException: InputObject GetListQueryInput has no fields declared. - Type: GetListQueryInput

@stalniy
Copy link
Author

stalniy commented Oct 21, 2019

I think you can find examples in tests for now

@AALGate
Copy link

AALGate commented Oct 21, 2019

I think you can find examples in tests for now

@stalniy thanks a lot for your reply

however the link for test is invalid, it gives me 404 error

https://github.com/ChilliCream/hotchocolate/blob/any_type/src/Core/Types.Tests/Types/Scalars/AnyTypeTests.cs

@kamsar
Copy link
Contributor

kamsar commented Apr 14, 2020

For posterity, the tests have moved here and if you actually want to return a serialized JToken instance that is not a C# type you need to first convert it to dictionaries and arrays for AnyType to properly serialize it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants