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

Configuration of AnyType #1303

Closed
stalniy opened this issue Dec 17, 2019 · 5 comments
Closed

Configuration of AnyType #1303

stalniy opened this issue Dec 17, 2019 · 5 comments
Assignees
Milestone

Comments

@stalniy
Copy link

stalniy commented Dec 17, 2019

Is your feature request related to a problem? Please describe.
Currently AnyType accepts any JSON value, for some cases I need to restrict AnyType to accept only specific shape of the JSON. For example, GeoJSON specification says that a geo point is a JSON object with 2 properties type and coordinates

This is atomic thing, so I don't want to define it as a separate GraphQL type and instead I see it as a ScalarType (I can't display information on map having only coordinates or only type it just doesn't make sense for me).

Describe the solution you'd like
I see 2 ways to achieve this.

  1. Allow to extend AnyType and restrict it but then it breaks Liskov substitution principle and it's not good
  2. Allow to conifgure AnyType. In this case, it'd be good to have a factory function for AnyType:
class GeoJSONPoint {
   public string Type;
   public double[] Coordinates;
}

AnyType.Create<GeoJSONPoint>()

// or 
AnyType.create(json => isValidGeoJSONPoint(json))
@michaelstaib
Copy link
Member

There is one important aspect here. Any is not JSON. It is a GraphQL literal. So you do not get the json string. You have a GraphQL literal which is internally represented by an IValueNode or a ReadOnlySpan<byte>.

If we would allow Any to be configured than any would behave the configured way throughout the schema. It would not be possible to have different Any behaviors on different fields.

If you want to have this than this is a field validation not a type requirement. Each type has to have one defined behavior throughout the schema.

You can however with all types register them under a different name making them a different type.

I do not yet see why we would need a factory for that.

The new Uuid can have a different serialization behavior depending on the schema and we pass that as a constructor argument.

SchemaBuilder.New()
    .AddType(new UuidType(name: "MySpecialType", format: 'D')
    ....
    .Create()

While this all are technicalities I also have may doubts about the value of this.

The downside to your approach with the any type is that the any type is not anything anymore.

It now has a fixed shape but does not give the GraphQL compilers, generators and tools any hint about its structure.

From a usage standpoint this might not be ideal and takes away the advantages of GraphQL being predictable and strongly typed.

Moreover, if you want to go down the route of a scalar in this case, wouldn't it be better to have a very specific scalar implementation for that. You do not need all the handling of all the possible type cases of any and the scalar would have a fixed representation in .NET which would save a lot of type conversions.

Also for you to consider is that with GraphQL 2020 there will be a new @specifiedBy(url:) directive which allows to reference to a scalar specification documents. This would essentially allow for GEO scalars that have full validation and typing support.

@michaelstaib michaelstaib self-assigned this Dec 17, 2019
@michaelstaib michaelstaib added the 🔍 investigate Indicates that an issue or pull request needs more information. label Dec 17, 2019
@michaelstaib michaelstaib added this to the 11.0.0 milestone Dec 17, 2019
@michaelstaib
Copy link
Member

graphql/graphql-spec#649

@stalniy
Copy link
Author

stalniy commented Dec 18, 2019

The purpose of AnyType.Create is not to change the behavior of AnyType but instead to create a custom type which works the same way as AnyType but with some restriction.

So that, I don't need to duplicate the same logic in my custom scalar implementation but instead want to pass just a validation logic.

However I understand your position as well. So, it's up to you to decide whether to implement it or close.

You do amazing work man, I like this project! :)

@Confusingboat
Copy link

Confusingboat commented Apr 22, 2020

I've recently run into this myself, also working with GeoJSON (I'm using the GeoJSON.NET library).

The best way I've come up with to solve this was by implementing my own scalar type, which unfortunately resulted in a bunch of code duplication (including a couple types that are internal to HC). I would have preferred to have simply inherited and overridden AnyType or another base class
since I want essentially all of what it does with regards to object types.

What I ended up doing was creating a new generic abstract class on top of scalar that borrows all the relevant bits from AnyType and provides a simple interface for implementing whatever type you want on top of it:

    public abstract class ObjectScalarType<T> : ScalarType where T : class
    {
        private ObjectToDictionaryConverter _objectToDictConverter;
        private ITypeConversion _converter;

        protected ObjectScalarType(NameString name) : base(name)
        {
        }

        protected override void OnCompleteType(
            ICompletionContext context,
            IDictionary<string, object> contextData)
        {
            _converter = context.Services.GetTypeConversion();
            _objectToDictConverter = new ObjectToDictionaryConverter(_converter);
            base.OnCompleteType(context, contextData);
        }

        public override bool IsInstanceOfType(IValueNode literal)
        {
            if (literal == null)
            {
                throw new ArgumentNullException(nameof(literal));
            }

            return false;
        }

        public override object ParseLiteral(IValueNode literal)
        {
            if (literal is NullValueNode _)
                return null;
            throw new ScalarSerializationException($"{Name} : {literal.GetType().Name}");
        }

        public override IValueNode ParseValue(object value) =>
            value is null
            ? NullValueNode.Default
            : ParseValue(value, new HashSet<object>());

        private IValueNode ParseValue(object value, ISet<object> set)
        {
            if (value is null)
            {
                return NullValueNode.Default;
            }

            Type type = value.GetType();

            if (set.Add(value))
            {
                if (value is IReadOnlyDictionary<string, object> dict)
                {
                    var fields = new List<ObjectFieldNode>();
                    foreach (KeyValuePair<string, object> field in dict)
                    {
                        fields.Add(new ObjectFieldNode(
                            field.Key,
                            ParseValue(field.Value, set)));
                    }
                    return new ObjectValueNode(fields);
                }

                if (value is IReadOnlyList<object> list)
                {
                    var valueList = new List<IValueNode>();
                    foreach (object element in list)
                    {
                        valueList.Add(ParseValue(element, set));
                    }
                    return new ListValueNode(valueList);
                }

                return ParseValue(_objectToDictConverter.Convert(value), set);
            }

            // TODO : resources
            throw new ScalarSerializationException(
                "Cycle in object graph detected.");
        }

        public sealed override object Serialize(object value) =>
            value is null
            ? null
            : _objectToDictConverter.Convert(BuildObjectToSerialize(value as T));

        protected abstract object BuildObjectToSerialize(T value);

        public override bool TryDeserialize(object serialized, out object value)
        {
            value = serialized;
            return true;
        }

        public sealed override Type ClrType => typeof(T);
    }

So the implementation becomes really slim, like this:

    public sealed class PointType : ObjectScalarType<Point>
    {
        public PointType() : base("Point") { }

        protected override object BuildObjectToSerialize(Point value) =>
            value == null
                ? null
                : new
                {
                    Type = value.Type.ToString(),
                    Coordinates = new[]
                    {
                        value.Coordinates.Longitude,
                        value.Coordinates.Latitude,
                        value.Coordinates.Altitude
                    }.Where(c => c.HasValue).ToArray()
                };
    }

This solution is primarily concerned with output at this time, so I've essentially skipped over the deserialization bits for now.

Anyhow, hope this helps someone.

@michaelstaib
Copy link
Member

@Confusingboat just as an info, we have started work on integrating GeoJSON into Hot Chocolate ... on our slack channel is a channel called spatial in which the discussion are also we have an issues tracking the progress:
#1747

It would be great if you chime into the discussion.

@michaelstaib michaelstaib removed the 🔍 investigate Indicates that an issue or pull request needs more information. label Sep 16, 2020
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

3 participants