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

Relay Guid Identifier Unable to convert type from Object to Guid #7110

Open
PHILLIPS71 opened this issue May 20, 2024 · 20 comments · May be fixed by #7678
Open

Relay Guid Identifier Unable to convert type from Object to Guid #7110

PHILLIPS71 opened this issue May 20, 2024 · 20 comments · May be fixed by #7678

Comments

@PHILLIPS71
Copy link
Contributor

PHILLIPS71 commented May 20, 2024

Product

Hot Chocolate

Version

14.0.0-p.100

Link to minimal reproduction

https://github.com/PHILLIPS71/HC-7110

Steps to reproduce

I've previously used Guid identifiers in relay without any issues, though after upgrading to version 14 it doesn't seem to work anymore.

What is expected?

The relay identifiers can be converted into a GUID

What is actually happening?

{
  library(where: { id: { eq: "TGlicmFyeTq+UuauxXXqSp5/h9rbTMR0" } }) {
    id
    name
    slug
  }
}
{
  "errors": [
    {
      "message": "Unexpected Execution Error",
      "locations": [
        {
          "line": 2,
          "column": 3
        }
      ],
      "path": [
        "library"
      ],
      "extensions": {
        "message": "Unable to convert type from `Object` to `Guid`",
        "stackTrace": "   at HotChocolate.Utilities.DefaultTypeConverter.Convert(Type from, Type to, Object source)\r\n   at HotChocolate.Data.Filters.Expressions.QueryableComparableOperationHandler.ParseValue(IValueNode node, Object parsedValue, IType type, QueryableFilterContext context)\r\n   at HotChocolate.Data.Filters.Expressions.QueryableComparableEqualsHandler.HandleOperation(QueryableFilterContext context, IFilterOperationField field, IValueNode value, Object parsedValue)\r\n   at HotChocolate.Data.Filters.Expressions.QueryableOperationHandlerBase.TryHandleOperation(QueryableFilterContext context, IFilterOperationField field, ObjectFieldNode node, Expression& result)\r\n   at HotChocolate.Data.Filters.FilterOperationHandler`2.TryHandleEnter(TContext context, IFilterField field, ObjectFieldNode node, ISyntaxVisitorAction& action)\r\n   at HotChocolate.Data.Filters.FilterVisitor`2.OnFieldEnter(TContext context, IFilterField field, ObjectFieldNode node)\r\n   at HotChocolate.Data.Filters.FilterVisitorBase`2.Enter(ObjectFieldNode node, TContext context)\r\n   at HotChocolate.Language.Visitors.SyntaxWalker`1.Enter(ISyntaxNode node, TContext context)\r\n   at HotChocolate.Language.Visitors.SyntaxVisitor`1.Visit[TNode,TParent](TNode node, TParent parent, TContext context)\r\n   at HotChocolate.Language.Visitors.SyntaxVisitor`1.VisitChildren(ObjectValueNode node, TContext context)\r\n   at HotChocolate.Language.Visitors.SyntaxVisitor`1.VisitChildren(ISyntaxNode node, TContext context)\r\n   at HotChocolate.Language.Visitors.SyntaxVisitor`1.Visit[TNode,TParent](TNode node, TParent parent, TContext context)\r\n   at HotChocolate.Language.Visitors.SyntaxVisitor`1.VisitChildren(ObjectFieldNode node, TContext context)\r\n   at HotChocolate.Language.Visitors.SyntaxVisitor`1.VisitChildren(ISyntaxNode node, TContext context)\r\n   at HotChocolate.Language.Visitors.SyntaxVisitor`1.Visit[TNode,TParent](TNode node, TParent parent, TContext context)\r\n   at HotChocolate.Language.Visitors.SyntaxVisitor`1.VisitChildren(ObjectValueNode node, TContext context)\r\n   at HotChocolate.Language.Visitors.SyntaxVisitor`1.VisitChildren(ISyntaxNode node, TContext context)\r\n   at HotChocolate.Language.Visitors.SyntaxVisitor`1.Visit[TNode,TParent](TNode node, TParent parent, TContext context)\r\n   at HotChocolate.Language.Visitors.SyntaxVisitor`1.Visit(ISyntaxNode node, TContext context)\r\n   at HotChocolate.Data.Filters.Expressions.QueryableFilterProvider.<ConfigureField>g__VisitFilterArgumentExecutor|11_0(IValueNode valueNode, IFilterInputType filterInput, Boolean inMemory)\r\n   at HotChocolate.Data.Filters.Expressions.QueryableFilterProvider.<>c__DisplayClass15_0`1.<CreateApplicator>b__0(IResolverContext context, Object input)\r\n   at HotChocolate.Data.Filters.Expressions.QueryableFilterProvider.QueryableQueryBuilder.Apply(IMiddlewareContext context)\r\n   at HotChocolate.Types.UnwrapFieldMiddlewareHelper.<>c__DisplayClass0_1.<<CreateDataMiddleware>b__1>d.MoveNext()\r\n--- End of stack trace from previous location ---\r\n   at HotChocolate.Types.UnwrapFieldMiddlewareHelper.<>c__DisplayClass0_1.<<CreateDataMiddleware>b__1>d.MoveNext()\r\n--- End of stack trace from previous location ---\r\n   at HotChocolate.Data.Projections.FirstOrDefaultMiddleware`1.InvokeAsync(IMiddlewareContext context)\r\n   at HotChocolate.Utilities.MiddlewareCompiler`1.ExpressionHelper.AwaitTaskHelper(Task task)\r\n   at HotChocolate.Execution.Processing.Tasks.ResolverTask.ExecuteResolverPipelineAsync(CancellationToken cancellationToken)\r\n   at HotChocolate.Execution.Processing.Tasks.ResolverTask.TryExecuteAsync(CancellationToken cancellationToken)"
      }
    }
  ],
  "data": {
    "library": null
  }
}

