Opened 8 years ago

Closed 6 years ago

#24865 closed New feature (fixed)

Add a management command to remove stale content types

Reported by: Kay Owned by: Tim Graham <timograham@…>
Component: contrib.contenttypes Version: dev
Severity: Normal Keywords: stale contenttypes
Cc: Kay, emorley@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

This patch creates a new method remove_contenttypes that, when called from a migration, will programmatically remove the stale content type.

Diff: https://github.com/django/django/compare/master...Protosac:removalUtility

Change History (20)

comment:1 Changed 8 years ago by Tim Graham

Component: Uncategorizedcontrib.contenttypes
Needs documentation: set
Needs tests: set
Summary: Adding a feature to programmatically remove stale content typesAdd a feature to programmatically remove stale content types
Triage Stage: UnreviewedAccepted
Version: 1.7master

Could you please send a pull request with tests and documentation? You may find the patch review checklist helpful.

comment:2 in reply to:  1 Changed 8 years ago by Kay

Replying to timgraham:

Could you please send a pull request with tests and documentation? You may find the patch review checklist helpful.

I've looked the specs over and I'm not sure this warrants a test. The current Django tests for update_contenttypes already test for interactive. All this change does is automatically respond to it's prompt in the very same way the test_interactive_true does.

def test_interactive_true(self):
        """
        interactive mode of update_contenttypes() (the default) should delete
        stale contenttypes.
        """
        management.input = lambda x: force_str("yes")
        with captured_stdout() as stdout:
            management.update_contenttypes(self.app_config)
        self.assertIn("Deleting stale content type", stdout.getvalue())
        self.assertEqual(ContentType.objects.count(), self.before_count)

As for documentation I was considering placing it in the contenttypes.txt and adding a Management section to describe the force_remove argument and remove_contenttypes method. If that's an inappropriate place, please make a recommendation. Thanks again.

Last edited 8 years ago by Kay (previous) (diff)

comment:3 Changed 8 years ago by Tim Graham

Tests would ensure that the remove_contenttypes() function performs the job it's expected to do (deleting content types for the given app).

For docs, I'd suggest to add a "Utilities" section which documents remove_contenttypes(). No need to document the update_contenttypes() method -- that should remain a private function in my opinion.

comment:4 Changed 8 years ago by Markus Holtermann

I think remove_contenttypes() should remain private as well. It's way too easy to loose data when using that function. I'd put the respective documentation for that method into the function's docstring.

comment:5 Changed 8 years ago by Markus Holtermann

Patch needs improvement: set

comment:6 Changed 8 years ago by Tim Graham

Markus, can you elaborate on the data loss concerns? I guess it's about the possibility of delete cascades which is why interactive prompts were originally added in #12339. I don't think there's much value in completing this ticket if it isn't a public API. I'd think documentation that explains the data-loss considerations would do more to educate users than keeping the function private would. If you have another idea on how to address the use case in #24820 I'm open to alternatives.

comment:7 Changed 8 years ago by Markus Holtermann

Tim, neither patch is guaranteed to work when a user has not applied all contenttypes migrations, as the model used inside the update_contenttypes is from the global apps, not from the latest applied migration state (see #24100 for that).

Anyway, I'm ok with making it a public API if we put a .. warning:: box around the docs and state the potential data loss due to cascading.

comment:8 Changed 8 years ago by Kay

Cc: Kay added

comment:9 Changed 8 years ago by Kay

Triage Stage: AcceptedReady for checkin

comment:10 Changed 8 years ago by Tim Graham

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: Ready for checkinAccepted

comment:11 Changed 8 years ago by Ed Morley

Cc: emorley@… added

comment:12 Changed 8 years ago by Tim Graham

Patch needs improvement: set

comment:13 Changed 8 years ago by Tim Graham

Markus and I chatted in IRC and came up with the idea of moving stale content types deletion out of the post_migrate signal update_contenttypes function and into a separate remove_stale_contenttypes management command. This would default to interactive=True but could be called with call_command('remove_stale_contenttypes', interactive=False) to delete without a prompt.

How does this sound?

comment:14 Changed 7 years ago by Tim Graham

Owner: changed from nobody to Tim Graham
Status: newassigned

I started working on the management command described in the previous comment: PR

comment:15 Changed 7 years ago by Tim Graham

Owner: Tim Graham deleted
Status: assignednew

I'd welcome someone continuing where I left off here.

comment:16 Changed 7 years ago by Amos Onn

I think that the other side of not removing stale content types for fear of data loss is leaving stale GenericForeignKeys, which will break when accessed (see #25931). Hard to say which is worse.
I think perhaps adding a switch to migrate is better: --remove_stale_contenttypes or --keep_stale_contenttypes, and if none are present then default to current interactive behaviour.

comment:17 Changed 7 years ago by Tim Graham

I doubt it's acceptable to add a contrib app specific option to migrate.

comment:18 Changed 6 years ago by Tim Graham

Patch needs improvement: unset
Summary: Add a feature to programmatically remove stale content typesAdd a management command to remove stale content types

I completed my old PR.

comment:19 Changed 6 years ago by Simon Charette

Triage Stage: AcceptedReady for checkin

comment:20 Changed 6 years ago by Tim Graham <timograham@…>

Owner: set to Tim Graham <timograham@…>
Resolution: fixed
Status: newclosed

In 6a2af014:

Fixed #24865 -- Added remove_stale_contenttypes management command.

Thanks Simon Charette for the review.

Note: See TracTickets for help on using tickets.
Back to Top