graphql-platform icon indicating copy to clipboard operation
graphql-platform copied to clipboard

Use object type with name matching entity name by default

Open glen-84 opened this issue 1 year ago • 10 comments

Is your feature request related to a problem?

When I added a second object type for ObjectType<User>, named ActiveUser, in addition to the existing User object type, my existing queries returning IQueryable<User> and Task<User?> started returning the ActiveUser object type.

(13.0.0-preview.52)

The solution you'd like

I assume that the engine finds two object types mapped to the User entity – if so, it should select the object type with a name matching the entity type by default. f.e., if the entity type is User and there is an object type with the same name (User), then use that object type, otherwise ...

... maybe it should also throw when there's no matching object type and there is more than one object type mapped to the same entity? Just selecting an arbitrary object type from a list seems unexpected.

Entity type Object type(s) Use object type
User User User
User AppUser AppUser (only one matching type)
User User, AppUser User (name matches)
User PrivateUser, PublicUser (exception)

Product

Hot Chocolate

glen-84 avatar Sep 15 '22 12:09 glen-84

I think it's rare that the type and the entity are named exactly the same. I think inferring here does more harm then good. For example if you were to rename the entity you would suddenly get a different object type.

You are using the Analyzer that's registering the types automatically, right? I think this is where the issues lies. Normally you don't have this problem, since you can just register one "default" type and specify the other types explicitly for certain fields. But the analyzer registers all types (presumably sorted, but I'm not sure how the default type is determined in this situation).

IMO it would be better to just introduce a heuristic through which the analyzer determines the default type to register. This default type of entity X is then automatically applied to all return types of entity X. If you have a second type for entity X you need to explicitly set the field returning entity X to be of that second type. I think this would be the "safest" route to go.

@michaelstaib what do you think?

tobias-tengler avatar Sep 15 '22 13:09 tobias-tengler

I think it's rare that the type and the entity are named exactly the same.

Really? I expected the opposite. 🙂

For example if you were to rename the entity you would suddenly get a different object type.

Only if one existed. Also the type selection is already unpredictable, based on whichever type was added last (I think).

You are using the Analyzer that's registering the types automatically, right?

Correct ...

public static IRequestExecutorBuilder AddApplicationTypes(this IRequestExecutorBuilder builder)
{
    builder.AddTypeExtension<Application.Modules.Users.Mutations.UserMutations>();
    builder.AddTypeExtension<Application.Modules.Users.Types.UserQueries>();
    builder.AddType<Application.Modules.Users.Types.UserProfileType>();
    builder.AddType<Application.Modules.Users.Types.UserType>();
    builder.AddType<Application.Modules.Core.Types.CountryType>();
    builder.AddType<Application.Modules.Users.Types.ActiveUserType>();
    return builder;
}

Is there any way to set a default type, so that I don't have to do ugly stuff like [GraphQLType("[User!]")] or [GraphQLType(nameof(User))] on all queries?

I thought that something like .BindRuntimeType<User>("User") might work, but it doesn't.

The only other option that I can think of would be to actually create an ActiveUser entity, and somehow map that to the same DB table without duplicating mapping data. Not even sure if that is possible yet.

glen-84 avatar Sep 15 '22 14:09 glen-84

Adding the ability to specify a default type was what I was proposing above 😅

I think at the moment you sadly have to specify the type explicitly...

tobias-tengler avatar Sep 15 '22 14:09 tobias-tengler

I found a workaround:

    .AddType<UserType>()
    .AddApplicationTypes()

i.e. Manually register the type that you want to be the default, and then call the Add*Types method.

I still think the default behaviour could be more defensive, and throw when there is more than one type registered with no default set.

glen-84 avatar Sep 16 '22 12:09 glen-84

With default you mean which type is binding Implicitly?

michaelstaib avatar Sep 21 '22 21:09 michaelstaib

One potential runtime issue here would be that, if you combined ActiveUser and User into a union the GraphQL engine would no longer be able to infer the type from the runtime type and you would be forced to implement the IsOfType resolution ... same goes for interfaces.

michaelstaib avatar Sep 21 '22 21:09 michaelstaib

With default you mean which type is binding Implicitly?

Well, if there's a way to specify which of multiple types is bound to the runtime type, then it becomes explicit. It can remain implicit when there's only one type. I guess that BindRuntimeType can't be used for this purpose?

One potential runtime issue here would be that, if you combined ActiveUser and User into a union the GraphQL engine would no longer be able to infer the type from the runtime type and you would be forced to implement the IsOfType resolution ... same goes for interfaces.

Not sure if I understand, but a union would probably require a separate runtime type for ActiveUser anyway?

glen-84 avatar Sep 22 '22 06:09 glen-84

In GraphQL you can have unions an interfaces ... like

union Users = User | ActiveUser

If they have the same runtime time then there is no way for the GraphQL engine to pick a winner. This is done by calling IsOfType on the GraphQL type. So if there is a a property that can distinguish between the two states or something from the resolver context then this is possible again. But if you do not take care of things like this you will get impossible runtime behavior.

michaelstaib avatar Sep 22 '22 06:09 michaelstaib

@glen-84 Not really. You can define interfaces and unions without a common runtime type. Best example for this is the node field. In the relay case i think we can make it work as we encode the type in the IDs. But this one for example can become tricky

type User {username: String}
type ActiveUser {username:String}

union Users = User | ActiveUser

type Query { users: [Users] }
{
    users {
        ... on ActiveUser { username }
       ... on User {username }
}

PascalSenn avatar Sep 22 '22 06:09 PascalSenn

At the moment I am thinking of something like ...

public class FooType : ObjectType<Foo> 
{
    protected override void Configure(IObjectTypeDescriptor<Foo> descriptor) 
    {
         descriptor.BindTypeExplicitly()
    }
}

we already have a mechanism like this for the scalar.

If we add the same behavior to the object types then also BindRuntimeType would make sense for these types.

BindRuntimeType could than be used to force casting or conversion behavior.

michaelstaib avatar Sep 22 '22 06:09 michaelstaib