Deploy preview for hasura-docs ready!

Built with commit 99c0bde

https://deploy-preview-3344--hasura-docs.netlify.com

Review app for commit 7bfdb15 deployed to Heroku: https://hge-ci-pull-3344.herokuapp.com
Docker image for server: hasura/graphql-engine:pull3344-7bfdb156

Review app for commit 347d65e deployed to Heroku: https://hge-ci-pull-3344.herokuapp.com
Docker image for server: hasura/graphql-engine:pull3344-347d65e6

Review app for commit dbbc3a0 deployed to Heroku: https://hge-ci-pull-3344.herokuapp.com
Docker image for server: hasura/graphql-engine:pull3344-dbbc3a02

One of the reasons it’s easy for these bugs to happen is that our test coverage of subscriptions is fairly poor. It would be silly to duplicate all of our existing query tests just to test subscriptions (and because it’s an unreasonable amount of effort, those tests never get written). Therefore, this PR adds a new transport option to the pytest test suite that allows query tests to be executed as if they were subscriptions.

This makes sense. Thank you.

One solution to this dilemma would be to hide our noncompliant support for multiple root fields in subscriptions behind a command-line flag that we can enable during testing. I think that’s a good compromise, but I’ll let other people weigh in before I make that call.

I think we should hide this extra power/non-compliance behind a flag so that our users don't rely on this feature.

Regarding the query generation logic for supporting multiple fields, I wanted to check Postgres's plan for these two statements:

select json_build_object('alias1', (query1), 'alias2', (query2))

vs the current implementation in the PR

select json_build_object('alias1', q1.root, 'alias2', q2.root)
from query1 as q1, query2 as q2

For example in this subscription:

subscription s {
  actors {
    id
  }
  artists {
    id
  }
}

Postgres's plans are the following:

 Result  (cost=15.98..15.99 rows=1 width=32) (actual time=1.903..1.904 rows=1 loops=1)
   InitPlan 2 (returns $1)
     ->  Aggregate  (cost=7.08..7.09 rows=1 width=32) (actual time=1.083..1.083 rows=1 loops=1)
           ->  Seq Scan on actors  (cost=0.00..4.03 rows=203 width=4) (actual time=0.041..0.130 rows=203 loops=1)
           SubPlan 1
             ->  Result  (cost=0.00..0.01 rows=1 width=32) (actual time=0.001..0.001 rows=1 loops=203)
   InitPlan 4 (returns $3)
     ->  Aggregate  (cost=8.88..8.89 rows=1 width=32) (actual time=0.797..0.797 rows=1 loops=1)
           ->  Seq Scan on artists  (cost=0.00..4.75 rows=275 width=4) (actual time=0.014..0.077 rows=275 loops=1)
           SubPlan 3
             ->  Result  (cost=0.00..0.01 rows=1 width=32) (actual time=0.000..0.001 rows=1 loops=275)

 Nested Loop  (cost=15.96..16.01 rows=1 width=32) (actual time=0.967..0.969 rows=1 loops=1)
   ->  Aggregate  (cost=7.08..7.09 rows=1 width=32) (actual time=0.420..0.420 rows=1 loops=1)
         ->  Seq Scan on actors  (cost=0.00..4.03 rows=203 width=4) (actual time=0.014..0.046 rows=203 loops=1)
         SubPlan 1
           ->  Result  (cost=0.00..0.01 rows=1 width=32) (actual time=0.000..0.000 rows=1 loops=203)
   ->  Aggregate  (cost=8.88..8.89 rows=1 width=32) (actual time=0.537..0.537 rows=1 loops=1)
         ->  Seq Scan on artists  (cost=0.00..4.75 rows=275 width=4) (actual time=0.008..0.050 rows=275 loops=1)
         SubPlan 2
           ->  Result  (cost=0.00..0.01 rows=1 width=32) (actual time=0.000..0.000 rows=1 loops=275)

From what I could briefly experiment, I couldn't see any practical difference, even though the second plan has a 'Nested Loop' in its plan as from query1 q1, query2 q2 would imply a join on q1 and q2 (which is not needed as both are independent) but given that q1 and q2 both have a single row, it doesn't seem to affect the execution time. The first form select json_build_object('alias1', (query1), 'alias2', (query2)) accurately represents what we want but since there is no practical difference between these two, I guess we could go with either.

Hello, is there any news?

The comment I wrote in #3239 (comment) still applies. As I said there:

It is on our backlog, so I assure we have not forgotten about it! But if anyone is interested in getting the fix in faster, by all means, feel free to pick up from where #3344 left off (and I can give pointers to anyone interested).

Volunteers welcome.

#3344

Would love whatever pointers you have so we can see if we can help get this in faster, @lexi-lambda

Hi, @24601. If you want to take this up yourself, I should warn you that it will involve writing a little Haskell. 🙂 If that doesn’t daunt you, you should start with the server CONTRIBUTING.md file, which describes how to get a build environment set up.

So Haskell and getting the build env set up isn't the part that I'm worried about inasmuch as what actual Hasura-codebase and process specific items there are and getting my head around them so I can decide if the time & risk is worth it for our devs and/or me to take this on.

We really want and need it but the workaround is currently to just use watchQuery on the client side in apollo, so it's not a sev 1, but it's definitely suboptimal. I need to be able to weigh the pain of that (and maintaining workarounds like this in our client code as well as the inefficiency on the client and server load side of constant watchQuery polls) vs the work items left here.

I can assign a cost/sizing to getting some Haskell know-how (either my own or a dev's) on the project and some time to getting a build env set up, can you shed some light on what tasks remain on the code side and getting the PR to the point that it would acceptable to the reviewers to actually merge so I can make this cost-benefit decision with those Hasura-specific code & process items as a little bit less of a blackbox? Right now I'd just be guessing and it's feels like a rabbit hole I'd be throwing a hapless dev into, but if I have an idea of how deep it might go because someone with a bit more insight can at least tell me the higher level task list, I could do this with a bit more confidence.

If that just isn't knowable yet, I totally get it. But it sounds like from this thread we know what needs to be done, it's just about assigning the time & energy to it.

Hi, @24601. If you want to take this up yourself, I should warn you that it will involve writing a little Haskell. 🙂 If that doesn’t daunt you, you should start with the server CONTRIBUTING.md file, which describes how to get a build environment set up.

@24601 We'll start working on this today. Unfortunately it has longer than expected to get to this issue.

Notes for @abooij :
The PR description should give you an idea on the status of the issue. We've introduced supporting multiple fields at the top level in subscriptions (the spec restricts it to one) but this shouldn't be exposed to our users. So maybe we can have a 'internal' flag (i.e, we provide no guarantee on the stability of this flag to our users) which will enable this behaviour for our tests.

@0x777 - Thanks, I fully understand how that goes! And we’re more than happy to contribute and be active participants in the community, just want to have some understanding of where and how we get involved so our contributions are in areas that benefit others and ourselves mutually the most. If I had to choose between doing this or getting a to_tsquery and @@ operator implemented natively without having to resort to actions or exposing functions so it’s a first-order citizen, I know which I’d decide to put our development resources towards.

Thanks for keeping this in-mind, we’re willing to help however we can.

@24601 We'll start working on this today. Unfortunately it has longer than expected to get to this issue.

Notes for @abooij :
The PR description should give you an idea on the status of the issue. We've introduced supporting multiple fields at the top level in subscriptions (the spec restricts it to one) but this shouldn't be exposed to our users. So maybe we can have a 'internal' flag (i.e, we provide no guarantee on the stability of this flag to our users) which will enable this behaviour for our tests.

Superseded by #4551

Review app https://hge-ci-pull-3344.herokuapp.com is deleted