Skip to content

Use correct type name when referencing type with ID<Type> #8504

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.

Sign up for GitHub

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

Merged
merged 13 commits into from
Nov 5, 2025
Merged

Use correct type name when referencing type with ID<Type> #8504

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
a0a2e3a
Honor GraphQLName`s and fluent definition type names for ID<T>
N-Olbert Aug 7, 2025
b6f3795
Reworked tests
N-Olbert Aug 7, 2025
2976ce4
Merge branch 'main' into 6256
michaelstaib Aug 28, 2025
6c755c8
Apply suggestions from code review
N-Olbert Sep 1, 2025
09b2c83
Adjustments from review
N-Olbert Sep 1, 2025
977e23d
Merge branch 'main' into 6256
N-Olbert Sep 1, 2025
e61fdd5
Reformat multiline string in `IdDescriptorTests` for consistent inden…
N-Olbert Sep 3, 2025
1561970
Second attempt on indentation
N-Olbert Sep 3, 2025
2307571
Merge branch 'main' into 6256
glen-84 Sep 19, 2025
cc0f9d9
Fixed CS8618 error
glen-84 Sep 19, 2025
eae3519
Merge branch 'main' into 6256
michaelstaib Sep 28, 2025
8cb39fd
Merge branch 'main' into 6256
michaelstaib Sep 30, 2025
0d109b8
Merge branch 'main' into 6256
michaelstaib Nov 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 3 additions & 26 deletions src/HotChocolate/Core/src/Types/Types/Relay/Attributes/IDAttribute.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,31 +168,8 @@ public class IDAttribute<T> : DescriptorAttribute
/// <inheritdoc cref="IDAttribute{T}"/>
public IDAttribute()
{
TypeName = typeof(T).Name;
}

/// <summary>
/// With the <see cref="IDAttribute.TypeName"/> property you can override the type name
/// of the ID. This is useful to rewrite a parameter of a mutation or query, to a specific
/// id.
/// </summary>
/// <example>
/// <para>
/// A field can be rewritten to an ID by adding <c>[ID]</c> to the resolver.
/// </para>
/// <code>
/// public class UserQuery
/// {
/// public User GetUserById([ID("User")] int id) => //....
/// }
/// </code>
/// <para>
/// The argument is rewritten to <c>ID</c> and expect an ID of type User.
/// Assuming `<c>User.id</c>` has the value 1. The following string is base64 encoded
/// </para>
/// </example>
public string? TypeName { get; }

/// <inheritdoc />
protected internal override void TryConfigure(
IDescriptorContext context,
Expand All @@ -202,13 +179,13 @@ protected internal override void TryConfigure(
switch (descriptor)
{
case IInputFieldDescriptor d:
d.ID(TypeName);
d.ID<T>();
break;
case IArgumentDescriptor d:
d.ID(TypeName);
d.ID<T>();
break;
case IObjectFieldDescriptor d:
d.ID(TypeName);
d.ID<T>();
break;
case IInterfaceFieldDescriptor d:
d.ID();
Copy link
Member

@michaelstaib michaelstaib Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not for the interface?

Copy link
Contributor Author

@N-Olbert N-Olbert Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michaelstaib Two reasons:

  • It was like this before (not a good reason 😅)
  • It doesn’t seem to make sense to me: If you don’t do crazy things with code-first-types, the interface name will never be the concrete type name, like for type Cat implements Node & IPet, the ID will be something like base64(concat("Cat", Id)) - and not base64(concat("IPet", Id)). Therefore, having something like public bool? IsHungry([ID<IPet>] int petId) is almost guaranteed to fail, as the ID will never start with "IPet".

With the current implementation public bool? IsHungry([ID<IPet>] int petId) should work fine as it is converted implicitly to public bool? IsHungry([ID] int petId). Note that I haven't tested this, but this is my current understanding of how it is supposed to work (will verify it during the weekend).

However... the implicit conversion from ID to ID may or may not be what the user expects, at least it is not obvious. Since HC has analyzers now: Wouldn’t it be better to simply throw a schema exception for interfaces here and instead have an analyzer that flags such cases and offers a code fix from ID to ID?

Your thoughts on this? Am I missing a case where this would/should work?

Copy link
Member

@michaelstaib michaelstaib Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct ... was just skimming through the code. In this case we only need the GraphQL type information but not the runtime type.

Copy link
Contributor Author

@N-Olbert N-Olbert Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Do you also agree on throwing an exception and / or adding an analyzer instead? The implicit rewrite seems problematic as the field now accepts any ID which is clearly not what the developer intended.

I can add this if we decide that it's desired (not as part of this PR, but as a follow-up).

Expand Down
15 changes: 6 additions & 9 deletions src/HotChocolate/Core/src/Types/Types/Relay/Extensions/RelayIdFieldExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,16 +74,15 @@ public static IInputFieldDescriptor ID(
return descriptor;
}

/// <inheritdoc cref="RelayIdFieldExtensions"/>
/// <param name="descriptor">the descriptor</param>
/// <inheritdoc cref="ID(IInputFieldDescriptor,string?)"/>
/// <typeparam name="T">
/// the type from which the <see cref="IDAttribute.TypeName">type name</see> is derived
/// </typeparam>
public static IInputFieldDescriptor ID<T>(this IInputFieldDescriptor descriptor)
{
ArgumentNullException.ThrowIfNull(descriptor);

RelayIdFieldHelpers.ApplyIdToField(descriptor, typeof(T).Name);
RelayIdFieldHelpers.ApplyIdToField<T>(descriptor);

return descriptor;
}
Expand All @@ -104,16 +103,15 @@ public static IArgumentDescriptor ID(
return descriptor;
}

/// <inheritdoc cref="RelayIdFieldExtensions"/>
/// <param name="descriptor">the descriptor</param>
/// <inheritdoc cref="ID(IInputFieldDescriptor,string?)"/>
/// <typeparam name="T">
/// the type from which the <see cref="IDAttribute.TypeName">type name</see> is derived
/// </typeparam>
public static IArgumentDescriptor ID<T>(this IArgumentDescriptor descriptor)
{
ArgumentNullException.ThrowIfNull(descriptor);

RelayIdFieldHelpers.ApplyIdToField(descriptor, typeof(T).Name);
RelayIdFieldHelpers.ApplyIdToField<T>(descriptor);

return descriptor;
}
Expand All @@ -134,16 +132,15 @@ public static IObjectFieldDescriptor ID(
return descriptor;
}

/// <inheritdoc cref="RelayIdFieldExtensions"/>
/// <param name="descriptor">the descriptor</param>
/// <inheritdoc cref="ID(IInputFieldDescriptor,string?)"/>
/// <typeparam name="T">
/// the type from which the <see cref="IDAttribute.TypeName">type name</see> is derived
/// </typeparam>
public static IObjectFieldDescriptor ID<T>(this IObjectFieldDescriptor descriptor)
{
ArgumentNullException.ThrowIfNull(descriptor);

RelayIdFieldHelpers.ApplyIdToField(descriptor, typeof(T).Name);
RelayIdFieldHelpers.ApplyIdToField<T>(descriptor);

return descriptor;
}
Expand Down
143 changes: 107 additions & 36 deletions src/HotChocolate/Core/src/Types/Types/Relay/Extensions/RelayIdFieldHelpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,21 +23,16 @@ internal static class RelayIdFieldHelpers
/// </remarks>
public static void ApplyIdToField(
IDescriptor<ArgumentConfiguration> descriptor,
string? typeName = null)
{
ArgumentNullException.ThrowIfNull(descriptor);

var extend = descriptor.Extend();

// rewrite type
extend.OnBeforeCreate(RewriteConfiguration);
string? typeName = null) =>
ApplyIdToFieldCore(descriptor, NodeIdNameDefinitionUnion.Create(typeName));

// add serializer if globalID support is enabled.
if (extend.Context.Features.Get<NodeSchemaFeature>()?.IsEnabled == true)
{
extend.OnBeforeCompletion((c, d) => AddSerializerToInputField(c, d, typeName));
}
}
/// <inheritdoc cref="ApplyIdToField(IDescriptor{ArgumentConfiguration},string?)"/>
/// <typeparam name="T">
/// the type from which the <see cref="IDAttribute.TypeName">type name</see> is derived
/// </typeparam>
public static void ApplyIdToField<T>(
IDescriptor<ArgumentConfiguration> descriptor) =>
ApplyIdToFieldCore(descriptor, NodeIdNameDefinitionUnion.Create<T>());

/// <summary>
/// Applies the <see cref="RelayIdFieldExtensions"><c>.ID()</c></see> to an argument
Expand All @@ -48,24 +43,16 @@ public static void ApplyIdToField(
/// </remarks>
public static void ApplyIdToField(
IDescriptor<OutputFieldConfiguration> descriptor,
string? typeName = null)
{
ArgumentNullException.ThrowIfNull(descriptor);

// rewrite type
descriptor.Extend().OnBeforeCreate(RewriteConfiguration);
string? typeName = null) =>
ApplyIdToFieldCore(descriptor, NodeIdNameDefinitionUnion.Create(typeName));

if (descriptor is IDescriptor<ObjectFieldConfiguration> objectFieldDescriptor)
{
var extend = objectFieldDescriptor.Extend();

// add serializer if globalID support is enabled.
if (extend.Context.Features.Get<NodeSchemaFeature>()?.IsEnabled == true)
{
ApplyIdToField(extend.Configuration, typeName);
}
}
}
/// <inheritdoc cref="ApplyIdToField(IDescriptor{OutputFieldConfiguration},string?)"/>
/// <typeparam name="T">
/// the type from which the <see cref="IDAttribute.TypeName">type name</see> is derived
/// </typeparam>
public static void ApplyIdToField<T>(
IDescriptor<OutputFieldConfiguration> descriptor) =>
ApplyIdToFieldCore(descriptor, NodeIdNameDefinitionUnion.Create<T>());

/// <summary>
/// Applies the <see cref="RelayIdFieldExtensions"><c>.ID()</c></see> to an argument
Expand All @@ -76,7 +63,8 @@ public static void ApplyIdToField(
/// </remarks>
internal static void ApplyIdToField(
ObjectFieldConfiguration configuration,
string? typeName = null)
NodeIdNameDefinitionUnion? nameDefinition = null,
TypeReference? dependsOn = null)
{
var placeholder = new ResultFormatterConfiguration(
(_, r) => r,
Expand All @@ -89,13 +77,77 @@ internal static void ApplyIdToField(
ctx,
(ObjectFieldConfiguration)def,
placeholder,
typeName),
nameDefinition),
configuration,
ApplyConfigurationOn.BeforeCompletion);
ApplyConfigurationOn.BeforeCompletion,
typeReference: dependsOn);

configuration.Tasks.Add(configurationTask);
}

internal static void ApplyIdToFieldCore(
IDescriptor<OutputFieldConfiguration> descriptor,
NodeIdNameDefinitionUnion? nameDefinition)
{
ArgumentNullException.ThrowIfNull(descriptor);

// rewrite type
descriptor.Extend().OnBeforeCreate(RewriteConfiguration);

if (descriptor is IDescriptor<ObjectFieldConfiguration> objectFieldDescriptor)
{
var extend = objectFieldDescriptor.Extend();

// add serializer if globalID support is enabled.
if (extend.Context.Features.Get<NodeSchemaFeature>()?.IsEnabled == true)
{
if (nameDefinition?.Type != null)
{
var dependsOn = extend.Context.TypeInspector.GetTypeRef(nameDefinition.Type);
ApplyIdToField(extend.Configuration, nameDefinition, dependsOn);
}
else
{
ApplyIdToField(extend.Configuration, nameDefinition);
}
}
}
}

public static void ApplyIdToFieldCore(
IDescriptor<ArgumentConfiguration> descriptor,
NodeIdNameDefinitionUnion? nameDefinition)
{
ArgumentNullException.ThrowIfNull(descriptor);

var extend = descriptor.Extend();

// rewrite type
extend.OnBeforeCreate(RewriteConfiguration);

// add serializer if globalID support is enabled.
if (extend.Context.Features.Get<NodeSchemaFeature>()?.IsEnabled == true)
{
if (nameDefinition?.Type == null)
{
extend.OnBeforeCompletion((c, d) =>
AddSerializerToInputField(c, d, nameDefinition));
}
else
{
var dependsOn = extend.Context.TypeInspector.GetTypeRef(nameDefinition.Type);

var configurationTask = new OnCompleteTypeSystemConfigurationTask(
(ctx, def) => AddSerializerToInputField(ctx, (ArgumentConfiguration)def, nameDefinition),
extend.Configuration,
ApplyConfigurationOn.BeforeCompletion,
typeReference: dependsOn);

extend.Configuration.Tasks.Add(configurationTask);
}
}
}

private static void RewriteConfiguration(
IDescriptorContext context,
FieldConfiguration configuration)
Expand Down Expand Up @@ -137,7 +189,7 @@ private static IExtendedType RewriteType(ITypeInspector typeInspector, ITypeInfo
internal static void AddSerializerToInputField(
ITypeCompletionContext completionContext,
ArgumentConfiguration configuration,
string? typeName)
NodeIdNameDefinitionUnion? nameDefinition)
{
var typeInspector = completionContext.TypeInspector;
IExtendedType? resultType;
Expand Down Expand Up @@ -165,6 +217,8 @@ internal static void AddSerializerToInputField(
completionContext.Type);
}

var typeName = GetIdTypeName(completionContext, nameDefinition, typeInspector);

var validateType = typeName is not null;
typeName ??= completionContext.Type.Name;
SetSerializerInfos(completionContext.DescriptorContext, typeName, resultType);
Expand All @@ -176,7 +230,7 @@ private static void AddSerializerToObjectField(
ITypeCompletionContext completionContext,
ObjectFieldConfiguration configuration,
ResultFormatterConfiguration placeholder,
string? typeName)
NodeIdNameDefinitionUnion? nameDefinition)
{
var typeInspector = completionContext.TypeInspector;
IExtendedType? resultType;
Expand All @@ -199,6 +253,8 @@ private static void AddSerializerToObjectField(
var serializerAccessor = completionContext.DescriptorContext.NodeIdSerializerAccessor;
var index = configuration.FormatterConfigurations.IndexOf(placeholder);

var typeName = GetIdTypeName(completionContext, nameDefinition, typeInspector);

typeName ??= completionContext.Type.Name;
SetSerializerInfos(completionContext.DescriptorContext, typeName, resultType);

Expand Down Expand Up @@ -279,4 +335,19 @@ internal static void SetSerializerInfos(IDescriptorContext context, string typeN
var feature = context.Features.GetOrSet<NodeSchemaFeature>();
feature.NodeIdTypes.TryAdd(typeName, runtimeTypeInfo.NamedType);
}

private static string? GetIdTypeName(ITypeCompletionContext completionContext,
NodeIdNameDefinitionUnion? nameDefinition,
ITypeInspector typeInspector)
{
var typeName = nameDefinition?.Literal;
if (nameDefinition?.Type is { } t)
{
var referencedType = typeInspector.GetType(t);
var foo = completionContext.GetType<IType>(TypeReference.Create(referencedType));
typeName = foo.NamedType().Name;
}

return typeName;
}
}
14 changes: 14 additions & 0 deletions src/HotChocolate/Core/src/Types/Types/Relay/NodeIdNameDefinitionUnion.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
namespace HotChocolate.Types.Relay;

/// <summary>
/// A discriminated union, containing either a literal or a type that defines
/// the name of the node identifier.
/// </summary>
internal record NodeIdNameDefinitionUnion(string? Literal, Type? Type)
{
public static NodeIdNameDefinitionUnion? Create(string? literal) =>
literal == null ? null : new NodeIdNameDefinitionUnion(literal, null);

public static NodeIdNameDefinitionUnion Create<T>() =>
new NodeIdNameDefinitionUnion(null, typeof(T));
}
2 changes: 1 addition & 1 deletion src/HotChocolate/Core/src/Types/Types/Relay/NodeResolverTypeInterceptor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ public override void OnAfterMergeTypeExtensions()
RelayIdFieldHelpers.AddSerializerToInputField(
CompletionContext,
argument,
fieldTypeDef.Name);
NodeIdNameDefinitionUnion.Create(fieldTypeDef.Name));

// As with the id argument, we also want to make sure that the ID field of
// the field result type is a non-null ID type.
Expand Down
Loading
Loading