Add `columns.list` function #3556

mathemancer · 2024-04-26T07:16:32Z

This adds an RPC function called columns.list that takes a database_id and table_oid as input, and returns information about the columns of the identified table.

Technical details

The function is documented in the new documentation added in this PR, and those docs should be sufficient to explain its usage, and the return format. If not, request a change to those docs.
The SQL test suite was modified to avoid using the pgTAP setup function feature, since that bogs the tests down too much.
This PR also provides an example of how and where I think we should organize the code for the RPC functions. Of note:
- mathesar.rpc.columns.list_ is kept pretty simple. It's responsible for getting a connection using the provided utility function, and then calls other functions to actually gather data.
- mathesar.rpc.columns.list_ doesn't directly use the connection to access the database, but it does call the db access function within a managed context.
- mathesar.utils.columns.get_raw_display_options is a bit of scaffolding for now, since the new model hasn't been made yet.
- However, its signature shouldn't change. In particular, it's supposed to manage access using the passed database_id and user
- The mathesar.rpc.utils.connect function is also a stub for now, but its signature should also stay the same.
- The tests are supposed to serve as an example of how I think we should proceed on testing the backend, at least until beta.
- No actual calls to the database for unit testing.
- More mocking to isolate functionality of piece being tested.
- mathesar.tests.rpc.test_endpoints tests that the expected functions are shown, at that each is defined with the expected decorators are applied. Nothing more.

Checklist

My pull request has a descriptive title (not a vague title like Update index.md).
My pull request targets the develop branch of the repository
My commit messages follow best practices.
My code follows the established code style of the repository.
I added tests for the changes I made (if applicable).
I added or updated documentation (if applicable).
I tried running the project locally and verified that there are no
visible errors.

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

Anish9901

Looks great overall, I've requested some minor changes.

Anish9901 · 2024-05-08T13:28:32Z

mathesar/rpc/columns.py

+from mathesar.utils.columns import get_raw_display_options
+
+
+class TypeOptions(TypedDict, total=False):


What is the total flag for?

In this case, setting it to False is an explicit flag to let the reader know that not all fields of the dictionary are required. It's not enforced in any way (except by linters).

Anish9901 · 2024-05-08T13:29:10Z

mathesar/rpc/utils.py

