tirumaraiselvan / graphql-engine

Blazing fast, instant realtime GraphQL APIs on Postgres with fine grained access control
https://hasura.io
Apache License 2.0
2 stars 0 forks source link

Remote relationships v2 brandon staging #41

Closed jberryman closed 5 years ago

jberryman commented 5 years ago

Again, hopefully this is helpful, otherwise I can break these commits up into something more granular. It's helpful for me to work through the code and clarify things as I try to understand things.

There are a few TODOs with questions, etc. I think I'll have many more!

jberryman commented 5 years ago

I spent a bunch of time thinking I could factor out another of the parameters in that function, but it's not quite possible... I really want to structure it and document it in a way that's a bit more clear still but I'll circle back later.

jberryman commented 5 years ago

@tirumaraiselvan I spent a bunch of time again with insertBatchResults and I'm still pretty concerned about it. I think I'd like to throw it back to you with the TODO comments . I think one thing I'd really love to see are some good unit tests for it. It doesn't look like there's much test coverage of this module at all in this branch but maybe those exist elsewhere.

EDIT actually it looks like maybe the remote joins tests failed for me, and so probably didn't get to excercise the relevant code (sorry, a bit scattered right now). Is there anything special needed to run those? e.g. getting

    @pytest.fixture(autouse=True)
    def transact(self, hge_ctx, graphql_service):
        print("In setup method")
        graphql_service.start()
        st_code, resp = hge_ctx.v1q_f(self.dir() + 'setup.yaml')
>       assert st_code == 200, resp
E       AssertionError: {'code': 'remote-schema-error', 'error': 'HTTP exception occurred while sending the request to http://localhost:4000',...eout', 'request': {'host': 'localhost', 'method': 'POST', 'path': '/', 'port': '4000', ...}}, 'path': '$.args[3].args'}
E       assert 400 == 200

I probably just need to update my testing script.

I didn't do any moving around of code to new modules yet, that seemed too disruptive for right now. You might suggest Vamshi review everything above rebuildFieldStrippingRemoteRels as well as execRemoteGQ. That represents stuff that has changed (but isn't brand new) in Execute.hs

jberryman commented 5 years ago

Ya it looks like the same test failures happen locally for me on tirumaraiselvan:remote-relationships-v2 as well.

tirumaraiselvan commented 5 years ago

Ya it looks like the same test failures happen locally for me on tirumaraiselvan:remote-relationships-v2 as well.

Yeah, there are few things to be implemented to pass those tests.