From 62243852b63bd974d935da14d57eb5de8beb5106 Mon Sep 17 00:00:00 2001 From: Laurence Isla Date: Tue, 23 Nov 2021 22:23:29 -0500 Subject: [PATCH] feat: Improve error message for ambiguous embedding - Adds a relevant hint that includes unambiguous embedding suggestions. - Joins origin and target into one single embedding key --- CHANGELOG.md | 1 + src/PostgREST/Error.hs | 46 +++++++++------- test/Feature/EmbedDisambiguationSpec.hs | 71 +++++++++++-------------- 3 files changed, 58 insertions(+), 60 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 878eebd98..55e8a9c32 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ This project adheres to [Semantic Versioning](http://semver.org/). - #1938, Allow escaping inside double quotes with a backslash, e.g. `?col=in.("Double\"Quote")`, `?col=in.("Back\\slash")` - @steve-chavez - #1075, Allow filtering top-level resource based on embedded resources filters. This is enabled by adding `!inner` to the embedded resource, e.g. `/projects?select=*,clients!inner(*)&clients.id=eq.12`- @steve-chavez, @Iced-Sun - #1988, Allow specifying `unknown` for the `is` operator - @steve-chavez +- #2031, Improve error message for ambiguous embedding and add a relevant hint that includes unambiguous embedding suggestions - @laurenceisla ### Fixed diff --git a/src/PostgREST/Error.hs b/src/PostgREST/Error.hs index cfd9b0c0f..bf676eb71 100644 --- a/src/PostgREST/Error.hs +++ b/src/PostgREST/Error.hs @@ -97,8 +97,8 @@ instance JSON.ToJSON ApiRequestError where "hint" .= ("If a new foreign key between these entities was created in the database, try reloading the schema cache." :: Text), "message" .= ("Could not find a relationship between " <> parent <> " and " <> child <> " in the schema cache" :: Text)] toJSON (AmbiguousRelBetween parent child rels) = JSON.object [ - "hint" .= ("By following the 'details' key, disambiguate the request by changing the url to /origin?select=relationship(*) or /origin?select=target!relationship(*)" :: Text), - "message" .= ("More than one relationship was found for " <> parent <> " and " <> child :: Text), + "hint" .= ("Try changing '" <> child <> "' to one of the following: " <> relHint rels <> ". Find the desired relationship in the 'details' key." :: Text), + "message" .= ("Could not embed because more than one relationship was found for '" <> parent <> "' and '" <> child <> "'" :: Text), "details" .= (compressedRel <$> rels) ] toJSON (AmbiguousRpc procs) = JSON.object [ "hint" .= ("Try renaming the parameters or the function itself in the database so function overloading can be resolved" :: Text), @@ -129,23 +129,31 @@ compressedRel Relationship{..} = fmtTbl Table{..} = tableSchema <> "." <> tableName fmtEls els = "[" <> T.intercalate ", " els <> "]" in - JSON.object $ [ - "origin" .= fmtTbl relTable - , "target" .= fmtTbl relForeignTable - ] ++ - case relCardinality of - M2M Junction{..} -> [ - "cardinality" .= ("m2m" :: Text) - , "relationship" .= (fmtTbl junTable <> fmtEls [junConstraint1] <> fmtEls [junConstraint2]) - ] - M2O cons -> [ - "cardinality" .= ("m2o" :: Text) - , "relationship" .= (cons <> fmtEls (colName <$> relColumns) <> fmtEls (colName <$> relForeignColumns)) - ] - O2M cons -> [ - "cardinality" .= ("o2m" :: Text) - , "relationship" .= (cons <> fmtEls (colName <$> relColumns) <> fmtEls (colName <$> relForeignColumns)) - ] + JSON.object $ + ("embedding" .= (tableName relTable <> " with " <> tableName relForeignTable :: Text)) + : case relCardinality of + M2M Junction{..} -> [ + "cardinality" .= ("many-to-many" :: Text) + , "relationship" .= (fmtTbl junTable <> fmtEls [junConstraint1] <> fmtEls [junConstraint2]) + ] + M2O cons -> [ + "cardinality" .= ("many-to-one" :: Text) + , "relationship" .= (cons <> fmtEls (colName <$> relColumns) <> fmtEls (colName <$> relForeignColumns)) + ] + O2M cons -> [ + "cardinality" .= ("one-to-many" :: Text) + , "relationship" .= (cons <> fmtEls (colName <$> relColumns) <> fmtEls (colName <$> relForeignColumns)) + ] + +relHint :: [Relationship] -> Text +relHint rels = T.intercalate ", " (hintList <$> rels) + where + hintList Relationship{..} = + let buildHint rel = "'" <> tableName relForeignTable <> "!" <> rel <> "'" in + case relCardinality of + M2M Junction{..} -> buildHint (tableName junTable) + M2O cons -> buildHint cons + O2M cons -> buildHint cons data PgError = PgError Authenticated SQL.UsageError type Authenticated = Bool diff --git a/test/Feature/EmbedDisambiguationSpec.hs b/test/Feature/EmbedDisambiguationSpec.hs index f3fa405cd..44bfa608f 100644 --- a/test/Feature/EmbedDisambiguationSpec.hs +++ b/test/Feature/EmbedDisambiguationSpec.hs @@ -19,20 +19,18 @@ spec = { "details": [ { - "cardinality": "m2o", + "cardinality": "many-to-one", "relationship": "message_sender_fkey[sender][id]", - "origin": "test.message", - "target": "test.person" + "embedding": "message with person" }, { - "cardinality": "m2o", + "cardinality": "many-to-one", "relationship": "message_sender_fkey[sender][id]", - "origin": "test.message", - "target": "test.person_detail" + "embedding": "message with person_detail" } ], - "hint": "By following the 'details' key, disambiguate the request by changing the url to /origin?select=relationship(*) or /origin?select=target!relationship(*)", - "message": "More than one relationship was found for message and sender" + "hint": "Try changing 'sender' to one of the following: 'person!message_sender_fkey', 'person_detail!message_sender_fkey'. Find the desired relationship in the 'details' key.", + "message": "Could not embed because more than one relationship was found for 'message' and 'sender'" } |] { matchStatus = 300 @@ -45,26 +43,23 @@ spec = { "details": [ { - "cardinality": "m2o", + "cardinality": "many-to-one", "relationship": "main_project[main_project_id][big_project_id]", - "origin": "test.sites", - "target": "test.big_projects" + "embedding": "sites with big_projects" }, { - "cardinality": "m2m", + "cardinality": "many-to-many", "relationship": "test.jobs[jobs_site_id_fkey][jobs_big_project_id_fkey]", - "origin": "test.sites", - "target": "test.big_projects" + "embedding": "sites with big_projects" }, { - "cardinality": "m2m", + "cardinality": "many-to-many", "relationship": "test.main_jobs[jobs_site_id_fkey][jobs_big_project_id_fkey]", - "origin": "test.sites", - "target": "test.big_projects" + "embedding": "sites with big_projects" } ], - "hint": "By following the 'details' key, disambiguate the request by changing the url to /origin?select=relationship(*) or /origin?select=target!relationship(*)", - "message": "More than one relationship was found for sites and big_projects" + "hint": "Try changing 'big_projects' to one of the following: 'big_projects!main_project', 'big_projects!jobs', 'big_projects!main_jobs'. Find the desired relationship in the 'details' key.", + "message": "Could not embed because more than one relationship was found for 'sites' and 'big_projects'" } |] { matchStatus = 300 @@ -77,20 +72,18 @@ spec = { "details": [ { - "cardinality": "m2o", + "cardinality": "many-to-one", "relationship": "agents_department_id_fkey[department_id][id]", - "origin": "test.agents", - "target": "test.departments" + "embedding": "agents with departments" }, { - "cardinality": "o2m", + "cardinality": "one-to-many", "relationship": "departments_head_id_fkey[id][head_id]", - "origin": "test.agents", - "target": "test.departments" + "embedding": "agents with departments" } ], - "hint": "By following the 'details' key, disambiguate the request by changing the url to /origin?select=relationship(*) or /origin?select=target!relationship(*)", - "message": "More than one relationship was found for agents and departments" + "hint": "Try changing 'departments' to one of the following: 'departments!agents_department_id_fkey', 'departments!departments_head_id_fkey'. Find the desired relationship in the 'details' key.", + "message": "Could not embed because more than one relationship was found for 'agents' and 'departments'" } |] { matchStatus = 300 @@ -106,32 +99,28 @@ spec = { "details": [ { - "cardinality": "m2m", + "cardinality": "many-to-many", "relationship": "test.whatev_jobs[whatev_jobs_site_id_1_fkey][whatev_jobs_project_id_1_fkey]", - "origin": "test.whatev_sites", - "target": "test.whatev_projects" + "embedding": "whatev_sites with whatev_projects" }, { - "cardinality": "m2m", + "cardinality": "many-to-many", "relationship": "test.whatev_jobs[whatev_jobs_site_id_1_fkey][whatev_jobs_project_id_2_fkey]", - "origin": "test.whatev_sites", - "target": "test.whatev_projects" + "embedding": "whatev_sites with whatev_projects" }, { - "cardinality": "m2m", + "cardinality": "many-to-many", "relationship": "test.whatev_jobs[whatev_jobs_site_id_2_fkey][whatev_jobs_project_id_1_fkey]", - "origin": "test.whatev_sites", - "target": "test.whatev_projects" + "embedding": "whatev_sites with whatev_projects" }, { - "cardinality": "m2m", + "cardinality": "many-to-many", "relationship": "test.whatev_jobs[whatev_jobs_site_id_2_fkey][whatev_jobs_project_id_2_fkey]", - "origin": "test.whatev_sites", - "target": "test.whatev_projects" + "embedding": "whatev_sites with whatev_projects" } ], - "hint": "By following the 'details' key, disambiguate the request by changing the url to /origin?select=relationship(*) or /origin?select=target!relationship(*)", - "message": "More than one relationship was found for whatev_sites and whatev_projects" + "hint": "Try changing 'whatev_projects' to one of the following: 'whatev_projects!whatev_jobs', 'whatev_projects!whatev_jobs', 'whatev_projects!whatev_jobs', 'whatev_projects!whatev_jobs'. Find the desired relationship in the 'details' key.", + "message": "Could not embed because more than one relationship was found for 'whatev_sites' and 'whatev_projects'" } |] { matchStatus = 300