From c8c1faeb3d06e3c974319c6cfc22a5407d3122f7 Mon Sep 17 00:00:00 2001 From: Natasha Singh Date: Tue, 11 Jun 2024 14:23:24 -0400 Subject: [PATCH 1/4] :recycle: Allow parent or child sample in relationship to be null --- dataservice/api/sample_relationship/models.py | 52 +++++++++++++------ 1 file changed, 36 insertions(+), 16 deletions(-) diff --git a/dataservice/api/sample_relationship/models.py b/dataservice/api/sample_relationship/models.py index 43bacb1c..99187932 100644 --- a/dataservice/api/sample_relationship/models.py +++ b/dataservice/api/sample_relationship/models.py @@ -32,18 +32,23 @@ class SampleRelationship(db.Model, Base): parent_id = db.Column( KfId(), db.ForeignKey('sample.kf_id'), - nullable=False, + nullable=True, doc='kf_id of one sample in the relationship') child_id = db.Column( KfId(), db.ForeignKey('sample.kf_id'), - nullable=False, + nullable=True, doc='kf_id of the other sample in the relationship') - external_parent_id = db.Column(db.Text()) - - external_child_id = db.Column(db.Text()) + external_parent_id = db.Column( + db.Text(), + doc='Name given to parent sample by contributor' + ) + external_child_id = db.Column( + db.Text(), + doc='Name given to child sample by contributor' + ) notes = db.Column( db.Text(), @@ -119,35 +124,50 @@ def validate_sample_relationship(target): if not target: return - # Get samples in relationship by id - parent = Sample.query.get(target.parent_id) - child = Sample.query.get(target.child_id) + parent_id = target.parent_id + child_id = target.child_id + print( + f"********* parent and child ids ********** {parent_id} {child_id}" + ) - # Check that both are existing samples - if not (parent and child): + # Check that at least 1 sample ID is non-null + if not (parent_id or child_id): raise DatabaseValidationError( SampleRelationship.__tablename__, "modify", - "Either parent sample or child sample or both does not exist" + "Both parent_id and child_id cannot be null" ) + # If a parent_id or child_id is not-null then check that the ID + # refers to an existing sample + for sample_id in [parent_id, child_id]: + if sample_id: + sample = Sample.query.get(sample_id) + if not sample: + raise DatabaseValidationError( + SampleRelationship.__tablename__, + "modify", + f"Either parent sample {target.parent_id} or " + f"child sample {target.child_id} or both does not exist" + ) + # Check for reverse relation sr = SampleRelationship.query.filter_by( - parent_id=child.kf_id, - child_id=parent.kf_id, + parent_id=child_id, + child_id=parent_id, ).first() if sr: raise DatabaseValidationError( SampleRelationship.__tablename__, "modify", - f"Reverse relationship, Parent: {target.parent.kf_id} -> Child: " - f"{target.child.kf_id}, not allowed since the SampleRelationship, " + f"Reverse relationship, Parent: {parent_id} -> Child: " + f"{child_id}, not allowed since the SampleRelationship, " f"Parent: {sr.parent_id} -> Child: {sr.child_id}, already exists" ) # Check for parent = child - if target.parent_id == target.child_id: + if parent_id == child_id: raise DatabaseValidationError( SampleRelationship.__tablename__, "modify", From cc2887806f2867e59c58356c23f540bd3282a258 Mon Sep 17 00:00:00 2001 From: Natasha Singh Date: Tue, 11 Jun 2024 14:23:57 -0400 Subject: [PATCH 2/4] :white_check_mark: Test that root samples (no parent) are allowed --- .../test_sample_relationship_models.py | 49 +++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/tests/sample_relationship/test_sample_relationship_models.py b/tests/sample_relationship/test_sample_relationship_models.py index 11e71e37..aa0c3c36 100644 --- a/tests/sample_relationship/test_sample_relationship_models.py +++ b/tests/sample_relationship/test_sample_relationship_models.py @@ -144,6 +144,55 @@ def test_foreign_key_constraint(self): db.session.commit() assert "does not exist" in str(e.value) + def test_null_parent_allowed(self): + """ + Test that we can represent a root of a sample tree: a null parent + with a non-null child + """ + _, rels = create_relationships() + + pid = rels[0].parent.participant_id + sample = Sample( + participant_id=pid, external_id="foo" + ) + db.session.add(sample) + db.session.commit() + + # Create sample relationship + data = { + "parent_id": None, + "child_id": sample.kf_id, + "external_parent_id": None, + "external_child_id": "foo", + } + r = SampleRelationship(**data) + + # Add to db + db.session.add(r) + db.session.commit() + + assert len(rels) + 1 == SampleRelationship.query.count() + + def test_null_parent_and_child_not_allowed(self): + """ + Test that we cannot create a relationship with a null parent and + child id + """ + # Create sample relationship + data = { + "parent_id": None, + "child_id": None, + "external_parent_id": None, + "external_child_id": None, + } + r = SampleRelationship(**data) + + # Add to db + db.session.add(r) + with pytest.raises(DatabaseValidationError) as e: + db.session.commit() + assert "cannot be null" in str(e.value) + def test_query_all_relationships(self): """ Test the class method query_all_relationships on SampleRelationship From b9ecb107c6f5cd12deb003d73da616331e323bdf Mon Sep 17 00:00:00 2001 From: Natasha Singh Date: Tue, 11 Jun 2024 14:24:19 -0400 Subject: [PATCH 3/4] :card_file_box: Migration for nullable parent/child sample --- .../c2f77debb22d_allow_null_parent_sample.py | 45 +++++++++++++++++++ 1 file changed, 45 insertions(+) create mode 100644 migrations/versions/c2f77debb22d_allow_null_parent_sample.py diff --git a/migrations/versions/c2f77debb22d_allow_null_parent_sample.py b/migrations/versions/c2f77debb22d_allow_null_parent_sample.py new file mode 100644 index 00000000..e7969dfe --- /dev/null +++ b/migrations/versions/c2f77debb22d_allow_null_parent_sample.py @@ -0,0 +1,45 @@ +""" +1.23.0 Allow parent or child sample in sample relationship to be null + +This allows us to enter a row where the parent is null and child is not which +represents a root of a sample tree + +Validation ensures that you cannot enter a sample relationship where both +the parent and child sample in the relationship are null + +Revision ID: c2f77debb22d +Revises: f07d043290d0 +Create Date: 2024-06-11 13:05:50.923693 + +""" +from alembic import op +import sqlalchemy as sa + + +# revision identifiers, used by Alembic. +revision = 'c2f77debb22d' +down_revision = 'f07d043290d0' +branch_labels = None +depends_on = None + + +def upgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.alter_column('sample_relationship', 'child_id', + existing_type=sa.VARCHAR(length=11), + nullable=True) + op.alter_column('sample_relationship', 'parent_id', + existing_type=sa.VARCHAR(length=11), + nullable=True) + # ### end Alembic commands ### + + +def downgrade(): + # ### commands auto generated by Alembic - please adjust! ### + op.alter_column('sample_relationship', 'parent_id', + existing_type=sa.VARCHAR(length=11), + nullable=False) + op.alter_column('sample_relationship', 'child_id', + existing_type=sa.VARCHAR(length=11), + nullable=False) + # ### end Alembic commands ### From f7783b1a954e95198f6deb5c58a9f04a50554986 Mon Sep 17 00:00:00 2001 From: Natasha Singh Date: Tue, 11 Jun 2024 14:27:05 -0400 Subject: [PATCH 4/4] :fire: Rm print statement --- dataservice/api/sample_relationship/models.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/dataservice/api/sample_relationship/models.py b/dataservice/api/sample_relationship/models.py index 99187932..60f900fc 100644 --- a/dataservice/api/sample_relationship/models.py +++ b/dataservice/api/sample_relationship/models.py @@ -126,9 +126,6 @@ def validate_sample_relationship(target): parent_id = target.parent_id child_id = target.child_id - print( - f"********* parent and child ids ********** {parent_id} {child_id}" - ) # Check that at least 1 sample ID is non-null if not (parent_id or child_id):