ease upgrade path for programmatic default values

[yaacovCR]

#3814 added default value validation and coercion, deprecating astFromValue() (which was unsafe, used serialize() to uncoerce input values provided in the internal format) and replacing it with a call to valueToLiteral() which safely operates on external values.

This PR makes that change backwards compatible by reintroducing it as a new config option of externalDefaultValue instead of replacing the existing option of defaultValue.

defaultValueLiteral stays as its open top-level config option, but is subsumed under externalDefaultValue within the GraphQLArgument/GraphQLInputField as it is always external.

[netlify]

✅ Deploy Preview for compassionate-pike-271cb3 ready!

Name	Link
🔨 Latest commit	a4d9d80
🔍 Latest deploy log	https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/674c7a7f45d9620008a48e9a
😎 Deploy Preview	https://deploy-preview-4296--compassionate-pike-271cb3.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[github-actions]

Hi @yaacovCR, I'm @github-actions bot happy to help you with this PR 👋

Supported commands

Please post this commands in separate comments and only one per comment:

• @github-actions run-benchmark - Run benchmark comparing base and merge commits for this PR
• @github-actions publish-pr-on-npm - Build package from this PR and publish it on NPM

[yaacovCR]

To summarize, cross-posting from #4288 (comment):

In v16, the available default value option on GraphQLArgumentConfig and GraphQLInputField was just defaultValue and that represented the internal value and so external values were never validated no matter how supplied. If the schema was built from SDL, we converted the default value AST into the internal value immediately by way of valueFromAST(), which just erased the default if it was invalid, if it was programmatically supplied, we just used whatever was suppllied.

In v17 currently, prior to the proposed #4296, we adopted @leebyron 's work and replaced that with two options, defaultValue and defaultValueLiteral where the former was the external value programmatically supplied, and the lateral was the ValueNode from the schema built from buildSchema() or extendSchema(), validating both of these within validateSchema(), and so we retain on GraphQLArgument and GraphQLInputField types a single defaultValue property of type DefaultValueUsage which either has a value or a literal property.

After #4296, we have both. On the config, to disambiguate, we use externalDefaultValue for the programmatically supplied option, so defaultValue as previously supplied stays the same. Within the objects, the new DefaultValueUsage object is stored under externalDefaultValue but the old internal default value if supplied will be under defaultValue. The new config options take precedence over the old.

Converting from the old to new and vice a versa

