Skip to content

Allow db only remove foreign key to work as database only - #95

Merged
s-cooper18 merged 2 commits into
mainfrom
s-cooper18/remove-foreign-key-without-state
Dec 12, 2025
Merged

Allow db only remove foreign key to work as database only#95
s-cooper18 merged 2 commits into
mainfrom
s-cooper18/remove-foreign-key-without-state

Conversation

@s-cooper18

@s-cooper18 s-cooper18 commented Dec 11, 2025

Copy link
Copy Markdown
Contributor

Remove foreign key for database was previously unable to apply as it could not find the name of the field within the state. This is modified to be able to function with only a name and model provided, even if it is not on the state by handling the error and allowing the ForeignKeyManager to be populated with an optional field value.

It is noted that the Field value is still required in order for other operations to occur however.

Allows the following to work:

    operations = [
        migrations.SeparateDatabaseAndState(
            database_operations=[
                operations.SaferRemoveFieldForeignKey(
                    model_name="portfolio",
                    name="my_favourite_foreign_key",
                ),
            ],
        ),
    ]

@github-actions

Copy link
Copy Markdown

Coverage Report Results

Name Stmts Miss Branch BrPart Cover
src/django_pg_migration_tools/indexes.py 9 0 0 0 100%
src/django_pg_migration_tools/management/commands/migrate_with_timeouts.py 138 0 20 0 100%
src/django_pg_migration_tools/operations.py 473 0 92 0 100%
src/django_pg_migration_tools/timeouts.py 73 0 30 0 100%
tests/django_pg_migration_tools/management/commands/test_migrate_with_timeouts.py 126 0 0 0 100%
tests/django_pg_migration_tools/test_indexes.py 33 0 0 0 100%
tests/django_pg_migration_tools/test_operations.py 1839 0 0 0 100%
tests/django_pg_migration_tools/test_package.py 3 0 0 0 100%
tests/django_pg_migration_tools/test_timeouts.py 75 0 0 0 100%
TOTAL 2769 0 142 0 100%

6 empty files skipped.

@Nickmika Nickmika left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I am no expert in this library, but the test and the SQL generated looks like I would expect. Thank you. Left one comment. 👍


operation.state_forwards(self.app_label, new_state)

# Do state only operation - has already been removed from state

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Do state only operation - has already been removed from state

Should this read "Do database only operation"?

Remove foreign key for database was previously unable to apply
as it could not find the name of the field within the state.
This is modified to be able to function with only a name
and model provided, even if it is not on the state by handling
the error and allowing the ForeignKeyManager to be populated
with an optional field value.

It is noted that the Field value is still required in order for
other operations to occur however.
@s-cooper18 s-cooper18 changed the title Allow db only remove foreign key to work as expected Allow db only remove foreign key to work as database only Dec 11, 2025
model_name: str,
column_name: str,
field: models.ForeignKey[models.Model],
field: models.ForeignKey[models.Model] | None,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🧀 It feels a little clunky to have this still being optional, but causes the add_fk_field to fail if the field is not added. Especially with having to do asserts from within the function in order to confirm that field is actually populated.

Alternative I can think of is having one manager where it is optional and another where it is not, but this seems like possible overkill

@s-cooper18
s-cooper18 force-pushed the s-cooper18/remove-foreign-key-without-state branch from 7470d0c to 1ae9b6a Compare December 11, 2025 06:00
@s-cooper18
s-cooper18 marked this pull request as ready for review December 11, 2025 06:05
@s-cooper18
s-cooper18 requested a review from a team as a code owner December 11, 2025 06:05

@marcelofern marcelofern left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Cool! I've left a comment about the CHANGELOG.md change
but please do :shipit:

Comment thread CHANGELOG.md Outdated
Comment on lines +13 to +15
- Fixed a bug where `SaferRemoveFieldForeignKey` relied on the Foreign Key also
existing in Django state, even when being performed as a database only operation.
The name and model are already provided as part of the operation.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not sure if this is valid markdown list item? I.e., isn't it missing a padding on the second and third lines like the other entries below?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Seems to be valid markdown, but makes it a "quote" instead. Have pushed a fix anyway

@s-cooper18
s-cooper18 force-pushed the s-cooper18/remove-foreign-key-without-state branch from 1ae9b6a to 798e58c Compare December 12, 2025 02:58
@s-cooper18
s-cooper18 merged commit cb0c0e8 into main Dec 12, 2025
11 checks passed
@s-cooper18
s-cooper18 deleted the s-cooper18/remove-foreign-key-without-state branch December 12, 2025 03:10
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.

3 participants