[Django] #24865: Adding a feature to programmatically remove stale content types
Django
-------------------------------+--------------------------------
Reporter: Protosac | Owner: nobody
Type: New feature | Status: new
Component: Uncategorized | Version: 1.7
Severity: Normal | Keywords: stale contenttypes
Triage Stage: Unreviewed | Has patch: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------
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
--
Ticket URL: <https://code.djangoproject.com/ticket/24865>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
--------------------------------------+------------------------------------
Reporter: Protosac | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: stale contenttypes | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: => 0
* component: Uncategorized => contrib.contenttypes
* needs_tests: => 1
* version: 1.7 => master
* needs_docs: => 1
* stage: Unreviewed => Accepted
Comment:
Could you please send a pull request with tests and documentation? You may
find the [https://docs.djangoproject.com/en/dev/internals/contributing
/writing-code/submitting-patches/#patch-review-checklist patch review
checklist] helpful.
--
Ticket URL: <https://code.djangoproject.com/ticket/24865#comment:1>
Django
--------------------------------------+------------------------------------
Reporter: Protosac | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: stale contenttypes | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 0
Comment (by Protosac):
Replying to [comment:1 timgraham]:
> Could you please send a pull request with tests and documentation? You
may find the [https://docs.djangoproject.com/en/dev/internals/contributing
/writing-code/submitting-patches/#patch-review-checklist 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/24865#comment:2>
Django
--------------------------------------+------------------------------------
Reporter: Protosac | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: stale contenttypes | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 0
Comment (by timgraham):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/24865#comment:3>
Django
--------------------------------------+------------------------------------
Reporter: Protosac | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: stale contenttypes | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Needs tests: 1 | Patch needs improvement: 0
Comment (by MarkusH):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/24865#comment:4>
Django
--------------------------------------+------------------------------------
Reporter: Protosac | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: stale contenttypes | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Changes (by MarkusH):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/24865#comment:5>
Django
--------------------------------------+------------------------------------
Reporter: Protosac | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: stale contenttypes | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/24865#comment:6>
Django
--------------------------------------+------------------------------------
Reporter: Protosac | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: stale contenttypes | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Comment (by MarkusH):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/24865#comment:7>
Django
--------------------------------------+------------------------------------
Reporter: Protosac | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: stale contenttypes | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Protosac):
* cc: Protosac (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/24865#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Protosac | Owner: nobody
Type: New feature | Status: new
contrib.contenttypes |
Severity: Normal | Resolution:
Keywords: stale contenttypes | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Protosac):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/24865#comment:9>
Django
--------------------------------------+------------------------------------
Reporter: Protosac | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 1 => 0
* stage: Ready for checkin => Accepted
* needs_tests: 1 => 0
* needs_docs: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/24865#comment:10>
Django
--------------------------------------+------------------------------------
Reporter: Protosac | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: stale contenttypes | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by edmorley):
* cc: emorley@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/24865#comment:11>
Django
--------------------------------------+------------------------------------
Reporter: Protosac | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: stale contenttypes | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/24865#comment:12>
Django
--------------------------------------+------------------------------------
Reporter: Protosac | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: stale contenttypes | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/24865#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: Protosac | Owner: timgraham
Type: New feature | Status: assigned
Component: | Version: master
contrib.contenttypes |
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* owner: nobody => timgraham
* status: new => assigned
Comment:
I started working on the management command described in the previous
comment: [https://github.com/django/django/pull/5382 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/24865#comment:14>
Django
--------------------------------------+------------------------------------
Reporter: Protosac | Owner:
Type: New feature | Status: new
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* owner: timgraham =>
* status: assigned => new
Comment:
I'd welcome someone continuing where I left off here.
--
Ticket URL: <https://code.djangoproject.com/ticket/24865#comment:15>
Django
--------------------------------------+------------------------------------
Reporter: Protosac | Owner:
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: stale contenttypes | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by amosonn):
I think that the other side of not removing stale content types for fear
of data loss is leaving stale `GenericForeignKey`s, 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/24865#comment:16>
Django
--------------------------------------+------------------------------------
Reporter: Protosac | Owner:
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: stale contenttypes | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
I doubt it's acceptable to add a contrib app specific option to `migrate`.
--
Ticket URL: <https://code.djangoproject.com/ticket/24865#comment:17>
Django
--------------------------------------+------------------------------------
Reporter: Protosac | Owner:
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: stale contenttypes | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 1 => 0
Comment:
I completed my [https://github.com/django/django/pull/5382 old PR].
--
Ticket URL: <https://code.djangoproject.com/ticket/24865#comment:18>
Django
Reporter: Protosac | Owner:
Type: New feature | Status: new
contrib.contenttypes |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by charettes):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/24865#comment:19>
Django
-------------------------------------+-------------------------------------
| Graham <timograham@…>
Type: New feature | Status: closed
Component: | Version: master
contrib.contenttypes |
Severity: Normal | Resolution: fixed
Keywords: stale contenttypes | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by Tim Graham <timograham@…>):
* status: new => closed
* owner: => Tim Graham <timograham@…>
* resolution: => fixed
Comment:
In [changeset:"6a2af01452966d10155b720f4f5e7b09c7e3e419" 6a2af014]:
{{{
#!CommitTicketReference repository=""
revision="6a2af01452966d10155b720f4f5e7b09c7e3e419"
Fixed #24865 -- Added remove_stale_contenttypes management command.
Thanks Simon Charette for the review.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24865#comment:20>