Coercing the external values for the defaults to the input values can be done easily using the coerceInputValue() or the coerceInputLiteral functions, (although it's not necessarily safe until the schema has been validated, in particular default values may contain themselves recursively which would be quite unsafe).

If you want to safely turn an internal value into something external, we have no function to do this in v16 or v17. But if you want, you can do what's done unsafely in v16 for introspection, you can use astFromValue() to move from an internal value to an AST, which is always external, and can now be supplied as the default via the defaultValueLiteral option.

[JoviDeCroock]

Not a big fan of externalDefaultValue, generally just not a big fan of giving multiple options to achieve the same thing. Even if this eases the upgrade path we are just multiplying the combinations

[yaacovCR]

Strong agreement in terms of the long term value of having a single way of specifying options. The addition of externalDefaultValue change comes with deprecation of the defaultValue option which will be removed in v18. This is similar to the deprecation of the underlying astFromValue() function in favor of valueFromLiteral() rather than the former.

The other benefit of this is that it is nice to mark the option within the option name as external, so as to perhaps prevent confusion in the future.

For context, the default value work was originally a collaboration between @leebryon and @andimarek => of note, when graphql-java implemented this (in 2021!!!) looks to me like they also preserved for now the ability to set the default value as internal:

https://github.com/graphql-java/graphql-java/blob/add6dc559452ae0749db28d6325664be07cf765f/src/main/java/graphql/schema/InputValueWithState.java#L31

public class InputValueWithState {
    private enum State {
        /**
         * Value was never set aka not provided
         */
        NOT_SET,
        /**
         * The value is an Ast literal
         */
        LITERAL,
        /**
         * The value is an external value
         */
        EXTERNAL_VALUE,
        /**
         * This is only used to preserve backward compatibility (for now): it is a value which is assumed to
         * be already coerced.
         * This will be removed at one point.
         */
        INTERNAL_VALUE
    }
   ...

(With that option preserved until today!)

Basically, I see your point, and for that reason I am closing #4303 => where it's significantly confusing to do, we should not give multiple options, even to ease the upgrade path. But I think in this case, the benefits outweigh the hopefully temporary additional complexity.

But maybe it might be good to loop in @hayes or @ardatan. I think this PR would help make it possible for pothos and maybe in graphql-tools to bring graphql v17 to their respective libraries without a breaking change, which might make help the ecosystem overall move forward.

[andimarek]

@yaacovCR as a GraphQL Java inside: we simply decided it is to painful for the ecosystem to break all use case where they use the "old" way. We really try to hard to minimize the impact of breaking changes and often we decide to support the old way as much as possible. I am not sure if we will ever clean that up tbh.

[hayes]

I think this change would enable pothos users to more safely upgrade to v17 without changes in Pothos.

I am less sure if this would allow pothos to support new v17 features in a backwards compatible way (ie. without releasing a new major version). This is probably fine though, and major bumps (with small migrations) in pothos for major graphql releases is probably reasonable.

The biggest issue on the Pothos side is that Pothos does not have a way to safely type the new externalDefaultValue, because all Pothos types are based in the 'internal input" and "internal result" types. So far, pothos has never considered "external" or "serialized" types anywhere in it's api.

I think pothos will likely do the following:

• add a way to provide an "external" type for scalars (defaulting to be the same as the internal type
• continue to support defaultValue typed based on the internal input type (marked as deprecated)
  • In v18, we would likely do the unsafe coerceOutputValue which should work for most scalars
• add support for externalDefaultValue in config

Some thoughts/questions I had:

• From a spec perspective, are the types of a external input and output types independent? And is this something Pothos should take into account when letting users define external types for their custom scalars
• Is there a way that Pothos can detect the version of GraphQL being used? It might be helpful if the graphql package exported a version or other indicator that could be used that doesn't require doing some sort of feature detection.
• Are there any real world examples where using coerceExternalValue would fail, or is it only unsafe because the input and result types (both internal, and external) are theoretically fully independent?
  • File scalars are one that comes to mind here

[hayes]

this will be removed in v18 right?

[yaacovCR]

yes, typo, thanks!

[hayes]

externalDefaultValue seems to be an overloaded term now. I understand why we want to unify the value and literal in one property on the instance, but have them separate on the config, but having a properties with the same name on the instance and config, with different types seems more confusing.

Would using something lie instance.externalDefault?: { value: unknown } | { literal: ConstValueNode } work here? Not sure if externalDefault is too generic.

[yaacovCR]

I like instance.externalDefault?: { value: unknown } | { literal: ConstValueNode } or even instance.default?: { value: unknown } | { literal: ConstValueNode } as we are going to retire defaultValue

[hayes]

v18?

[yaacovCR]

😊

[yaacovCR]

• From a spec perspective, are the types of a external input and output types independent? And is this something Pothos should take into account when letting users define external types for their custom scalars

I think the spec section that might describe this best would be somewhere her:
https://spec.graphql.org/draft/#sec-Scalars.Result-Coercion-and-Serialization
https://spec.graphql.org/draft/#sec-Scalars.Input-Coercion

It is my understanding that the the external scalar type is independent of its position in terms of input vs output, but the coercion rules are not necessarily always inversible, hence the problem of supplying default values in terms of the internal coerced types for input.

• Is there a way that Pothos can detect the version of GraphQL being used? It might be helpful if the graphql package exported a version or other indicator that could be used that doesn't require doing some sort of feature detection.

we have over here a string and a semver info object: https://github.com/graphql/graphql-js/blob/16.x.x/src/version.ts

• Are there any real world examples where using coerceExternalValue would fail, or is it only unsafe because the input and result types (both internal, and external) are theoretically fully independent?
  • File scalars are one that comes to mind here

This is the main one that I have come across. I think the blast radius of this change may be smaller than I think. There is the example from #3051 but I think there the schema representation is off, but execution would still be correct.

[yaacovCR]

So far, pothos has never considered "external" or "serialized" types anywhere in it's api.

I see this code in type-options.ts:

    export interface ScalarTypeOptions<
      Types extends SchemaTypes = SchemaTypes,
      ScalarInputShape = unknown,
      ScalarOutputShape = unknown,
    > extends BaseTypeOptions<Types> {
      // Serializes an internal value to include in a response.
      serialize: (outputValue: ScalarOutputShape) => unknown;    //    <====== This unknown really could be the external type?
      // Parses an externally provided value to use as an input.
      parseValue?: GraphQLScalarValueParser<ScalarInputShape>;
      // Parses an externally provided literal value to use as an input.
      parseLiteral?: GraphQLScalarLiteralParser<ScalarInputShape>;
    }

I think you can change the unknown above to the external type, which should be the same type as any externalDefaultValue, but that you can't change the corresponding thing within parseValue because you have to be prepared for bad inputs. In theory you should have to be prepared for bad "inputs" to your serialize function, but TS will take care of that for you when you properly type it? I think? But you can never be sure about the inputs to parseValue, because those come from external.

I think! Not sure if the above is correct and/or helps, but pothos is cool. :)

[hayes]

I think! Not sure if the above is correct and/or helps, but pothos is cool. :)

Glad you like it!
Yeah, I think we could definitely do something there. The harder part is most people aren't defining their scalars in Pothos, they are usually pulling from graphql-scalars, and then just providing some type information about custom scalars when instantiating the builder.

The builder needs to know the types of all the scalars without referencing the implementation, so that fields can be typed correctly. Today you specify an input and an output type, but I think we'll just have to extend that with an external type that can be used for defaults.

[yaacovCR]

Ah. I was assuming that the external type could be inferred from the serialize/coerceOutputValue method return type….