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

fix: Orphaned encrypted_value records #701

Merged
merged 7 commits into from
Mar 13, 2024
Merged

Conversation

hsinn0
Copy link
Contributor

@hsinn0 hsinn0 commented Mar 6, 2024

  • When a credential was deleted, its related encrypted_value records were not being deleted.
  • Implemented a trigger to make sure that those records are deleted.

[#182121168]

There will be more commits, including tests, triggers for other databases and indexes to address delete operation performance.

- When a credential was deleted, its related encrypted_value records were not being deleted.
- Implemented a trigger to make sure that those records are deleted.

[#182121168]
@cf-gitbot
Copy link

We have created an issue in Pivotal Tracker to manage this. Unfortunately, the Pivotal Tracker project is private so you may be unable to view the contents of the story.

The labels on this github issue will be updated when the story is started.

@hsinn0 hsinn0 force-pushed the 182121168-del-encrypted_values branch 2 times, most recently from f3a4de3 to fc35826 Compare March 9, 2024 01:57
@hsinn0
Copy link
Contributor Author

hsinn0 commented Mar 9, 2024

Rewrote mysql trigger to use cursor instead of temp table. Also got rid of previous commit with test code change as the added code to the existing test cases had issues giving inconsistent result. Will add new test cases instead.

- Rewrote the trigger not to use temporary tables.

[#182121168]
@hsinn0 hsinn0 force-pushed the 182121168-del-encrypted_values branch 4 times, most recently from e57a34d to 84a2735 Compare March 12, 2024 06:35
- MySQL trigger to delete associated encrypted_value records when credential_versions are deleted
- Implemented triggers on both credential and credential_version table because a credential_version record can be deleted not only when a credential is deleted but also directly.
- This will cause h2 & postgresql test cases to fail until triggers are
  implemented for those databases.
[#182121168]

Co-authored-by: Bruce Ricard <[email protected]>
@hsinn0 hsinn0 force-pushed the 182121168-del-encrypted_values branch from 84a2735 to 7ea712c Compare March 12, 2024 21:12
- Added PostgreSQL trigger to delete associated encrypted_value records when credential_versions are deleted.

[#182121168]
@hsinn0 hsinn0 force-pushed the 182121168-del-encrypted_values branch from 0788cb4 to 82cef8d Compare March 13, 2024 00:15
@hsinn0 hsinn0 changed the title fix: Orphaned encrypted_value records in MySQL fix: Orphaned encrypted_value records Mar 13, 2024
hsinn0 added 2 commits March 13, 2024 17:24
* Do not assert on the count of the "encrypted_value" table when using
  the H2 in-memory test database.

Co-Authored-by: Bruce Ricard <[email protected]>
[#182121168](https://www.pivotaltracker.com/story/show/182121168)
* To help debug by logging whether the assertion runs or not

Co-Authored-by: Bruce Ricard <[email protected]>
[#182121168](https://www.pivotaltracker.com/story/show/182121168)
@hsinn0 hsinn0 marked this pull request as ready for review March 13, 2024 21:33
Co-Authored-by: Bruce Ricard <[email protected]>
Copy link
Contributor

@bruce-ricard bruce-ricard left a comment

Choose a reason for hiding this comment

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

Amazing work

@bruce-ricard
Copy link
Contributor

bruce-ricard commented Mar 13, 2024

This PR attempts to resolve #231

@hsinn0 hsinn0 merged commit ab3f142 into main Mar 13, 2024
2 checks passed
@hsinn0 hsinn0 deleted the 182121168-del-encrypted_values branch March 13, 2024 22:07
@hsinn0 hsinn0 restored the 182121168-del-encrypted_values branch March 13, 2024 23:39
@hsinn0 hsinn0 deleted the 182121168-del-encrypted_values branch March 14, 2024 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

5 participants