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

message_id should not be nullable #681

Closed
wants to merge 2 commits into from

Conversation

csgui
Copy link
Contributor

@csgui csgui commented Nov 5, 2024

message_id should not be nullable.

The tables mock_proposals and mock_signatures have an UNIQUE constraint on message_id, which suggests a one-to-one relationship between messages and mock_proposals, as well as between messages and mock_signatures. Therefore, message_id is used as a foreign key and should not allow NULL values.

@csgui csgui requested review from zone117x and rafaelcr November 5, 2024 15:12
@smcclellan
Copy link
Contributor

@zone117x Is it possible for a mock proposal or mock signature to have no message (no corresponding entry in the messages table)?

Copy link
Collaborator

@rafaelcr rafaelcr left a comment

Choose a reason for hiding this comment

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

@csgui The mock proposals table stores peer info data, which sometimes belongs directly to a message and some other times it belongs to a mock signature. In the latter case, the message is attached to the signature itself, not the peer info.

(I consolidated these two into a single table to avoid repetition since it's practically the same schema)

If you add this constraint, chainhook will crash when inserting mock signatures.

@csgui
Copy link
Contributor Author

csgui commented Nov 5, 2024

@rafaelcr Thanks for the info!

I see the advantages of this approach in reducing schema complexity and avoiding duplicate table structures. However, in terms of data integrity - if a message is deleted (considering the CASCADE), we lose the peer info. But what if this peer info was actually related to a signature? Is this a real possibility?

@rafaelcr
Copy link
Collaborator

rafaelcr commented Nov 5, 2024

@csgui not really. Messages write the data they need in that moment onto the DB, i.e. they don't look for repeated peer info so they can mark the relationship if it existed previously (it actually won't ever find repeated peer info because it writes block data in the same row).

If a message gets deleted and deletes peer info with it, no other data will be affected.

@csgui
Copy link
Contributor Author

csgui commented Nov 6, 2024

Closing this PR, as this approach does not adequately address the problem. I will tackle the issue in a separate PR.

@csgui csgui closed this Nov 6, 2024
@csgui csgui deleted the fix/message_id_not_null branch November 6, 2024 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants