fix: Validate GraphQL schemas #3152
Conversation
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3152 +/- ##
===========================================
- Coverage 78.08% 78.00% -0.09%
===========================================
Files 353 354 +1
Lines 34661 34660 -1
===========================================
- Hits 27065 27034 -31
- Misses 5978 6001 +23
- Partials 1618 1625 +7
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 16 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| explainEnum, | ||
|
|
||
| indexFieldInput, | ||
| validation := gql.ValidateDocument(&s.schema, doc, gql.SpecifiedRules) |
There was a problem hiding this comment.
thought: I love the near total lack of documentation in the gql-go lib...
todo: This is quite an odd flow, as we appear to be validating the doc against an s.schema that does not yet contain the contents of doc. Can you please document this call quite heavily.
There was a problem hiding this comment.
I think you have the validation flow backwards Andy.
The ValidateDocument call is to validate that the given SDL document is valid relative to the schema defined up till now (eg, first run only the "default" schema is defined).
This is the correct flow as I understand it. A comment can help but it doesn't seem unusual or out of place to me.
There was a problem hiding this comment.
The ValidateDocument call is to validate that the given SDL document is valid relative to the schema defined up till now (eg, first run only the "default" schema is defined).
That's fine, but need to be documented, particularly given that the SDL is partial atm and does not fully represent the changes that will be made to the GQL-schema once encorporated. There is a lot of GQL stuff that we do/add that will not be validated by this call.
There was a problem hiding this comment.
Agreed, all the generated stuff after this won't get validated, but I don't think there's any implication at this call site that it's doing anything more than validating the user provided SDL, which needs to be valid on its own before continuing on with any future schema generation stuff.
There was a problem hiding this comment.
Yes, I'm totally on board with all of that.
The reason I'm asking for documentation is so that the next person who comes across this line (including our future selves) doesn't need to think terribly hard about why it is done here, and why the premature validation is okay here (and potentially come to the wrong conclusion, wasting even more time and thought).
There was a problem hiding this comment.
I've added documentation that hopefully makes it a bit more clear as to what's happening
There was a problem hiding this comment.
Looks good, thanks Keenan - should help a fair bit :)
There was a problem hiding this comment.
It looks good, but the location of the ValidateDocument looks odd and needs documentation.
thought: You blended a lot of seemingly random refactors into to main change, without documenting them in the PR description or committing them separately - whilst they are nice changes, presenting them to reviewers like this made the review quite a lot harder than it needed to be.
Sorry it was a bit messy. I put this PR together very quickly last night and should have probably just waited to finish it properly. |
There was a problem hiding this comment.
Sorry it was a bit messy. I put this PR together very quickly last night and should have probably just waited to finish it properly.
No worries at all, it was a small PR after all, and I'm plenty guilty of rushing stuff out in the evenings :)
Looks great, and is great to get this in, especially before the release :)
| explainEnum, | ||
|
|
||
| indexFieldInput, | ||
| validation := gql.ValidateDocument(&s.schema, doc, gql.SpecifiedRules) |
There was a problem hiding this comment.
Looks good, thanks Keenan - should help a fair bit :)
Relevant issue(s)
Resolves #3151
Description
This PR fixes an issue where GraphQL schemas were never validated.
Tasks
How has this been tested?
Updated integration tests.
Specify the platform(s) on which this was tested: