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 27 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.
74 changes: 74 additions & 0 deletions db/links/operations/create.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
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,
referrer_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(
referrer_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, [referrer_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
)


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.

17 changes: 12 additions & 5 deletions db/tables/operations/select.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,26 +13,33 @@ def reflect_table(name, schema, engine, metadata=None, connection_to_use=None):


def reflect_table_from_oid(oid, engine, connection_to_use=None):
tables = reflect_tables_from_oids([oid], engine, connection_to_use)
return tables.get(oid, None)


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)
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 == oid)
.where(pg_class.c.oid.in_(oids))
)
result = execute_statement(engine, sel, connection_to_use)
schema, table_name = result.fetchall()[0]
return reflect_table(table_name, schema, engine, connection_to_use=connection_to_use)
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):
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 []
1 change: 1 addition & 0 deletions mathesar/api/exceptions/error_codes.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,3 +38,4 @@ class ErrorCodes(Enum):
URLDownloadError = 4404
URLNotReachableError = 4405
URLInvalidContentType = 4406
InvalidLinkChoice = 4408
18 changes: 17 additions & 1 deletion mathesar/api/exceptions/mixins.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from rest_framework.serializers import Serializer
from rest_framework.serializers import ListSerializer, Serializer
from rest_framework.utils.serializer_helpers import ReturnList
from rest_framework_friendly_errors.mixins import FriendlyErrorMessagesMixin
from django.core.exceptions import ValidationError as DjangoValidationError
Expand Down Expand Up @@ -40,6 +40,22 @@ def build_pretty_errors(self, errors, serializer=None):
child_errors = field.build_pretty_errors(errors[error_type])
pretty += child_errors
continue
if isinstance(field, ListSerializer) and type(errors[error_type]) == list:
pretty_child_errors = []
for index, child_error in enumerate(errors[error_type]):
child_field = field.child
initial_data = self.initial_data.get(error_type, None)
if initial_data is not None:
child_field.initial_data = self.initial_data[error_type][index]
else:
child_field.initial_data = None
if isinstance(child_error, str):
pretty_child_errors.append(self.get_field_error_entry(child_error, field))
else:
child_errors = child_field.build_pretty_errors(child_error)
pretty_child_errors.extend(child_errors)
pretty.extend(pretty_child_errors)
continue
if self.is_pretty(error):
if 'field' not in error or error['field'] is None or str(error['field']) == 'None':
error['field'] = error_type
Expand Down
12 changes: 12 additions & 0 deletions mathesar/api/exceptions/validation_exceptions/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,18 @@ def __init__(
super().__init__(None, self.error_code, message, field, details)


class InvalidLinkChoiceAPIException(MathesarValidationException):
error_code = ErrorCodes.InvalidLinkChoice.value

def __init__(
self,
message="Invalid Link type",
field=None,
details=None,
):
super().__init__(None, self.error_code, message, field, details)


class MultipleDataFileAPIException(MathesarValidationException):
error_code = ErrorCodes.MultipleDataFiles.value

Expand Down
89 changes: 89 additions & 0 deletions mathesar/api/serializers/links.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
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 (
InvalidLinkChoiceAPIException,
)
from mathesar.api.serializers.shared_serializers import (
MathesarPolymorphicErrorMixin,
ReadWritePolymorphicSerializerMappingMixin,
)
from mathesar.models import Table


class OneToOneSerializer(MathesarErrorMessageMixin, serializers.Serializer):
reference_column_name = serializers.CharField()
reference_table = serializers.PrimaryKeyRelatedField(queryset=Table.current_objects.all())
referent_table = serializers.PrimaryKeyRelatedField(queryset=Table.current_objects.all())
# TODO Fix hacky link_type detection by reflecting it correctly
link_type = serializers.CharField(default="one-to-one")

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('reference_column_name'),
reference_table.oid,
validated_data.get('referent_table').oid,
unique_link=self.is_link_unique()
)
return validated_data


class OneToManySerializer(OneToOneSerializer):
link_type = serializers.CharField(default="one-to-many")

def is_link_unique(self):
return False


class MapColumnSerializer(MathesarErrorMessageMixin, 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()
link_type = serializers.CharField(default="many-to-many")

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(validated_data))
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, data):
link_type = data.get('link_type', None)
if link_type is None:
raise InvalidLinkChoiceAPIException()
return link_type
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",
"reference_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",
"reference_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",
"reference_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
1 change: 1 addition & 0 deletions mathesar/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

db_router = routers.DefaultRouter()
db_router.register(r'tables', db_viewsets.TableViewSet, basename='table')
db_router.register(r'links', db_viewsets.LinkViewSet, basename='links')
db_router.register(r'schemas', db_viewsets.SchemaViewSet, basename='schema')
db_router.register(r'databases', db_viewsets.DatabaseViewSet, basename='database')
db_router.register(r'data_files', db_viewsets.DataFileViewSet, basename='data-file')
Expand Down