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

Add project_id column to rule_instances #3570

Merged
merged 3 commits into from
Jun 11, 2024
Merged

Conversation

dmjb
Copy link
Contributor

@dmjb dmjb commented Jun 10, 2024

This provides a more elegant solution to the ON CASCADE DELETE problem with the rule_instance table. Tested locally.

Summary

Provide a brief overview of the changes and the issue being addressed.
Explain the rationale and any background necessary for understanding the changes.
List dependencies required by this change, if any.

Fixes #(related issue)

Change Type

Mark the type of change your PR introduces:

  • Bug fix (resolves an issue without affecting existing features)
  • Feature (adds new functionality without breaking changes)
  • Breaking change (may impact existing functionalities or require documentation updates)
  • Documentation (updates or additions to documentation)
  • Refactoring or test improvements (no bug fixes or new functionality)

Testing

Outline how the changes were tested, including steps to reproduce and any relevant configurations.
Attach screenshots if helpful.

Review Checklist:

  • Reviewed my own code for quality and clarity.
  • Added comments to complex or tricky code sections.
  • Updated any affected documentation.
  • Included tests that validate the fix or feature.
  • Checked that related changes are merged.

This provides a more elegant solution to the `ON CASCADE DELETE`
problem.
@dmjb dmjb requested a review from a team as a code owner June 10, 2024 13:33
Copy link
Member

@evankanderson evankanderson left a comment

Choose a reason for hiding this comment

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

Overall, I like this simplification. Maybe we're okay at the moment with causing minder failures during rollout due to adding NOT NULL columns to the schema, but I wanted to check.

If you want to split it up, I would add the column with no constraints and the new queries, and then do a second migration PR that does the backfill and adds the constraint.

WHERE ri.profile_id = pf.id;

-- now we can add the not null constraint
ALTER TABLE rule_instances ALTER COLUMN project_id SET NOT NULL;
Copy link
Member

Choose a reason for hiding this comment

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

Won't this cause current-minder to fail after the DB migration until the new deployment occurs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, will remove this.

BEGIN;

-- add column, leave as not null until we populate the column
ALTER TABLE rule_instances ADD COLUMN project_id UUID REFERENCES projects(id) ON DELETE CASCADE;
Copy link
Member

Choose a reason for hiding this comment

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

Do we want or need an index on project_id here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At this point in time, we don't use the new column in the queries since the tuple of (name, profile_id, entity_type) is still unique. As I add more queries, I will add more indices based on common query patterns.

@dmjb dmjb force-pushed the rule_instance_project_id_fk branch 2 times, most recently from e10f877 to 136703e Compare June 11, 2024 09:25
@dmjb dmjb force-pushed the rule_instance_project_id_fk branch from 136703e to 62d6507 Compare June 11, 2024 09:30
@coveralls
Copy link

Coverage Status

coverage: 53.166% (+0.02%) from 53.143%
when pulling 16d2643 on rule_instance_project_id_fk
into 16f95ce on main.

@dmjb dmjb merged commit 90904dc into main Jun 11, 2024
22 checks passed
@dmjb dmjb deleted the rule_instance_project_id_fk branch June 11, 2024 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants