astFromValue fails with a custom scalar serializing to an object value

[ardatan]

When you have a scalar with a serialize function that returns an object, it is not possible to convert it to AST.
So it doesn't allow you to print a schema that has an input value with an object as a default value.

Reproduction -> #4086
Proposed fix -> #4087

const JSONScalar = new GraphQLScalarType({
  name: "JSON",
  serialize(value) {
    return value;
  },
});

const schema = new GraphQLSchema({
  types: [JSONScalar],
  query: new GraphQLObjectType({
    name: "Query",
    fields: {
      test: {
        type: GraphQLString,
        args: {
          i: {
            type: JSONScalar,
            defaultValue: { object: true },
          },
        },
      },
    },
  }),
});

[yaacovCR]

I think this would be solved alternatively by: #3814 .

The relevant code bit would be:

    const literal =
      arg.defaultValue.literal ??
      valueToLiteral(arg.defaultValue.value, arg.type);

where the first condition reads the literal from the preserved literal if the schema was created from an AST, and the second bit converts the value to a literal in a type-safe manner if the schema was created programattically.

see
https://github.com/yaacovCR/graphql-executor/blob/57e32d78041e0263991b7ae2488c62fde32ae930/src/utilities/printSchema.ts#L261-L263

[ardatan]

So your PR introduces a new method in the leaf types called valueToLiteral right?
Looks more elegant actually. But your PR looks a big change. Maybe we can try to fix this area specifically first before a huge refactor. What do you think?

[ardatan]

There is also this issue which is a bit related;
#4088
@yaacovCR I am curious what do you think?

[yaacovCR]

Yes I think it should also be covered => custom scalars may have to define custom valueToLiteral methods

just to be clear, while my name is on the rebased PR, all the work is from @leebyron (and the feedback he received from others, of course) — my role has just been trying to keep it alive by rebasing it periodically.

I hope it eventually gets in!

[dotansimha]

@yaacovCR I agree it will be solved by #3814 , but the seems like a bigger refactor.

I think there's room for improvement in the current impl, without refactoring it. I tend to agree with @ardatan 's take that this seems like a bug that should be fixed.

[yaacovCR]

I assume what held up this PR initially is that it seems to use the serialize() method to "uncoerce" an input value, while serialize() is only meant to be valid for output values.

I had a previous discussion about this with @jaydenseric if I remember correctly:

jaydenseric/graphql-upload#194

Rereading that, it seems like I believed at the time that we were heading to adopting serialize() as a way of uncoercing, but looking at the final version of the PR by @leebyron at #3049, it looks like that was called out as unsound by @andimarek and avoided within the valid code paths, although we do use it to give a more helpful error message.

I think this should be closed within v17 by #3812 and #4209, so I am going to close this issue.

[ardatan]

We have this problem in v16 currently and v17 is far from being released. My PRs aimed to fix this in a more straight forward way in v16. I am not sure if I understand why all of those quicker solutions are rejected as v17 is not even close yet.

[yaacovCR]

I have no personal objection to a stop-gap solution for v16 only, but I am afraid that I don't feel confident that this bug fix would not end up being a breaking change in some significant respect, and that's mostly because I find the input value section of the specification and its implementation quite challenging for my deteriorating cognitive skills -- I was quite happy to simply rebase @leebyron 's PRs rather than work on those issues on my own! I posted above some thoughts as to what the potential issue might be.

Perhaps some of the work done from the PRs in v17 can be backported without the entire rearchitecting? Or maybe we could input from others who worked on that set of PRs, @leebyron @andimarek or the ever helpful @benjie .

They might be able to bless this as a patch release to v16.