Skip to content

Ensure tag name and relationship uniqueness #1124

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Ensure tag name and relationship uniqueness #1124

wants to merge 1 commit into from

Conversation

nscuro
Copy link
Member

@nscuro nscuro commented Apr 10, 2025

Description

Ensures tag name and relationship uniqueness.

  • Adds missing (unique) index on tag names.
  • Adds composite primary keys to all join tables of tag relationships. Thus prevents duplicate entries, and voids the need for indexes on individual columns.
  • Changes tag relationships from ordered Lists to Sets. We don't need ordering here, and it only adds overhead.
  • Changes equals and hashCode of Tag to be based on name, rather than id.
  • Removes and re-wires any duplicate records during the DB migration.

Addressed Issue

Fixes DependencyTrack/hyades#1755

Additional Details

N/A

Checklist

  • I have read and understand the contributing guidelines
  • This PR fixes a defect, and I have provided tests to verify that the fix is effective
  • This PR implements an enhancement, and I have provided tests to verify that it works as intended
  • This PR introduces changes to the database model, and I have updated the migration changelog accordingly
  • This PR introduces new or alters existing behavior, and I have updated the documentation accordingly

@nscuro nscuro added the defect Something isn't working label Apr 10, 2025
@nscuro nscuro added this to the 5.6.0 milestone Apr 10, 2025
@nscuro nscuro changed the title Ensure tag name uniqueness Ensure tag name and relationship uniqueness Apr 10, 2025
@nscuro nscuro force-pushed the tag-dupes branch 7 times, most recently from 3cb8685 to f456f22 Compare April 10, 2025 16:19
Copy link

codacy-production bot commented Apr 10, 2025

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
+0.00% (target: -1.00%) 85.42% (target: 70.00%)
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (7dea143) 27762 22009 79.28%
Head commit (b8a135e) 27770 (+8) 22015 (+6) 79.28% (+0.00%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#1124) 48 41 85.42%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

@nscuro nscuro marked this pull request as ready for review April 11, 2025 12:35
sahibamittal
sahibamittal previously approved these changes Apr 14, 2025
Copy link
Collaborator

@sahibamittal sahibamittal left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏼

@nscuro
Copy link
Member Author

nscuro commented Apr 14, 2025

@sahibamittal Had to rebase this PR to resolve a merge conflict caused by #1126. Please have another look when you get the chance!

sahibamittal
sahibamittal previously approved these changes Apr 14, 2025
Comment on lines 1403 to 1408
<!--
Disable deferred constraint checking during this transaction.
Without this, we would not be able to add primary keys after
having deleted records from a table before.
-->
<sql>SET CONSTRAINTS ALL IMMEDIATE</sql>
Copy link
Member Author

Choose a reason for hiding this comment

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

I did a final test run with duplicate records across all tag tables, and the migration ended up failing with:

cannot ALTER TABLE "NOTIFICATIONRULE_TAGS" because it has pending trigger events

This happened when the migration tried to add primary keys. It turns out that, if a table has deferred foreign key constraints, you cannot modify rows and alter the table schema in the same transaction: https://github.com/friek/til/blob/master/postgres/alter-table-pending-triggers.md

The solution is to disable deferred constraints during the transaction, such that foreign keys are checked immediately and cannot block table altering.

sahibamittal
sahibamittal previously approved these changes Apr 16, 2025
@nscuro nscuro force-pushed the tag-dupes branch 3 times, most recently from a7e1750 to 65734f8 Compare April 22, 2025 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
defect Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Database schema allows for duplicate tag names and relationships
2 participants