Skip to content
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.

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

Async scalars with access to context and info? #2663

Closed
borekb opened this issue Jun 17, 2020 · 16 comments
Closed

Async scalars with access to context and info? #2663

borekb opened this issue Jun 17, 2020 · 16 comments
Labels

Comments

@borekb
Copy link

borekb commented Jun 17, 2020

In our resolver map, we have two quite different things:

  1. Resolvers, which are functions called by graphql-js with useful objects like info or context, and they are usually async.
  2. Custom scalars, which are objects, don't have access to info or context (or anything request-related) and their functions like serialize aren't async.

I wonder if scalars could be more resolver-like?!

Concrete example

Our use case is price rounding. In our case, GraphQL server code doesn't know how to round – it needs to ask a backend API for rounding rules – but it is GraphQL code's responsibility to ensure that values are rounded.

Most prices in our schema are represented by a Money type that looks like this:

type Money {
  amount: MonetaryAmount! # custom scalar
  currencyCode: String!
}

We can then do something like this, which works well:

const resolvers = {
  Query: {
    Money: {
      amount: async (money, _, { dataSources: { backendApi } }) =>
        round(money.amount, money.currencyCode, backendApi),
    },
  }
};

async function round(amount, currency, backendApi) {
  const roundingInfo = await backendApi.getRoundingInfo(...);
  // etc.
}

Some parts of our schema, however, use the scalar directly, skipping the Money type. For example:

type GoogleEcommerceDataLayerActionField {
  id: String!
  revenue: MonetaryAmount
  tax: MonetaryAmount
}

I'd love to be able to write resolver-like code for it, like this:

// pseudocode!

const resolvers = {
  __customScalars: {
    MonetaryAmount: async (value, { config, dataSources: { backendApi } }) =>
      round(value, config.currencyCode, backendApi),
  },
};

Currently, we need to solve this on the specific field level, for example, if there's a field like:

type GoogleEcommercePurchaseDataLayer {
  actionField: GoogleEcommerceDataLayerActionField
}

we can hook into actionField and process the rounding there. But it would be safer and easier if we could solve it at the type level.

What do you think? Has this been considered before? I didn't find much but maybe my search skills have failed me.

@danielrearden
Copy link
Contributor

Thanks for contributing this feature request.

This idea has been discussed before here.

The resolver map you're using is something specific to graphql-tools and libraries that use graphql-tools (like apollo-server), not graphql-js. graphql-tools lets you pass in custom scalar implementations using the resolver map, but that arguably should be a separate configuration option in the first place. Field resolution and custom scalars are mostly unrelated concepts. Scalars and other types are not "resolved" in the sense that fields are. Fields represent actual values which are determined at runtime based on the request -- types are static and represent the range of possible values for a field or input.

The context and info objects provided to field resolvers are specific to generating the output values returned by the server. But scalars can represent input values like arguments, input object fields or arguments and that same information would not be available in those contexts.

@borekb
Copy link
Author

borekb commented Jun 17, 2020

@danielrearden Thanks a lot, I didn't realize scalars can also be inputs which makes it more tricky. (I also didn't realize that resolver map is specific to graphql-tools, thanks for pointing that out.)

Still, when inputs are parsed via parseValue / parseLiteral, we're probably already in a context of a specific request, right? Meaning that higher-level libraries like Apollo Server already had a chance to run their context-creation functions, so scalars could possibly be given the context argument I'd think.

The info and args arguments probably don't have any meaningful values when scalars are inputs, so they'd probably be undefined – not great, not terrible 😄.

Lastly, what about asynchronicity? Could scalar parsing / serialization function be async, or is there some hard limitation why they cannot?


Alternatively, are there other ideas on how to run a resolver-like function (with an async backend API call) for all fields that are of a certain scalar type?

@borekb
Copy link
Author

borekb commented Jun 17, 2020

Specifically, could something like this work?

const myScalar = new GraphQLScalarType({

  // has access to most things like a resolver, returns a Promise
  serialize: async (value, args, context, info) =>
    round(value, context.backendApi),

  // demonstrates async input validation
  parseValue: async (value, context) => {
    const rounded = await round(value, context.backendApi);
    if (value !== rounded) {
      throw new GraphQLError("Please provide a rounded number");
    }
    return value;
  },

  parseLiteral: async (ast, context) => /* similar to above */,

});

@IvanGoncharov
Copy link
Member

@borekb Scalars can be used as default values inside SDL and during schema building we don't have any context at all:

input SomeType {
  someField: SomeScalar = 5
}

We need to parse it and validate it during schema building without context and synchronously.
If you have request specific logic it should leave in your resolvers this issue contains some ideas on how to implement it: #2268 (comment)

If none of them works you can you please describe your use case in more detail?

@borekb
Copy link
Author

borekb commented Jun 19, 2020

@IvanGoncharov Our GraphQL server deals with financial amounts as JS numbers which is something we cannot change right now and means that we sometimes get numbers like 0.30000000000000004. The GraphQL server should ensure that when sending such numbers to clients, they are rounded by a backend's rounding policy.

We have a schema like this:

# Like Float but rounded by backend's rounding policy
scalar MonetaryAmount

type Money {
  amount: MonetaryAmount!
  currencyCode: String!
}

Query {
  aFieldUsingScalar: MonetaryAmount!
  aFieldUsingObjectType: Money!
  
  # ... and many more fields that have Money or MonetaryAmount somewhere in them
}

Fields that use the Money type can use a single resolver like this:

const resolvers = {
  Query: {
    Money: {
      amount: async (money, _, { dataSources: { backendApi } }) => {
        const roundingInfo = await backendApi.getRoundingInfo(...);
        round(money.amount, roundingInfo.decimalPlaces);
      }
    },
  }
};

When new fields are added to our schema and they use the Money type, they automatically get the rounding treatment which is great.

But how to do it for scalars? (There are reasons out of our control why certain fields use the scalar directly, skipping the Money type.) I think something like this would be an ideal solution:

const MonetaryAmount = new GraphQLScalarType({
  serialize: async (value, context) => {
    const roundingInfo = await context.backendApi.getRoundingInfo(...);
    round(money.amount, roundingInfo.decimalPlaces);
  }
});

IF we ignore inputs for now, do you think the serialize's signature could be changed to support such use case?


About inputs and especially the default values in SDL, that's a great example that I didn't realize before. So do I get this right?:

  • When inputs are parsed from a query, the 'parse' functions could theoretically be async, and be given context as an argument. (Not sure if a good idea or not but it would probably work, technically.)
  • When scalars are used as default values in SDL, there is no way this could ever be async, and there's certainly no context object at that point.

Is that correct?


Incidentally, just yesterday we changed the types of our inputs from MonetaryAmount back to Floats. To prevent schema mistakes, we even throw from the 'parse' functions:

  MonetaryAmount: new GraphQLScalarType({
    name: 'MonetaryAmount',
    description: 'MonetaryAmount is a Float that is rounded by the backend\'s rounding policy',
    serialize: value => value,
    parseValue: () => throwOnCustomScalarInput('MonetaryAmount'),
    parseLiteral: () => throwOnCustomScalarInput('MonetaryAmount'),
  }),

The reasoning was that we cannot control user input (nothing stops them from sending 0.30000000000000004) and we do not have a way to round the numbers in the parse functions, or even check whether the number is already rounded or not (who knows, maybe the backend rounding policy is 17 decimal places).


Overall, I see now how inputs are tricky, or probably just downright impossible to make async. But what about outputs? It would be very useful if serialize could be async and had access to context; could that be done?

@danielrearden
Copy link
Contributor

@borekb If you have some resolution logic that's duplicated across multiple fields, then you should extract this logic into a separate function that can then be called from each resolver. If the resolvers are identical, then you can even do something like:

const resolvers = {
  Query: {
    someField: roundValue
  },
  SomeType: {
    someOtherField: roundValue
  },
}

If you're using Apollo Server or graphql-tools and building your schema using SDL, you can also utilize schema directives to the same effect:

type Query {
  someField: Float @roundValue
}

type SomeType {
  someOtherField: Float @roundValue
}

In other words, if you're just trying to avoid repetition in your code when building your schema, there are existing avenues for doing so.

@IvanGoncharov
Copy link
Member

IvanGoncharov commented Jun 19, 2020

IF we ignore inputs for now, do you think the serialize's signature could be changed to support such use case?

We use serialize for printing schemas with default values and also for sending default values inside introspection. So we need to also keep it request-independent.

When scalars are used as default values in SDL, there is no way this could ever be async, and there's certainly no context object at that point.

Yes, it true for both parsing/printing SDL and sending/receiving introspection results.

When inputs are parsed from a query, the 'parse' functions could theoretically be async, and be given context as an argument. (Not sure if a good idea or not but it would probably work, technically.)

Theoretically yes, practically it would result in very strange API where a function doesn't receive most of the parameters and can respond with a promise only in certain situations.

Moreover awaiting inside every serialize will kill your performance since every await (even if the promise is already resolved) goes through microtasks queue, what's even worse is that by making more leaves return promises you forcing more of execution to be done in async mode. It will totally kill performance if you returning big arrays of such scalars, for context please see: #723 (comment)
So what's why we limiting async to only functions that absolutely need it like resolve and resolveType.

What you can do is to use wrappers for numbers that you want to wrap and address rounding it before or during serilization:

class NumberWithRounding {
  constructor(private value: Number) {}
  valueOf() { return this.value; }
  round(decimalPlaces) { return round(value, decimalPlaces); }
}

const MonetaryAmount = new GraphQLScalarType({
  serialize: (value) => new NumberWithRounding(value),
});

// ...
const roundingInfo = await context.backendApi.getRoundingInfo(...);
JSON.stringify(result, (_key, value) {
  if (value instanceof NumberWithRounding) {
    return value.round(roundingInfo.decimalPlaces);
  }
  return value;
})

@borekb
Copy link
Author

borekb commented Jun 19, 2020

@IvanGoncharov Thanks a lot, this nicely explains why scalars are sync and have to be this way. Really appreciated!

Also thanks for the example, that's a clever solution that I think should achieve what we're after. I'm just not sure where to put the code after //... – that place would need to run after each scalar is serialized, right? What is such place in the request pipeline?

@borekb
Copy link
Author

borekb commented Jun 19, 2020

@danielrearden We're already de-duplicating rounding logic in a way you suggested – we have a single function that many resolvers call. The issue is with the "many resolvers" part – we have to remember to correctly implement rounding resolvers for fields that happen to use scalars (vs. the Money type, which handles rounding "automatically"). This is relatively fragile so I'm looking for a solution to that.

@IvanGoncharov
Copy link
Member

Also thanks for the example, that's a clever solution that I think should achieve what we're after. I'm just not sure where to put the code after //... – that place would need to run after each scalar is serialized, right? What is such place in the request pipeline?

@borekb After execution is finished but before you send respond.
You can research on how to customize serialize function for your framework, e.g. in express, you can specify json replacer https://expressjs.com/en/api.html#app-settings-property
If you can't customize serializer for some reason you can write a function that get completed result of query traverse it and calls value.round.
Also, you can try Apollo's middlewares as @danielrearden suggested above.

@yaacovCR
Copy link
Contributor

Another option is to make use of the mapSchema function from graphql-tools

https://www.graphql-tools.com/docs/schema-directives#full-mapschema-api

You can easily iterate through your executable schema and find every object field of that scalar type and modify the resolver to do whatever you need. This is a parallel approach to existing middleware options.

@yaacovCR
Copy link
Contributor

Even though the link references directives you can use map schema to do whatever you need even without directives just based on the field type...

@borekb
Copy link
Author

borekb commented Jun 19, 2020

you can use map schema to do whatever you need even without directives just based on the field type

@yaacovCR That sounds very useful, thanks!

@yaacovCR
Copy link
Contributor

I should say that mapSchema as currently implemented creates a new schema with all new types that are rewired together.

This does mean that not every schema is compatible with mapSchema.

Specifically, resolveType resolvers for interfaces should return type names instead of actual types, maps of types should be keyed by typeName rather than the actual type, etc, etc.

All GraphQL type system extensions should be in the extensions field, as any annotations of GraphQL type system objects that are not in the extensions field will be lost.

@IvanGoncharov
Copy link
Member

Closing since it looks like a bunch of alternative solutions suggested.

@yaacovCR
Copy link
Contributor

Note that the use case in the related ticket #3539 relates to customizing error message languages, with the parse/serialize functions the normal place where these errors and messages are generated.

The implementation within #3600 that allows access to context is meant for only scalar-specific work such as the above. For that reason, the functions remain synchronous, and access to info is not provided.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants