Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add link Table API #1374

Merged
merged 30 commits into from
May 31, 2022
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
c983bcc
Add Links Serializer
silentninja May 5, 2022
83dba6e
Add one to one foreign key and one to many foreign key links serializ…
silentninja May 6, 2022
de99d78
Add viewset for one to one foreign key and one to many foreign key links
silentninja May 6, 2022
687a702
Add test cases for `ManyToMany` links
silentninja May 7, 2022
d98ed6a
Add function to create many to many to the `db` module
silentninja May 7, 2022
2a253e7
Add Many to Many Links serializer
silentninja May 7, 2022
06a4a67
Add support for specifying column names when creating a Many to Many …
silentninja May 7, 2022
ec95f6d
Add API test cases to test `many to many links` and self-referential …
silentninja May 8, 2022
d5289f4
Change `one to one` link API schema to field `column_name` to `refere…
silentninja May 10, 2022
be623a7
Change `link` identifiers in `link` API schema to use expanded name i…
silentninja May 10, 2022
5e829ed
Merge branch 'master' into mathesar-972-link-table
silentninja May 10, 2022
d5dff57
Remove type hints
silentninja May 10, 2022
fb8f7cd
Merge remote-tracking branch 'origin/mathesar-972-link-table' into ma…
silentninja May 10, 2022
c822407
Remove stale test case
silentninja May 10, 2022
da4d4a8
Change PrimaryKeyRelatedField fields query set to use non-reflecting …
silentninja May 10, 2022
e0b10e6
Fix `db.tables.operations.select.reflect_table_from_oid` to use `db.t…
silentninja May 12, 2022
5f5294e
Merge 'master' into 'mathesar-972-link-table'
silentninja May 13, 2022
3dc41c0
Merge branch 'master' into mathesar-972-link-table
silentninja May 14, 2022
e8d392d
Add default link_type to Link Serializers in order to get correct lin…
silentninja May 14, 2022
4d24f8e
Merge remote-tracking branch 'origin/mathesar-972-link-table' into ma…
silentninja May 14, 2022
d8e5a85
Merge branch 'master' into mathesar-972-link-table
silentninja May 14, 2022
5e6f877
Fix `MathesarErrorMessageMixin` to handle `ListSerializers`
silentninja May 14, 2022
0077c9b
Merge branch 'master' into mathesar-972-link-table
silentninja May 16, 2022
56dd2e6
Rename `OneToOneSerializer referent_column_name` to `reference_column…
silentninja May 16, 2022
91d2e2d
Rename `OneToOneSerializer referent_column_name` to `reference_column…
silentninja May 16, 2022
dc3da74
Merge remote-tracking branch 'origin/mathesar-972-link-table' into ma…
silentninja May 16, 2022
48dd06b
Fix serialize error mixin to handle `ListSerializer` errors
silentninja May 16, 2022
ab1084d
Merge branch 'master' into mathesar-972-link-table
silentninja May 30, 2022
3da93cb
Refactor create many to many links function to use `create_mathesar_t…
silentninja May 31, 2022
23d4ada
Merge branch 'master' into mathesar-972-link-table
mathemancer May 31, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Empty file added db/links/__init__.py
Empty file.
Empty file added db/links/operations/__init__.py
Empty file.
67 changes: 67 additions & 0 deletions db/links/operations/create.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
from alembic.operations import Operations
from alembic.migration import MigrationContext
from sqlalchemy import MetaData

from db.columns.base import MathesarColumn
from db.constraints.utils import naming_convention
from db.tables.operations.select import reflect_table_from_oid, reflect_tables_from_oids
from db.tables.utils import get_primary_key_column


def create_foreign_key_link(engine, schema, column_name, referrer_table_oid, referent_table_oid, unique_link=False):
with engine.begin() as conn:
referent_table = reflect_table_from_oid(referent_table_oid, engine, conn)
referrer_table = reflect_table_from_oid(referrer_table_oid, engine, conn)
primary_key_column = get_primary_key_column(referent_table)
metadata = MetaData(bind=engine, schema=schema, naming_convention=naming_convention)
opts = {
'target_metadata': metadata
}
ctx = MigrationContext.configure(conn, opts=opts)
op = Operations(ctx)
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
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@mathemancer mathemancer May 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.



def create_many_to_many_link(engine, schema, map_table_name, referents):
with engine.begin() as conn:
referent_tables_oid = [referent['referent_table'] for referent in referents]
referent_tables = reflect_tables_from_oids(referent_tables_oid, engine, conn)
metadata = MetaData(bind=engine, schema=schema, naming_convention=naming_convention)
opts = {
'target_metadata': metadata
}
ctx = MigrationContext.configure(conn, opts=opts)
op = Operations(ctx)
op.create_table(map_table_name, schema=schema)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@silentninja silentninja May 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

25 changes: 25 additions & 0 deletions db/tables/operations/select.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,31 @@ def reflect_table_from_oid(oid, engine, connection_to_use=None):
return reflect_table(table_name, schema, engine, connection_to_use=connection_to_use)


def reflect_tables_from_oids(oids, engine, connection_to_use=None):
silentninja marked this conversation as resolved.
Show resolved Hide resolved
metadata = MetaData()

with warnings.catch_warnings():
warnings.filterwarnings("ignore", message="Did not recognize type")
pg_class = Table("pg_class", metadata, autoload_with=engine)
pg_namespace = Table("pg_namespace", metadata, autoload_with=engine)
sel = (
select(pg_namespace.c.nspname, pg_class.c.relname, pg_class.c.oid)
.select_from(
join(
pg_class,
pg_namespace,
pg_class.c.relnamespace == pg_namespace.c.oid
)
)
.where(pg_class.c.oid.in_(oids))
)
results = execute_statement(engine, sel, connection_to_use).fetchall()
tables = {}
for (schema, table_name, table_oid) in results:
tables[table_oid] = reflect_table(table_name, schema, engine, connection_to_use=connection_to_use)
return tables


def get_table_oids_from_schema(schema_oid, engine):
metadata = MetaData()

Expand Down
Empty file added db/tests/links/__init__.py
Empty file.
1 change: 1 addition & 0 deletions mathesar/api/db/viewsets/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,4 @@
from mathesar.api.db.viewsets.records import RecordViewSet # noqa
from mathesar.api.db.viewsets.schemas import SchemaViewSet # noqa
from mathesar.api.db.viewsets.tables import TableViewSet # noqa
from mathesar.api.db.viewsets.links import LinkViewSet # noqa
12 changes: 12 additions & 0 deletions mathesar/api/db/viewsets/links.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
from rest_framework.mixins import CreateModelMixin, ListModelMixin
from rest_framework.viewsets import GenericViewSet
from mathesar.api.pagination import DefaultLimitOffsetPagination
from mathesar.api.serializers.links import LinksMappingSerializer


class LinkViewSet(CreateModelMixin, ListModelMixin, GenericViewSet):
serializer_class = LinksMappingSerializer
pagination_class = DefaultLimitOffsetPagination

def get_queryset(self):
return []
83 changes: 83 additions & 0 deletions mathesar/api/serializers/links.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
from rest_framework import serializers

from db.links.operations.create import create_foreign_key_link, create_many_to_many_link
from mathesar.api.exceptions.mixins import MathesarErrorMessageMixin
from mathesar.api.exceptions.validation_exceptions.exceptions import ColumnSizeMismatchAPIException
from mathesar.api.serializers.shared_serializers import (
MathesarPolymorphicErrorMixin,
ReadWritePolymorphicSerializerMappingMixin,
)
from mathesar.models import Table


class OneToOneSerializer(MathesarErrorMessageMixin, serializers.Serializer):
referent_column_name = serializers.CharField()
reference_table = serializers.PrimaryKeyRelatedField(queryset=Table.current_objects.all())
referent_table = serializers.PrimaryKeyRelatedField(queryset=Table.current_objects.all())

def is_link_unique(self):
return True

def create(self, validated_data):
reference_table = validated_data['reference_table']
create_foreign_key_link(
reference_table.schema._sa_engine,
reference_table._sa_table.schema,
validated_data.get('referent_column_name'),
reference_table.oid,
validated_data.get('referent_table').oid,
unique_link=self.is_link_unique()
)
return validated_data


class OneToManySerializer(OneToOneSerializer):

def is_link_unique(self):
return False


class MapColumnSerializer(serializers.Serializer):
column_name = serializers.CharField()
referent_table = serializers.PrimaryKeyRelatedField(queryset=Table.current_objects.all())


class ManyToManySerializer(MathesarErrorMessageMixin, serializers.Serializer):
referents = MapColumnSerializer(many=True)
mapping_table_name = serializers.CharField()

def create(self, validated_data):
referents = validated_data['referents']
referent_tables_oid = [
{'referent_table': map_table_obj['referent_table'].oid, 'column_name': map_table_obj['column_name']} for
map_table_obj in validated_data['referents']]
create_many_to_many_link(
referents[0]['referent_table'].schema._sa_engine,
referents[0]['referent_table']._sa_table.schema,
validated_data.get('mapping_table_name'),
referent_tables_oid,
)
return validated_data


class LinksMappingSerializer(
MathesarPolymorphicErrorMixin,
ReadWritePolymorphicSerializerMappingMixin,
serializers.Serializer
):
def create(self, validated_data):
serializer = self.serializers_mapping.get(self.get_mapping_field())
return serializer.create(validated_data)

serializers_mapping = {
"one-to-one": OneToOneSerializer,
"one-to-many": OneToManySerializer,
"many-to-many": ManyToManySerializer
}
link_type = serializers.CharField(required=True)

def get_mapping_field(self):
link_type = self.initial_data.get('link_type', None)
if link_type is None:
raise ColumnSizeMismatchAPIException()
return link_type
51 changes: 51 additions & 0 deletions mathesar/tests/api/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,57 @@ def _create_table(table_name, schema='Data Types'):
return _create_table


@pytest.fixture
def create_self_referential_table(self_referential_filename, create_schema):
with open(self_referential_filename, 'rb') as csv_file:
data_file = DataFile.objects.create(file=File(csv_file))

def _create_table(table_name, schema='Patents'):
schema_model = create_schema(schema)
return create_table_from_csv(data_file, table_name, schema_model)

return _create_table


@pytest.fixture
def create_related_table(create_schema):
def read_table(related_csv_filename_tuple):
with open(related_csv_filename_tuple[0], 'rb') as csv_file:
data_file = DataFile.objects.create(file=File(csv_file))
with open(related_csv_filename_tuple[1], 'rb') as ref_csv_file:
ref_data_file = DataFile.objects.create(file=File(ref_csv_file))

def _create_table(table_name, ref_table_name, schema='Patents'):
schema_model = create_schema(schema)
return create_table_from_csv(data_file, table_name, schema_model), create_table_from_csv(
ref_data_file,
ref_table_name,
schema_model
)

return _create_table

return read_table


@pytest.fixture
def create_foreign_key_table(foreign_key_csv_filename_tuple, create_related_table):
return create_related_table(foreign_key_csv_filename_tuple)


@pytest.fixture
def create_multi_column_foreign_key_table(multi_column_foreign_key_csv_filename_tuple, create_related_table):
return create_related_table(multi_column_foreign_key_csv_filename_tuple)


@pytest.fixture
def create_invalid_related_data_foreign_key_table(
invalid_related_data_foreign_key_csv_filename_tuple,
create_related_table
):
return create_related_table(invalid_related_data_foreign_key_csv_filename_tuple)


@pytest.fixture
def table_for_reflection(test_db_name):
engine = create_mathesar_engine(test_db_name)
Expand Down
9 changes: 9 additions & 0 deletions mathesar/tests/api/test_constraint_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,15 @@ def _verify_primary_and_unique_constraints(response):
assert set(['unique', 'primary']) == set([constraint_data['type'] for constraint_data in constraints_data])


def _verify_foreign_key_constraint(constraint_data, columns, name, referent_table, referent_columns):
assert constraint_data['referent']['table'] == referent_table
assert constraint_data['columns'] == columns
assert constraint_data['referent']['columns'] == referent_columns
assert constraint_data['name'] == name
assert constraint_data['type'] == 'foreignkey'
assert 'id' in constraint_data and type(constraint_data['id']) == int


def _verify_unique_constraint(constraint_data, columns, name):
assert constraint_data['columns'] == columns
assert constraint_data['name'] == name
Expand Down
115 changes: 115 additions & 0 deletions mathesar/tests/api/test_links.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
import pytest
from sqlalchemy import Column, Integer, MetaData, String
from sqlalchemy import Table as SATable
from django.core.cache import cache

from db.tables.operations.select import get_oid_from_table
from db.tests.types import fixtures
from mathesar import models

engine_with_types = fixtures.engine_with_types
engine_email_type = fixtures.engine_email_type
temporary_testing_schema = fixtures.temporary_testing_schema


@pytest.fixture
def column_test_table(patent_schema):
engine = patent_schema._sa_engine
column_list_in = [
Column("mycolumn0", Integer, primary_key=True),
Column("mycolumn1", Integer, nullable=False),
Column("mycolumn2", Integer, server_default="5"),
Column("mycolumn3", String),
]
db_table = SATable(
"anewtable",
MetaData(bind=engine),
*column_list_in,
schema=patent_schema.name
)
db_table.create()
db_table_oid = get_oid_from_table(db_table.name, db_table.schema, engine)
table = models.Table.current_objects.create(oid=db_table_oid, schema=patent_schema)
return table


def test_one_to_one_link_create(column_test_table, client, create_table):
cache.clear()
table_2 = create_table('Table 2')
data = {
"link_type": "one-to-one",
"referent_column_name": "col_1",
"reference_table": table_2.id,
"referent_table": column_test_table.id,
}
response = client.post(
"/api/db/v0/links/",
data=data,
)
assert response.status_code == 201


def test_one_to_many_link_create(column_test_table, client, create_table):
cache.clear()
table_2 = create_table('Table 2')
data = {
"link_type": "one-to-many",
"referent_column_name": "col_1",
"reference_table": table_2.id,
"referent_table": column_test_table.id,
}
response = client.post(
"/api/db/v0/links/",
data=data,
)
assert response.status_code == 201


def test_one_to_many_self_referential_link_create(column_test_table, client):
cache.clear()
data = {
"link_type": "one-to-many",
"referent_column_name": "col_1",
"reference_table": column_test_table.id,
"referent_table": column_test_table.id,
}
response = client.post(
"/api/db/v0/links/",
data=data,
)
assert response.status_code == 201


def test_many_to_many_self_referential_link_create(column_test_table, client):
cache.clear()
data = {
"link_type": "many-to-many",
"mapping_table_name": "map_table",
"referents": [
{'referent_table': column_test_table.id, 'column_name': "link_1"},
{'referent_table': column_test_table.id, 'column_name': "link_2"}
],
}
response = client.post(
"/api/db/v0/links/",
data=data,
)
assert response.status_code == 201


def test_many_to_many_link_create(column_test_table, client, create_table):
cache.clear()
table_2 = create_table('Table 2')
data = {
"link_type": "many-to-many",
"mapping_table_name": "map_table",
"referents": [
{'referent_table': column_test_table.id, 'column_name': "link_1"},
{'referent_table': table_2.id, 'column_name': "link_2"}
],
}
response = client.post(
"/api/db/v0/links/",
data=data,
)
assert response.status_code == 201
Loading