diff --git a/CHANGELOG.md b/CHANGELOG.md index b26ed38673..faa3311672 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,7 @@ This project adheres to [Semantic Versioning](http://semver.org/). - #3727, Clarify "listening" logs - @steve-chavez - #3795, Clarify `Accept: vnd.pgrst.object` error message - @steve-chavez - #3841, Log `503` client error to stderr - @taimoorzaeem + - #3697, #3602, Handle queries on non-existing table gracefully - @taimoorzaeem ### Changed @@ -38,6 +39,7 @@ This project adheres to [Semantic Versioning](http://semver.org/). - Previously, this would silently return 200 - OK on the root endpoint, but don't provide any usable endpoints. - #3757, Remove support for `Prefer: params=single-object` - @joelonsql + This preference was deprecated in favor of Functions with an array of JSON objects + - #3697, #3602, Querying non-existent table now returns `PGRST205` error instead of empty json - @taimoorzaeem ## [12.2.7] - 2025-02-03 diff --git a/docs/references/errors.rst b/docs/references/errors.rst index 4344f67a22..132d25ac62 100644 --- a/docs/references/errors.rst +++ b/docs/references/errors.rst @@ -294,6 +294,10 @@ Related to a :ref:`schema_cache`. Most of the time, these errors are solved by : | | | in the ``columns`` query parameter is not found. | | PGRST204 | | | +---------------+-------------+-------------------------------------------------------------+ +| .. _pgrst205: | 404 | Caused when the :ref:`table specified ` in | +| | | the URI is not found. | +| PGRST205 | | | ++---------------+-------------+-------------------------------------------------------------+ .. _pgrst3**: diff --git a/src/PostgREST/ApiRequest/Types.hs b/src/PostgREST/ApiRequest/Types.hs index e1bdeea4d7..0795fd0b7f 100644 --- a/src/PostgREST/ApiRequest/Types.hs +++ b/src/PostgREST/ApiRequest/Types.hs @@ -42,6 +42,7 @@ import PostgREST.SchemaCache.Identifiers (FieldName, import PostgREST.SchemaCache.Relationship (Relationship, RelationshipsMap) import PostgREST.SchemaCache.Routine (Routine (..)) +import PostgREST.SchemaCache.Table (Table) import Protolude @@ -92,6 +93,7 @@ data ApiRequestError | UnacceptableSchema [Text] | UnsupportedMethod ByteString | ColumnNotFound Text Text + | TableNotFound Text Text [Table] | GucHeadersError | GucStatusError | OffLimitsChangesError Int64 Integer diff --git a/src/PostgREST/Error.hs b/src/PostgREST/Error.hs index 8f386a8000..d7b38ca338 100644 --- a/src/PostgREST/Error.hs +++ b/src/PostgREST/Error.hs @@ -47,6 +47,7 @@ import PostgREST.SchemaCache.Relationship (Cardinality (..), RelationshipsMap) import PostgREST.SchemaCache.Routine (Routine (..), RoutineParam (..)) +import PostgREST.SchemaCache.Table (Table (..)) import Protolude @@ -86,6 +87,7 @@ instance PgrstError ApiRequestError where status UnsupportedMethod{} = HTTP.status405 status LimitNoOrderError = HTTP.status400 status ColumnNotFound{} = HTTP.status400 + status TableNotFound{} = HTTP.status404 status GucHeadersError = HTTP.status500 status GucStatusError = HTTP.status500 status OffLimitsChangesError{} = HTTP.status400 @@ -253,6 +255,12 @@ instance JSON.ToJSON ApiRequestError where toJSON (ColumnNotFound relName colName) = toJsonPgrstError SchemaCacheErrorCode04 ("Could not find the '" <> colName <> "' column of '" <> relName <> "' in the schema cache") Nothing Nothing + toJSON (TableNotFound schemaName relName tbls) = toJsonPgrstError + SchemaCacheErrorCode05 + ("Could not find relation '" <> schemaName <> "." <> relName <> "' in the schema cache") + Nothing + (JSON.String <$> tableNotFoundHint schemaName relName tbls) + -- | -- If no relationship is found then: -- @@ -350,6 +358,16 @@ noRpcHint schema procName params allProcs overloadedProcs = | null overloadedProcs = Fuzzy.getOne fuzzySetOfProcs procName | otherwise = (procName <>) <$> Fuzzy.getOne fuzzySetOfParams (listToText params) +-- | +-- Do a fuzzy search in all tables in the same schema and return closest result +tableNotFoundHint :: Text -> Text -> [Table] -> Maybe Text +tableNotFoundHint schema tblName tblList + = fmap (\tbl -> "Perhaps you meant the relation '" <> schema <> "." <> tbl <> "'") perhapsTable + where + perhapsTable = Fuzzy.getOne fuzzyTableSet tblName + fuzzyTableSet = Fuzzy.fromList [ tableName tbl | tbl <- tblList, tableSchema tbl == schema] + + compressedRel :: Relationship -> JSON.Value -- An ambiguousness error cannot happen for computed relationships TODO refactor so this mempty is not needed compressedRel ComputedRelationship{} = JSON.object mempty @@ -640,6 +658,7 @@ data ErrorCode | SchemaCacheErrorCode02 | SchemaCacheErrorCode03 | SchemaCacheErrorCode04 + | SchemaCacheErrorCode05 -- JWT authentication errors | JWTErrorCode00 | JWTErrorCode01 @@ -689,6 +708,7 @@ buildErrorCode code = case code of SchemaCacheErrorCode02 -> "PGRST202" SchemaCacheErrorCode03 -> "PGRST203" SchemaCacheErrorCode04 -> "PGRST204" + SchemaCacheErrorCode05 -> "PGRST205" JWTErrorCode00 -> "PGRST300" JWTErrorCode01 -> "PGRST301" diff --git a/src/PostgREST/Plan.hs b/src/PostgREST/Plan.hs index 23ca5aef94..cdcc165859 100644 --- a/src/PostgREST/Plan.hs +++ b/src/PostgREST/Plan.hs @@ -152,14 +152,14 @@ dbActionPlan dbAct conf apiReq sCache = case dbAct of wrappedReadPlan :: QualifiedIdentifier -> AppConfig -> SchemaCache -> ApiRequest -> Bool -> Either Error CrudPlan wrappedReadPlan identifier conf sCache apiRequest@ApiRequest{iPreferences=Preferences{..},..} headersOnly = do - rPlan <- readPlan identifier conf sCache apiRequest + rPlan <- readPlan False identifier conf sCache apiRequest (handler, mediaType) <- mapLeft ApiRequestError $ negotiateContent conf apiRequest identifier iAcceptMediaType (dbMediaHandlers sCache) (hasDefaultSelect rPlan) if not (null invalidPrefs) && preferHandling == Just Strict then Left $ ApiRequestError $ InvalidPreferences invalidPrefs else Right () return $ WrappedReadPlan rPlan SQL.Read handler mediaType headersOnly identifier mutateReadPlan :: Mutation -> ApiRequest -> QualifiedIdentifier -> AppConfig -> SchemaCache -> Either Error CrudPlan mutateReadPlan mutation apiRequest@ApiRequest{iPreferences=Preferences{..},..} identifier conf sCache = do - rPlan <- readPlan identifier conf sCache apiRequest + rPlan <- readPlan False identifier conf sCache apiRequest mPlan <- mutatePlan mutation identifier apiRequest sCache rPlan if not (null invalidPrefs) && preferHandling == Just Strict then Left $ ApiRequestError $ InvalidPreferences invalidPrefs else Right () (handler, mediaType) <- mapLeft ApiRequestError $ negotiateContent conf apiRequest identifier iAcceptMediaType (dbMediaHandlers sCache) (hasDefaultSelect rPlan) @@ -173,7 +173,7 @@ callReadPlan identifier conf sCache apiRequest@ApiRequest{iPreferences=Preferenc proc@Function{..} <- mapLeft ApiRequestError $ findProc identifier paramKeys (dbRoutines sCache) iContentMediaType (invMethod == Inv) let relIdentifier = QualifiedIdentifier pdSchema (fromMaybe pdName $ Routine.funcTableName proc) -- done so a set returning function can embed other relations - rPlan <- readPlan relIdentifier conf sCache apiRequest + rPlan <- readPlan True relIdentifier conf sCache apiRequest let args = case (invMethod, iContentMediaType) of (InvRead _, _) -> DirectArgs $ toRpcParams proc qsParams' (Inv, MTUrlEncoded) -> DirectArgs $ maybe mempty (toRpcParams proc . payArray) iPayload @@ -326,8 +326,8 @@ resolveQueryInputField ctx field opExpr = withTextParse ctx $ resolveTypeOrUnkno -- | Builds the ReadPlan tree on a number of stages. -- | Adds filters, order, limits on its respective nodes. -- | Adds joins conditions obtained from resource embedding. -readPlan :: QualifiedIdentifier -> AppConfig -> SchemaCache -> ApiRequest -> Either Error ReadPlanTree -readPlan qi@QualifiedIdentifier{..} AppConfig{configDbMaxRows, configDbAggregates} SchemaCache{dbTables, dbRelationships, dbRepresentations} apiRequest = +readPlan :: Bool -> QualifiedIdentifier -> AppConfig -> SchemaCache -> ApiRequest -> Either Error ReadPlanTree +readPlan fromCallPlan qi@QualifiedIdentifier{..} AppConfig{configDbMaxRows, configDbAggregates} SchemaCache{dbTables, dbRelationships, dbRepresentations} apiRequest = let -- JSON output format hardcoded for now. In the future we might want to support other output mappings such as CSV. ctx = ResolverContext dbTables dbRepresentations qi "json" @@ -346,7 +346,8 @@ readPlan qi@QualifiedIdentifier{..} AppConfig{configDbMaxRows, configDbAggregate addLogicTrees ctx apiRequest =<< addRanges apiRequest =<< addOrders ctx apiRequest =<< - addFilters ctx apiRequest (initReadRequest ctx $ QueryParams.qsSelect $ iQueryParams apiRequest) + addFilters ctx apiRequest =<< + searchTable fromCallPlan qi dbTables (initReadRequest ctx $ QueryParams.qsSelect $ iQueryParams apiRequest) -- Build the initial read plan tree initReadRequest :: ResolverContext -> [Tree SelectItem] -> ReadPlanTree @@ -744,6 +745,14 @@ validateAggFunctions aggFunctionsAllowed (Node rp@ReadPlan {select} forest) | not aggFunctionsAllowed && any (isJust . csAggFunction) select = Left AggregatesNotAllowed | otherwise = Node rp <$> traverse (validateAggFunctions aggFunctionsAllowed) forest +-- We only search for the table when readPlan is not called from call plan. This +-- is because we reuse readPlan in callReadPlan to search for function name +searchTable :: Bool -> QualifiedIdentifier -> TablesMap -> ReadPlanTree -> Either ApiRequestError ReadPlanTree +searchTable fromCallPlan qi@QualifiedIdentifier{..} tableMap readPlanTree = + case (fromCallPlan, HM.lookup qi tableMap) of + (False, Nothing) -> Left (TableNotFound qiSchema qiName (HM.elems tableMap)) + _ -> Right readPlanTree + addFilters :: ResolverContext -> ApiRequest -> ReadPlanTree -> Either ApiRequestError ReadPlanTree addFilters ctx ApiRequest{..} rReq = foldr addFilterToNode (Right rReq) flts @@ -967,7 +976,7 @@ mutatePlan mutation qi ApiRequest{iPreferences=Preferences{..}, ..} SchemaCache{ typedColumnsOrError = resolveOrError ctx tbl `traverse` S.toList iColumns resolveOrError :: ResolverContext -> Maybe Table -> FieldName -> Either ApiRequestError CoercibleField -resolveOrError _ Nothing _ = Left NotFound +resolveOrError ctx Nothing _ = Left $ TableNotFound (qiSchema (qi ctx)) (qiName (qi ctx)) (HM.elems (tables ctx)) resolveOrError ctx (Just table) field = case resolveTableFieldName table field Nothing of CoercibleField{cfIRType=""} -> Left $ ColumnNotFound (tableName table) field diff --git a/test/io/test_big_schema.py b/test/io/test_big_schema.py index 1e6ecf6c8a..a1022df421 100644 --- a/test/io/test_big_schema.py +++ b/test/io/test_big_schema.py @@ -7,8 +7,8 @@ from postgrest import * -def test_requests_wait_for_schema_cache_reload(defaultenv): - "requests that use the schema cache (e.g. resource embedding) wait for the schema cache to reload" +def test_requests_with_resource_embedding_wait_for_schema_cache_reload(defaultenv): + "requests that use the schema cache with resource embedding wait long for the schema cache to reload" env = { **defaultenv, @@ -34,6 +34,33 @@ def test_requests_wait_for_schema_cache_reload(defaultenv): assert plan_dur > 10000.0 +def test_requests_without_resource_embedding_wait_for_schema_cache_reload(defaultenv): + "requests that use the schema cache without resource embedding wait less for the schema cache to reload" + + env = { + **defaultenv, + "PGRST_DB_SCHEMAS": "apflora", + "PGRST_DB_POOL": "2", + "PGRST_DB_ANON_ROLE": "postgrest_test_anonymous", + "PGRST_SERVER_TIMING_ENABLED": "true", + } + + with run(env=env, wait_max_seconds=30) as postgrest: + # reload the schema cache + response = postgrest.session.get("/rpc/notify_pgrst") + assert response.status_code == 204 + + postgrest.wait_until_scache_starts_loading() + + response = postgrest.session.get("/tpopmassn") + assert response.status_code == 200 + + plan_dur = parse_server_timings_header(response.headers["Server-Timing"])[ + "plan" + ] + assert plan_dur < 10000.0 + + # TODO: This test fails now because of https://github.com/PostgREST/postgrest/pull/2122 # The stack size of 1K(-with-rtsopts=-K1K) is not enough and this fails with "stack overflow" # A stack size of 200K seems to be enough for succeess @@ -65,6 +92,6 @@ def test_should_not_fail_with_stack_overflow(defaultenv): with run(env=env, wait_max_seconds=30) as postgrest: response = postgrest.session.get("/unknown-table?select=unknown-rel(*)") - assert response.status_code == 400 + assert response.status_code == 404 data = response.json() - assert data["code"] == "PGRST200" + assert data["code"] == "PGRST205" diff --git a/test/io/test_io.py b/test/io/test_io.py index ff5619fb97..0488628a0e 100644 --- a/test/io/test_io.py +++ b/test/io/test_io.py @@ -973,11 +973,10 @@ def test_log_level(level, defaultenv): r'- - postgrest_test_anonymous \[.+\] "GET /unknown HTTP/1.1" 404 - "" "python-requests/.+"', output[2], ) + + assert len(output) == 5 assert "Connection" and "is available" in output[3] - assert "Connection" and "is available" in output[4] - assert "Connection" and "is used" in output[5] - assert "Connection" and "is used" in output[6] - assert len(output) == 7 + assert "Connection" and "is used" in output[4] def test_no_pool_connection_required_on_bad_http_logic(defaultenv): diff --git a/test/spec/Feature/ConcurrentSpec.hs b/test/spec/Feature/ConcurrentSpec.hs index d52d54666f..fd215c347f 100644 --- a/test/spec/Feature/ConcurrentSpec.hs +++ b/test/spec/Feature/ConcurrentSpec.hs @@ -24,12 +24,8 @@ spec = it "should not raise 'transaction in progress' error" $ raceTest 10 $ get "/fakefake" - `shouldRespondWith` [json| - { "hint": null, - "details":null, - "code":"42P01", - "message":"relation \"test.fakefake\" does not exist" - } |] + `shouldRespondWith` + [json| {"code":"PGRST205","details":null,"hint":"Perhaps you meant the relation 'test.factories'","message":"Could not find relation 'test.fakefake' in the schema cache"} |] { matchStatus = 404 , matchHeaders = [] } diff --git a/test/spec/Feature/Query/DeleteSpec.hs b/test/spec/Feature/Query/DeleteSpec.hs index 7e3c83d10e..dcd9263d4b 100644 --- a/test/spec/Feature/Query/DeleteSpec.hs +++ b/test/spec/Feature/Query/DeleteSpec.hs @@ -109,7 +109,12 @@ spec = context "totally unknown route" $ it "fails with 404" $ - request methodDelete "/foozle?id=eq.101" [] "" `shouldRespondWith` 404 + request methodDelete "/foozle?id=eq.101" [] "" + `shouldRespondWith` + [json| {"code":"PGRST205","details":null,"hint":"Perhaps you meant the relation 'test.foo'","message":"Could not find relation 'test.foozle' in the schema cache"} |] + { matchStatus = 404 + , matchHeaders = [] + } context "table with limited privileges" $ do it "fails deleting the row when return=representation and selecting all the columns" $ diff --git a/test/spec/Feature/Query/InsertSpec.hs b/test/spec/Feature/Query/InsertSpec.hs index 0f6a1b6e7a..c2e9c1717e 100644 --- a/test/spec/Feature/Query/InsertSpec.hs +++ b/test/spec/Feature/Query/InsertSpec.hs @@ -477,7 +477,7 @@ spec actualPgVersion = do {"id": 204, "body": "yyy"}, {"id": 205, "body": "zzz"}]|] `shouldRespondWith` - [json|{} |] + [json| {"code":"PGRST205","details":null,"hint":"Perhaps you meant the relation 'test.articles'","message":"Could not find relation 'test.garlic' in the schema cache"} |] { matchStatus = 404 , matchHeaders = [] } diff --git a/test/spec/Feature/Query/MultipleSchemaSpec.hs b/test/spec/Feature/Query/MultipleSchemaSpec.hs index 7b25e2ffaf..b2b75ff095 100644 --- a/test/spec/Feature/Query/MultipleSchemaSpec.hs +++ b/test/spec/Feature/Query/MultipleSchemaSpec.hs @@ -64,7 +64,13 @@ spec = } it "doesn't find another_table in schema v1" $ - request methodGet "/another_table" [("Accept-Profile", "v1")] "" `shouldRespondWith` 404 + request methodGet "/another_table" + [("Accept-Profile", "v1")] "" + `shouldRespondWith` + [json| {"code":"PGRST205","details":null,"hint":null,"message":"Could not find relation 'v1.another_table' in the schema cache"} |] + { matchStatus = 404 + , matchHeaders = [] + } it "fails trying to read table from unkown schema" $ request methodGet "/parents" [("Accept-Profile", "unkown")] "" `shouldRespondWith` diff --git a/test/spec/Feature/Query/QuerySpec.hs b/test/spec/Feature/Query/QuerySpec.hs index c0bda5a66f..056f02c2bc 100644 --- a/test/spec/Feature/Query/QuerySpec.hs +++ b/test/spec/Feature/Query/QuerySpec.hs @@ -24,7 +24,12 @@ spec = do describe "Querying a nonexistent table" $ it "causes a 404" $ - get "/faketable" `shouldRespondWith` 404 + get "/faketable" + `shouldRespondWith` + [json| {"code":"PGRST205","details":null,"hint":"Perhaps you meant the relation 'test.private_table'","message":"Could not find relation 'test.faketable' in the schema cache"} |] + { matchStatus = 404 + , matchHeaders = [] + } describe "Filtering response" $ do it "matches with equality" $ @@ -819,14 +824,12 @@ spec = do , matchHeaders = [matchContentTypeJson] } - it "cannot request a partitioned table as parent from a partition" $ + -- we only search for foreign key relationships after checking the + -- the existence of first table, #3869 + it "table not found error if first table does not exist" $ get "/car_model_sales_202101?select=id,name,car_models(id,name)&order=id.asc" `shouldRespondWith` - [json| - {"hint":"Perhaps you meant 'car_model_sales' instead of 'car_model_sales_202101'.", - "details":"Searched for a foreign key relationship between 'car_model_sales_202101' and 'car_models' in the schema 'test', but no matches were found.", - "code":"PGRST200", - "message":"Could not find a relationship between 'car_model_sales_202101' and 'car_models' in the schema cache"} |] - { matchStatus = 400 + [json| {"code":"PGRST205","details":null,"hint":"Perhaps you meant the relation 'test.car_model_sales'","message":"Could not find relation 'test.car_model_sales_202101' in the schema cache"} |] + { matchStatus = 404 , matchHeaders = [matchContentTypeJson] } @@ -841,14 +844,10 @@ spec = do , matchHeaders = [matchContentTypeJson] } - it "cannot request partitioned tables as children from a partition" $ + it "table not found error if first table does not exist" $ get "/car_models_default?select=id,name,car_model_sales(id,name)&order=id.asc" `shouldRespondWith` - [json| - {"hint":"Perhaps you meant 'car_model_sales' instead of 'car_models_default'.", - "details":"Searched for a foreign key relationship between 'car_models_default' and 'car_model_sales' in the schema 'test', but no matches were found.", - "code":"PGRST200", - "message":"Could not find a relationship between 'car_models_default' and 'car_model_sales' in the schema cache"} |] - { matchStatus = 400 + [json| {"code":"PGRST205","details":null,"hint":"Perhaps you meant the relation 'test.car_model_sales'","message":"Could not find relation 'test.car_models_default' in the schema cache"} |] + { matchStatus = 404 , matchHeaders = [matchContentTypeJson] } diff --git a/test/spec/Feature/Query/UpdateSpec.hs b/test/spec/Feature/Query/UpdateSpec.hs index 3faaab4518..d1c4a09ef2 100644 --- a/test/spec/Feature/Query/UpdateSpec.hs +++ b/test/spec/Feature/Query/UpdateSpec.hs @@ -17,7 +17,12 @@ spec = do it "indicates no table found by returning 404" $ request methodPatch "/fake" [] [json| { "real": false } |] - `shouldRespondWith` 404 + `shouldRespondWith` + [json| {"code":"PGRST205","details":null,"hint":"Perhaps you meant the relation 'test.factories'","message":"Could not find relation 'test.fake' in the schema cache"} |] + { matchStatus = 404 + , matchHeaders = [] + } + context "on an empty table" $ it "succeeds with status code 204" $ @@ -343,7 +348,7 @@ spec = do {"id": 204, "body": "yyy"}, {"id": 205, "body": "zzz"}]|] `shouldRespondWith` - [json|{} |] + [json| {"code":"PGRST205","details":null,"hint":"Perhaps you meant the relation 'test.articles'","message":"Could not find relation 'test.garlic' in the schema cache"} |] { matchStatus = 404 , matchHeaders = [] }