From d16972e8da736f817be01a38603458a48b9aedd7 Mon Sep 17 00:00:00 2001 From: Laurence Isla Date: Mon, 11 Apr 2022 17:02:19 -0500 Subject: [PATCH] Fix misleading disambiguation error where `relationship` looks like valid syntax * Add columns for the m2m relationship --- CHANGELOG.md | 2 + src/PostgREST/Error.hs | 9 ++-- test/spec/Feature/EmbedDisambiguationSpec.hs | 46 +++++++++++++++----- test/spec/fixtures/privileges.sql | 1 + test/spec/fixtures/schema.sql | 6 +++ 5 files changed, 48 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d6145d779..49d11e6e1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,8 @@ This project adheres to [Semantic Versioning](http://semver.org/). - #2042, Keep working when EMFILE(Too many open files) is reached. - @steve-chavez - #2147, Ignore `Content-Type` headers for `GET` requests when calling RPCs. Previously, `GET` without parameters, but with `Content-Type: text/plain` or `Content-Type: application/octet-stream` would fail with `404 Not Found`, even if a function without arguments was available. - ``` + - #2155, Ignore `max-rows` on POST, PATCH, PUT and DELETE - @steve-chavez + - #2239, Fix misleading disambiguation error where the content of the `relationship` key looks like valid syntax - @laurenceisla ### Changed diff --git a/src/PostgREST/Error.hs b/src/PostgREST/Error.hs index bf676eb71..721ce0afc 100644 --- a/src/PostgREST/Error.hs +++ b/src/PostgREST/Error.hs @@ -126,23 +126,22 @@ instance JSON.ToJSON ApiRequestError where compressedRel :: Relationship -> JSON.Value compressedRel Relationship{..} = let - fmtTbl Table{..} = tableSchema <> "." <> tableName - fmtEls els = "[" <> T.intercalate ", " els <> "]" + fmtEls els = "(" <> T.intercalate ", " els <> ")" in 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]) + , "relationship" .= (tableName junTable <> " using " <> junConstraint1 <> fmtEls (colName <$> junColumns1) <> " and " <> junConstraint2 <> fmtEls (colName <$> junColumns2)) ] M2O cons -> [ "cardinality" .= ("many-to-one" :: Text) - , "relationship" .= (cons <> fmtEls (colName <$> relColumns) <> fmtEls (colName <$> relForeignColumns)) + , "relationship" .= (cons <> " using " <> tableName relTable <> fmtEls (colName <$> relColumns) <> " and " <> tableName relForeignTable <> fmtEls (colName <$> relForeignColumns)) ] O2M cons -> [ "cardinality" .= ("one-to-many" :: Text) - , "relationship" .= (cons <> fmtEls (colName <$> relColumns) <> fmtEls (colName <$> relForeignColumns)) + , "relationship" .= (cons <> " using " <> tableName relTable <> fmtEls (colName <$> relColumns) <> " and " <> tableName relForeignTable <> fmtEls (colName <$> relForeignColumns)) ] relHint :: [Relationship] -> Text diff --git a/test/spec/Feature/EmbedDisambiguationSpec.hs b/test/spec/Feature/EmbedDisambiguationSpec.hs index 44bfa608f..28ada4bad 100644 --- a/test/spec/Feature/EmbedDisambiguationSpec.hs +++ b/test/spec/Feature/EmbedDisambiguationSpec.hs @@ -20,12 +20,12 @@ spec = "details": [ { "cardinality": "many-to-one", - "relationship": "message_sender_fkey[sender][id]", + "relationship": "message_sender_fkey using message(sender) and person(id)", "embedding": "message with person" }, { "cardinality": "many-to-one", - "relationship": "message_sender_fkey[sender][id]", + "relationship": "message_sender_fkey using message(sender) and person_detail(id)", "embedding": "message with person_detail" } ], @@ -37,6 +37,30 @@ spec = , matchHeaders = [matchContentTypeJson] } + it "errs when there's a table and view that point to the same fk (composite pk)" $ + get "/activities?select=fst_shift(*)" `shouldRespondWith` + [json| + { + "details": [ + { + "cardinality": "one-to-many", + "relationship": "fst_shift using activities(id, schedule_id) and unit_workdays(fst_shift_activity_id, fst_shift_schedule_id)", + "embedding": "activities with unit_workdays" + }, + { + "cardinality": "one-to-many", + "relationship": "fst_shift using activities(id, schedule_id) and unit_workdays_fst_shift(fst_shift_activity_id, fst_shift_schedule_id)", + "embedding": "activities with unit_workdays_fst_shift" + } + ], + "hint": "Try changing 'fst_shift' to one of the following: 'unit_workdays!fst_shift', 'unit_workdays_fst_shift!fst_shift'. Find the desired relationship in the 'details' key.", + "message": "Could not embed because more than one relationship was found for 'activities' and 'fst_shift'" + } + |] + { matchStatus = 300 + , matchHeaders = [matchContentTypeJson] + } + it "errs when there are o2m and m2m cardinalities to the target table" $ get "/sites?select=*,big_projects(*)" `shouldRespondWith` [json| @@ -44,17 +68,17 @@ spec = "details": [ { "cardinality": "many-to-one", - "relationship": "main_project[main_project_id][big_project_id]", + "relationship": "main_project using sites(main_project_id) and big_projects(big_project_id)", "embedding": "sites with big_projects" }, { "cardinality": "many-to-many", - "relationship": "test.jobs[jobs_site_id_fkey][jobs_big_project_id_fkey]", + "relationship": "jobs using jobs_site_id_fkey(site_id) and jobs_big_project_id_fkey(big_project_id)", "embedding": "sites with big_projects" }, { "cardinality": "many-to-many", - "relationship": "test.main_jobs[jobs_site_id_fkey][jobs_big_project_id_fkey]", + "relationship": "main_jobs using jobs_site_id_fkey(site_id) and jobs_big_project_id_fkey(big_project_id)", "embedding": "sites with big_projects" } ], @@ -73,12 +97,12 @@ spec = "details": [ { "cardinality": "many-to-one", - "relationship": "agents_department_id_fkey[department_id][id]", + "relationship": "agents_department_id_fkey using agents(department_id) and departments(id)", "embedding": "agents with departments" }, { "cardinality": "one-to-many", - "relationship": "departments_head_id_fkey[id][head_id]", + "relationship": "departments_head_id_fkey using agents(id) and departments(head_id)", "embedding": "agents with departments" } ], @@ -100,22 +124,22 @@ spec = "details": [ { "cardinality": "many-to-many", - "relationship": "test.whatev_jobs[whatev_jobs_site_id_1_fkey][whatev_jobs_project_id_1_fkey]", + "relationship": "whatev_jobs using whatev_jobs_site_id_1_fkey(site_id_1) and whatev_jobs_project_id_1_fkey(project_id_1)", "embedding": "whatev_sites with whatev_projects" }, { "cardinality": "many-to-many", - "relationship": "test.whatev_jobs[whatev_jobs_site_id_1_fkey][whatev_jobs_project_id_2_fkey]", + "relationship": "whatev_jobs using whatev_jobs_site_id_1_fkey(site_id_1) and whatev_jobs_project_id_2_fkey(project_id_2)", "embedding": "whatev_sites with whatev_projects" }, { "cardinality": "many-to-many", - "relationship": "test.whatev_jobs[whatev_jobs_site_id_2_fkey][whatev_jobs_project_id_1_fkey]", + "relationship": "whatev_jobs using whatev_jobs_site_id_2_fkey(site_id_2) and whatev_jobs_project_id_1_fkey(project_id_1)", "embedding": "whatev_sites with whatev_projects" }, { "cardinality": "many-to-many", - "relationship": "test.whatev_jobs[whatev_jobs_site_id_2_fkey][whatev_jobs_project_id_2_fkey]", + "relationship": "whatev_jobs using whatev_jobs_site_id_2_fkey(site_id_2) and whatev_jobs_project_id_2_fkey(project_id_2)", "embedding": "whatev_sites with whatev_projects" } ], diff --git a/test/spec/fixtures/privileges.sql b/test/spec/fixtures/privileges.sql index b6899e012..78aa30405 100644 --- a/test/spec/fixtures/privileges.sql +++ b/test/spec/fixtures/privileges.sql @@ -134,6 +134,7 @@ GRANT ALL ON TABLE , schedules , activities , unit_workdays + , unit_workdays_fst_shift , stuff , loc_test , v1.parents diff --git a/test/spec/fixtures/schema.sql b/test/spec/fixtures/schema.sql index 43f0ed851..0d3fd4c95 100644 --- a/test/spec/fixtures/schema.sql +++ b/test/spec/fixtures/schema.sql @@ -2013,6 +2013,12 @@ add constraint fst_shift foreign key (fst_shift_activity_id, fst_shift add constraint snd_shift foreign key (snd_shift_activity_id, snd_shift_schedule_id) references activities (id, schedule_id); +create view unit_workdays_fst_shift as +select unit_id, day, fst_shift_activity_id, fst_shift_schedule_id +from unit_workdays +where fst_shift_activity_id is not null + and fst_shift_schedule_id is not null; + -- for a pre-request function create or replace function custom_headers() returns void as $$ declare