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

Port changes from graphql-js v0.4.18 (Oct 2015 spec) #117

Merged
merged 71 commits into from
May 30, 2016

Conversation

sogko
Copy link
Member

@sogko sogko commented Mar 7, 2016

Started work on porting changes from graphql-js v0.4.18 which is based on the latest Oct 2015 spec.

Previous port was based on v0.4.3.


Plan:

  • error/
  • languages/
  • types/
  • execution/
  • validation/
  • Fix go lint warnings (Fixed all warnings except for missing comments for exported functions or vars etc)

Notable changes:

Changes to public/exported vars / func

Previous New Risk
graphql.NAME_REGEXP graphql.NameRegExp Low
graphql.CollectFieldsParam.OperationType graphql.CollectFieldsParam.RuntimeType Low
graphql.GetVisitFn(*visitorOptions, bool, string) graphql.GetVisitFn(*visitorOptions, string, bool) Low
graphql.NewTypeInfo(*Schema) graphql.NewTypeInfo(*TypeInfoConfig) Low

Update: After a certain point, the coverage starts to decrease by around 6%. I'll work on it after porting the whole thing.
Update: Coverage went back up +3%

Cheers!

Merge latest changes from HEAD
- Include originalError in GraphQL error to preserve any additional info
	- Fixes graphql-go#251
	- graphql/graphql-js@3d27953

- Remove remaining references to u2028 and u2029
  	- graphql/graphql-js@5ee1edc
@sogko sogko changed the title Port changes from graphql-js v0.4.18 (Oct 2015 spec) [WIP] Port changes from graphql-js v0.4.18 (Oct 2015 spec) Mar 7, 2016
Parents: c55e9ac1ca
Author: Lee Byron <lee@leebyron.com>
Date: 21 January 2016 at 4:18:39 PM SGT

Ensure NamedType is a known Node
Parents: 9ddb2b579b
Author: Lee Byron <lee@leebyron.com>
Date: 21 January 2016 at 3:30:46 PM SGT

Move the extension definition out of type definition
@sogko sogko added this to the 0.4.18 milestone Mar 7, 2016
sogko and others added 21 commits March 8, 2016 06:50
Parents: 1651039cf4
Author: Lee Byron <lee@leebyron.com>
Date: 21 January 2016 at 3:10:44 PM SGT

flow type the parser

---

Commit: 6f2b66df332625a8f2269836f774e91d750e00ac [6f2b66d]
Parents: 4bd4b33a5a
Author: Lee Byron <lee@leebyron.com>
Date: 1 October 2015 at 9:21:18 AM SGT

Clearer lex errors for unprintable unicode

Fixes graphql-go#183

---

Commit: 969095e9f6be0bb13a69c715c6ee4910814a065b [969095e]
Parents: 5d4d531f23
Author: Lee Byron <lee@leebyron.com>
Date: 25 September 2015 at 6:51:32 AM SGT
Commit Date: 25 September 2015 at 6:53:31 AM SGT

[RFC] Clarify and restrict unicode support

This proposal alters the parser grammar to be more specific about what unicode characters are allowed as source, restricts those characters interpretted as white space or line breaks, and clarifies line break behavior relative to error reporting with a non-normative note.

Implements graphql/graphql-spec#96

---
Parents: 4977c9eedc
Author: Lee Byron <lee@leebyron.com>
Date: 3 February 2016 at 9:33:33 AM SGT

Remove remaining references to u2028 and u2029
Parents: 1651039cf4
Author: Lee Byron <lee@leebyron.com>
Date: 21 January 2016 at 3:10:44 PM SGT

flow type the parser

Note: Added check for invalid operation token (valid tokens: `mutation`, `subscription`, `query`)

----

Commit: 58965620674a0245a0cc4b7ef190a450c04753cd [5896562]
Parents: 9234c6da0e
Author: Dmitry Minkovsky <dminkovsky@gmail.com>
Date: 17 February 2016 at 12:38:37 AM SGT
Commit Date: 17 February 2016 at 6:52:00 AM SGT

Fix comment
Parents: 529257b8d3
Author: Lee Byron <lee@leebyron.com>
Date: 2 October 2015 at 9:10:21 AM SGT

[RFC] Type condition optional on inline fragments.

Implements graphql/graphql-spec#100
Parents: 2cfbfcec58
Author: Lee Byron <lee@leebyron.com>
Date: 2 October 2015 at 6:37:42 AM SGT
Commit Date: 2 October 2015 at 6:44:20 AM SGT

[RFC] Make operation name optional.

Implements graphql/graphql-spec#99
9046c14d135e7e0785b6a43cd0e0ceef7e8773b4 [9046c14]
Parents:
657fbbba26
Author:
Lee Byron <lee@leebyron.com>
Date:
24 September 2015 at 7:49:45 AM SGT

