Skip to content

Clarify usage of the variables argument of parseLiteral #2657

Closed
@Cito

Description

@Cito

The parseLiteral method of scalar and enum types expects the variables as second argument (see the definition of the GraphQLScalarLiteralParser type). This argument is passed in valueFromAST.

However, the standard scalar types provided with GraphQL.js and the enum type do not make use of this variables argument at all, it's simply ignored. Also, the default parseLiteral function for scalars does not pass the variables. I don't see it being used in other libraries (graphql-iso-date, graphql-scalars, graphql-type-json) either. This functionality is currently also neither tested nor documented (see #2567).

Before adding tests and documentation, we need to clarify the purpose of this second argument to parseLiteral and must come up with a reasonable use case. Otherwise the argument should be removed, since it makes the API more complicated, and causes problems (in languages where argument passing is handled more strictly than in JavaScript) when this parameter is not considered.

The variables argument was added in 714ee98 by @leebyron with the comment in the commit message that "supporting variable values within custom scalar literals can be valuable". Can somebody come up with a concrete example for that? @IvanGoncharov mentioned complex (composite) scalars types like JSON, but I would like to see more concretely how it would be used here.

Another problem with this parameter is that parseLiteral is acutally called twice, once without variables (during validation with ValueOfCorrectType) and during execution, with variables. See also #383 where it is suggested to use memoization to solve this, but this would not work well with the additional variables argument. Note that there is also an UnusedVariables rule in validation which prevents the use of variables which are not used explicitly in the document (and would be used only implicitly via parseLiteral).

Activity

andimarek

andimarek commented on Jun 29, 2020

@andimarek
Contributor

@Cito As @IvanGoncharov mentioned it is essential for more complex Scalars like JSON. See https://github.com/taion/graphql-type-json/blob/master/src/index.js#L42 for a real life implementation.

The general explanation is:
A custom Scalar can accept arbitrary Ast Elements and these elements can include variable references which need to be resolved by the custom scalar itself (it can't be done by graphql.js directly).
For example you could imagine a Money scalar which accepts {value: "12.0", currency: "EUR"} as literal. This Scalar should also be able to process variable references.:

query myQuery($euros: String) {
  transfer(money: {value: $euros , currency: "EUR"}): Boolean
}

variables: {euros: "12.0"}

This parseLiteral method will now receive {value: $euros, currency: "EUR"} and needs to convert it to a real value: this is why it also needs access to the variables in order to resolve the $euros reference to "12.0".

I hope that helps.

Cito

Cito commented on Jul 3, 2020

@Cito
MemberAuthor

@andimarek - I have tried this out now and created the following test. I think something like that should be included in the test suite, to have better coverage of custom scalars. The test confirms that variables are passed properly to parseLiteral. It also reveals the problem that when the query is validated with ValuesOfCorrectTypeRule, the parseLiteral function is called without variables.

// @flow strict

import { expect } from 'chai';
import { describe, it } from 'mocha';

import { GraphQLError } from '../../error';
import { graphqlSync } from '../../graphql';
import type { ValueNode } from '../../language';
import {
  GraphQLFloat,
  GraphQLObjectType,
  GraphQLScalarType,
  GraphQLSchema,
} from '../../type';
import { valueFromASTUntyped } from '../../utilities';
import isFinite from '../../polyfills/isFinite';
import inspect from '../../jsutils/inspect';
import type { ObjMap } from '../../jsutils/ObjMap';

class Money {
  amount: number;
  currency: string;

  constructor(amount: number, currency: string) {
    this.amount = amount;
    this.currency = currency;
  }
}

function serializeMoney(outputValue: mixed): mixed {
  if (!outputValue instanceof Money) {
    throw new GraphQLError(
      `Cannot serialize money value: ${inspect(outputValue)}`,
    );
  }
  return { ...outputValue };
}

function parseMoneyValue(inputValue: mixed): mixed {
  if (!inputValue instanceof Money) {
    throw new GraphQLError(`Cannot parse money value: ${inspect(inputValue)}`);
  }
  return inputValue;
}

function parseMoneyLiteral(
  valueNode: ValueNode,
  variables: ?ObjMap<mixed>,
): Money {
  const money: Money = (valueFromASTUntyped(valueNode, variables): any);
  if (variables) {
    // variables are not set when checked with ValuesIOfCorrectTypeRule
    if (
      !money ||
      !isFinite(money.amount) ||
      typeof money.currency !== 'string'
    ) {
        throw new GraphQLError(
          `Cannot parse literal money value: ${inspect(money)}`,
        );
    }
  }
  return new Money(money.amount, money.currency);
}

const MoneyScalar = new GraphQLScalarType({
  name: 'Money',
  serialize: serializeMoney,
  parseValue: parseMoneyValue,
  parseLiteral: parseMoneyLiteral,
});

const moneyValue = new Money(42, 'DM');

const schema = new GraphQLSchema({
  query: new GraphQLObjectType({
    name: 'Query',
    fields: {
      balance: {
        type: MoneyScalar,
        resolve: () => moneyValue,
      },
      toEuros: {
        type: GraphQLFloat,
        args: { money: { type: MoneyScalar } },
        resolve: (_root, { money }) => {
          const amount = money.amount;
          if (!amount) {
            return amount;
          }
          switch (money.currency) {
            case 'DM':
              return amount * 0.5;
            case 'EUR':
              return amount;
            default:
              throw new Error(
                `Cannot convert to euros: ${inspect(money)}`,
              );
          }
        },
      },
    },
  }),
});

describe('Money Scalar', () => {
  it('serialize', () => {
    const source = `
      {
        balance
      }
    `;

    const result = graphqlSync({ schema, source });
    expect(result).to.deep.equal({
      data: {
        balance: {
          amount: 42,
          currency: 'DM',
        },
      },
    });
  });

  it('parseValue', () => {
    const source = `
      query Money($money: Money!) {
        toEuros(money: $money)
      }
    `;
    const variableValues = { money: moneyValue };

    const result = graphqlSync({ schema, source, variableValues });
    expect(result).to.deep.equal({
      data: {
        toEuros: 21,
      },
    });
  });

  it('parseLiteral', () => {
    const source = `
      query Money($amount: Float!, $currency: String!) {
        toEuros(money: {amount: $amount, currency: $currency})
      }
    `;
    const variableValues = { ...moneyValue };

    const result = graphqlSync({ schema, source, variableValues });
    expect(result).to.deep.equal({
      data: {
        toEuros: 21,
      },
    });
  });
});
andimarek

andimarek commented on Jul 5, 2020

@andimarek
Contributor

@Cito I am not involved in the maintenance for this project. @IvanGoncharov or others can give you feedback.

yaacovCR

yaacovCR commented on May 22, 2022

@yaacovCR
Contributor

Can this be closed? Should be covered by #2696 and #3065 (comment)

yaacovCR

yaacovCR commented on Oct 7, 2024

@yaacovCR
Contributor

Closed more definitively by #3812

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      No branches or pull requests

        Participants

        @Cito@andimarek@yaacovCR@IvanGoncharov

        Issue actions

          Clarify usage of the variables argument of parseLiteral · Issue #2657 · graphql/graphql-js