Relevant log output

No response

Additional context

If you decode TGlicmFyeTq+UuauxXXqSp5/h9rbTMR0 it looks invalid (Library:RuJ�Lt)

@michaelstaib michaelstaib added the Next Next up on the backlog. label May 28, 2024
@michaelstaib michaelstaib added this to the HC-14.0.0 milestone May 28, 2024
@michaelstaib
Copy link
Member

Thanks for the repro ... we will have a look at this.

@Hanskrogh
Copy link
Contributor

Is there any status on this?

@michaelstaib
Copy link
Member

this is still an issue with 130

@michaelstaib michaelstaib removed their assignment Jul 16, 2024
@glen-84 glen-84 removed their assignment Sep 2, 2024
@A360JMaxxgamer
Copy link
Collaborator

I ran into the same issue. I already start to dig into the problem and created a unittest which reproduces the problem. It seems that the ids are deserialized two times. I hope to fix this problem soon and will do a pr then.

@A360JMaxxgamer
Copy link
Collaborator

I created a branch which reproduces the problem https://github.com/ChilliCream/graphql-platform/tree/mha/7110-id-filter-bug.

The problem seems to be that Member type is null in RelayIdFilterFieldExtensions. That's why a wrong Id serializer is created.

So far I did not find the right spot where the Member should be set because my knowledge of the code is not good enough. I will try to find the solution but maybe somehow knows it right away.

   public static IFilterOperationFieldDescriptor ID(
        this IFilterOperationFieldDescriptor descriptor)
    {
        if (descriptor is null)
        {
            throw new ArgumentNullException(nameof(descriptor));
        }

        descriptor
            .Extend()
            .OnBeforeCompletion((c, d) =>
            {
                var returnType = d.Member is null ? typeof(string) : d.Member.GetReturnType(); // <-- d.Member is null
                var returnTypeInfo = c.DescriptorContext.TypeInspector.CreateTypeInfo(returnType);
                d.Formatters.Push(CreateSerializer(c, returnTypeInfo.NamedType));
            });

        return descriptor;
    }