[RFC] Move input field uniqueness validator

This proposes moving input field uniqueness assertion from the parser to the validator. This simplifies the parser and allows these errors to be reported as part of the collection of validation errors which is actually more valuable.

A follow-up RFC against the spec will be added
85bcbffcaf950b6a6dacae97ee351041f6f406c1 [85bcbff]
Parents:
a941554fc5
Author:
Greg Hurrell <greg@hurrell.net>
Date:
3 October 2015 at 8:59:25 AM SGT

Declare total war on misuse of "it's"

Also fixed one use of "descendents" that happened to be on one of the
touched lines.
6741c3192d0c0d5a1f6a9185adcf083b01699935 [6741c31]
Parents:
72e90c0db1
Author:
Hyohyeon Jeong <asiandrummer@gmail.com>
Date:
9 February 2016 at 3:16:33 AM SGT
Committer:
Lee Byron <lee@leebyron.com>
Commit Date:
9 February 2016 at 4:15:57 PM SGT

Provides a correct OperationType without name in GraphQLPrinter
This replaces the mechanism of returning errors or lists of errors from a validator step to instead report those errors via calling a function on the context.

This simplifies the implementation of the visitor mechanism, but also opens the doors for future rules being specified as warnings instead of errors.

Commit:
5e545cce0104708a4ac6e994dd5f837d1d30a09b [5e545cc]
Parents:
ef7c755c58
Author:
Lee Byron <lee@leebyron.com>
Date:
17 November 2015 at 11:32:13 AM SGT
This provides a performance improvement and simplification to the validator by providing two new generic visitor utilities. One for tracking a TypeInfo instance alongside a visitor instance, and another for stepping through multiple visitors in parallel. The two can be composed together.

Rather than 23 passes of AST visitation with one rule each, this now performs one pass of AST visitation with 23 rules. Since visitation is costly but rules are inexpensive, this nets out to a much faster overall validation, especially noticable for very large queries.

Commit:
957704188b0a103c5f2fe0ab99479267d5d1ae43 [9577041]
Parents:
439a3e2f4f
Author:
Lee Byron <lee@leebyron.com>
Date:
17 November 2015 at 12:33:54 PM SGT

----

[Validation] Memoize collecting variable usage.

During multiple validation passes we need to know about variable usage within a de-fragmented operation. Memoizing this ensures each pass is O(N) - each fragment is no longer visited per operation, but once total.

In doing so, `visitSpreadFragments` is no longer used, which will be cleaned up in a later PR

Commit:
2afbff79bfd2b89f03ca7913577556b73980f974 [2afbff7]
Parents:
88acc01b99
Author:
Lee Byron <lee@leebyron.com>
Date:
17 November 2015 at 9:54:30 AM SGT
Fixes graphql-go#254

Commit:
699dc10eeea70a8a341c68ea1a34dc0f2f8a6906 [699dc10]
Parents:
3a6d35b59f
Author:
Lee Byron <lee@leebyron.com>
Date:
1 December 2015 at 11:31:52 AM SGT
Commit Date:
1 December 2015 at 11:31:54 AM SGT
Commit:
b2bdae08cfcb1dd7ee78fff156bcdfbf09b2cd47 [b2bdae0]
Parents:
699dc10eee
Author:
Lee Byron <lee@leebyron.com>
Date:
1 December 2015 at 11:46:35 AM SGT
Commit Date:
1 December 2015 at 11:53:03 AM SGT
Labels:
HEAD

-----

Updated `visitor.VisitInParallel` to accept variadic arg
Commit:
bd87fd312061a2cedcf590d0eca5a56ab26c1b5c [bd87fd3]
Parents:
b2bdae08cf
Author:
Lee Byron <lee@leebyron.com>
Date:
1 December 2015 at 12:09:47 PM SGT
Tests for visitWithTypeInfo, support for editing
Commit:
32a54926803ee6b353ffa26c87dfc2c846d4c4f7 [32a5492]
Parents:
bd87fd3120
Author:
Lee Byron <lee@leebyron.com>
Date:
1 December 2015 at 12:53:27 PM SGT
Commit Date:
1 December 2015 at 12:58:22 PM SGT
Commit:
f79ba42e30d9f1380f378440e4fe66b1aa428386 [f79ba42]
Parents:
4256230557
Author:
Dmitry Minkovsky <dminkovsky@gmail.com>
Date:
21 February 2016 at 6:03:39 AM SGT
Commit Date:
21 February 2016 at 12:06:22 PM SGT

---

Tests to demonstrate problem

Commit:
42562305570b04a5b7ec1ee7714f020f366023c9 [4256230]
Parents:
75e7be0028
Author:
Dmitry Minkovsky <dminkovsky@gmail.com>
Date:
21 February 2016 at 5:40:46 AM SGT
Commit Date:
21 February 2016 at 12:06:07 PM SGT

---

Note: See PR graphql-go#298 for context: graphql/graphql-js#298
Commit:
da5c4b0814887382067dcaeaddd6fed8a76a3614 [da5c4b0]
Parents:
160c67a74e
Author:
Lee Byron <lee@leebyron.com>
Date:
29 September 2015 at 5:13:20 AM SGT
Commit:
87c6a274fb24e08a2c85a93a9294e5711eed874b [87c6a27]
Parents:
a0c30d3475
Author:
Adam Miskiewicz <adam.skevy@mac.com>
Date:
17 October 2015 at 4:55:00 AM SGT
Commit:
8a589f6e88ecf675ce34d39d813a7b271dae46aa [8a589f6]
Parents:
3bc93a7440
Author:
Adam Miskiewicz <adam.skevy@mac.com>
Date:
7 September 2015 at 5:32:29 AM SGT
Labels:
HEAD
Commit:
87c6a274fb24e08a2c85a93a9294e5711eed874b [87c6a27]
Parents:
a0c30d3475
Author:
Adam Miskiewicz <adam.skevy@mac.com>
Date:
17 October 2015 at 4:55:00 AM SGT
Commit:
d2c005afc87353eceadc03592e74bd6e44063e8e [d2c005a]
Parents:
f79ba42e30
Author:
Lee Byron <lee@leebyron.com>
Date:
23 February 2016 at 8:08:25 AM SGT
sogko added 19 commits April 12, 2016 08:40
(Note: already ported previously, refactored error message)

During multiple validation passes we need to know about variable usage within a de-fragmented operation. Memoizing this ensures each pass is O(N) - each fragment is no longer visited per operation, but once total.

In doing so, `visitSpreadFragments` is no longer used, which will be cleaned up in a later PR

Commit:
2afbff79bfd2b89f03ca7913577556b73980f974 [2afbff7]
Parents:
88acc01b99
Author:
Lee Byron <lee@leebyron.com>
Date:
17 November 2015 at 9:54:30 AM SGT
Commit:
a921397 [a921397]
Parents:
Commit:
1b639e3a6538d2184e1a2b96c410d164a20c08ed [1b639e3]
Parents:
71b7b4aa2d
Author:
Lee Byron <lee@leebyron.com>
Date:
2 February 2016 at 11:07:08 AM SGT
When deep field collisions are detected, the existing validator interleaved the resulting fields in a way that made reading the error more difficult. This solves the issue by internally tracking two parallel lists of blame nodes, one for each side of the comparison, and concat-ing only at the end to ensure a stable non-interleaved output.

Commit:
228215a704e9d7f67078fc2652eafa6b6e22026f [228215a]
Parents:
fee4fe322f
Author:
Lee Byron <lee@leebyron.com>
Date:
13 November 2015 at 8:24:24 AM SGT
As pointed out in graphql-go#53, our validation is more conservative than it needs to be in some cases. Particularly, two fields which could not overlap are still treated as a potential conflict.

This change loosens this rule, allowing fields which can never both apply in a frame of execution to diverge.

Commit:
d71e063fdd1d4c376b4948147e54438b6f1e13de [d71e063]
Parents:
228215a704
Author:
Lee Byron <lee@leebyron.com>
Date:
13 November 2015 at 9:39:54 AM SGT
Commit Date:
13 November 2015 at 10:01:11 AM SGT
The currently supported directives: @Skip and @include do not create an ambiguous or erroneous query when used on conflicting fields in a divergent fashion. In fact, this may be intended when many different conditions should result in the same outcome. For example:

```graphql
{
  field @include(if: $firstCondition)
  field @include(if: $secondCondition)
}
```

This example could be considered as the intent of fetching `field` given `$firstCondition || $secondCondition`. While this example is contrived, there are more complex examples where such fields are nested within fragments that this condition is reasonable.

Commit:
9b11df2efc66ad3c07e0d373a7e04a3ba5ee581a [9b11df2]
Parents:
f010e86c3d
Author:
Lee Byron <lee@leebyron.com>
Date:
13 November 2015 at 11:48:47 AM SGT
Commit:
657fbbba26ed1fa18915b2a9f050a5fae08a7469 [657fbbb]
Parents:
9cedbc3ffe
Author:
Lee Byron <lee@leebyron.com>
Date:
24 September 2015 at 7:39:39 AM SGT
…type

Commit:
1d14db78b8e38d6cb5b0698dadc774ed794f7398 [1d14db7]
Parents:
81e759620b
Author:
Lee Byron <lee@leebyron.com>
Date:
17 November 2015 at 12:56:43 PM SGT
Commit:
e81cf39750e8c2dbde7a7910e13893aa644e02dd [e81cf39]
Parents:
1d14db78b8
Author:
Lee Byron <lee@leebyron.com>
Date:
17 November 2015 at 1:00:16 PM SGT
Commit:
71b7b4aa2d0c82c71efb9c941dbd93e65e93a21c [71b7b4a]
Parents:
3eb5b7aa57
Author:
Lee Byron <lee@leebyron.com>
Date:
2 February 2016 at 9:43:02 AM SGT
Fairly dramatic improvement of some validation rules by short-circuiting branches which do not need to be checked and memoizing the process of collecting fragment spreads from a given context - which is necessary by multiple rules.

Commit:
0bc9088187b9902ab19c0ec34e0e9f036dc9d9ea [0bc9088]
Parents:
7278b7c92c
Author:
Lee Byron <lee@leebyron.com>
Date:
13 November 2015 at 4:26:05 PM SGT
…/freiksenet/graphql-js into freiksenet-better-error-messages-for-inputs

Commit:
284415e00e905e9d3ff24ad931d7aa9ea849a312 [284415e]
Parents:
2f10ad44e7, ab13f32c1c
Author:
Lee Byron <lee@leebyron.com>
Date:
18 November 2015 at 3:48:47 AM SGT
There were a few places in the validation rules that were not using the
constants found in `src/language/kinds.js`. This commit replaces those strings
with their respective constants.

This can be considered a cleanup.
Commit:
17998532216b520a899a196f54bcb59c1faa512a [1799853]
Parents:
662e316f04
Author:
Brandon Wood <woodb@users.noreply.github.com>
Date:
17 September 2015 at 9:52:56 AM SGT
This adds a new method to the validation context which when given an Operation definition, returns a list of all Fragment definitions recursively referenced via fragment spreads.

This new method is then used to ensure no fragments are unused.

The implementation of this method is the same in principle as the one which used to be inline in the validation rule, but has been unfolded from recursion to use a while loop.
Commit:
eef8d97f64b5b5fa0df79435c4fe237976867573 [eef8d97]
Parents:
568dc52a4f
Author:
Lee Byron <lee@leebyron.com>
Date:
17 November 2015 at 9:31:46 AM SGT
Commit:
f010e86c3d3057dee36d6096ed2600492c41f4e7 [f010e86]
Parents:
6a3eac753d
Author:
Lee Byron <lee@leebyron.com>
Date:
13 November 2015 at 10:11:42 AM SGT
Fairly dramatic improvement of some validation rules by short-circuiting branches which do not need to be checked and memoizing the process of collecting fragment spreads from a given context - which is necessary by multiple rules.

Commit:
0bc9088187b9902ab19c0ec34e0e9f036dc9d9ea [0bc9088]
Parents:
7278b7c92c
Author:
Lee Byron <lee@leebyron.com>
Date:
13 November 2015 at 4:26:05 PM SGT
This replaces the mechanism of returning errors or lists of errors from a validator step to instead report those errors via calling a function on the context.

This simplifies the implementation of the visitor mechanism, but also opens the doors for future rules being specified as warnings instead of errors.

Commit:
5e545cce0104708a4ac6e994dd5f837d1d30a09b [5e545cc]
Parents:
ef7c755c58
Author:
Lee Byron <lee@leebyron.com>
Date:
17 November 2015 at 11:32:13 AM SGT
- The ones left are "uncommented exported func/var/methods etc"
@coveralls
Copy link

Coverage Status

Coverage increased (+3.2%) to 90.754% when pulling ee2f8c6 on sogko:sogko/0.4.18 into 909ca9f on graphql-go:master.

@sogko sogko changed the title [WIP] Port changes from graphql-js v0.4.18 (Oct 2015 spec) Port changes from graphql-js v0.4.18 (Oct 2015 spec) Apr 18, 2016
@pspeter3
Copy link

pspeter3 commented May 3, 2016

What is stopping this from being merged?

@chris-ramon
Copy link
Member

What is stopping this from being merged?

Reviews! - I've being reviewing it for a while, did not finish yet; but would be awesome to have some feedback from others as well! 👍 hopefully be landing on master in the upcoming days. Thanks for caring about this tho @pspeter3!

// serialize(value) {
// return value % 2 === 1 ? value : null;
// }
// });
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These examples would be a lot more useful if they were written in Go.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jvatic That's a great idea 👍

Should we address this is in a separate PR? It would be great to get help with refining the documentation (currently golint complains that there are too many exported variables/methods/structs etc that are missing comments)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, a separate PR makes sense.

- removed commented out unused variables
@coveralls
Copy link

Coverage Status

Coverage increased (+3.2%) to 90.751% when pulling c4394ae on sogko:sogko/0.4.18 into 909ca9f on graphql-go:master.

@sogko sogko merged commit a5cf5f2 into graphql-go:master May 30, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants