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

Preserve defaultValue literals #3074

Open
wants to merge 1 commit into
base: coerce-input-literal
Choose a base branch
from

Conversation

leebyron
Copy link
Contributor

@leebyron leebyron commented May 10, 2021

Depends on #3092
Fixes #3051

This change solves the problem of default values defined via SDL not always resolving correctly through introspection by preserving the original GraphQL literal in the schema definition. This changes argument and input field definitions defaultValue field from just the "value" to a new GraphQLDefaultValueUsage type which contains either or both "value" and "literal" fields.

Since either of these fields may be set, new functions for resolving a value or literal from either have been added - getLiteralDefaultValue and getCoercedDefaultValue - these replace uses that either assumed a set value or were manually converting a value back to a literal.

Here is the flow for how a default value defined in an SDL would be converted into a functional schema and back to an SDL:

Before this change:

(SDL) --parse-> (AST) --coerceInputLiteral--> (defaultValue config) --valueToAST--> (AST) --print --> (SDL)

coerceInputLiteral performs coercion which is a one-way function, and valueToAST is unsafe and set to be deprecated in #3049.

After this change:

(SDL) --parse-> (defaultValue literal config) --print --> (SDL)

@andimarek
Copy link
Contributor

I think that is the right direction and as far as I understand you want that merged before the other bigger changes.

The more comprehensive end state we are aiming for In GraphQL Java is this:

public enum ValueState {
    NOT_SET,
    LITERAL,
    EXTERNAL_VALUE,
    INTERNAL_VALUE // this is deprecated and should not be used going forward, will be removed in the future
}

to capture the different states of the default value:
NOT_SET you probably don't need because you JS has undefined.

This is needed because we need to differentiate between external input value which will be converted to literal via valueToLiteral and the legacy behaviour of using a not well defined already coerced internal value, which will continue to use astFromValue.

@leebyron leebyron force-pushed the default-value-literals branch 2 times, most recently from 4b4cb69 to 3ab211e Compare May 10, 2021 21:53
@leebyron
Copy link
Contributor Author

Yeah, I'm treating this as a prerequisite dependency of #3049, similar to a few of the other changes I have up right now. I'm hoping to keep that diff as focused as possible, and this change should have very little effect other than resolving this issue.

The more comprehensive end state we are aiming for In GraphQL Java is this

That sounds right to me! Yes we use undefined to represent not set. I'm also considering performing the uncoerce step at schema construction time so that "INTERNAL_VALUE" is only a state the configuration can be in, rather than the final reified schema types. But that's for the later change :)

src/type/defaultValues.js Outdated Show resolved Hide resolved
@leebyron
Copy link
Contributor Author

Updated to do proper memoization and error if multiple sources of truth are presented in a config

leebyron added a commit that referenced this pull request May 11, 2021
Depends on #3074

By way of introducing type `VariableValues`, allows `getVariableValues` to return both the coerced values as well as the original sources, which are then made available in `ExecutionContext`.

While variable sources are not used directly here, they're used directly in #3065. This PR is pulled out as a pre-req to aid review
leebyron added a commit that referenced this pull request May 11, 2021
Depends on #3074

By way of introducing type `VariableValues`, allows `getVariableValues` to return both the coerced values as well as the original sources, which are then made available in `ExecutionContext`.

While variable sources are not used directly here, they're used directly in #3065. This PR is pulled out as a pre-req to aid review
leebyron added a commit that referenced this pull request May 11, 2021
Depends on #3074

By way of introducing type `VariableValues`, allows `getVariableValues` to return both the coerced values as well as the original sources, which are then made available in `ExecutionContext`.

While variable sources are not used directly here, they're used directly in #3065. This PR is pulled out as a pre-req to aid review
leebyron added a commit that referenced this pull request May 12, 2021
Depends on #3074

By way of introducing type `VariableValues`, allows `getVariableValues` to return both the coerced values as well as the original sources, which are then made available in `ExecutionContext`.

