Add link Table API #1374

silentninja · 2022-05-10T17:41:32Z

Fixes #972

Adds APIs to support creating relationships between columns.

Technical details
As per the discussion in #972, the finalised APIs are

interface OneToOneOrOneToMany {
  link_type: "one-to-one" | "one-to-many";
  reference_table: number;
  reference_column_name: string;
  referent_table: number;
}

interface ManyToMany {
  link_type: "many-to-many";
  mapping_table_name: string;
  referents: {
    referent_table: number;
    column_name: string;
  }[];
}

type LinkTablePostRequest = OneToOneOrOneToMany | ManyToMany;

Checklist

My pull request has a descriptive title (not a vague title like Update index.md).
My pull request targets the master 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.

…er and operations to created the link

…link

…links

…nt_column_name`

…nstead of abbreviated names.

…thesar-972-link-table

…queryset

codecov-commenter · 2022-05-11T00:11:06Z

Codecov Report

Merging #1374 (3da93cb) into master (a73f496) will increase coverage by 0.04%.
The diff coverage is 95.04%.

@@            Coverage Diff             @@
##           master    #1374      +/-   ##
==========================================
+ Coverage   92.99%   93.03%   +0.04%     
==========================================
  Files         120      123       +3     
  Lines        4865     4982     +117     
==========================================
+ Hits         4524     4635     +111     
- Misses        341      347       +6

Flag	Coverage Δ
pytest-backend	`93.03% <95.04%> (+0.04%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...api/exceptions/validation_exceptions/exceptions.py	`88.88% <75.00%> (-2.42%)`	⬇️
mathesar/api/exceptions/mixins.py	`84.21% <80.00%> (-0.98%)`	⬇️
mathesar/api/db/viewsets/links.py	`88.88% <88.88%> (ø)`
mathesar/api/serializers/links.py	`97.72% <97.72%> (ø)`
db/links/operations/create.py	`100.00% <100.00%> (ø)`
db/tables/operations/select.py	`100.00% <100.00%> (ø)`
mathesar/api/db/viewsets/__init__.py	`100.00% <100.00%> (ø)`
mathesar/api/exceptions/error_codes.py	`100.00% <100.00%> (ø)`
mathesar/urls.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a73f496...3da93cb. Read the comment docs.

mathemancer

I left some specific comments. The big one to deal with is about creating the foreign key columns as a single step.

mathemancer · 2022-05-11T05:20:39Z

db/links/operations/create.py

+        }
+        ctx = MigrationContext.configure(conn, opts=opts)
+        op = Operations(ctx)
+        op.create_table(map_table_name, schema=schema)


I think you should use the standard Mathesar function for creating a table. If nothing else, we'll want the default columns so this fits into the rest of the framework, correct?

create_mathesar_table doesn't respect the transaction. The below test case fails due to create_table not being part of the transaction and ends up creating the tables although the foreign key constraint step fails. I am not sure if I am doing something wrong

def test_table_creation(engine_with_schema): engine, schema = engine_with_schema supported_types = get_supported_alter_column_types( engine, friendly_names=False, ) sa_type = supported_types.get("INTEGER") metadata = MetaData(bind=engine, schema=schema, naming_convention=naming_convention) with engine.begin() as conn: metadata.reflect() opts = { 'target_metadata': metadata } ctx = MigrationContext.configure(conn, opts=opts) referent_column = MathesarColumn( "reference", sa_type() ) target_table = create_mathesar_table( "target_table", schema, [referent_column], engine, metadata, ) fk_column = MathesarColumn( "fk_col", sa_type() ) base_table = create_mathesar_table( "base_table", schema, [fk_column], engine, metadata ) op = Operations(ctx) # op.create_unique_constraint(None, target_table.name, [referent_column.name], schema=schema) try: op.create_foreign_key( None, base_table.name, target_table.name, [fk_column.name], [referent_column.name], source_schema=schema, referent_schema=schema ) except: pass schema_obj = reflect_schema(engine, schema) tables = get_table_oids_from_schema(schema_obj.oid, engine) # Table should not be created as foreign key creation step would have failed due to referent column missing the unique constraint assert len(tables) == 0

The transaction fails if create_mathesar_table uses a connection to bind instead of engine when creating the table

