Improve error reporting for union member types#2472
Open
Cito wants to merge 332 commits intographql:17.x.xfrom
Open
Improve error reporting for union member types#2472Cito wants to merge 332 commits intographql:17.x.xfrom
Cito wants to merge 332 commits intographql:17.x.xfrom
Conversation
src/type/validate.js
Outdated
| } | ||
|
|
||
| const includedTypeNames = Object.create(null); | ||
| const includedInvalidTypes = Object.create(null); |
Member
There was a problem hiding this comment.
I think it strange behavior where after fixing one error you would get the same error in the next location.
We should definitely prevent implicit cascading errors but I think we should never mask explicit errors made by the user.
Also if we add this error reduction trick we need to add it in many other places (e.g. if you implement non-interface types) for consistency.
So I think moving check to the beginning of the loop and adding continue would be enough.
Member
Author
There was a problem hiding this comment.
Note that this does not hide any errors - there is only one message, but all locations are reported and the same message applies to them all.
not actually possible!
`graphql` provides TS types since `14.5.0` (released 3 years ago) and we fully switched to TS in `15.0.0` so I think it's time to drop runtime typechecks. Motivations: This type checks were added long time ago since we shifted towards TS we just maintained them without adding new ones. In general, this check increase bundle size add runtime cost and we can't realistically check all arguments to all functions. Instead we should focus on adding more asserts on stuff that can't be checked by TS.
After we drop runtime typechecks that were duplicating TS types this function became a wrapper for 'assertValidSchema' so all implementations that rely on it can now use 'assertValidSchema' directly.
`operation` is included within `exeContext`
…ql#3639) = introduces `buildPerEventExecutionContext` that creates an `ExecutionContext` for each subscribe event from the original `ExecutionContext` used to create the event stream = `subscribe` now directly builds the `ExecutionContext` instead of relying on `createSourceEventStream` = introduces `createSourceEventStreamImpl` and `executeImpl` functions that operate on the built `ExecutionContext` rather the `ExecutionArgs` = `subscribe` calls the `createSourceEventStreamImpl` function on the original context and eventuallys calls `executeImpl` on the per event context created by `buildEventExecutionContext`. Motivation: 1. remove unnecessary `buildExecutionContext` call, reducing duplicate work 2. paves the way for easily enhancing the `buildPerEventExecutionContext` to add a new `perEventContextFactory` argument to augment the context argument passed to resolvers as need per event.
* support async benchmark tests * Add benchmarks for sync and async list fields
Co-authored-by: Rob Richard <rob@1stdibs.com>
…cution (graphql#3418) Co-authored-by: Ivan Goncharov <ivan.goncharov.ua@gmail.com>
As part of graphql#3649 research I added all options explicitly and also enable ones that didn't require any code change. We should enable rest of them in a separate PRs.
Continuation of graphql#3670 Basically allow to have code like this: ```js parse(document, GraphQLServerConfig.parserOptions) ``` to pass TS check with 'exactOptionalPropertyTypes'
* Fix coerceInputValue test name * fix grammar
* parser: limit maximum number of tokens
Motivation: Parser CPU and memory usage is linear to the number of tokens in a
document however in extreme cases it becomes quadratic due to memory exhaustion.
On my mashine it happens on queries with 2k tokens.
For example:
```
{ a a <repeat 2k times> a }
```
It takes 741ms on my machine.
But if we create document of the same size but smaller number of
tokens it would be a lot faster.
Example:
```
{ a(arg: "a <repeat 2k times> a" }
```
Now it takes only 17ms to process, which is 43 time faster.
That mean if we limit document size we should make this limit small
since it take only two bytes to create a token, e.g. ` a`.
But that will hart legit documents that have long tokens in them
(comments, describtions, strings, long names, etc.).
That's why this PR adds a mechanism to limit number of token in
parsed document.
Also exact same mechanism implemented in graphql-java, see:
graphql-java/graphql-java#2549
I also tried alternative approach of counting nodes and it gives
slightly better approximation of how many resources would be consumed.
However comparing to the tokens, AST nodes is implementation detail of graphql-js
so it's imposible to replicate in other implementation (e.g. to count
this number on a client).
* Apply suggestions from code review
Co-authored-by: Yaacov Rydzinski <yaacovCR@gmail.com>
Co-authored-by: Yaacov Rydzinski <yaacovCR@gmail.com>
Motivation: I was inspecting code for unrelated reason and just spotted this pattern. I think it worth to refactor it since it easier to understand intention looking on code that uses Set's has/add methods. Benchmark shows no measurable changes in CPU/memory usages.
Motivation: Spotted during investigation of unrelated issue. Maps and sets are iteratable, so no need to call these methods.
Explanation: `Object.create(null)` returns value of `any` type. So bellow construct is not reported by TS even in "strict" mode: ```ts const foo = Object.create(null); ``` Fixing this issue in `extendSchema` requires adding more code since we can't put all extensions nodes into one collection without loosing typesafety.
…s` (graphql#3655) * polish: add tests for more testUtils adds tests for expectPromise and expectEqualPromisesOrValues * break out expectMatchingValues into another file * apply review feedback
To simply suggest a larger cohort of reviewers for more complex changes, and add the caveat that merged changes may be reverted if consensus has not been reached at a GraphQL-JS WG meeting.
graphql#4022) As surfaced in [Discord](https://discord.com/channels/625400653321076807/862957336082645006/1206980831915282532) this currently is a breaking change in the 16.x.x release line which is preventing folks from upgrading towards a security fix. This PR should result in a patch release on the 16 release line. This change was originally introduced to support CFW and browser environments which should still be supported with the `typeof` check CC @n1ru4l This also adds a check whether `.env` is present as in the DOM using `id="process"` defines that as a global which we don't want to access on accident. as shown in graphql#4017 Bundles also target `process.env.NODE_ENV` specifically which fails when it replaces `globalThis.process.env.NODE_ENV` as this becomes `globalThis."production"` which is invalid syntax. Fixes graphql#3978 Fixes graphql#3918 Fixes graphql#3928 Fixes graphql#3758 Fixes graphql#3934 This purposefully does not account for graphql#3925 as we can't address this without breaking CF/plain browsers so the small byte-size increase will be expected for bundled browser environments. As a middle ground we did optimise the performance here. We can revisit this for v17. Most bundlers will be able to tree-shake this with a little help, in graphql#4075 (comment) you can find a conclusion with a repo where we discuss a few. - Next.JS by default replaces [`process.env.NODE_ENV`](https://github.com/vercel/next.js/blob/b0ab0fe85fe8c93792051b058e060724ff373cc2/packages/next/webpack.config.js#L182) you can add `typeof process` linearly - Vite allows you to specify [`config.define`](https://vitejs.dev/config/shared-options.html#define) - ESBuild by default will replace `process.env.NODE_ENV` but does not support replacing `typeof process` - Rollup has a plugin for this https://www.npmjs.com/package/@rollup/plugin-replace Supersedes graphql#4021 Supersedes graphql#4019 Supersedes graphql#3927 > This now also adds a documentation page on how to remove all of these
Co-authored-by: Saihajpreet Singh <saihajpreet.singh@gmail.com>
…aphql#4180) fixes up just merged graphql#4015 this was actually intended to be in graphql#4015, but due to branch confusion not originally included now we also have a test!
Errata: Left typescript at v5.4.x as deep dependency typedoc requires v5.4.x. Left eslint v8.x as not all plugins are compatible with v9. Code changes, in no particular order: 1. Prettier formatting changes. 2. Prettier moved to an async API, but the `writeGeneratedFile` utility, which previously included prettifying was passed as a callback function to TS and had to stay sync, so prettifying was separated into a separate async function -- the callback function luckily did not seem to actually requiring another round of prettifying, as it just involved renaming. All the other callsites of the new utility had to be made async. In the alternate, I investigated @prettier/sync and the lower-level make-synchronized and make-synchronous packages, but I could not get them working. 3. Plenty of eslint rule changes! I have tried to make sure that the rule list orders now match the linked documentation, so that further updates might be easier. 4. Minor docusaurus config tweaks to get the build to pass.
fixed by deps upgrade
Deprecates `valueFromAST()` and adds `coerceInputLiteral()` as an additional export from `coerceInputValue`. The implementation is almost exactly the same as `valueFromAST()` with a slightly more strict type signature . `coerceInputLiteral()` and only `coerceInputLiteral()` properly supports fragment variables in addition to operation variables.
…th nested fragments (graphql#4168) (graphql#4199)
[graphql#3074 rebased on main](graphql#3074). Depends on graphql#3809 @leebyron comments from original PR (edited, hopefully correctly): > Fixes graphql#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 (EDIT: but not both!) "value" and "literal" fields. > > 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 graphql#3049. > > **After this change:** > > ``` > (SDL) --parse-> (defaultValue literal config) --print --> (SDL) > ``` Co-authored-by: Lee Byron <lee.byron@robinhood.com>
We're winding down work on CCN in favor of True Schema Nullability. More context: graphql/nullability-wg#37
Improving directive tests by adding a new example for directives used on
other directive arguments:
```graphql
directive @myDirective(arg:String) on ARGUMENT_DEFINITION
directive @myDirective2(arg:String @myDirective) on FIELD
```
As far as I could see that was not clearly documented through tests
before.
[graphql#3077 rebased on main](graphql#3077). Depends on graphql#3810 @leebyron comments from original PR: > 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 graphql#3065. This PR is pulled out as a pre-req to aid review --------- Co-authored-by: Lee Byron <lee.byron@robinhood.com>
[graphql#3065 rebased on main](graphql#3065). Depends on graphql#3811 @leebyron comments from original PR: > **Provides the "Value to Literal" methods in this [data flow chart](https://user-images.githubusercontent.com/50130/118379946-51ac5300-b593-11eb-839f-c483ecfbc875.png).** > > * Adds `valueToLiteral()` which takes an external input value and translates it to a literal, allowing for custom scalars to define this behavior. > > **This also adds important changes to Input Coercion, especially for custom scalars:** > > * The value provided to `parseLiteral` is now `ConstValueNode` and the second `variables` argument has been removed. For all built-in scalars this has no effect, but any custom scalars which use complex literals no longer need to do variable reconciliation manually (in fact most do not -- this has been an easy subtle bug to miss). > This behavior is possible with the addition of `replaceVariables` Changes to the original: 1. Instead of changing the signature of `parseLiteral()`, a new method `parseConstLiteral()` has been added with the simpler signature. `parseLiteral()` has been marked for deprecation. 2. `replaceVariables()` has access to operation and fragment variables. Co-authored-by: Lee Byron <lee.byron@robinhood.com>
…od (graphql#4209) `parseLiteral()` has been marked as deprecated, prompting but not forcing our users to convert to `parseConstLiteral()`. With this additional change: - in v17, if not supplied, the new `parseConstLiteral()` method will be left as undefined, and during execution, we will fall back to`parseLiteral()`. - in v18, we will remove `parseLiteral()` and set up a default function for `parseConstLiteral()` when not supplied. Prior to this change, users of custom scalars with custom `parseLiteral()` functions who did not provide a custom `parseConstLiteral()` function would just get the default `parseConstLiteral()` function during execution, which might not work as expected. This scheme will work except for users of custom scalars who want to embed experimental fragment variables in their custom scalars, which will only work with the new `parseConstLiteral()` method.
.**){kind=link}
Particularly, verify that it first sorts by lexicographic distance and then naturally.
yaacovCR
force-pushed
the
fix-validation-order
branch
from
2ca0fda to
3eba0c8
Compare
✅ Deploy Preview for compassionate-pike-271cb3 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Contributor
|
@Cito I went ahead and rebased against main. |
yaacovCR
reviewed
Sep 30, 2024
Contributor
yaacovCR
left a comment
yaacovCR
left a comment
There was a problem hiding this comment.
Seems like a good idea, left some comments/questions!
| } | ||
|
|
||
| const includedTypeNames = new Set<string>(); | ||
| const includedInvalidTypes = new Set<string>(); |
Contributor
There was a problem hiding this comment.
can we just changed this to a single Set, visitedTypeNames?
| ); | ||
| includedInvalidTypes.add(typeString); | ||
| } | ||
| continue; |
Contributor
There was a problem hiding this comment.
this reminds me of #4181 => what do you think about changing the error message string to something that references both errors, at least if there are repetitions?
'Something like Union type can only include unique Object types, ....'
What do you think?
First make sure that the included type is an object type, because that would be the more severe problem. Only then we can be sure that the type has a name and check for duplicates. Also, create only one error per different non-object type.
yaacovCR
force-pushed
the
fix-validation-order
branch
from
3eba0c8 to
d390b94
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
19 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First make sure that the included type is an object type, because that would be the more severe problem. Only then we can be sure that the type has a name and check for duplicates. Also, create only one error per different non-object type.