From 53ca4da79d70f5dff99c40958c46204b6992d0b9 Mon Sep 17 00:00:00 2001 From: Lyndon Maydwell Date: Mon, 2 May 2022 13:33:17 +1000 Subject: [PATCH] Fixing URL parameter for variable not supported bug in REST endpoints for Dates PR-URL: https://github.com/hasura/graphql-engine-mono/pull/4345 GitOrigin-RevId: 0b739530757ee1081d764d9582e3c0e648861d4e --- CHANGELOG.md | 2 + .../graphql/core/api-reference/restified.rst | 6 +-- server/src-lib/Hasura/Server/Rest.hs | 52 ++++++------------- .../endpoint_pg_date_url_variable.yaml | 7 +++ server/tests-py/queries/endpoints/setup.yaml | 17 +++++- .../tests-py/queries/endpoints/teardown.yaml | 5 +- server/tests-py/test_endpoints.py | 3 ++ 7 files changed, 48 insertions(+), 44 deletions(-) create mode 100644 server/tests-py/queries/endpoints/endpoint_pg_date_url_variable.yaml diff --git a/CHANGELOG.md b/CHANGELOG.md index 09697cea7cf..95f930f3cf0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,8 @@ - server: fix parsing remote relationship json definition from 1.x server catalog on migration (fix #7906) - server: Don't drop nested typed null fields in actions (fix #8237) - server: fixes remote relationships on actions (fix #8399) +- server: fixes url/query date variable bug in REST endpoints +- server: makes url/query variables in REST endpoints assume string if other types not applicable - console: add remote database relationships for views - console: bug fixes for RS-to-RS relationships - cli: avoid exporting hasura-specific schemas during hasura init (#8352) diff --git a/docs-old/graphql/core/api-reference/restified.rst b/docs-old/graphql/core/api-reference/restified.rst index f4f95e01841..e5cd8561003 100644 --- a/docs-old/graphql/core/api-reference/restified.rst +++ b/docs-old/graphql/core/api-reference/restified.rst @@ -65,12 +65,8 @@ Variables can be supplied via route patterns, url query variables, and request b * Missing nullable variables are interpreted as ``NULL`` * Boolean variables are interpreted as ``Boolean`` * Boolean flags without values e.g. ``/api/rest/myquery?mybool`` are interpreted as ``true`` - * String variables are interpreted as ``String`` - * UUID variables are interpreted as ``String`` - * ID variables are interpreted as ``String`` * Number, Int, Float, and Double variables are interpreted as ``Number`` - * **All other types or mismatches currently report variable type errors** - + * **All other types are currently interpreted as ``String``** (Note: Further validation is performed during query execution) When making a request to this API only one endpoint should match. If multiple endpoints match your request this is considered an error and will report so via a 500 status code. If endpoints exist with your path, but none matching your HTTP method then a 405 error will be returned listing the methods that you can use. diff --git a/server/src-lib/Hasura/Server/Rest.hs b/server/src-lib/Hasura/Server/Rest.hs index cc1692d1a5c..e9e1008e0a9 100644 --- a/server/src-lib/Hasura/Server/Rest.hs +++ b/server/src-lib/Hasura/Server/Rest.hs @@ -51,45 +51,23 @@ alignVars defVars parseVars = (M.fromList (map (\v -> (G._vdName v, v)) defVars)) (M.fromList (mapMaybe (\(k, v) -> (,v) <$> G.mkName k) parseVars)) +-- | `resolveVar` is responsible for decoding variables sent via REST request. +-- These can either be via body (represented by Right) or via query-param or URL param (represented by Left). +-- A variable can be expected, unexpected, or missing (represented by These, This, and That). resolveVar :: G.Name -> These G.VariableDefinition (Either Text J.Value) -> Either Text (Maybe Value) -resolveVar _ (This _expectedVar) = Right Nothing -resolveVar varName (That _providedVar) = Left $ "Unexpected variable " <> toTxt @G.Name varName -resolveVar varName (These expectedVar providedVar) = - -- TODO: See CustomTypes.hs for SCALAR types +resolveVar _ (This _expectedVar) = Right Nothing -- If a variable is expected but missing, assign a missing value `Nothing` to it for resolution in query execution. This allows Null defaulting. +resolveVar varName (That _providedVar) = Left $ "Unexpected variable " <> toTxt @G.Name varName -- If a variable is unexpected but present, throw an error. +resolveVar _varName (These _expectedVar (Right bodyVar)) = Right (Just bodyVar) -- Variables sent via body can be passed through to execution without parsing. +resolveVar varName (These expectedVar (Left l)) = case G._vdType expectedVar of - G.TypeNamed (G.Nullability nullable) typeName -> case providedVar of - Right r -> Right (Just r) - Left l - | typeName == stringScalar -> Right $ Just $ J.String l -- "String" -- Note: Strings don't need to be decoded since the format already matches. - | otherwise -> - case (J.decodeStrict (T.encodeUtf8 l), nullable) of - (Just J.Null, True) -> pure Nothing - (decoded, _) - | typeName == boolScalar && T.null l -> Right $ Just $ J.Bool True -- Key present but value missing for bools defaults to True. - | typeName == G._UUID -> Right $ Just $ J.String l - | typeName == G._uuid -> Right $ Just $ J.String l - | typeName == idScalar -> Right $ Just $ J.String l -- "ID" -- Note: Console doesn't expose this as a column type. - | otherwise -> case decoded of - (Just J.Null) -> Left $ "Null or missing value for non-nullable variable: " <> G.unName varName - (Just x@(J.Bool _)) - | typeName == boolScalar -> pure $ Just x -- "Boolean" - | typeName == G._Bool -> pure $ Just x - | otherwise -> Left $ "Expected " <> toTxt typeName <> " for variable " <> G.unName varName <> " got Bool" - (Just x@(J.Number _)) - | typeName == intScalar -> pure $ Just x -- "Int" - | typeName == floatScalar -> pure $ Just x -- "Float" - | typeName == G._Number -> pure $ Just x - | typeName == G._Double -> pure $ Just x - | typeName == G._float8 -> pure $ Just x - | typeName == G._numeric -> pure $ Just x - | otherwise -> Left $ "Expected " <> toTxt typeName <> " for variable " <> G.unName varName <> " got Number" - _ -> Left ("Type of URL parameter for variable " <> G.unName varName <> " not supported - Consider putting it in the request body: " <> tshow l) - -- TODO: This is a fallthrough case and is still required - -- but we can move checks for template variables being - -- scalars into the schema-cache construction. - G.TypeList _ _ -> case providedVar of - Right r -> Right (Just r) - Left l -> Left ("The variable type for the expected variable " <> toTxt @G.Name varName <> ", with value " <> tshow l <> " is not supported.") + G.TypeList _ _ -> Left $ "List variables are not currently supported in URL or Query parameters. (Variable " <> toTxt @G.Name varName <> ", with value " <> tshow l <> ")" + G.TypeNamed (G.Nullability nullable) typeName + | typeName == boolScalar && T.null l -> Right $ Just $ J.Bool True -- Booleans indicated true by a standalone key. + | nullable && T.null l -> Right Nothing -- Missing value, but nullable variable sets value to null. + | otherwise -> case J.decodeStrict (T.encodeUtf8 l) of -- We special case parsing of bools and numbers and pass the rest through as literal strings. + Just v@(J.Bool _) | typeName `elem` [G._Bool, boolScalar] -> Right $ Just v + Just v@(J.Number _) | typeName `elem` [intScalar, floatScalar, G._Number, G._Double, G._float8, G._numeric] -> Right $ Just v + _ -> Right $ Just $ J.String l mkPassthroughRequest :: EndpointMetadata GQLQueryWithText -> VariableValues -> GQLReq GQLQueryText mkPassthroughRequest queryx resolvedVariables = diff --git a/server/tests-py/queries/endpoints/endpoint_pg_date_url_variable.yaml b/server/tests-py/queries/endpoints/endpoint_pg_date_url_variable.yaml new file mode 100644 index 00000000000..bc27b290ec6 --- /dev/null +++ b/server/tests-py/queries/endpoints/endpoint_pg_date_url_variable.yaml @@ -0,0 +1,7 @@ +description: Successful call of an endpoint with date and point +url: /api/rest/with_date/2010-1-1/{"type":"Point","coordinates":[-48.23456,20.12345]} +method: GET +status: 200 +query: +response: + test_table: [] diff --git a/server/tests-py/queries/endpoints/setup.yaml b/server/tests-py/queries/endpoints/setup.yaml index b75d072d907..151b6e3a9df 100644 --- a/server/tests-py/queries/endpoints/setup.yaml +++ b/server/tests-py/queries/endpoints/setup.yaml @@ -3,9 +3,12 @@ args: - type: run_sql args: sql: | + CREATE EXTENSION IF NOT EXISTS postgis; create table test_table( first_name text, last_name text, + created_at date, + point GEOMETRY, id UUID NOT NULL DEFAULT gen_random_uuid() ); @@ -38,6 +41,8 @@ args: query: "query ($first_name: String!, $last_name:String!) { test_table(where: {first_name: { _eq: $first_name } last_name: { _eq: $last_name }}) { first_name last_name } }" - name: query_with_uuid_arg query: "query ($id: uuid!) { test_table(where: {id: { _neq: $id }}) { first_name last_name } }" + - name: query_with_date_arg + query: "query ($date: date!, $point: geometry!) { test_table(where: {created_at: {_gt: $date}, point: {_st_equals: $point}}) { created_at } }" - name: query_with_uuid_args query: "query ($ids: [uuid!]!) { test_table(where: {id: { _in: $ids }}) { first_name last_name } }" - name: simple_subscription @@ -110,6 +115,17 @@ args: collection_name: test_collection query_name: query_with_uuid_arg +- type: create_rest_endpoint + args: + url: with_date/:date/:point + name: with_date + methods: + - GET + definition: + query: + collection_name: test_collection + query_name: query_with_date_arg + - type: create_rest_endpoint args: url: with_uuids @@ -121,7 +137,6 @@ args: collection_name: test_collection query_name: query_with_uuid_args - - type: create_rest_endpoint args: url: to_be_dropped diff --git a/server/tests-py/queries/endpoints/teardown.yaml b/server/tests-py/queries/endpoints/teardown.yaml index e4ba0e2db37..9fc45b0249e 100644 --- a/server/tests-py/queries/endpoints/teardown.yaml +++ b/server/tests-py/queries/endpoints/teardown.yaml @@ -18,6 +18,9 @@ args: - type: drop_rest_endpoint args: name: with_uuid +- type: drop_rest_endpoint + args: + name: with_date - type: drop_rest_endpoint args: name: with_template @@ -28,4 +31,4 @@ args: - type: run_sql args: sql: | - drop table test_table + drop table test_table \ No newline at end of file diff --git a/server/tests-py/test_endpoints.py b/server/tests-py/test_endpoints.py index 5e1327d8a12..10eb6277079 100644 --- a/server/tests-py/test_endpoints.py +++ b/server/tests-py/test_endpoints.py @@ -87,3 +87,6 @@ class TestCustomEndpoints: def test_query_validation(self, hge_ctx, transport): check_query_f(hge_ctx, self.dir() + '/endpoint_query_validation.yaml', transport) + + def test_endpoint_with_pg_date_url_variable(self, hge_ctx, transport): + check_query_f(hge_ctx, self.dir() + '/endpoint_pg_date_url_variable.yaml', transport)