Closed lucianjon closed 2 years ago
Merging #141 (e801f28) into main (9950bc0) will increase coverage by
0.78%
. The diff coverage is82.45%
.
@@ Coverage Diff @@
## main #141 +/- ##
==========================================
+ Coverage 69.61% 70.40% +0.78%
==========================================
Files 24 24
Lines 2630 2602 -28
==========================================
+ Hits 1831 1832 +1
+ Misses 668 650 -18
+ Partials 131 120 -11
Impacted Files | Coverage Δ | |
---|---|---|
execution.go | 78.63% <79.54%> (+1.75%) |
:arrow_up: |
query_execution.go | 77.00% <92.30%> (+3.07%) |
:arrow_up: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 9950bc0...e801f28. Read the comment docs.
Background
There are problems with
ExecutableSchema.ExecuteQuery
which have been hiding bugs. It's quite a long function that does many things and the fact it's a receiver function onExecutableSchema
means it has access to a lot of mutable state it probably shouldn't. Eventually I'd like to entirely redo this function as I don't like the mutex and using shared state approach. I'd rather each instance of a query execution have access to their own copy of the variables. I think extracting out the core execution pipeline fromExecutableSchema
will make bramble more modular too.Bugs Fixed
One of the first things the function does is filter down the schema based on the permissions in the request context. Unfortunately there were plenty of cases further down that just accessed the schema from
ExecutableSchema.MergedSchema
rather than using the filtered schema, essentially bypassing the authorization.Changes
graphql.GetOperationContext(ctx)
as that will actually panic.formatResponseData
andunionAndTrimSelectionSet
. There was only one legitimate error informatResponseData
and that was from callingjson.Marshal
. Given this data is already guaranteed to be JSON I decided to panic instead.ExecutableSchema
that have no business being on it. There was another bug hiding inresolveSchema
due to this.