Add records.list rpc function
#3691
Conversation
There was a problem hiding this comment.
Looks good!
-
I skimmed the code. No red flags stuck out to me. Though I imagine Anish will go over in it more detail.
-
I ran it and verified that it works.
-
I stress-tested the
limit,offset, andorderparameters a bit and couldn't find anything amiss. -
The general structure of the output looks good.
The only (slight) concern I have is this...
The cell values for some types are not formatted the same way as in the REST API. For example:
| REST | RPC |
|---|---|
1960-07-24 AD |
1960-07-24 |
2014-04-21T17:30:44.0 AD |
2014-04-21T17:30:44 |
P0Y0M0DT777H55M39S |
777:55:39 |
It's hard for me to quickly identify whether the change in formatting will be problematic or not. We would certainly notice this later down the road during QA though, so I'm okay kicking this can down the road and seeing what problems pop up. They'll be easier to address when we have this new RPC API better integrated into the front end, because at that point we'll be able to make a more informed decision about the quickest path to fixing any issues (i.e. front end or back end).
I'll look into the formatting. Great catch, thank you! |
|
@Anish9901 When you review: I just ended up emulating the current REST endpoint responses w.r.t. the formatting. I validated this by adding tests adapted directly from the python tests for type formatting. However, there were some preexisting problems that remain as of this PR:
I think we can address these issues as feature requests in a future PR. |
Also added a merged migration to fix multiple leaf nodes issue.
There was a problem hiding this comment.
The PR looks great overall @mathemancer, and I find the idea of using the attnums as column identifiers instead of referring to them by their names brilliant! However, I think I have found a pretty significant bug here which is worth fixing.
db/sql/00_msar.sql
Outdated
| SELECT string_agg(format('msar.format_data(%I) AS %I', attname, attnum), ', ') | ||
| FROM pg_catalog.pg_attribute | ||
| WHERE | ||
| attrelid = tab_id AND attnum > 0 AND has_column_privilege(attrelid, attnum, 'SELECT'); |
There was a problem hiding this comment.
This breaks for a table from which previously existing column(s) have been dropped.
Here is the specific error:
mathesar=# select msar.list_records_from_table('"Library Management"."Items"'::regclass::oid, NULL, NULL, NULL, NULL, NULL, NULL);
ERROR: column "........pg.dropped.4........" does not exist
LINE 5: ...data("Acquisition Date") AS "3", msar.format_data("........p...
^
QUERY:
WITH count_cte AS (
SELECT count(1) AS count FROM "Library Management"."Items"
), results_cte AS (
SELECT msar.format_data(id) AS "1", msar.format_data("Barcode") AS "2", msar.format_data("Acquisition Date") AS "3", msar.format_data("........pg.dropped.4........") AS "4", msar.format_data("Book") AS "5" FROM "Library Management"."Items" ORDER BY "1" ASC LIMIT NULL OFFSET NULL
)
SELECT jsonb_build_object(
'results', jsonb_agg(row_to_json(results_cte.*)),
'count', max(count_cte.count)
)
FROM results_cte, count_cte
CONTEXT: PL/pgSQL function msar.list_records_from_table(oid,integer,integer,jsonb,jsonb,jsonb,jsonb) line 19 at EXECUTEThere was a problem hiding this comment.
Nice catch! I've pushed a fix and a test. Should be fine now.
Related to #3690
Adds an initial
records.listRPC function.Technical details
The
filter,group, andsearchkwargs aren't wired up yet, but can be submitted.As usual, the included documentation should be sufficient to describe the functions for review.
Checklist
Update index.md).developbranch of the repositoryvisible errors.
Developer Certificate of Origin
Developer Certificate of Origin