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

Unify foreign key options in create_table, create_constraint and column level constraints #628

Draft
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

kvch
Copy link
Contributor

@kvch kvch commented Jan 27, 2025

This PR is the beginning of unifying constraint options all over pgroll. This addresses the missing options in column level foreign key constraints and create_constraint operation.

Now more foreign key options are supported:

  • match_type in column definitions and in create_constraint
  • on_update in column definitions and in create_constraint
  • on_delete_set_columns in create_constraint

It also comes with several refactorings and simplifications:

  • ForeignKeyReferenceOnDeleteNOACTION and others are renamed to ForeignKeyOnDeleteNOACTION, because my eyes were bleeding from the long names
  • The same TableForeignKeyReference is used in both constraints of create_table and create_constraint operation
  • ConstraintSQLWriter is used in adding new columns, creating constraints and creating tables
  • ConstraintSQLWriter can produce not validated constraints
  • convertFkConstraint in sql2pgroll is the unified way to process FK constraints of columns and tables

What comes next in future PRs?

  • I am extracting ConstraintSQLWriter into a separate file and adding unit tests, as it is used from several parts of the codebase now
  • I am adopting ConstraintSQLWriter to support other constraint types for columns and creating constraints
  • I am unifying unique, primary_key and check constraints options
  • I am adding support for deferrable and initially deferred constraints on columns and in sql2pgroll

break
}
return true
return !constraint.SkipValidation
Copy link
Contributor Author

@kvch kvch Jan 28, 2025

Choose a reason for hiding this comment

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

The constraint writer already supports skipping validation. But I am adding it to sql2pgroll later.

@kvch kvch marked this pull request as ready for review January 28, 2025 16:32
Copy link
Collaborator

@andrew-farries andrew-farries left a comment

Choose a reason for hiding this comment

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

I think this looks good 👍

It would be good to separate out the simpler changes (eg constant renames) from other changes so that individual PRs don't get too big.

},
"on_update": {
"description": "On update behavior of the foreign key constraint",
"$ref": "#/$defs/ForeignKeyOnDelete",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like the wrong $ref 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.

No, it is on purpose. ON UPDATE and ON DELETE have the same actions, so we can reuse it in both places. But I can rename it to make clearer that it is intentional.

@kvch
Copy link
Contributor Author

kvch commented Jan 29, 2025

Requires #639.
Until that PR is merged, it can stay open as a draft.

@kvch kvch marked this pull request as draft January 29, 2025 12:10
kvch added a commit that referenced this pull request Jan 29, 2025
This PR renames `ForeignKeyReferenceOnDelete` to `ForeignKeyAction`.
The name was too long. Also, `ON DELETE` and `ON UPDATE` referential
actions are the
same. This name makes it clearer that we are reusing this enum for both
options.

Extracted from #628
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.

2 participants