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

Working implementation of visitor and printer #10

Merged
merged 48 commits into from
Sep 25, 2015

Conversation

sogko
Copy link
Member

@sogko sogko commented Sep 23, 2015

Built upon PR #8

Details

visitor

  • visitor is used / will be used by printer and validator
  • Ported from graphql-js visit()
  • Walks through an AST using a depth first traversal, calling visitor's Enter function when entering a node and visitor's Leave function when leaving a node
  • Visitor's functions
    • type VisitFunc func(p VisitFuncParams) (action string, result interface{})
    • The value of returned action string defines the behaviour of the visit function upon return
      • const visitor.ActionNoChange
        • No changes to node, regardless of result interface{} returned
      • const visitor.ActionBreak
        • Stop traversal of tree, ignore result interface{} returned
      • const visitor.ActionSkip
        • Skip traversal of sub-tree, ignore result interface{} returned
      • const visitor.ActionUpdate
        • Update the value of current node with value of returned result interface{}. Return result = nil to remove node.
  • See printer's printDocASTReducer for working usage of visitor

printer

  • printer is used to transform/reduce an ast.Node into a string.
  • Uses visitor and pre-defined visitor.VisitorOptions to traverse AST and reduce each node to a string (printDocASTReducer)
  • Inverse of parser (which transforms string into an ast.Node)
// pseudo-code
query := `{ field }`
astDoc := parse(query)
printed := print(astDoc)
assert(query == printed)

Misc

  • Added language/parser/schema_parser_test.go test to complete port of all graphql-js/language/__tests__/* tests

Test

$ go build && go test ./...
ok      github.com/chris-ramon/graphql-go   0.027s
?       github.com/chris-ramon/graphql-go/errors    [no test files]
ok      github.com/chris-ramon/graphql-go/executor  0.097s
?       github.com/chris-ramon/graphql-go/language/ast  [no test files]
?       github.com/chris-ramon/graphql-go/language/kinds    [no test files]
ok      github.com/chris-ramon/graphql-go/language/lexer    0.017s
?       github.com/chris-ramon/graphql-go/language/location [no test files]
ok      github.com/chris-ramon/graphql-go/language/parser   0.034s
ok      github.com/chris-ramon/graphql-go/language/printer  0.042s
?       github.com/chris-ramon/graphql-go/language/source   [no test files]
ok      github.com/chris-ramon/graphql-go/language/visitor  0.016s
?       github.com/chris-ramon/graphql-go/testutil  [no test files]
ok      github.com/chris-ramon/graphql-go/types 0.018s
?       github.com/chris-ramon/graphql-go/validator [no test files]

- Still raw. It does not have a way to resolve fields yet.
- At least `GraphQL()` can run without panicking.
- Added one `executor` test from `graphql-js` while figuring out how to resolve a GraphQLSchema
- Will work on GraphQL types/models first
- Still raw. It does not have a way to resolve fields yet.
- At least `GraphQL()` can run without panicking.
- Added one `executor` test from `graphql-js` while figuring out how to resolve a GraphQLSchema
- Will work on GraphQL types/models first
* master:
  Fixed `graphQLError` to use `graphql-go/language/ast` instead of `go/ast`
  Updated file organization for `ast` packages
…tring)

- Passed `TestBasicGraphQLExample` (hello world)
- `GraphQLInt`
- `GraphQLString`
- `GraphQLFloat`
- `GraphQLBoolean`
- `GraphQLID`
- `GraphQLInt`
- `GraphQLString`
- `GraphQLFloat`
- `GraphQLBoolean`
- `GraphQLID`
…cutor.ExecuteParams.Root: map[string]interface{}`

- Resolve function: `GraphQLFieldResolveFn: func(p GQLFRParams) interface{}`
- Fefault resolveFn currently can automatically resolve fields with values (e.g. `string` etc) and functions with `func() interface{}` signature)
- Currently unable to resolve nested fields
- `types/*.go` is WIP, will need to be cleaned up and ensure that all type definitions are implemented

Notes:
- Golang doesn't quite like variables that refers to itself at initialisation
```go
// initialisation loop error
var __Type = {
	Type: __Type
}
```
  Currently have a temporary workaround `__setTypesFor__Type`, but exploring other options
  to allow user to define `GraphQLObjectTypes` that refers to itself (cyclic ref).
  One option: `GraphQLObjectTypes.AddFieldConfig(fieldName string, fieldConfig *GraphQLFieldConfig)`
…tesArbritraryCode` test

- Requires code clean up
- `types` package is still not completed
- TODO next: port `executor` tests
- Ported 7/16 tests
- Passed 6/7 tests
- Ported 16/16 tests
 - Passed 12/16 tests
 - Failed 3/16
   - TestCorrectlyThreadsArguments
   - TestDoesNotIncludeArgumentsThatWereNotSet
   - TestFailsWhenAnIsTypeOfCheckIsNotMet
 - Skipped 1/16
   - TestCorrectFieldOrderingDespiteExecutionOrder
     - Golang maps are unordered (order is non-deterministic & random)
* fork/master:
  update source.NewSource to receive source pointer
  mark Parser as done 🌟 & add language related todos
- Passed 14/16
- Failed 1/16
- Skipped 1/6
…phql_test.go` tests

- No failing tests (Passed 49, Skipped 1)
- Filled up `StarWarsSchema` implementation
- Updated `graphql_test.go` expected result structure
  - Might need to rethink GraphQLResult? The resultant data structure isn't as concise as JSON or JS Object
  - Maybe can utilise `json/encoding` package
  - Can discuss about re-looking at the `grapql-go` API, to make it much more easier and pleasant to use
    - For e.g.: see `sangria-graphql` (Scala implementation of `graphaql`
- Basic skeleton for `executor` basically done
  - Need to port more tests
  - Need to port more types
- Removed previously added `SetName()` interface, not required
- Re-organized `executor.go` to match `graphql-js` for easy maintenance
- Used `GraphQLObjectType.AddFieldConfig()` instead
  - Ported from `execution/__tests__/executor_schema.js`
- Updated GraphQLResult return by not nesting GraphQLResult within GraphQLResult
  - Flattened GraphQLResult.Data and appended all errors into GraphQLResult.Errors
  - Ported from `execution/__tests__/abstract.js`
  - Ported from `execution/__tests__/directives.js`
  - Fixed a couple of parser and executor bugs
  - Ported from `execution/__tests__/lists.js`
  - Mark 2 tests as `skipped`/TODO
    - `TestListsNonNullListOfNonNullFuncReturnsNull` `TestListsNonNullListOfNonNullObjectsReturnsNull`
]  - Fixed `location.GetLocation()` bug when source is nil
  - Ported from `execution/__tests__/variables.js`
  - Mark 11 tests as `skipped`,
    - waiting to implement `print()` and `visit()`
    - needed to walk through ast tree to print useful error stack
    - other than inaccurate error message, behaviour work as expected
- Added `GetValue() interface{}` method to `ast.Value` interface
  - Will be needed in other parts of the library anyway
  - Implemented `GetValue()` for structs that implement `ast.Value`
  - Ported from `execution/__tests__/nonnull.js`
    - Some tests are marked as `skipped`, pending a test to set equality for errors slice
  - Ported from `execution/__tests__/mutations.js`
  - All passed
  - Updated `defaultResolveFn` to automatically resolve struct fields
     - Will use `json` struct tags to map GraphQLObjectType field with golang Struct field
…ted default ResolverFn

- This commit will be a useful diff to see how to use `json` tags to resolve structs automatically by default
- Note that `blogAuthor` objectType's `pic` field still has to define its own resolve function because the struct field is a function, not a scalar
  - Ported from `execution/__tests__/union-interface.js`
  - All passed
  - Implemented some of the required `resolveFn` in introspective types.

This completes porting all `executor` tests from `graphql-js`
- Ported all 6 tests from `graphql-js`
- 5 pass, 1 skip due to partial pass
- Possibly issue with `parser`. Need to fully port all `parser` tests
- Minor error with the QueryDocumentKeys key map
- All test pass
```go
const (
	ActionNoChange
	ActionBreak
	ActionRemove
	ActionUpdate
)

```
- Passed all `graphql-js` tests

The `visitor` takes in an `ast.Node` and returns `interface{}` which usually is in a form of `map[string]interface{}`

The `printer` uses `visitor` to traverse the AST tree and reduce it into a `graphql` query string.
- Unmarked related tests as `skipped` and passed
  - Ported from `language/__tests__/schema-printer.js`
  - Ported from `language/__tests__/schema-parser.js`
- Removed unused helper functions and re-organized
- But we'll leave `printer` to accept ast.Node only
- Since `visitor` can panic, add a fallback to `printer`. Try to minimise runtime error wherever we can.
- Remove unused structs
- Set `action string` to `ActionNoChange` as default behaviour for visit funds (empty string)
…behaviour is skipping traversal of sub-tree)
@chris-ramon
Copy link
Member

Amazing @sogko!, I am so glad for ur great work, I am continue reviewing the previos pr, looking very good so far, as soon as I am done reviewing I will comment on it, then next I will reviewing this one.

Again thanks for ur amazing work on this lib 🌟.

@sogko
Copy link
Member Author

sogko commented Sep 23, 2015

Thanks for even taking the time to look into this 👍🏻

If you want to feel more comfortable and make some changes to my PRs directly, you can create a separate branch (develop branch for e.g.) and I can re-submit the PRs into that instead of the master branch.

You can then mess around with it without worrying about committing too much to my PRs.

And once you feel comfortable with the state of the develop branch, you can safely merge it back to master for public use.

@chris-ramon
Copy link
Member

Thanks for feedback @sogko 👍 , I think we should keep going forward to merging directly to master.

We have good quantity of test covering lib which give us enough confidence to do so, besides that I am going to add converge with coveralls.io so I think we are fine.

@sogko sogko mentioned this pull request Sep 24, 2015
chris-ramon added a commit that referenced this pull request Sep 25, 2015
Working implementation of `visitor` and `printer`
@chris-ramon chris-ramon merged commit dbf98b3 into graphql-go:master Sep 25, 2015
@sogko sogko deleted the fork/visitor branch October 29, 2015 02:16
lenaten pushed a commit to lenaten/graphql-go that referenced this pull request Nov 2, 2015
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.

2 participants