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

Communication: Reintroduce foreign key constraints and offline primary keys #70

Merged
merged 6 commits into from
Nov 30, 2024

Conversation

PaRangger
Copy link
Collaborator

@PaRangger PaRangger commented Oct 24, 2024

Motivation

In the hotfix for version 1.0.1 and 1.0.2 the foreign key constraints for reactions were removed. Additionally, the primary key constraints for posts and reactions were altered to include the server_id. These changes were done to prevent the application from crashing. This however introduced the problem that while the app was running without crashes, the data integrity is not given anymore.

Description

Pull Request #58 fixed an issue with the answer threads not being shown. This issue also seemed to be the origin of the failing primary and foreign key constraints. Therefore I reverted the changes to include the previous FK and PK checks as they were.

Closes #62

Copy link
Collaborator

@FelberMartin FelberMartin left a comment

Choose a reason for hiding this comment

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

Tested the App and works as expected without any crashes.

Also tested the migration from the 1.0.3 release to this app state, everything works fine 👍

@PaRangger PaRangger marked this pull request as ready for review October 24, 2024 14:38
Copy link
Contributor

@julian-wls julian-wls left a comment

Choose a reason for hiding this comment

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

Just tested it, and everything seems to work fine.

Copy link
Contributor

@TimOrtel TimOrtel left a comment

Choose a reason for hiding this comment

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

Please add some unit Tests to verify that the referential integrity works by writing tests that add messages, delete messages, delete conversations etc.

Copy link
Contributor

@julian-wls julian-wls left a comment

Choose a reason for hiding this comment

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

Ran MetisDaoTest and tested adding posts, deleting posts and post reactions. Worked as expected, reapprove.

@FelberMartin FelberMartin added ready to merge This PR can be merged database changes This PR includes database changes and should be treated with extra care when merging and removed ready for review This PR can be reviewed labels Nov 29, 2024
@FelberMartin FelberMartin merged commit 8396c11 into develop Nov 30, 2024
5 checks passed
@FelberMartin FelberMartin deleted the bugfix/reimplement-foreign-keys branch November 30, 2024 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
database changes This PR includes database changes and should be treated with extra care when merging ready to merge This PR can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Communication: Index constraints cause crashes
5 participants