While variable sources are not used directly here, they're used directly in #3065. This PR is pulled out as a pre-req to aid review
leebyron added a commit that referenced this pull request May 13, 2021
Depends on #3074

By way of introducing type `VariableValues`, allows `getVariableValues` to return both the coerced values as well as the original sources, which are then made available in `ExecutionContext`.

While variable sources are not used directly here, they're used directly in #3065. This PR is pulled out as a pre-req to aid review
leebyron added a commit that referenced this pull request May 14, 2021
Depends on #3074

By way of introducing type `VariableValues`, allows `getVariableValues` to return both the coerced values as well as the original sources, which are then made available in `ExecutionContext`.

While variable sources are not used directly here, they're used directly in #3065. This PR is pulled out as a pre-req to aid review
@leebyron leebyron force-pushed the default-value-literals branch 2 times, most recently from 297f2c3 to 13e3148 Compare May 15, 2021 05:00
@leebyron leebyron changed the base branch from input-value-config to coerce-input-literal May 15, 2021 05:01
leebyron added a commit that referenced this pull request May 15, 2021
Depends on #3074

By way of introducing type `VariableValues`, allows `getVariableValues` to return both the coerced values as well as the original sources, which are then made available in `ExecutionContext`.

While variable sources are not used directly here, they're used directly in #3065. This PR is pulled out as a pre-req to aid review
leebyron added a commit that referenced this pull request May 15, 2021
Depends on #3074

By way of introducing type `VariableValues`, allows `getVariableValues` to return both the coerced values as well as the original sources, which are then made available in `ExecutionContext`.

While variable sources are not used directly here, they're used directly in #3065. This PR is pulled out as a pre-req to aid review
leebyron added a commit that referenced this pull request May 15, 2021
Depends on #3074

By way of introducing type `VariableValues`, allows `getVariableValues` to return both the coerced values as well as the original sources, which are then made available in `ExecutionContext`.

While variable sources are not used directly here, they're used directly in #3065. This PR is pulled out as a pre-req to aid review
leebyron added a commit that referenced this pull request May 15, 2021
Depends on #3074

By way of introducing type `VariableValues`, allows `getVariableValues` to return both the coerced values as well as the original sources, which are then made available in `ExecutionContext`.

While variable sources are not used directly here, they're used directly in #3065. This PR is pulled out as a pre-req to aid review
leebyron added a commit that referenced this pull request May 15, 2021
Depends on #3074

By way of introducing type `VariableValues`, allows `getVariableValues` to return both the coerced values as well as the original sources, which are then made available in `ExecutionContext`.

While variable sources are not used directly here, they're used directly in #3065. This PR is pulled out as a pre-req to aid review
@leebyron leebyron force-pushed the default-value-literals branch 2 times, most recently from 448367e to e2002dc Compare June 1, 2021 19:49
@leebyron leebyron added the PR: bug fix 🐞 requires increase of "patch" version number label Jun 1, 2021
Fixes #3051

This change solves the problem of default values defined via SDL not always resolving correctly through introspection by preserving the original GraphQL literal in the schema definition. This changes argument and input field definitions `defaultValue` field from just the "value" to a new `GraphQLDefaultValueUsage` type which contains either or both "value" and "literal" fields.

Since either of these fields may be set, new functions for resolving a value or literal from either have been added - `getLiteralDefaultValue` and `getCoercedDefaultValue` - these replace uses that either assumed a set value or were manually converting a value back to a literal.

Here is the flow for how a default value defined in an SDL would be converted into a functional schema and back to an SDL:

**Before this change:**

```
(SDL) --parse-> (AST) --coerceInputLiteral--> (defaultValue config) --valueToAST--> (AST) --print --> (SDL)
```

`coerceInputLiteral` performs coercion which is a one-way function, and `valueToAST` is unsafe and set to be deprecated in #3049.

**After this change:**

```
(SDL) --parse-> (defaultValue literal config) --print --> (SDL)
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: bug fix 🐞 requires increase of "patch" version number
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants