-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: handle queries on non-existing table gracefully #3869
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto on "relation" |
||
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" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 = | ||
Comment on lines
+329
to
+330
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This Let's try and do it correctly now, even if it requires a refactor. Otherwise tech debt accrues and refactors become more laborious later. |
||
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 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "relation" term might be confused with "relationship". I think we can be more precise here and instead refer to "table" or "view", we have a
tableIsView :: Bool
field already.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although it doesn't make sense to discriminate table or view if we don't know what we're looking for. Maybe you could omit the "relation" term altogether, or change it to "table or view".