Graphql-dotnet: InputObjectGraphType resolve function ignored

[shoe-diamente]

Description

By defining something like:

```c#
public class CreateUserInputValue
{
public int? OtherName { get; set; }
}

public class CreateUserInput : InputObjectGraphType<CreateUserInputValue>
{
    public CreateUserInput()
    {
        Field<IdGraphType>()
            .Name("id")
            .Resolve(context =>
            {
                return context.Source.OtherName;
            });
    }
}

it doesn't seem like `id` is correctly mapped to `OtherName`.

### Steps to reproduce

With the code above and by calling:

```graphql
mutation {
  test(input:{id:1244}) {
    id
  }
}

with:

```c#
public class Mutation : ObjectGraphType
{
public Mutation()
{
Field>()
.Name("test")
.Argument>("input", "")
.ResolveAsync(async context =>
{
var input = context.GetArgument("input");
// ... breakpoint
return null;
});
}
}

the `OtherName` value is `null`.

### Expected result

I would expect `OtherName` to be `1244`.

### Actual result

`OtherName` is `null`.

### Environment

```xml
<PackageReference Include="GraphQL" Version="2.4.0" />

[joemcbride]

This is currently not supported. This is related to #850 #851 #864 #1045 (and some others I'm sure I'm missing).

[gitmustashio]

Can you explain a bit what this feature is ? I can't figure out what he's trying to do from the code example.

[shoe-diamente]

@gitmustashio I'm trying to map a GraphQL input field to a specific C# input property which is not necessarily the one with a similar name.

[stephenlautier]

Was expecting the same, we always done resolvers, and luckily always worked by luck; Now just had a property with a different name and it wasn't working

[flieks]

i tried this workaround for mutations
overriding the base class which contains an enum called DataType

but then you would get an error
Ambiguous match found.
casting the input type
var question = context.GetArgument<QuestionInput>("questionCreate");

[sungam3r]

@flieks Please open a separate Issue about Ambiguous match found. I would like to fix this error, I most likely know what it is.

[flieks]

@sungam3r ok thanks
https://github.com/graphql-dotnet/graphql-dotnet/issues/1313

[sungam3r]

@shoe-diamente For input types resolving does not make sense. Honestly, it should not be possible at all to set a resolve function for them. But the type system is such that this is problematic. Another acceptable solution would be to generate an error when setting the resolver.

it doesn't seem like id is correctly mapped to OtherName.

What is your goal? If you need to initialize OtherName with some computed value (which the client does not transmit) then now it can be done in the constructor of your object. See #1286

[sungam3r]

the OtherNull value is null.

@shoe-diamente did you mean OtherName?

[shoe-diamente]

@sungam3r Yes, typo. Thanks.

[sungam3r]

@shoe-diamente I had time to return to the issues that I once entered on my personal todo list. This is one of them. I have some thoughts on how to change the design so that such a problem does not arise in principle. This will require changing part of the base classes. In short, the InputObjectGraphType should not have Resolver property for its fields at all. Without a doubt, if I do decide to make this change, it will be 100% backward incompatible. However, I see this as a useful change.

[shoe-diamente]

@sungam3r That's great to hear. Thanks 😃

[bogdankolomiec706]

Hi,

Today I also encountered the same isssue.
<PackageReference Include="GraphQL" Version="3.0.0-preview-1352" />

It turned out I had wrong expectations about fields resolvers in InputObjectGraphType. It is really misleading, especially when ObjectGraphType works in this way.

I thought it a very cool feature that helps to separate and decouple graph naming for clients and my internal model naming (until I figured out I was wrong :) ).

