-
Notifications
You must be signed in to change notification settings - Fork 2.8k
subscriptions can now be explained #2650
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
Conversation
|
Deploy preview for hasura-docs ready! Built with commit cc52320 |
|
Deploy preview for hasura-docs ready! Built with commit fdedb61 |
|
Review app for commit fdedb61 deployed to Heroku: https://hge-ci-pull-2650.herokuapp.com |
lexi-lambda
left a comment
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’ve left a handful of minor questions/comments, but this generally LGTM.
| , MonadIO m | ||
| ) | ||
|
|
||
| -- | to validate arguments |
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 think the syntax Haddock expects for documenting arguments is the -- ^ some description form (which goes under the type rather than above), and in fact I believe doing it this way actually causes Haddock to barf. I personally think it’s a bit silly, but oh well.
| data LiveQueryOp | ||
| = LQMultiplexed !LQM.MxOp | ||
| | LQFallback !LQF.FallbackOp | ||
| data LiveQueryOpG f m |
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.
Can you tell me what the G stands for in these types? “Grammar”? “General”?
|
|
||
| type LiveQueryOpPartial = LiveQueryOpG FallbackOpPartial MultiplexedOpPartial | ||
|
|
||
| -- | Creates a partial live query operation, used in both |
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.
What exactly is a “partial” live query operation—which parts of a “full” live query does it include and which parts does it lack?
| ] | ||
| -- get the variables which don't conifrm to the | ||
| -- 'non-null scalar' rule | ||
| getNonConfirmingVariables usedVariables = |
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.
Should this be “conforming” rather than “confirming”?
| data SubscriptionExplainOutput | ||
| = SubscriptionExplainOutput | ||
| { _seoIsMultiplexable :: !Bool | ||
| -- only exists when subscription is not multiplexed |
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.
Similar to the comment about function arguments above, you can make this comment show up in the haddocks if you put it beneath the field instead of above it and use the -- ^ syntax.
| GR.UVSessVar ty sessVar -> | ||
| return $ fromResVars ty [ "user", T.toLower sessVar] | ||
| GR.UVSQL sqlExp -> return sqlExp | ||
| type FallbackOpPartial = (GR.QueryRootFldUnresolved, Set.HashSet G.Variable) |
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.
Minor, mostly unimportant comment, but: I wonder if there’s a good way to encode into the type system that this HashSet must be non-empty.
|
Integrated into/subsumed by 18e8fba. |
|
Review app https://hge-ci-pull-2650.herokuapp.com is deleted |
Currently the 'Analyze' button only works with queries. This PR adds support for subscriptions which lets one check
Affected components
Related Issues
#2541
Solution and Design
This is part of the current
v1/graphql/explainendpoint. For a subscription the response structure will be as follows:{ "is_multiplexable": true, "reason": null, "sql": "\n select\n _subs.result_id, _fld_resp.root as result\n from\n unnest(\n $1::uuid[], $2::json[]\n ) _subs (result_id, result_vars)\n left outer join lateral\n (\n SELECT coalesce(json_agg(\"root\" ), '[]' ) AS \"root\" FROM (SELECT row_to_json((SELECT \"_1_e\" FROM (SELECT \"_0_root.base\".\"id\" AS \"id\", \"_0_root.base\".\"name\" AS \"name\" ) AS \"_1_e\" ) ) AS \"root\" FROM (SELECT * FROM \"public\".\"artists\" WHERE ((\"public\".\"artists\".\"id\") = (((\"_subs\".\"result_vars\"#>>ARRAY['variables', 'artist_id']))::integer)) ) AS \"_0_root.base\" ) AS \"_2_root\" \n ) _fld_resp ON ('true')\n ", "plan": [ "Nested Loop Left Join (cost=2.40..243.00 rows=100 width=48)", " -> Function Scan on _subs (cost=0.01..1.00 rows=100 width=48)", " -> Aggregate (cost=2.39..2.40 rows=1 width=32)", " -> Index Scan using pk_artists on artists (cost=0.15..2.37 rows=1 width=25)", " Index Cond: (id = ((_subs.result_vars #>> '{variables,artist_id}'::text[]))::integer)", " SubPlan 1", " -> Result (cost=0.00..0.01 rows=1 width=32)" ] }The fields are:
is_multiplexable: Iftruethe subscription can be multiplexed.reason: Why the server thinks that the subscription cannot be multiplexed (nullifis_multiplexedistrue).sql: The sql used in fetching the results for the subscription. In case of multiplexed subscriptions, the query will be different (to accommodate fetching the results of multiple subscriptions in the same query).plan: Postgres'sexplain analyzefor the abovesql.Steps to test and verify
Needs console changes
Limitations, known bugs & workarounds
None