From efec0bf9caf19853456617812cc2abf627a80699 Mon Sep 17 00:00:00 2001 From: Gil Mizrahi Date: Tue, 8 Feb 2022 19:39:17 +0200 Subject: [PATCH] server/postgres: LIMIT 1 on object relationships PR-URL: https://github.com/hasura/graphql-engine-mono/pull/3489 Co-authored-by: Abby Sassel <3883855+sassela@users.noreply.github.com> GitOrigin-RevId: f09a75508545cdbc34cf8728fad95bbb00bde018 --- .circleci/server-upgrade-downgrade/run.sh | 4 + CHANGELOG.md | 1 + server/graphql-engine.cabal | 1 + .../Backends/Postgres/Translate/Types.hs | 13 +- .../Test/ObjectRelationshipsLimitSpec.hs | 325 ++++++++++++++++++ ...mit_offset_orderby_relationship_query.yaml | 2 +- .../limit_orderby_relationship_query.yaml | 2 +- server/tests-py/test_graphql_queries.py | 8 +- 8 files changed, 351 insertions(+), 5 deletions(-) create mode 100644 server/tests-hspec/Test/ObjectRelationshipsLimitSpec.hs diff --git a/.circleci/server-upgrade-downgrade/run.sh b/.circleci/server-upgrade-downgrade/run.sh index e432d45c0e7..700db86bb32 100755 --- a/.circleci/server-upgrade-downgrade/run.sh +++ b/.circleci/server-upgrade-downgrade/run.sh @@ -198,6 +198,8 @@ get_server_upgrade_tests() { --deselect test_schema_stitching.py::TestAddRemoteSchemaTbls::test_add_conflicting_table \ --deselect test_events.py \ --deselect test_graphql_queries.py::TestGraphQLQueryFunctions \ + --deselect test_graphql_queries.py::TestGraphQLExplainCommon::test_limit_orderby_relationship_query \ + --deselect test_graphql_queries.py::TestGraphQLExplainCommon::test_limit_offset_orderby_relationship_query \ 1>/dev/null 2>/dev/null set +x # Choose the subset of jobs to run based on possible parallelism in this buildkite job @@ -235,6 +237,8 @@ run_server_upgrade_pytest() { --deselect test_graphql_mutations.py::TestGraphqlInsertPermission::test_user_with_no_backend_privilege \ --deselect test_graphql_mutations.py::TestGraphqlMutationCustomSchema::test_update_article \ --deselect test_graphql_queries.py::TestGraphQLQueryEnums::test_introspect_user_role \ + --deselect test_graphql_queries.py::TestGraphQLExplainCommon::test_limit_orderby_relationship_query \ + --deselect test_graphql_queries.py::TestGraphQLExplainCommon::test_limit_offset_orderby_relationship_query \ -v $tests_to_run set +x cd - diff --git a/CHANGELOG.md b/CHANGELOG.md index f3ca3eb4241..e86f0314680 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -115,6 +115,7 @@ count ( - server: implement update mutations for MS SQL Server (closes #7834) - server: support nested output object types in actions (#4796) - server: action webhook requests now include a User-Agent header (fix #8070) +- server: postgres: return a single entry per row (selected randomly) when an object relationship is misconfigured one-to-many (fix #7936). - console: action/event trigger transforms are now called REST connectors - console: fix list of tables (and schemas) being unsorted when creating a new trigger event (fix #6391) - console: fix custom field names breaking browse table sorting and the pre-populating of the edit row form diff --git a/server/graphql-engine.cabal b/server/graphql-engine.cabal index 410aae34e1e..e621023013f 100644 --- a/server/graphql-engine.cabal +++ b/server/graphql-engine.cabal @@ -995,6 +995,7 @@ test-suite tests-hspec Test.HelloWorldSpec Test.LimitOffsetSpec Test.ObjectRelationshipsSpec + Test.ObjectRelationshipsLimitSpec Test.OrderingSpec Test.ServiceLivenessSpec Test.ViewsSpec diff --git a/server/src-lib/Hasura/Backends/Postgres/Translate/Types.hs b/server/src-lib/Hasura/Backends/Postgres/Translate/Types.hs index 12e5cf46d20..5f628a4ca1c 100644 --- a/server/src-lib/Hasura/Backends/Postgres/Translate/Types.hs +++ b/server/src-lib/Hasura/Backends/Postgres/Translate/Types.hs @@ -149,7 +149,18 @@ instance Hashable ObjectSelectSource objectSelectSourceToSelectSource :: ObjectSelectSource -> SelectSource objectSelectSourceToSelectSource ObjectSelectSource {..} = - SelectSource _ossPrefix _ossFrom _ossWhere noSortingAndSlicing + SelectSource _ossPrefix _ossFrom _ossWhere sortingAndSlicing + where + sortingAndSlicing = SortingAndSlicing noSorting limit1 + noSorting = NoSorting Nothing + -- We specify 'LIMIT 1' here to mitigate misconfigured object relationships with an + -- unexpected one-to-many/many-to-many relationship, instead of the expected one-to-one/many-to-one relationship. + -- Because we can't detect this misconfiguration statically (it depends on the data), + -- we force a single (or null) result instead by adding 'LIMIT 1'. + -- Which result is returned might be non-deterministic (though only in misconfigured cases). + -- Proper one-to-one/many-to-one object relationships should not be semantically affected by this. + -- See: https://github.com/hasura/graphql-engine/issues/7936 + limit1 = SelectSlicing (Just 1) Nothing data ObjectRelationSource = ObjectRelationSource { _orsRelationshipName :: !RelName, diff --git a/server/tests-hspec/Test/ObjectRelationshipsLimitSpec.hs b/server/tests-hspec/Test/ObjectRelationshipsLimitSpec.hs new file mode 100644 index 00000000000..d7231568fd4 --- /dev/null +++ b/server/tests-hspec/Test/ObjectRelationshipsLimitSpec.hs @@ -0,0 +1,325 @@ +{-# LANGUAGE QuasiQuotes #-} +{-# LANGUAGE RecordWildCards #-} + +-- | Testing manual, misconfigured object relationships. +-- Specifically, having manual relationships with one-to-many relationships. +-- Test case for bug reported at https://github.com/hasura/graphql-engine/issues/7936 +module Test.ObjectRelationshipsLimitSpec (spec) where + +import Harness.Backend.Postgres as Postgres +import Harness.GraphqlEngine qualified as GraphqlEngine +import Harness.Quoter.Graphql +import Harness.Quoter.Sql +import Harness.Quoter.Yaml +import Harness.State (State) +import Harness.Test.Feature qualified as Feature +import Test.Hspec +import Prelude + +-------------------------------------------------------------------------------- + +-- * Preamble + +spec :: SpecWith State +spec = + Feature.feature + Feature.Feature + { Feature.backends = + [ Feature.Backend + { name = "Postgres", + setup = postgresSetup, + teardown = postgresTeardown + } + ], + Feature.tests = tests + } + +-------------------------------------------------------------------------------- + +-- * Tests + +-- | Many of these may return non-deterministic results because graphql-engine +-- need to choose one of the available rows in a misconfigured manual relationship +-- that has a one-to-many relationship instead of the expected one-to-one. +-- +-- Because of that, we use 'shouldReturnOneOfYaml' and list all of the possible (valid) +-- expected results. +tests :: SpecWith State +tests = do + it "Query by id" $ \state -> + shouldReturnOneOfYaml + ( GraphqlEngine.postGraphql + state + [graphql| +query { + hasura_article(where: {id: {_eq: 1}}) { + id + author { + id + } + } +} +|] + ) + [ [yaml| +data: + hasura_article: + - id: 1 + author: + id: 1 +|], + [yaml| +data: + hasura_article: + - id: 1 + author: + id: 3 +|] + ] + + it "Query limit 2" $ \state -> + shouldReturnOneOfYaml + ( GraphqlEngine.postGraphql + state + [graphql| +query { + hasura_article(limit: 2) { + id + author { + id + } + } +} +|] + ) + [ [yaml| +data: + hasura_article: + - id: 1 + author: + id: 1 + - id: 2 + author: + id: 2 +|], + [yaml| +data: + hasura_article: + - id: 1 + author: + id: 3 + - id: 2 + author: + id: 2 +|] + ] + + it "where author name" $ \state -> + shouldReturnOneOfYaml + ( GraphqlEngine.postGraphql + state + [graphql| +query { + hasura_article(where: { author: { name: { _eq: "Author 1" } } }) { + id + author { + id + } + } +} +|] + ) + [ [yaml| +data: + hasura_article: + - id: 1 + author: + id: 1 +|], + [yaml| +data: + hasura_article: + - id: 1 + author: + id: 3 +|] + ] + + it "order by author id" $ \state -> + shouldReturnOneOfYaml + ( GraphqlEngine.postGraphql + state + [graphql| +query { + hasura_article(order_by: {author: {id: asc}}) { + id + author { + id + } + } +} +|] + ) + [ [yaml| +data: + hasura_article: + - id: 1 + author: + id: 1 + - id: 2 + author: + id: 2 +|], + [yaml| +data: + hasura_article: + - id: 2 + author: + id: 2 + - id: 1 + author: + id: 3 +|] + ] + + it "count articles" $ \state -> + shouldReturnYaml + ( GraphqlEngine.postGraphql + state + [graphql| +query { + hasura_article_aggregate { + aggregate { + count + } + } +} +|] + ) + [yaml| +data: + hasura_article_aggregate: + aggregate: + count: 2 +|] + +-------------------------------------------------------------------------------- + +-- * Postgres backend + +---------------------- + +-- ** Setup + +postgresSetup :: State -> IO () +postgresSetup state = do + -- Clear and reconfigure the metadata + GraphqlEngine.setSource state Postgres.defaultSourceMetadata + postgresSetupTables + postgresInsertValues + postgresTrackTables state + postgresCreateRelationships state + +postgresSetupTables :: IO () +postgresSetupTables = do + -- Setup tables + Postgres.run_ + [sql| +CREATE TABLE hasura.author +( + id SERIAL PRIMARY KEY, + name TEXT, + created_at TIMESTAMP +); +|] + + Postgres.run_ + [sql| +CREATE TABLE hasura.article ( + id SERIAL PRIMARY KEY, + author_name TEXT +); +|] + +postgresInsertValues :: IO () +postgresInsertValues = do + Postgres.run_ + [sql| +INSERT INTO hasura.author + (name, created_at) +VALUES + ( 'Author 1', '2017-09-21 09:39:44' ), + ( 'Author 2', '2017-09-21 09:50:44' ), + ( 'Author 1', '2017-09-21 09:55:44' ); +|] + + Postgres.run_ + [sql| +INSERT INTO hasura.article + (author_name) +VALUES + ( 'Author 1' ), + ( 'Author 2' ); +|] + +postgresTrackTables :: State -> IO () +postgresTrackTables state = do + GraphqlEngine.post_ + state + "/v1/metadata" + [yaml| +type: pg_track_table +args: + source: postgres + table: + schema: hasura + name: author +|] + GraphqlEngine.post_ + state + "/v1/metadata" + [yaml| +type: pg_track_table +args: + source: postgres + table: + schema: hasura + name: article +|] + +postgresCreateRelationships :: State -> IO () +postgresCreateRelationships state = do + GraphqlEngine.post_ + state + "/v1/metadata" + [yaml| +type: pg_create_object_relationship +args: + source: postgres + table: + schema: hasura + name: article + name: author + using: + manual_configuration: + remote_table: + schema: hasura + name: author + column_mapping: + author_name: name +|] + +---------------------- + +-- ** Teardown + +postgresTeardown :: State -> IO () +postgresTeardown _ = do + Postgres.run_ + [sql| +DROP TABLE hasura.article; +|] + Postgres.run_ + [sql| +DROP TABLE hasura.author; +|] diff --git a/server/tests-py/queries/explain/limit_offset_orderby_relationship_query.yaml b/server/tests-py/queries/explain/limit_offset_orderby_relationship_query.yaml index 41e0e37eb0c..bf7173dc119 100644 --- a/server/tests-py/queries/explain/limit_offset_orderby_relationship_query.yaml +++ b/server/tests-py/queries/explain/limit_offset_orderby_relationship_query.yaml @@ -20,5 +20,5 @@ response: AS "_3_e" ) ) AS "root" FROM (SELECT * FROM "public"."article" WHERE (''true'') ) AS "_0_root.base" LEFT OUTER JOIN LATERAL (SELECT "_1_root.or.author.base"."id" AS "root.or.author.pg.id" FROM (SELECT * FROM "public"."author" WHERE (("_0_root.base"."author_id") - = ("id")) ) AS "_1_root.or.author.base" ) AS "_2_root.or.author" ON (''true'') ORDER + = ("id")) LIMIT 1 ) AS "_1_root.or.author.base" ) AS "_2_root.or.author" ON (''true'') ORDER BY "root.or.author.pg.id" DESC NULLS FIRST LIMIT 3 OFFSET 2) AS "_4_root" ' diff --git a/server/tests-py/queries/explain/limit_orderby_relationship_query.yaml b/server/tests-py/queries/explain/limit_orderby_relationship_query.yaml index 1d4bfd265f9..78238ff0f53 100644 --- a/server/tests-py/queries/explain/limit_orderby_relationship_query.yaml +++ b/server/tests-py/queries/explain/limit_orderby_relationship_query.yaml @@ -20,5 +20,5 @@ response: AS "_3_e" ) ) AS "root" FROM (SELECT * FROM "public"."article" WHERE (''true'') ) AS "_0_root.base" LEFT OUTER JOIN LATERAL (SELECT "_1_root.or.author.base"."id" AS "root.or.author.pg.id" FROM (SELECT * FROM "public"."author" WHERE (("_0_root.base"."author_id") - = ("id")) ) AS "_1_root.or.author.base" ) AS "_2_root.or.author" ON (''true'') ORDER + = ("id")) LIMIT 1 ) AS "_1_root.or.author.base" ) AS "_2_root.or.author" ON (''true'') ORDER BY "root.or.author.pg.id" DESC NULLS FIRST LIMIT 2 ) AS "_4_root" ' diff --git a/server/tests-py/test_graphql_queries.py b/server/tests-py/test_graphql_queries.py index fcdec92d364..9ad5f3a7e45 100644 --- a/server/tests-py/test_graphql_queries.py +++ b/server/tests-py/test_graphql_queries.py @@ -1249,7 +1249,7 @@ class TestFallbackUnauthorizedRoleCookie: @classmethod def dir(cls): return 'queries/unauthorized_role' - + def test_fallback_unauth_role_jwt_cookie_not_set(self, hge_ctx, transport): check_query_f(hge_ctx, self.dir() + '/cookie_header_absent_unauth_role_set.yaml', transport, add_auth=False) @@ -1263,7 +1263,7 @@ class TestMissingUnauthorizedRoleAndCookie: @classmethod def dir(cls): return 'queries/unauthorized_role' - + def test_error_unauth_role_not_set_jwt_cookie_not_set(self, hge_ctx, transport): check_query_f(hge_ctx, self.dir() + '/cookie_header_absent_unauth_role_not_set.yaml', transport, add_auth=False) @@ -1297,9 +1297,13 @@ class TestGraphQLExplainCommon: def test_limit_orderby_column_query(self, hge_ctx): self.with_admin_secret("query", hge_ctx, self.dir() + '/limit_orderby_column_query.yaml') + @pytest.mark.skip_server_upgrade_test + # skipping due to https://github.com/hasura/graphql-engine/issues/7936 def test_limit_orderby_relationship_query(self, hge_ctx): self.with_admin_secret("query", hge_ctx, self.dir() + '/limit_orderby_relationship_query.yaml') + @pytest.mark.skip_server_upgrade_test + # skipping due to https://github.com/hasura/graphql-engine/issues/7936 def test_limit_offset_orderby_relationship_query(self, hge_ctx): self.with_admin_secret("query", hge_ctx, self.dir() + '/limit_offset_orderby_relationship_query.yaml')