Repository: hasura/graphql-engine
File: server/tests-py/test_v1_queries.py (Python)

Code before:
    def test_export_replace(self, hge_ctx):
        url = '/v1/query'
        export_query = {
            'type': 'export_metadata',
            'args': {}
        }
        headers = {}
        if hge_ctx.hge_key is not None:
            headers['X-Hasura-Admin-Secret'] = hge_ctx.hge_key
        # we are exporting the metadata here after creating it though
        # the metadata APIs
        export_code, export_resp, _ = hge_ctx.anyq(url, export_query, headers)
        assert export_code == 200, export_resp
        replace_query = {
            'type': 'replace_metadata',
            'args': export_resp
        }
        # we are replacing the metadata with the exported metadata from the
        # `export_metadata` response.
        replace_code, replace_resp, _ = hge_ctx.anyq(url, replace_query, headers)
        assert replace_code == 200, replace_resp
        # we are again exporting the metadata to assert that the metadata exported
        # after creating the metadata through the metadata APIs and the metadata
        # exported after importing the metdata using the `replace_metadata` to be
        # the same. Doing this check, ensures that the metadata has been properly
        # imported by the graphql-engine.
        export_code_1, export_resp_1, _ = hge_ctx.anyq(url, export_query, headers)
        assert export_code_1 == 200
        assert export_resp == export_resp_1

@usefixtures('per_method_tests_db_state')
class TestRunSQL:

    def test_select_query(self, hge_ctx):
        check_query_f(hge_ctx, self.dir() + '/sql_select_query.yaml')
        hge_ctx.may_skip_test_teardown = True

    def test_select_query_read_only(self, hge_ctx):
        check_query_f(hge_ctx, self.dir() + '/sql_select_query_read_only.yaml')
        hge_ctx.may_skip_test_teardown = True

    def test_set_timezone(self, hge_ctx):
        check_query_f(hge_ctx, self.dir() + '/sql_set_timezone.yaml')
        hge_ctx.may_skip_test_teardown = True

    def test_sql_timezone_error(self, hge_ctx):
        check_query_f(hge_ctx, self.dir() + '/sql_set_timezone_error.yaml')

    def test_sql_query_as_user_error(self, hge_ctx):
        check_query_f(hge_ctx, self.dir() + '/sql_query_as_user_error.yaml')


Review comment (bug):
This is definitely better but if you think about it, this content
> we are again exporting the metadata to assert that the metadata exported
        after creating the metadata through the metadata APIs and the metadata
        exported after importing the metdata using the `replace_metadata` to be
        the same.

can again be inferred by reading the code easily. What is hard to understand is this part:

> Doing this check, ensures that the metadata has been properly
        imported by the graphql-engine

I spent a good few minutes trying to understand how this catches the current bug at hand, you are exporting, then importing and then exporting again, why would anything change? The 'why' is the most important part that should go into the comment: 

> This test catches incorrect key names in `export_metadata` serialization, for example if `allowlist` is misspelled as `allow_list`, the first export metadata would have `{..., "allow_list":, ...}` but when you import this and export the metadata again, this incorrect key will be absent because `replace_metadata` doesn't import the incorrectly serialized metadata and therefore there is nothing about allowlist that can be exported the second time and hence the two exports will differ. 

Code after:
    def test_export_replace(self, hge_ctx):
        url = '/v1/query'
        export_query = {
            'type': 'export_metadata',
            'args': {}
        }
        headers = {}
        if hge_ctx.hge_key is not None:
            headers['X-Hasura-Admin-Secret'] = hge_ctx.hge_key
        # we are exporting the metadata here after creating it though
        # the metadata APIs
        export_code, export_resp, _ = hge_ctx.anyq(url, export_query, headers)
        assert export_code == 200, export_resp
        replace_query = {
            'type': 'replace_metadata',
            'args': export_resp
        }
        # we are replacing the metadata with the exported metadata from the
        # `export_metadata` response.
        replace_code, replace_resp, _ = hge_ctx.anyq(url, replace_query, headers)
        assert replace_code == 200, replace_resp
        # This test catches incorrect key names(if any) in the export_metadata serialization,
        # for example, A new query collection is added to the allow list using the
        # add_collection_to_allowlist metadata API. When
        # the metadata is exported it will contain the allowlist. Now, when this
        # metadata is imported, if the graphql-engine is expecting a different key
        # like allow_list(instead of allowlist) then the allow list won't be imported.
        # Now, exporting the metadata won't contain the allowlist key
        # because it wasn't imported properly and hence the two exports will differ.
        export_code_1, export_resp_1, _ = hge_ctx.anyq(url, export_query, headers)
        assert export_code_1 == 200
        assert export_resp == export_resp_1

@usefixtures('per_method_tests_db_state')
class TestRunSQL:

    def test_select_query(self, hge_ctx):
        check_query_f(hge_ctx, self.dir() + '/sql_select_query.yaml')
        hge_ctx.may_skip_test_teardown = True

    def test_select_query_read_only(self, hge_ctx):
        check_query_f(hge_ctx, self.dir() + '/sql_select_query_read_only.yaml')
        hge_ctx.may_skip_test_teardown = True

    def test_set_timezone(self, hge_ctx):
        check_query_f(hge_ctx, self.dir() + '/sql_set_timezone.yaml')
        hge_ctx.may_skip_test_teardown = True

    def test_sql_timezone_error(self, hge_ctx):
        check_query_f(hge_ctx, self.dir() + '/sql_set_timezone_error.yaml')