@A360JMaxxgamer
Copy link
Collaborator

@michaelstaib I am sorry. I don't find the correct spot. Maybe the unittest and the text above helps you to find the bug. Give me a ping if there is anything I can do.

@michaelstaib michaelstaib modified the milestones: HC-14.0.0, HC-15.0.0 Sep 24, 2024
@michaelstaib michaelstaib removed the Next Next up on the backlog. label Sep 24, 2024
@PHILLIPS71
Copy link
Contributor Author

PHILLIPS71 commented Oct 15, 2024

After further investigation, I was able to resolve the issue, though I don't have enough context to be sure it won't cause issues elsewhere.

It seems the problem lies in the OptimizedNodeIdSerializer class, specifically in the Parse function in this section:

if (serializer.ValueSerializer.IsSupported(runtimeType))
{
    valueSerializer = serializer.ValueSerializer;
}
else if (valueSerializer is null)
{
    valueSerializer = TryResolveSerializer(runtimeType);
}

if (valueSerializer is null)
{
    throw SerializerMissing(typeName, rentedBuffer);
}

var value = ParseValue(valueSerializer, serializer.TypeName, span.Slice(delimiterIndex + delimiterOffset));
Clear(rentedBuffer);
return value;

In this case, runtimeType is a string, and we want to use the GuidNodeIdValueSerializer to parse the string. However, this condition if (serializer.ValueSerializer.IsSupported(runtimeType)) fails because GuidNodeIdValueSerializer doesn't support strings. As a result, it falls into TryResolveSerializer which returns a StringNodeIdValueSerializer.

While this works for types like integers, it doesn't for GUIDs when using the new compressed format, which is enabled by default. When the compressed string is later processed to convert it to a GUID, it fails giving us the Unable to convert type from 'Object' to 'Guid' error as we cannot convert RuJ�Lt to a GUID.

To fix this, I think we need to modify the IsSupported method in GuidNodeIdValueSerializer to accept strings. This would allow us to decode the compressed string into a GUID string and then eventually convert that into the GUID as expected.

@michaelstaib If this approach seems reasonable and you're not concerned about it causing any issues, I'm happy to submit a PR.

@michaelstaib
Copy link
Member

This would not fix the issue just just put a workaround in because another component passes on the wrong type information. The issue is with filtering, and we have to fix it there.

@michaelstaib
Copy link
Member

In filtering probably the runtime type is not set, as far as I can tell from this issue.

@michaelstaib
Copy link
Member

@A360JMaxxgamer was on the right track.

@PHILLIPS71
Copy link
Contributor Author

PHILLIPS71 commented Oct 15, 2024

@michaelstaib I see. In that case, it looks like the FilterOperationFieldDescriptor doesn't have a constructor that initialize the Member property, at least none that I can find. I've attached the relevant code where it's being created, but I'm not entirely sure how everything connects or where you would retrieve the MemberInfo to pass in or set it, if it's available.

public IFilterOperationFieldDescriptor Operation(int operationId)
{
    var fieldDescriptor =
        Operations.FirstOrDefault(t => t.Definition.Id == operationId);

    if (fieldDescriptor is null)
    {
        fieldDescriptor = FilterOperationFieldDescriptor.New(
            Context,
            operationId,
            Definition.Scope);
        Operations.Add(fieldDescriptor);
    }

    return fieldDescriptor;
}

@A360JMaxxgamer
Copy link
Collaborator

A360JMaxxgamer commented Nov 4, 2024

I created a pr. It fixes the issue but it feels a bit hacky. I did not find a better solution. But I now understand the problem. before we tried to set the member data which was used to deserialize the value. Actually we could never use that or set that. The member type on the filterfieldoperation can be different for various inputs. The field type for e.g. "nin" on the schema is always IdFilterOperationNin therefore we can not determine the runtime type for a specific id when we complete that type.

I tried to move the logic to the input parser. The input parser has a target type. So when the parser sees a formatter for ids it tries to pass the targetType to it.

@michaelstaib michaelstaib modified the milestones: HC-15.0.0, HC-14.2.0 Nov 4, 2024
@michaelstaib
Copy link
Member

@A360JMaxxgamer I will have a look through the PR and then we see how we can make it work.

@jbenettius
Copy link

Checking in on this issue. I just came across this when upgrading to 14.1.0. My scenario is slightly different but also related to deserializing Guids from a node id.
https://github.com/jbenettius/Hotchocolate_Issues/tree/main/NodeIdIssue

@A360JMaxxgamer
Copy link
Collaborator

@jbenettius I had the same error message. I was able to fix it by registering the serializer with usage of url safe base64 encoding

 .AddDefaultNodeIdSerializer(useUrlSafeBase64: true)
 .AddGlobalObjectIdentification()

@jbenettius
Copy link

@A360JMaxxgamer thank you for the recommendation, but unfortunately that did not work for me.

@adraut
Copy link

adraut commented Nov 14, 2024

The .AddDefaultNodeIdSerializer(useUrlSafeBase64: true) did not work for me either on version 14.0. What did work was turning off the new id format.
.AddDefaultNodeIdSerializer(outputNewIdFormat: false)

@A360JMaxxgamer @jbenettius

@jbenettius
Copy link

@adraut thank you for the suggestion, but that also did not work for me.

I was able to successful deserialize the Id by implementing my own INodeIdValueSerializer, for Guid, changing the parsing to the N format. This directly relates to @A360JMaxxgamer's findings of it using the wrong serializer for Guids.

@jonmaylandvitrolife
Copy link

@michaelstaib
Are there any updates on this?
We're really looking forward to try out HC14, but this issue holds us back.

@michaelstaib michaelstaib modified the milestones: HC-14.2.0, HC-14.4.0 Dec 3, 2024
@Badabum
Copy link

Badabum commented Dec 3, 2024

workaround for those who want to specify custom Guid format (tested on v 15.0.0-pre3)

// add custom node id value serializer
internal sealed class GuidCustomNodeIdValueSerializer : INodeIdValueSerializer
{
    public bool IsSupported(Type type) => type == typeof(Guid) || type == typeof(Guid?);

    public NodeIdFormatterResult Format(Span<byte> buffer, object value, out int written)
    {
        if (value is Guid g)
        {
            return Utf8Formatter.TryFormat(g, buffer, out written, format: 'D') // desired format
                ? NodeIdFormatterResult.Success
                : NodeIdFormatterResult.BufferTooSmall;
        }

        written = 0;
        return NodeIdFormatterResult.InvalidValue;
    }

    public bool TryParse(ReadOnlySpan<byte> buffer, [NotNullWhen(true)] out object? value)
    {
        if (Utf8Parser.TryParse(buffer, out Guid parsedValue, out _, 'D')) // specify format here (or pass as constructor param)
        {
            value = parsedValue;
            return true;
        }

        value = null;
        return false;
    }
}

// where DI for hotchocolate is configured
services
.AddNodeIdValueSerializer<GuidCustomNodeIdValueSerializer>()
.AddGlobalObjectIdentification();
//...other registrations

// remove default `GuidNodeIdValueSerializer` which is internal
 services
            .Where(t => 
                t.ServiceType == typeof(INodeIdValueSerializer) 
                && t.ImplementationInstance != null 
                && t.ImplementationInstance.GetType().Name.Contains("GuidNodeIdValueSerializer"))
            .ToList()
            .ForEach(s => services.Remove(s));

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

Successfully merging a pull request may close this issue.

9 participants