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

[WIP] Working draft version of validator and a couple of validation rules #71

Merged
merged 26 commits into from Nov 23, 2015

Conversation

sogko
Copy link
Member

@sogko sogko commented Nov 9, 2015

Related #67

Status: Ready for merge

  • all rules have been ported
    • ArgumentsOfCorrectType.js
    • DefaultValuesOfCorrectType.js
    • FieldsOnCorrectType.js
    • FragmentsOnCompositeTypes.js
    • KnownArgumentNames.js
    • KnownDirectives.js
    • KnownFragmentNames.js
    • KnownTypeNames.js
    • LoneAnonymousOperation.js
    • NoFragmentCycles.js
    • NoUndefinedVariables.js
    • NoUnusedFragments.js
    • NoUnusedVariables.js
    • OverlappingFieldsCanBeMerged.js
    • PossibleFragmentSpreads.js
    • ProvidedNonNullArguments.js
    • ScalarLeafs.js
    • UniqueArgumentNames.js
    • UniqueFragmentNames.js
    • UniqueOperationNames.js
    • VariablesAreInputTypes.js
    • VariablesInAllowedPosition.js
  • all tests have been ported
    • ArgumentsOfCorrectType.js
    • DefaultValuesOfCorrectType.js
    • FieldsOnCorrectType.js
    • FragmentsOnCompositeTypes.js
    • KnownArgumentNames.js
    • KnownDirectives.js
    • KnownFragmentNames.js
    • KnownTypeNames.js
    • LoneAnonymousOperation.js
    • NoFragmentCycles.js
    • NoUndefinedVariables.js
    • NoUnusedFragments.js
    • NoUnusedVariables.js
    • OverlappingFieldsCanBeMerged.js
    • PossibleFragmentSpreads.js
    • ProvidedNonNullArguments.js
    • ScalarLeafs.js
    • UniqueArgumentNames.js
    • UniqueFragmentNames.js
    • UniqueOperationNames.js
    • VariablesAreInputTypes.js
    • VariablesInAllowedPosition.js
  • maintain or improve coverage

Notes

  • Ported a portion of tests to verify validator is working at some level.
  • Updated visitor implementation.
    • Proved to be tricky because current implementation allows user to edit the tree in-memory
    • for e.g. printer reduces the ast.Node into a string
    • Need thorough clean up but working at the moment.
  • Rules have to exists under graphql, due it being tightly-coupled.
    • Total number of rules: 22
    • Currently planned to write all rules in rules.go.
    • Else, split it up to many files
    • But tests are filed under rules/ folder.
  • Should be relatively straight forward to implement rules and its tests since now we have something to work on.

- Ported a portion of tests to verify validator is working at some level.
- Updated `visitor` implementation.
  - Proved to be tricky because current implementation allows user to edit the tree in-memory
  - for e.g. `printer` reduces the `ast.Node` into a string
  - Need thorough clean up but working at the moment.
- Rules have to exists under `graphql`, due it being tightly-coupled.
  - Total number of rules: 22
  - Currently planned to write all rules in `rules.go`.
  - Else, split it up to many files
  - But tests are filed under `rules/` folder.
- TODO:
  - Complete port of tests for `ArgumentsOfCorrectTypeRule` and `KnownTypeNamesRule`
  - Implement other validation rules.
    - Should be relatively straight forward since now we have examples on how to implement it.
- Add an empty package
- All tests passed
- Update `ParseLiteral` funcs for built-in `scalars` to return nil for incorrect types.
  - Will be used by `validator` to raise errors arg values of incorrect types

```
ok  	github.com/graphql-go/graphql	0.195s
?   	github.com/graphql-go/graphql/examples/hello-world	[no test files]
?   	github.com/graphql-go/graphql/examples/http	[no test files]
?   	github.com/graphql-go/graphql/gqlerrors	[no test files]
?   	github.com/graphql-go/graphql/language/ast	[no test files]
?   	github.com/graphql-go/graphql/language/kinds	[no test files]
ok  	github.com/graphql-go/graphql/language/lexer	0.012s
?   	github.com/graphql-go/graphql/language/location	[no test files]
ok  	github.com/graphql-go/graphql/language/parser	0.031s
ok  	github.com/graphql-go/graphql/language/printer	0.081s
?   	github.com/graphql-go/graphql/language/source	[no test files]
ok  	github.com/graphql-go/graphql/language/visitor	0.014s
ok  	github.com/graphql-go/graphql/rules	0.080s
ok  	github.com/graphql-go/graphql/testutil	0.016s
```
@sogko sogko self-assigned this Nov 10, 2015
@chris-ramon
Copy link
Member

Awesome! 🌟 ... thanks for working on the validator @sogko!

Pretty need, I've being trying-out the lib in a small demo project and sometimes my graphql operations are syntactically correct but semantically incorrect, not valid one against my previously defined schema, this is def going to give feedback about invalid operations against schemas.

Merge latest changes from HEAD
* sogko/master:
  re-export name & description
  Remove Get prefix from Type methods
  gqlerrors.GQLFormattedErrorSlice -> gqlerrors.FormattedErrors
  graphql.GQLFRParams -> graphql.ResolveParams
  graphql.FieldConfig -> graphql.Field
  graphql.FieldConfigMap -> graphql.Fields
  graphql.Graphql -> graphql.Do
  Fix declared but not used errors
- Go's default coverage tool won't cover tests in a different folder.
- Need better organization for tests in root folder
- Re-checked the number of tests against `graphql-js` to ensure that all tests have been ported
@sogko
Copy link
Member Author

sogko commented Nov 18, 2015

Completed working version of the validator

  • All rules are consolidated into rules.go.
  • Tests are separated into multiple files in root folder
    • for e.g. rules_arguments_of_correct_type_test.go
    • Experimented with having a rule tests in its separate folder but coverage tool won't give an accurate report.
    • Possible alternative is to consolidate all rule tests in rules_test.go but downside is that it might be hard to maintain one huge file. (~3500 LOC).

/cc @chris-ramon

@chris-ramon chris-ramon mentioned this pull request Nov 23, 2015
2 tasks
@chris-ramon
Copy link
Member

Thanks a lot @sogko! 🌟

Looks good to me! 👍 ... merging this one and handling minor fixes on #78

chris-ramon added a commit that referenced this pull request Nov 23, 2015
[WIP] Working draft version of `validator` and a couple of `validation rules`
@chris-ramon chris-ramon merged commit d7593b6 into graphql-go:master Nov 23, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants