-
Notifications
You must be signed in to change notification settings - Fork 22
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
Schema and statement validation prior to execution #40
Conversation
…t of a mandatory validation phase
I think I understand the issue - a query failing validation (because of this subscriptionType issue) is returning more than just the errors key, including things that don't serialize to JSON. I'll patch this. |
There were actually additional bugs hidden by a bug in the fragment cycles validator which was inappropriately short-circuiting the DFS. I've fixed these bugs and added a couple missing unit tests for things that were breaking the Graphiql starter project. |
Just had simple test in GraphiQL, looks like it work fine now. Thanks a lot for your great work. The validation really makes huge difference on usability of the library. It helps provide much better error messages. |
Do you have more commits coming? If not, I might release a new version tonight. |
I'm not planning to commit any more unless we discover more bugs, so feel free to release a new version. Thanks! |
Found a new issue in inspector: https://github.com/tendant/graphql-clj-starter/tree/testing-validation-loc Error result:
|
Ok - taking a look at this. Can you clarify which query you used to produce this error? |
It came from introspection query. Just open graphiql, the error will show up in graphql result. |
Problem might relate to reloading of schema. Looks like after updating schema in dev environment. Introspection query returns:
When try to run any query after changing schema in dev server, I got below error for all root query fields:
|
BTW: I tried to use validator/validate-schema* function, it still has the same issue. |
(let [resolver (resolver/create-resolver-fn (:schema (:state validated-statement)) resolver-fn)] | ||
(assoc-in validated-statement [:state :resolver] resolver))))))) | ||
|
||
(def prepare (memoize prepare*)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is better not to use memoize. User can choose to use memoize if needed, they can also choose how to memoize.
Although memoize will get higher performance at the expense of higher memory use. However it is better leave it to user to decide whether use it or not.
Another good thing about leaving this to the user is that user can choose to use memoize if they wish, or they can use enhanced version of memoize with customized caching policy.
(graphql-clj.type/create-type-meta-fn graphql-clj.type/demo-schema)) | ||
(let [type-schema (type/create-schema graphql-clj.introspection/introspection-schema)] | ||
(execute nil type-schema nil "query{__schema{types {name kind}}}"))) | ||
([validated-document] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For public api of execute, we should try to keep it to the bare minimal. User can create they own version of execute if needed.
It is easier to support only one execute function and maintain backward compatibility in the long run.
The problem goes away when all memoize function calls are removed. Also, I would like to propose to expose execute function to be as simple as we can:
User can cache validated schema, validated statement and resolver-fn as they wish. |
Another issue with the validated schema and statement: validation result is more than 10x bigger than parsing result. Below are size comparison for schema "graphql-clj-starter.graphql/starter-schema"
Below are size comparison for statement "{droid}"
Looks like the validated data contains the schema in addition to the result. |
(filter #(= (:type-name %) root-name)) | ||
first | ||
:fields | ||
(map (juxt (comp box/box->val :field-name) (comp box/box->val :type-name))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug here: Root fields return nil as field type, when root field is using composite type like NOT_NULL, LIST etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Root field type can't be expressed use string value of type-name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed this for list types. I think non-null types are working (there is a test for this with loremIpsum of type String!
) but I may be missing something.
(if (or recursive? (not (meta d))) | ||
(do | ||
(assert (= (first d) 'clojure.spec/def)) ;; Protect against unexpected statement eval | ||
(try (eval d) (catch Compiler$CompilerException _ d))) ;; Squashing errors here to provide better error messages in validation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(assert (= (first d) 'clojure.spec/def))
does not prevent against all unexpected statement eval. The best way is not to use eval at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't think of another way to dynamically generate these specs. We could avoid using spec - we could use prismatic schema instead or something like that. Other suggestions?
Sounds good re: removing memoize and the single arity for the execute function - I'll make those changes. Re: size of the validation output - I'm waiting to see how validation output interacts with a simplified execution phase to understand exactly what we need to keep, then we can remove everything else and it will be small. |
I've made the changes discussed above - @tendant worth taking another look. For additional context, there are a lot of outstanding issues that concern me, but I don't know how many of them we want to fix in a single PR:
Given all of this, we could either merge this and hope for the best, or remove validation as a requirement for the execution phase, and perhaps add a different execution function for previously "prepared" inputs. WDYT? |
I will test it and merge it, if there is no major issue. As long as we keep the public API simple and consistent, it is not too hard to change the internal implementation if needed. |
Merged. Thanks you @aew for your great contribution! |
Fixes a number of outstanding bugs and incompatibilities between validation output and execution input. Backwards compatible.
Changes include:
graphql-clj.type/create-schema
in favor ofgraphql-clj.validator/validate-schema
This is a large change because the parser was eliminating line and column metadata necessary for producing validation error messages. The executor was coupled to parser output rather than validator output. This PR proposes that the executor take the validator output as an input rather than the parser output.
Related discussion: #22