https://github.com/centerofci/mathesar/blob/b2467dcf63de786c0e8063821ff317ef608ec3a6/db/tables/operations/create.py#L28
~~table.create(engine)~~
table.create(connection)

Argh, what a pain.

I think it would still be better to use the create_mathesar_table function to create the table, since it's important that we have the pieces needed (id column at a minimum). We should then file a bug to deal with the fact that transactions aren't working properly.

I really want to avoid ad-hoc table creation calls scattered through the codebase since we already have a default column involved and we're going to add more default metadata, etc. over time.

mathemancer · 2022-05-11T05:25:12Z

db/links/operations/create.py

+        column = MathesarColumn(
+            column_name, primary_key_column.type
+        )
+        op.add_column(referrer_table.name, column, schema=schema)
+        if unique_link:
+            op.create_unique_constraint(None, referrer_table.name, [column_name], schema=schema)
+        op.create_foreign_key(
+            None,
+            referrer_table.name,
+            referent_table.name,
+            [column.name],
+            [primary_key_column.name],
+            source_schema=schema,
+            referent_schema=schema
+        )


Unless there's a convincing reason not to, I think you should create the column with the unique constraint and foreign key included, then add that column to the table. This would make this simpler and more aligned with the foreign key creation in tb/tables/operations/split.py. I can think of no reason to do it in separate operations given that we're not supporting multi-column foreign key creation with this.

Isn't creating a foreign key constraint in individual steps much more flexible, as it could allow for multiple columns, although we don't use it now. Does creating a Foreign key as part of the column bring in any advantage?

I think the main advantage is that it's easier to see the logic. I acknowledge, however, that it's more flexible in separate steps. If you want to keep separate steps, I can go with it.

mathemancer · 2022-05-11T05:37:47Z

db/links/operations/create.py

+        for referent in referents:
+            referent_table_oid = referent['referent_table']
+            referent_table = referent_tables[referent_table_oid]
+            col_name = referent['column_name']
+            primary_key_column = get_primary_key_column(referent_table)
+            column = MathesarColumn(
+                col_name, primary_key_column.type
+            )
+            op.add_column(map_table_name, column, schema=schema)
+            op.create_foreign_key(
+                None,
+                map_table_name,
+                referent_table.name,
+                [column.name],
+                [primary_key_column.name],
+                source_schema=schema,
+                referent_schema=schema
+            )


Why not use your create_foreign_key_link function? Nested transactions don't commit until the outermost transaction is complete. If there's a reason not to call the other function, the comment from above about creating the column with the needed pieces then adding it applies here as well.

It seemed redundant as both the signature are the same, but I agree it would be better to use a common API.

In that case, consider extracting the common code from the two functions and using it as a private method.

db/tables/operations/select.py

seancolsen

@silentninja I'm not able to get any requests to succeed, as spec'ed in your schema. Below are four scenarios that I've tried. Each scenario lists the request I sent. In each scenario, I expect a success response. And in each scenario, I observed an error (with the exact response shown that I received).

To me it seems like there are probably a number of small things in this PR that need to be adjusted in order for my API requests to succeed. For example, I'd like to use the property reference_table_id instead of reference_table. And in the "one-to-one" case, I think we should be specifying reference_column_name not referent_column_name.

One to one

Request:

{
  "link_type": "one-to-one",
  "reference_table_id": 4,
  "reference_column_name": "planet_id_1",
  "referent_table_id": 2
}

Response:

[
  {
    "code": 2002,
    "field": "referent_column_name",
    "message": "This field is required.",
    "detail": {}
  },
  {
    "code": 2007,
    "field": "reference_table",
    "message": "This field is required.",
    "detail": {}
  },
  {
    "code": 2007,
    "field": "referent_table",
    "message": "This field is required.",
    "detail": {}
  }
]

Many to one

Request:

{
  "link_type": "many-to-one",
  "reference_table_id": 4,
  "reference_column": { "name": "planet_id_1" },
  "referent_table_id": 2
}

Response:

[
  {
    "code": 4999,
    "message": "'NoneType' object has no attribute 'create'",
    "field": null,
    "detail": null
  }
]

One to many

Request:

{
  "link_type": "one-to-many",
  "reference_table_id": 4,
  "referent_table_id": 2,
  "referent_column_name": "moon_id"
}

Response:

[
  {
    "code": 2007,
    "field": "reference_table",
    "message": "This field is required.",
    "detail": {}
  },
  {
    "code": 2007,
    "field": "referent_table",
    "message": "This field is required.",
    "detail": {}
  }
]

Many to many

Request:

{
  "link_type": "many-to-many",
  "mapping_table_name": "moon_planet",
  "referents": [
    { "table_id": 4, "column_name": "moon_id" },
    { "table_id": 2, "column_name": "planet_id" }
  ]
}

Response:

[
  {
    "code": 4999,
    "message": "unhashable type: 'dict'",
    "field": null,
    "detail": null
  }
]

…thesar-972-link-table

silentninja · 2022-05-14T06:13:41Z

@seancolsen

One to one error - I made changes to the API schema, please do try out the schema listed in the PR's description
Many to one error - There is no Many to one type, I added correct validation error instead of throwing a 500 error
One to many - Please do try out the schema listed in the PR's description.
Many to many - Fixed the error handler to return correct validation, it should throw a correct error now

…_name`

…_name` in test cases

…thesar-972-link-table

mathemancer

The only blocking change I have to request is to figure out how to avoid the bare table creation. I think it's already a problem since it's not created with the default column(s), and it may get further from how other tables are created over time. The other request is a suggestion.

mathemancer · 2022-05-19T05:27:44Z

db/links/operations/create.py

+        }
+        ctx = MigrationContext.configure(conn, opts=opts)
+        op = Operations(ctx)
+        op.create_table(map_table_name, schema=schema)


Argh, what a pain.

I think it would still be better to use the create_mathesar_table function to create the table, since it's important that we have the pieces needed (id column at a minimum). We should then file a bug to deal with the fact that transactions aren't working properly.

I really want to avoid ad-hoc table creation calls scattered through the codebase since we already have a default column involved and we're going to add more default metadata, etc. over time.

mathemancer · 2022-05-19T05:36:00Z

db/links/operations/create.py

+        for referent in referents:
+            referent_table_oid = referent['referent_table']
+            referent_table = referent_tables[referent_table_oid]
+            col_name = referent['column_name']
+            primary_key_column = get_primary_key_column(referent_table)
+            column = MathesarColumn(
+                col_name, primary_key_column.type
+            )
+            op.add_column(map_table_name, column, schema=schema)
+            op.create_foreign_key(
+                None,
+                map_table_name,
+                referent_table.name,
+                [column.name],
+                [primary_key_column.name],
+                source_schema=schema,
+                referent_schema=schema
+            )


In that case, consider extracting the common code from the two functions and using it as a private method.

# Conflicts: # mathesar/api/exceptions/error_codes.py

…able` function

mathemancer

I think it looks good; let's get it merged!

Test cases for the requested changes are passing with the latest commits

silentninja added 10 commits May 5, 2022 15:37

Add Links Serializer

c983bcc

Add one to one foreign key and one to many foreign key links serializ…

83dba6e

…er and operations to created the link

Add viewset for one to one foreign key and one to many foreign key links

de99d78

Add test cases for ManyToMany links

687a702

Add function to create many to many to the db module

d98ed6a

Add Many to Many Links serializer

2a253e7

Add support for specifying column names when creating a Many to Many …

06a4a67

…link

Add API test cases to test many to many links and self-referential …

ec95f6d

…links

Change one to one link API schema to field column_name to `refere…

d5289f4

…nt_column_name`

Change link identifiers in link API schema to use expanded name i…

be623a7

…nstead of abbreviated names.

silentninja requested review from a team, pavish, mathemancer and seancolsen and removed request for a team and pavish May 10, 2022 17:41

silentninja added 3 commits May 10, 2022 21:42

Merge branch 'master' into mathesar-972-link-table

5e829ed

Remove type hints

d5dff57

Merge remote-tracking branch 'origin/mathesar-972-link-table' into ma…

fb8f7cd

…thesar-972-link-table

silentninja changed the title ~~Mathesar 972 link table~~ Add link Table API May 10, 2022

silentninja added 2 commits May 10, 2022 22:29

Remove stale test case

c822407

Change PrimaryKeyRelatedField fields query set to use non-reflecting …

da4d4a8

…queryset

seancolsen self-assigned this May 11, 2022

seancolsen added the pr-status: review A PR awaiting review label May 11, 2022

mathemancer requested changes May 11, 2022

View reviewed changes

seancolsen previously requested changes May 11, 2022

View reviewed changes

mathemancer self-assigned this May 12, 2022

silentninja added 2 commits May 14, 2022 08:29

Merge remote-tracking branch 'origin/mathesar-972-link-table' into ma…

4d24f8e

…thesar-972-link-table

Merge branch 'master' into mathesar-972-link-table

d8e5a85

silentninja requested a review from seancolsen May 14, 2022 04:36

Fix MathesarErrorMessageMixin to handle ListSerializers

5e6f877

silentninja added 5 commits May 16, 2022 15:56

Merge branch 'master' into mathesar-972-link-table

0077c9b

Rename OneToOneSerializer referent_column_name to `reference_column…

56dd2e6

…_name`

Rename OneToOneSerializer referent_column_name to `reference_column…

91d2e2d

…_name` in test cases

Merge remote-tracking branch 'origin/mathesar-972-link-table' into ma…

dc3da74

…thesar-972-link-table

Fix serialize error mixin to handle ListSerializer errors

48dd06b

silentninja requested a review from mathemancer May 17, 2022 04:22

mathemancer requested changes May 19, 2022

View reviewed changes

silentninja added pr-status: revision A PR awaiting follow-up work from its author after review and removed pr-status: review A PR awaiting review labels May 20, 2022

silentninja assigned silentninja and unassigned seancolsen and mathemancer May 20, 2022

silentninja added 2 commits May 30, 2022 20:32

Merge branch 'master' into mathesar-972-link-table

ab1084d

# Conflicts: # mathesar/api/exceptions/error_codes.py

Refactor create many to many links function to use `create_mathesar_t…

3da93cb

…able` function

silentninja assigned mathemancer and unassigned silentninja May 31, 2022

silentninja requested a review from mathemancer May 31, 2022 01:30

mathemancer approved these changes May 31, 2022

View reviewed changes

Merge branch 'master' into mathesar-972-link-table

23d4ada

mathemancer enabled auto-merge May 31, 2022 06:49

silentninja removed the request for review from seancolsen May 31, 2022 07:25

mathemancer merged commit f656572 into master May 31, 2022

mathemancer deleted the mathesar-972-link-table branch May 31, 2022 07:27

seancolsen mentioned this pull request Jun 6, 2022

Finish Link Table UI #1422

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add link Table API #1374

Add link Table API #1374

silentninja commented May 10, 2022 •

edited by seancolsen

Loading

codecov-commenter commented May 11, 2022 •

edited

Loading

mathemancer left a comment

mathemancer May 11, 2022

silentninja May 13, 2022 •

edited by kgodey

Loading

silentninja May 13, 2022

mathemancer May 19, 2022

mathemancer May 11, 2022

silentninja May 13, 2022

mathemancer May 19, 2022 •

edited

Loading

mathemancer May 11, 2022

silentninja May 13, 2022

mathemancer May 19, 2022

seancolsen left a comment

silentninja commented May 14, 2022

mathemancer left a comment

mathemancer May 19, 2022

mathemancer May 19, 2022

mathemancer left a comment

Add link Table API #1374

Add link Table API #1374

Conversation

silentninja commented May 10, 2022 • edited by seancolsen Loading

Checklist

Developer Certificate of Origin

codecov-commenter commented May 11, 2022 • edited Loading

Codecov Report

mathemancer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silentninja May 13, 2022 • edited by kgodey Loading

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 May 19, 2022 • edited Loading

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

seancolsen left a comment

Choose a reason for hiding this comment

One to one

Many to one

One to many

Many to many

silentninja commented May 14, 2022

mathemancer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathemancer left a comment

Choose a reason for hiding this comment

silentninja commented May 10, 2022 •

edited by seancolsen

Loading

codecov-commenter commented May 11, 2022 •

edited

Loading

silentninja May 13, 2022 •

edited by kgodey

Loading

mathemancer May 19, 2022 •

edited

Loading