Skip to content

feat: Require explicit on_delete for ForeignKeyField #1896

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 4 commits into
base: develop
Choose a base branch
from

Conversation

Hammer2900
Copy link

This change removes the default CASCADE behavior for the on_delete parameter in ForeignKeyField, making it mandatory. This prevents accidental data loss due to implicit cascading deletes. All tests have been updated to provide an explicit on_delete value.

Fixes #1801

Description

This prevents accidental data loss due to implicit cascading deletes, as developers might not be aware of the default CASCADE behavior. By making on_delete mandatory, we ensure that the deletion behavior is always explicitly defined.

Motivation and Context

The default on_delete=CASCADE behavior was identified as a potential source of data loss in issue #1801. Users pointed out that accidental deletions could lead to unintended cascading deletes, especially if developers were not fully aware of the default setting. This change aims to improve data safety by requiring explicit configuration of the deletion behavior.

How Has This Been Tested?

Ran the full test suite using pytest.
Tested on Python 3.13 with SQLite backends.

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added the changelog accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Tsybulskyi Evhenyi added 2 commits February 25, 2025 14:52
This change removes the default CASCADE behavior for the on_delete parameter
in ForeignKeyField, making it mandatory.  This prevents accidental data loss
due to implicit cascading deletes.  All tests have been updated to provide
an explicit on_delete value.

Fixes tortoise#1801
@waketzheng
Copy link
Contributor

@Hammer2900 Why do you use fields.RESTRICT instead of fields.CASCADE for test?

Copy link

codspeed-hq bot commented Feb 26, 2025

CodSpeed Performance Report

Merging #1896 will not alter performance

Comparing Hammer2900:on_delete_should_be_required (ea380de) with develop (21463c7)

Summary

✅ 16 untouched benchmarks

@henadzit
Copy link
Contributor

I like the change! A few notes:

  • CI is failing
  • Could you please update the CHANGELOG with a new version (0.25.0) and a helpful description of the change? We need to tell people what was the default value and what they need to do.
  • @waketzheng How this change is going to affect aerich?

@Hammer2900
Copy link
Author

@waketzheng Thanks for the review. I used fields.RESTRICT in the tests primarily to ensure that the tests are focused on verifying the mandatory nature of the on_delete parameter.
While fields.CASCADE would also work, fields.RESTRICT is a good choice because avoids any potential side effects of cascading deletes during testing. It clearly demonstrates that we are intentionally choosing a specific deletion behavior, rather than relying on a default.

Furthermore, choosing fields.RESTRICT aligns with the overall goal of the issue, which was to move away from the potentially dangerous default fields.CASCADE behavior. By using fields.RESTRICT in the tests, we reinforce the idea that a deliberate choice about deletion behavior is now required.

@henadzit I'll try to add more to the CHANGELOG tonight.

@waketzheng
Copy link
Contributor

  • @waketzheng How this change is going to affect aerich?

I will make a PR to aerich after this one merged.

This change removes the default CASCADE behavior for the `on_delete` parameter
in ForeignKeyField, making it mandatory. This prevents accidental data loss
due to implicit cascading deletes.

Code Changes:
- Updated ForeignKeyField definition to require on_delete.
- Updated all overloaded function signatures to match the new definition.
- Updated tests to provide an explicit on_delete value.

Documentation Changes:
- Added a dedicated section for the `on_delete` parameter.
- Updated examples to include explicit `on_delete` values.

Fixes tortoise#1801
@Hammer2900
Copy link
Author

@henadzit I've added a separate section with an anchor (.. _on-delete:) for the on_delete documentation, rather than making it a subsection within the ForeignKeyField section. I felt this gives it more prominence and makes it easier to link to from other parts of the documentation, given its importance and the breaking change.I believe this structure makes the documentation clearer and more maintainable. Let me know if you have any concerns or alternative suggestions.
And i encountered an issue with documentation generation on Python 3.13 due to the importlib_metadata module. In Python 3.13, importlib.metadata is part of the standard library, but the docs/conf.py file was explicitly importing the external importlib_metadata package, which caused a ModuleNotFoundError. I was able to get the documentation to build locally by changing the import statement in conf.py to

import importlib.metadata as importlib_metadata

However, I'm hesitant to include this change in the PR because it might break compatibility with older Python versions (<=3.13) where the external importlib_metadata package is required. I've left the conf.py file unchanged in this PR.
This is just a heads-up that there's a documentation build issue on Python 3.13 that needs to be addressed separately, likely with a conditional import in conf.py

@waketzheng
Copy link
Contributor

@Hammer2900 Cloud you fix the conflicts?

@henadzit
Copy link
Contributor

@Hammer2900 can you please also make on_delete required for OneToOneField?

Sorry for taking so long to review!

@henadzit henadzit requested a review from Copilot March 27, 2025 13:34
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR makes the deletion behavior of ForeignKeyField relationships explicit by requiring an explicit on_delete parameter, thereby preventing accidental data loss caused by implicit cascading deletes. The changes update various test and example files to include on_delete=fields.RESTRICT.

Reviewed Changes

Copilot reviewed 35 out of 39 changed files in this pull request and generated no comments.

Show a summary per file
File Description
tests/model_setup/model_bad_rel2.py Added on_delete=fields.RESTRICT to the ForeignKeyField in Event model.
tests/model_setup/model_bad_rel1.py Added on_delete=fields.RESTRICT to the ForeignKeyField in Event model.
examples/schema_create.py Updated the Event model FK to include on_delete=fields.RESTRICT.
examples/relations_with_unique.py Updated the Student model FK to include on_delete=fields.RESTRICT.
examples/relations_recursive.py Added on_delete=fields.RESTRICT to the recursive FK in Employee model.
examples/relations.py Updated the Event model FK to include on_delete=fields.RESTRICT.
examples/pydantic/tutorial_4.py Updated the Event model FK to include on_delete=fields.RESTRICT.
examples/pydantic/tutorial_3.py Updated the Event model FK to include on_delete=fields.RESTRICT.
examples/pydantic/recursive.py Added on_delete=fields.RESTRICT to the recursive FK in Employee model.
examples/pydantic/early_init.py Updated the Event model FK to include on_delete=fields.RESTRICT.
examples/pydantic/basic.py Updated the Event model FK to include on_delete=fields.RESTRICT.
examples/group_by.py Updated the Book model FK (Author relation) to include on_delete=fields.RESTRICT.
examples/global_table_name_generator.py Updated the BlogPost model FK to include on_delete=fields.RESTRICT.
examples/functions.py Updated the Event model FK to include on_delete=fields.RESTRICT.
examples/complex_prefetching.py Updated the Event model FK to include on_delete=fields.RESTRICT.
examples/complex_filtering.py Updated the Event model FK to include on_delete=fields.RESTRICT.
Files not reviewed (4)
  • CHANGELOG.rst: Language not supported
  • docs/contrib/pydantic.rst: Language not supported
  • docs/getting_started.rst: Language not supported
  • docs/models.rst: Language not supported

@Hammer2900
Copy link
Author

@henadzit Sure. I'll take a look at it this evening.

@henadzit
Copy link
Contributor

henadzit commented Apr 7, 2025

@Hammer2900 are you still interested in working on this? If not, no problem, I can pick this up.

@Hammer2900
Copy link
Author

I apologize, but due to circumstances, I can't stay at the computer constantly right now. So, if possible, could someone else finish it up, as there's not much left to do.

@waketzheng
Copy link
Contributor

@henadzit There are two PRs about requires on_delete. Cloud you finish this one and then we can close another.

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.

on_delete should be required
3 participants