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

RFC: Fix ambiguity with null variable values and default values #418

Merged
merged 7 commits into from
Apr 18, 2018

Commits on Mar 6, 2018

  1. RFC: Fix ambiguity with null variable values and default values

    > This is a **behavioral change** which changes how explicit `null` values interact with variable and argument default values. This also changes a validation rule which makes the rule more strict.
    
    There is currently ambiguity and inconsistency in how `null` values are coerced and resolved as part of variable values, default values, and argument values. This inconsistency and ambiguity can allow for `null` values to appear at non-null arguments, which might result in unforseen null-pointer-errors.
    
    This appears in three distinct but related issues:
    
    **Validation: All Variable Usages are Allowed**
    
    The explicit value `null` may be used as a default value for a variable with a nullable type, however this rule asks to treat a variable's type as non-null if it has a default value. Instead this rule should specifically only treat the variable's type as non-null if the default value is not `null`.
    
    Additionally, the `AreTypesCompatible` algorithm is underspecificied, which could lead to further misinterpretation of this validation rule.
    
    **Coercing Variable Values**
    
    `CoerceVariableValues()` allows the explicit `null` value to be used instead of a default value. This can result in a null value flowing to a non-null argument due to the validation rule mentioned above. Instead a default value must be used even when an explicit `null` value is provided. This is also more consistent with the explanation for validation rule "Variable Default Value Is Allowed"
    
    Also, how to treat an explicit `null` value is currently underspecified. While an input object explains that a `null` value should result in an explicit `null` value at the input object field, there is no similar explaination for typical scalar input types. Instead, `CoerceVariableValues()` should explicitly handle the `null` value to make it clear a `null` is the resulting value in the `coercedValues` Map.
    
    **Coercing Argument Values**
    
    The `CoerceArgumentValues()` algorithm is intentionally similar to `CoerceVariableValues()` and suffers from the same inconsistency. Explicit `null` values should not take precedence over default values, and should also be explicitly handled rather than left to underspecified input scalar coercion.
    leebyron committed Mar 6, 2018
    Configuration menu
    Copy the full SHA
    ef26891 View commit details
    Browse the repository at this point in the history

Commits on Mar 30, 2018

  1. Updated based on feedback.

    This updates this proposal to be a bit broader in scope however much narrower in breaking behavior changes.
    
    Mirroring the changes in graphql/graphql-js#1274, this update better defines the difference between a "required" and "non-null" argument / input field as a non-null typed argument / input-field with a default value is no longer required. As such the validation rule which prohibited queries from using non-null variables and default values has been removed. This also adds clarity to the input field validation - this rule has existed in the GraphQL.js reference implementation however was found missing within the spec.
    
    This also updates the CoerceVariableValues() and CoerceArgumentValues() algorithms to retain explicit null values overriding a default value (minimizing breaking changes), however critically adding additional protection to CoerceArgumentValues() to explicitly block null values from variables - thus allowing the older pattern of passing a nullable variable into a non-null argument while limiting the problematic case of an explicit null value at runtime.
    leebyron committed Mar 30, 2018
    Configuration menu
    Copy the full SHA
    3dc6d1b View commit details
    Browse the repository at this point in the history

Commits on Apr 3, 2018

  1. One step further towards the idealized "from scratch" proposal, this …

    …makes it more explicitly clear that changing the effective type of a variable definition is only relevent when supporting legacy clients and suggests that new services should not use this behavior.
    
    I like that this balances a clear description of how this rule should work for existing services along with a stricter and therefore safer future path for new services.
    leebyron committed Apr 3, 2018
    Configuration menu
    Copy the full SHA
    2fbeb5d View commit details
    Browse the repository at this point in the history

Commits on Apr 5, 2018

  1. Editing AreTypesCompatible() to avoid trailing "Otherwise return fals…

    …e" statements for easier reading. Functionality is equivalent.
    leebyron committed Apr 5, 2018
    Configuration menu
    Copy the full SHA
    ee6220b View commit details
    Browse the repository at this point in the history

Commits on Apr 6, 2018

  1. Update "All Variable Usages are Allowed" to remove breaking change.

    Also attempts to improve clarity and formatting and adds an example case.
    leebyron committed Apr 6, 2018
    Configuration menu
    Copy the full SHA
    429bf2b View commit details
    Browse the repository at this point in the history
  2. Configuration menu
    Copy the full SHA
    3147689 View commit details
    Browse the repository at this point in the history

Commits on Apr 18, 2018

  1. Final review edits

    leebyron committed Apr 18, 2018
    Configuration menu
    Copy the full SHA
    03197f8 View commit details
    Browse the repository at this point in the history