@sungam3r, I aggree thtat generating error when setting the resolver is much better solution then not working silently (explicit over implicit).
But I don't get why you think resolving for input types does not make sense, could please explain?
Also, you asked What is your goal? - I thought the goal is to separate external graph and internal model names and values corespondence, resolvers are aimed to perform exactly this role. It seems to me that feature you referenced is not about this (#1286).

Do we consider this issue closed/solved? I didn't find any open issue related to this problem?
@joemcbride, all isues that you mentioned are closed: #850 #851 #864 #1045

It seems to me that initial problem with which @stephenlautier, @shoe-diamente came is not solved for now.

Could we continue to discuss solution?
Will be glad to hear from you.

[mac2000]

I agree with @sungam3r rather than throwing an error we should use strongly typed features of language and do not allow to use resolvers at all, wondering if you can share your thoughts about required changes so anybody can participate

There is no clear notes about resolvers on input types in spec, but there is a note:

The GraphQL Object type (ObjectTypeDefinition) defined above is inappropriate for re‐use here, because Object types can contain fields that define arguments or contain references to interfaces and unions, neither of which is appropriate for use as an input argument. For this reason, input objects have a separate type in the system.

Also if you will try to do it in apollo server (which uses reference facebook library) like so:

const { ApolloServer, gql } = require("apollo-server");

const users = [
  { id: "1", username: "@mac" },
  { id: "2", username: "@maria" },
];

const typeDefs = gql`
  type User {
    id: ID!
    username: String!
  }

  type Query {
    users: [User]!
  }

  input AddUserInput {
    username: String!
  }

  type Mutation {
    addUser(input: AddUserInput!): User
  }
`;

const resolvers = {
  Query: {
    users: () => users,
  },
  AddUserInput: {
    username: (parent, args, context, info) => {
      return "hello";
    },
  },
  Mutation: {
    addUser: (_, { input }) => {
      input.id = 1 + Math.max.apply( null, users.map(({ id }) => id) );
      users.push(input);
      return input;
    },
  },
};

const server = new ApolloServer({ typeDefs, resolvers });

server.listen().then(({ url }) => console.log(url));

Immediately at startup you will got an error:

node_modules/graphql-tools/dist/generate/addResolveFunctionsToSchema.js:73
  throw new _1.SchemaError(typeName + " was defined in resolvers, but it's not an object");

[Error: AddUserInput was defined in resolvers, but it's not an object]

which indeed makes sense because input type is not an object type

[sungam3r]

But I don't get why you think resolving for input types does not make sense, could please explain?

Because there is nothing to resolve for input types. Input types exist for input values. If you want additional computed input values then place this logic inside resolver for output type field where you call context.GetArgument.

[Shane32]

I'd like to work on this for 4.0

[sungam3r]

It is a very comprehensive issue which breaks initial design and type system.

[Shane32]

I know. Hopefully I can make it happen with backwards compatibility for the majority of use cases. But yeah, diving into it already, it looks like a pain. It's just that the current design is worse than it appears. For instance, loading a scalar from a variable calls the scalar's Serialize method in one area of the code, and ParseValue in another. Based on the xml comments, this shouldn't happen ... but it does. I think when I'm done with it the code will be a lot cleaner and straightforward.

[sungam3r]

I talked about entirely removing Resolve from input objects.

[Jonatthu]

@sungam3r I have a use case!!!
But because of resolve is being ignored this is not possible,
Is there any way to enable it?

using System.Collections.Generic;
using GraphQL.Types;
using Generatthor.Designer.Core.Api.Extensions;
using Generatthor.Designer.Core.Api.Services;
using Generatthor.Designer.Core;
using GraphQL;
using System;
using System.Linq;
using System.ComponentModel.DataAnnotations;
using Generatthor.Generated.Api.Models.Permissions;
using Generatthor.Generated.Models.Requests;
using Generatthor.Generated.Api.Enums;
using Generatthor.Designer.Core.Api.Models;
using Generatthor.Generated.Api.Types;
using Generatthor.Generated.Api.Mutations;
using Generatthor.Generated.Api.Mutations.Inputs;
using Generatthor.Generated.Models.Response;
using Generatthor.Generated.Services;

namespace Generatthor.Generated.Api.Mutations.Inputs
{
    /// <summary>
    /// Ripple Add Request
    /// </summary>
    public class RippleAddInputGraphType : InputObjectGraphType<RippleAddRequest>
    {
        public RippleAddInputGraphType(
            IUserContext<GlobalPermissionsEnum> userContext,
            IAuthPermissionsService<GlobalPermissionsEnum> authPermissionsService,
            IValidationService validationService,
            IWalletService walletService
        )
        {
            Name = "RippleAddInputGraphType";

            Field<StringGraphType>(
                name: "Name",
                description: "",
                resolve: ctx => ctx.Source.Name
            ).RequirePermission(GlobalPermissionsEnum.Ripple_Name_Add);

            Field<StringGraphType>(
                name: "Description",
                description: "",
                resolve: ctx => ctx.Source.Description
            ).RequirePermission(GlobalPermissionsEnum.Ripple_Description_Add);

            Field<StringGraphType>(
                name: "WalletId",
                description: "",
                resolve: ctx => 
                {
                    if (ctx.Source.Wallet == null && ctx.Source.WalletId == null) 
                    {
                        throw new Exception("WalletId or Wallet properties are both null, you must provide one of them.");
                    }

                    if (ctx.Source.Wallet != null && ctx.Source.WalletId != null) 
                    {
                        throw new Exception("WalletId or Wallet properties are both defined, you must provide only one of them.");
                    }

                    return ctx.Source.WalletId;
                }
            ).RequirePermission(GlobalPermissionsEnum.Ripple_WalletId_Add);

            Field<DateTimeGraphType>(
                name: "AddedDate",
                description: "",
                resolve: ctx => ctx.Source.AddedDate
            ).RequirePermission(GlobalPermissionsEnum.Ripple_AddedDate_Add);

            #region Computed Properties

            #endregion

            #region Computed File Properties

            #endregion

            #region Navigation Properties

            FieldAsync<WalletAddInputGraphType>(
                name: "Wallet",
                resolve: async context =>
                {
                    WalletAddRequest request = context.Source.Wallet;
                    request.IsDeepCreate = true;
                    validationService.ValidateRequest(context, request);

                    WalletGetResponse response = await walletService.AddAsync(request, context.CancellationToken);
                    return response;
                }
            ).RequirePermission(GlobalPermissionsEnum.Wallet_Add);

            #endregion

        }
    }
}

[Shane32]

I’m planning, if I have time, to work this into version 4. At present I don’t have time. It will be a rather major rewrite.

[Jonatthu]

@Shane32 ok I probably can work around this by creating functions being called in the main mutation, but I think resolve function should be called! Even when seems not useful turns out that actually is!

[Jonatthu]

@Shane32 what part of the code needs work? Could you guide me what files need work? Maybe I can take the time to see how to contribute

[Shane32]

I'd like to explain, but I don't understand it well enough yet. It really requires quite a bit of investigation along with time to determine the least number of changes in order to achieve the goal here.

Below is a rough draft of the changes I think are required.

When GraphQL deserializes a complex input type from a literal, something like this should happen:

• InputObjectGraphType has a ParseLiteral and ParseValue method similar to scalars
• ParseLiteral is called with some type of IResolveInputFieldContext parameter
• The implementation can query the fields with something like GetField<> (similar to GetArgument<>), in order to populate the object it is creating with its child fields.
• Calling GetField<> to get a child scalar calls ParseLiteral of the scalar, whereas calling GetField<> to get a child complex object calls ParseLiteral of the InputObjectGraphType.
• Eventually the created object is returned. All of the above code will have a default implementation which works, hopefully, in the same manner that it does now. This should be easy for a InputObjectGraphType<T> - see ObjectExtensions.ToObject.

Then:

• Calling GetArgument<> on a IResolveFieldContext runs the above code, and then if the return value is not of the type requested, it will attempt a conversion via the ValueConverter.

Similar changes are made for parsing variables (vs literals). All the variables can then be parsed a single time prior to execution of the document, rather than twice as it is now.

When all that is accomplished, we may wish to change how fields are defined on InputObjectGraphTypes, perhaps to remove the resolve parameter, as it will be the graph type that defines how to serialize the value into an object (similar to scalars), not a parameter on the field.

One significant bug this process will fix is the fact that currently when parsing variables, values get passed through both ParseValue and ParseLiteral. This should never occur, and will not occur with this new code.

It will also be much easier to understand, debug, and change how complex objects are deserialized from a literal or variables. It will also deduplicate a bunch of this related code which is spread into multiple areas of the project currently.

Another benefit is that when input union types are introduced into the spec, it should be a breeze to implement them.

As I said before, I think this consists of basically a complete rewrite of the input object parsing code, particularly for variables. It's no small change. There will sure to be challenges and complications to overcome, and on top of everything, we must try to retain a certain degree of compatibility with existing code.

As to where to begin, that's a tough question. I'm not really sure. Maybe the best idea is to just start rewriting ExecutionHelper.GetVariableValue from scratch and see where that leads. I started by trying to separate parsing scalar variables from literals but it wasn't getting me anywhere.

[Shane32]

I'm not sure, but it might help if #354 (or something similar) was completed first.

[Shane32]

@sungam3r I'm going to start on this task along the lines described in my prior post in this thread. Please let me know if you have any comments before I begin. It will probably change a lot of code, so I don't want to go down the wrong path right off the bat.

There will be breaking changes for sure, but my goal will be to have as little impact as possible for the majority of users. So hopefully the Harness/Star Wars sample will not change (specifically, the HumanInputType) because default implementations of the new InputObjectGraphType.ParseLiteral and InputObjectGraphType.ParseValue methods will provide backwards-compatible functionality.

[Shane32]

Looking back at the migration notes for 4.0, most everything else we have done until now has been little housekeeping items, edge cases, or minor performance improvements. This feature is what I hope to be the compelling reason to switch to 4.0. (But perhaps this is an edge case also.)

[sungam3r]

Please let me know if you have any comments before I begin. It will probably change a lot of code, so I don't want to go down the wrong path right off the bat.

Yes, there is really a lot of code to change. It will take me a couple of days to sort things out and remember and describe what I wanted to do. This is a big refactoring.

[sungam3r]

Now it takes a lot of time to review. There are also household chores. If we start doing this work, then it is quite possible that it will take all the time remaining until the end of the weekend. January 11 is my first working day after holidays.

[mkhatuntcov-korewireless]

Has same issue and just wanna say thank you guys for your work.
Just wandering. Looks like this is regular object-object mapper task.

I am thinking to use some standard object-object mapper like:
https://docs.automapper.org/en/stable/

Again thanks for your work.

[sungam3r]

We are delaying the release of version 4. In the course of its preparation, errors were discovered that require fixing. In addition, we have added a few more features.

[Shane32]

@mkhatuntcov-korewireless The new ParseDictionary method in #2165 should allow you to fully customize input object deserialization behavior. It does not use the Resolve field at the moment, but any functionality that would otherwise be within Resolve you can replicate within ParseDictionary. Is that satisfactory? I think in the end we will be removing the Resolve field from input object graphs, or perhaps replacing the signature with something appropriate to input graphs, but this will not be done for v4.

Can we close this issue? @sungam3r @mkhatuntcov-korewireless Do we want to keep it open?

[sungam3r]

I think in the end we will be removing the Resolve field from input object graphs, or perhaps replacing the signature with something appropriate to input graphs, but this will not be done for v4.

Yes. That is why I keep this issue open. The point is to change the design for input/output types.