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.

Sign up for GitHub

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

Jump to bottom

Implement a foreign key for subjects #98

Merged
merged 1 commit into from
Jul 27, 2022
Merged

Implement a foreign key for subjects #98

merged 1 commit into from
Jul 27, 2022

Conversation

rhmdnd
Copy link
Owner

@rhmdnd rhmdnd commented Jul 20, 2022

We want to be able to nest subjects and build a hierarchical
relationship between them. This commit does that by adding a database
migration that adds a new column for parent_id and adds a FK
relationship to it.

This approach was discussed in #84

@openshift-ci openshift-ci bot requested review from mrogers950 and Vincent056 July 20, 2022 20:22
@mrogers950
Copy link
Collaborator

migrations/000002_add_subject_parent_id.down.sql is supposed to be empty? otherwise looks good to me.

@rhmdnd
Copy link
Owner Author

rhmdnd commented Jul 20, 2022

migrations/000002_add_subject_parent_id.down.sql is supposed to be empty? otherwise looks good to me.

No, that should revert the changes added in the corresponding up migration. I'll fix that up before we merge this, but I wanted get this proposal up to get feedback on the FK approach.

@rhmdnd rhmdnd force-pushed the add-fk-to-subject-table branch from 0743b6d to 1e51850 Compare July 20, 2022 23:05
rhmdnd
rhmdnd commented Jul 20, 2022
View reviewed changes
tests/integration/integration_test.go Outdated
@@ -218,3 +282,39 @@ func TestMigration(t *testing.T) { // nolint:paralleltest // database tests shou
assert.Equal(t, false, dirty, "Database state mismatch: want %t but got %t", false, dirty)
assert.Equal(t, err, nil, "Error downgrading the database: %s", err)
}

func TestSubjectMigration(t *testing.T) { // nolint:paralleltest // database tests should run serially
Copy link
Owner Author

Choose a reason for hiding this comment

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

We could also consider adding a test that ensures the FK does what it should by:

  1. Create a parent subject
  2. Create a child subject with parent_id pointing to the parent subject
  3. Delete the parent subject and ensure both are removed

Although - I'm not sure how likely this scenario is.

@rhmdnd rhmdnd force-pushed the add-fk-to-subject-table branch from 1e51850 to e91b614 Compare July 21, 2022 14:30
@rhmdnd
Copy link
Owner Author

rhmdnd commented Jul 21, 2022

migrations/000002_add_subject_parent_id.down.sql is supposed to be empty? otherwise looks good to me.

I added a valid downgrade and test coverage exercising the downgrade.

@rhmdnd rhmdnd force-pushed the add-fk-to-subject-table branch 2 times, most recently from 02961d5 to 7a01c4e Compare July 21, 2022 17:54
@rhmdnd rhmdnd force-pushed the add-fk-to-subject-table branch from 7a01c4e to c75194a Compare July 25, 2022 18:40
Vincent056
Vincent056 approved these changes Jul 27, 2022
View reviewed changes
Copy link
Collaborator

@Vincent056 Vincent056 left a comment

Choose a reason for hiding this comment

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

I think it all looks good, wondering if we need to test the case where parent subject is removed after being referenced?

@openshift-ci openshift-ci bot added the lgtm label Jul 27, 2022
@rhmdnd
Copy link
Owner Author

rhmdnd commented Jul 27, 2022

I think it all looks good, wondering if we need to test the case where parent subject is removed after being referenced?

Yeah - I'll add a test for that today. Thanks for the review!

@rhmdnd rhmdnd force-pushed the add-fk-to-subject-table branch from c75194a to 8912cd8 Compare July 27, 2022 14:43
@openshift-ci openshift-ci bot removed the lgtm label Jul 27, 2022
@rhmdnd rhmdnd force-pushed the add-fk-to-subject-table branch from 8912cd8 to f3d595f Compare July 27, 2022 18:13
@rhmdnd
Implement a foreign key for subjects
2d401ae 
We want to be able to nest subjects and build a hierarchical
relationship between them. This commit does that by adding a database
migration that adds a new column for parent_id and adds a FK
relationship to it.

This approach was discussed in #84
@rhmdnd rhmdnd force-pushed the add-fk-to-subject-table branch from f3d595f to 2d401ae Compare July 27, 2022 20:31
Vincent056
Vincent056 approved these changes Jul 27, 2022
View reviewed changes
Copy link
Collaborator

@Vincent056 Vincent056 left a comment

Choose a reason for hiding this comment

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

/lgtm
Thanks for adding the test

@openshift-ci openshift-ci bot added the lgtm label Jul 27, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 27, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhmdnd, Vincent056

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit 2ccec38 into main Jul 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrogers950 mrogers950 mrogers950 left review comments

@Vincent056 Vincent056 Vincent056 approved these changes

Assignees

@Vincent056 Vincent056

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rhmdnd @mrogers950 @Vincent056 @openshift-merge-robot