+    Args:
+        db_id: The Django id corresponding to the Database.
+    """
+    print("User is: ", user)


You missed a print statement here.

I actually left that on purpose for the moment. My thinking is that this function body is going to be replaced once we have the permissions stuff worked out. For now, I'd like to leave that as a placeholder for logic that will use the user to get a connection credential.

Anish9901 · 2024-05-08T13:37:12Z

mathesar/database/base.py

+def get_psycopg_connection(db_model):
+    """
+    Get a psycopg connection, given a Database model.
+
+    Args:
+        db_model: The Django model corresponding to the Database.
+    """
+    return psycopg.connect(
+        host=db_model.host,
+        port=db_model.port,
+        dbname=db_model.db_name,
+        user=db_model.username,
+        password=db_model.password,
+    )


Why not have this in connect() itself? I thought we discussed that we'd always need to call connect with appropriate arguments and won't be opening random connections elsewhere.

For any user request, this is true. However, we have a few things (installation, upgrade) that won't have a user associated with them. I wanted to leave this function available for those use cases.

Anish9901 · 2024-05-08T13:41:25Z

db/sql/0_msar.sql

+  WHEN 1 << 2 THEN 'year'
+  WHEN 1 << 1 THEN 'month'
+  WHEN 1 << 3 THEN 'day'
+  WHEN 1 << 10 THEN 'hour'
+  WHEN 1 << 11 THEN 'minute'
+  WHEN 1 << 12 THEN 'second'
+  WHEN (1 << 2) | (1 << 1) THEN 'year to month'
+  WHEN (1 << 3) | (1 << 10) THEN 'day to hour'
+  WHEN (1 << 3) | (1 << 10) | (1 << 11) THEN 'day to minute'
+  WHEN (1 << 3) | (1 << 10) | (1 << 11) | (1 << 12) THEN 'day to second'
+  WHEN (1 << 10) | (1 << 11) THEN 'hour to minute'
+  WHEN (1 << 10) | (1 << 11) | (1 << 12) THEN 'hour to second'
+  WHEN (1 << 11) | (1 << 12) THEN 'minute to second'


This is amazing!!!

Anish9901 · 2024-05-08T14:08:07Z

mathesar/tests/rpc/test_columns.py

+from mathesar.models.users import User
+
+
+def test_columns_list(rf, monkeypatch):


Can you please add a docstring for the fixtures used here?

Good catch. I'll add a docstring at the top of the file. I think that's going to be better in general, since fixtures are defined in such a way that they're file-wide and not restricted to any given test function.

mathemancer · 2024-05-09T06:29:48Z

Okay, I think I've addressed everything, @Anish9901 .

Anish9901

Looks great!

mathemancer added 5 commits April 18, 2024 15:01

add initial db connection getting stub

115485d

Merge branch 'architectural_overhaul' into permissions_stub

aeaab4f

first pass at functions to build the type_options JSON

518b7e1

add column info getting function and utils

6cb7ba0

use different style to denote arrays, clean up code

96338c7

mathemancer linked an issue Apr 26, 2024 that may be closed by this pull request

Implement stub function to get DB connection during development #3549

Closed

mathemancer added 16 commits April 29, 2024 15:58

add/improve docstrings of column info getter SQL functions

2904112

remove dependency on mathesar_types existing

66ff45c

add SQL tests for interval field builder

c2e9df5

test get_type_options function, fix bug

a6c2983

add tests for textual type options

cee460b

move some setup functions into callers for efficiency

1d465b8

move remaining setup functions into callers for efficiency

fd3077e

add get_column_info test

d44c0b8

add column info getter in python layer

31829ae

wire column info getter up to RPC endpoint

993ee06

remove valid_target_types as it should go somewhere global

784276c

fix typo in docstring

77ed379

add class to validate and describe column info response

b2f15ac

stub out permissions checks

2c15a07

add classes to specify type options and defaults

45f3994

add class describing return value overall

41ff1b2

seancolsen assigned mathemancer May 6, 2024

mathemancer added 6 commits May 6, 2024 23:35

add tests for wiring of RPC functions

4f585cd

move permission check to display_option function

3a5a394

add test for column lister RPC function

b7c91d1

fix linting issues

68b5ce0

reduce test scope, improve focus

9278b6f

add test for wiring of column info getter to DB

847ded4

mathemancer requested a review from Anish9901 May 7, 2024 08:06

mathemancer assigned Anish9901 and unassigned mathemancer May 7, 2024

mathemancer changed the base branch from develop to architectural_overhaul May 7, 2024 08:30

mathemancer added the pr-status: review A PR awaiting review label May 7, 2024

mathemancer added this to the Beta milestone May 7, 2024

mathemancer marked this pull request as ready for review May 7, 2024 08:33

mathemancer changed the title ~~Permissions stub~~ Add columns.list function May 7, 2024

Anish9901 requested changes May 8, 2024

View reviewed changes

document fixtures in test_columns.py

6169653

mathemancer requested a review from Anish9901 May 9, 2024 06:29

fix fixture name typo

7740f76

seancolsen mentioned this pull request May 9, 2024

Process record summaries in DB layer #3561

Open

7 tasks

Anish9901 approved these changes May 9, 2024

View reviewed changes

Anish9901 merged commit ddab8b9 into architectural_overhaul May 9, 2024
34 checks passed

Anish9901 deleted the permissions_stub branch May 9, 2024 13:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `columns.list` function #3556

Add `columns.list` function #3556

mathemancer commented Apr 26, 2024 •

edited

Anish9901 left a comment

Anish9901 May 8, 2024

mathemancer May 9, 2024

Anish9901 May 8, 2024

mathemancer May 9, 2024

Anish9901 May 8, 2024

mathemancer May 9, 2024

Anish9901 May 8, 2024

Anish9901 May 8, 2024

mathemancer May 9, 2024

mathemancer commented May 9, 2024

Anish9901 left a comment

		from mathesar.utils.columns import get_raw_display_options


		class TypeOptions(TypedDict, total=False):

		from mathesar.models.users import User


		def test_columns_list(rf, monkeypatch):

Add columns.list function #3556

Add columns.list function #3556

Conversation

mathemancer commented Apr 26, 2024 • edited

Checklist

Developer Certificate of Origin

Anish9901 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathemancer commented May 9, 2024

Anish9901 left a comment

Choose a reason for hiding this comment

Add `columns.list` function #3556

Add `columns.list` function #3556

mathemancer commented Apr 26, 2024 •

edited