apollographql / federation-hotchocolate

HotChocolate support for Apollo Federation
https://www.apollographql.com/docs/federation/
MIT License
16 stars 8 forks source link

bug: Unable to input UUID as argument for an ID field #49

Open anshulshah96 opened 1 year ago

anshulshah96 commented 1 year ago

In ArgumentParser.TryGetValue<T>, the casting of all string value node type is done in the following manner:

value = (T)scalarType.ParseLiteral(valueNode)!;

Refer here.

This causes the following error from the subgraph when the input type for an ID field is UUID:

{
  "errors": [
    {
      "message": "Unexpected Execution Error",
      "locations": [
        {
          "line": 2,
          "column": 3
        }
      ],
      "path": [
        "_entities"
      ],
      "extensions": {
        "message": "Unable to cast object of type 'System.String' to type 'System.Guid'.",
        "stackTrace": "   at ApolloGraphQL.HotChocolate.Federation.Helpers.ArgumentParser.TryGetValue[T](IValueNode valueNode, IType type, String[] path, Int32 i, T& value)\n   at ApolloGraphQL.HotChocolate.Federation.Helpers.ArgumentParser.TryGetValue[T](IValueNode valueNode, IType type, String[] path, Int32 i, T& value)\n   at ApolloGraphQL.HotChocolate.Federation.Helpers.ArgumentParser.GetValue[T](IValueNode valueNode, IType type, String[] path)\n   at lambda_method15(Closure, IResolverContext)\n   at ApolloGraphQL.HotChocolate.Federation.Helpers.EntitiesResolver.ResolveAsync(ISchema schema, IReadOnlyList`1 representations, IResolverContext context)\n   at HotChocolate.Types.ResolveObjectFieldDescriptorExtensions.<>c__DisplayClass3_0`1.<<Resolve>b__0>d.MoveNext()\n--- End of stack trace from previous location ---\n   at HotChocolate.Types.Helpers.FieldMiddlewareCompiler.<>c__DisplayClass9_0.<<CreateResolverMiddleware>b__0>d.MoveNext()\n--- End of stack trace from previous location ---\n   at HotChocolate.Execution.Processing.Tasks.ResolverTask.ExecuteResolverPipelineAsync(CancellationToken cancellationToken)\n   at HotChocolate.Execution.Processing.Tasks.ResolverTask.TryExecuteAsync(CancellationToken cancellationToken)"
      }
    }
  ]
}
dariuszkuc commented 1 year ago

Hello 👋 UUID is a custom scalar so the logic would have to account for mapping custom scalar (Any) to a custom scalar (UUID). I am unsure whether we will be able to support it.

As a workaround you will need to manually parse it

[ReferenceResolver]
public static Foo GetByFooBar(
    [LocalState] ObjectValueNode data
    Data repository)
{
    // TODO implement logic here by manually reading values from local state data
}
LavaToaster commented 1 year ago

@dariuszkuc Is this intentional, because this is handled transparently in the previous federation implementation?

dariuszkuc commented 1 year ago

Did it actually work? The code is pretty much the same this vs that.

Looks like the only difference is around handling non-null types - I only applied the check for object types as it seemed working fine for other types (looks like their fix made it to 13.6.0).

dariuszkuc commented 1 year ago

The underlying issue is with the IdType. Fields using Guid by itself work fine.

When we invoke the TryGetValue it attempts to fetch the IdType which serializes as a string and it doesn't know that the underlying type is actually Guid (i.e. we don't know that IdType should be wrapping UuidType). So the logic there would somehow need to know to parse the string value (ID) and then try to parse it again to Guid. You would encounter the same problem with any other custom scalar.

Since ID scalar is serialized to String we can also do following workaround

[ReferenceResolver]
public static Foo GetByGuid(
    string id
    Data repository)
{
    var guid = Guid.Parse(id);
    // TODO implement logic here by manually reading values from local state data
}

I am unsure whether HotChocolate allows you to provide custom biding for existing scalar types - IF it does support it then maybe you could provide your own ParseLiteral logic to also support GuidType.

dariuszkuc commented 1 year ago

related: #19 and #20

LavaToaster commented 1 year ago

Ah ok that makes sense, we weren't using ID types before so that would explain why we didn't see it.

anshulshah96 commented 1 year ago

I tried the above suggestion of using string in the ReferenceResolvers. But I still run into the following error:

{
  "data": {},
  "errors": [
    {
      "message": "invalid type for variable: '<variable1>'",
      "extensions": {
        "name": "<variable1>",
        "code": "VALIDATION_INVALID_TYPE_VARIABLE"
      }
    },
    {
      "message": "invalid type for variable: '<variable2>'",
      "extensions": {
        "name": "<variable2>",
        "code": "VALIDATION_INVALID_TYPE_VARIABLE"
      }
    }
  ]
}
dariuszkuc commented 1 year ago

Please provide a link to a repository that reproduces the issue. Otherwise it is very hard to determine what is the underlying issue.