Move cleanup management command to contrib.sessions

[rasca]

Right now it's in core.management

I propose moving it inside contrib.sessions.management seen that other contrib commands are inside their packages.

[Preston Holmes]

I think the note about "(only expired sessions at the moment)" has always left the door open for other non-sessions related cleanup. But this hasn't proven to be the case in 4-5 years, and it could be argued that any future cleanup should not be coupled to sessions.

I'd be moderately in favor of making the command name more explicit, perhaps "clearsessions", with a deprecation for the old one.

[Tomáš Ehrlich]

Fixed in pull ​https://github.com/django/django/pull/418

[Preston Holmes]

This is a great start - the patch will need docs (mostly a move of current docs), including an item in the release notes, and tests - the test can probably be just the old tests pointing to the new command. Rather than raise a deprecation warning - the old cleanup command could output a terminal message (in red) about the move.

Pull requests should generally be opened for complete patches, ready for final review.

[Tomáš Ehrlich]

Ok, it was easier for me to pull request rather than submit patch here…

I just don't agree with any deprecation warning. The cleanup command do the same job as it always did -- it cleans expired sessions and it requires Sessions installed. I don't think it will be removed in future. Each app will probably provide it's own clean* method and cleanup will collect them.

Possible feature in this scenario would be new setting, where one could define list of all clean* commands triggered by cleanup command. This would be handy, eg: django-registration provides cleanupregistration. This would be quiet easily implemented. Should I post a patch for it or open new ticket for this feature?

Last thing, I just can't find any tests for old cleanup command. I have to probably write them…

[Tomáš Ehrlich]

Cleansessions command and documentation

[Preston Holmes]

Sorry about the confusion on pull requests - it is an area for growth as we transition to github.

I agree about not deprecating cleanup - it can remain as a simple alias for now.

I do think we pull it from the documentation entirely for now (but see ramble below)

Any new features of a future implementation of the cleanup command are beyond the scope of this particular ticket, and yes - would require its own ticket. However I can tell you that the approach of using a setting is probably a non-starter. Perhaps allow any app to define a 'cleanup' command - and the core cleanup command would use the command discovery machinery to find and run each one of them. However that will take some design consideration, as it is an entirely new behavior for management commands, and becomes perhaps a bit too implicit/magical. If this were the approach taken, the sessions cleanup - would once again be called cleanup - and so perhaps these issues should be coupled. In otherwords, make that decision first.

So to rephrase, cleanup would become a new "special" management command - which will simply call the cleanup command in any app, and the current sessions cleanup command would be one such example located in contrib.sessions.

[Tomáš Ehrlich]

The pull is there: ​https://github.com/django/django/pull/420

And yes, I assume that future implementation of cleanup would require more discussion. I'll leave it for now.

[Preston Holmes]

So I'm going to retract the whole idea of a "special" pattern for cleanup - as pointed by Aymeric in IRC and myself in comment 1 - there hasn't proven to be a big need for a generic cleanup routine, and implementing this pattern would be a case of YAGNI design.

New tests are still needed - preferably inside contrib.sessions, not in admin_scripts where I would have expected the old ones to be, but I agree - this doesn't seem to be currently tested.

There is also a tiny cleanup needed in bin/daily-cleanup.py

[Tomáš Ehrlich]

Sorry for delay,
new updates were added to pull request, documentation and tests.

[Preston Holmes]

refs #18194

we should probably integrate the work of the following patch into this work - which isn't directly related to the ticket it's attached to.

https://code.djangoproject.com/attachment/ticket/18194/sessions-cleanup-files-1.patch

[Tomáš Ehrlich]

It's there, but right now is cleanup method implemented for database and file based session store only. There're also missing tests for file based clean up method…

[Tomáš Ehrlich]

Alright, I've found, that file-based sessions doesn't support deletion of expired files and patch above doesn't provided it either. It is in another patch from #18194, https://code.djangoproject.com/attachment/ticket/18194/files-sessions-server-expiry-3.patch. However, I can't agree with it's implementation. See related ticket for more details.

[Rohan Jain]

The changes which the linked patches from #18194 bring in are updated in a pull request I have posted over github: ​https://github.com/django/django/pull/452. I also mentioned this ticket over there.

[Tomáš Ehrlich]

Hi crodjer,
you've included only https://code.djangoproject.com/attachment/ticket/18194/sessions-cleanup-files-1.patch which is not enough as I mentioned before:

Alright, I've found, that file-based sessions doesn't support deletion of expired files and patch above doesn't provided it either. It is in another patch from #18194, https://code.djangoproject.com/attachment/ticket/18194/files-sessions-server-expiry-3.patch. However, I can't agree with it's implementation. See related ticket for more details.

I've already applied that patch to this commit ​https://github.com/elvard/django/commit/969822aa8b51af7840d5809b649101b291d26623

As I said, it doesn't work, since file-based sessions doesn't have any "expiration" mechanism. It's included in the other patch, but I can't agree with it's implementation, so I opened discussion at #18194.

[Aymeric Augustin]

The discussion has diverged from the original report :/

cleanup is a dependency of core on contrib, and such dependencies shouldn't exist. This means me must move the cleanup command to django.contrib.sessions and give it a sessions-specific name. clearsessions will do. Can we limit the scope of this ticket to this change? I have a patch ready for this.

There hasn't been an obvious need for a more generic cleanup mechanism over the last five years. Designing one would be a new feature. This should be done separately from this bugfix. (IMHO it's much better to keep separate management commands because they may profit from being run at different times, on different servers, etc.)

It isn't useful to provide two different ways to achieve exactly the same effect; for this reason the undocumented and legacy daily_cleanup.py script should be removed rather than documented.

Finally, there's the question of clearing expired sessions in backends other than the database backend. This means pushing the clean up logic inside the backends, like the latest pull request does. This doesn't make sense with the cookie backend — data is stored by the clients. This isn't doable with the cache backend because we can't iterate over all keys — cache might handle its own expiration. This only leaves the file backend, which is tracked by #18194.

Note that there are two different goals in clearing expired sessions:

• freeing storage space;
• prevent an attacker from re-using a session indefinitely — this is probably why #18194 was classified as a release blocker.

[Aymeric Augustin]

In fact daily_cleanup.py should have been removed in #5522, like make-messages.py and compile-messages.py.

[Aymeric Augustin <aymeric.augustin@…>]

In b760503a270484679ad0e06b73e3ed417a113221:

[1.5.x] Fixed #18978 -- Moved cleanup command to sessions.

This removes a dependency of 'core' on 'contrib'.

Backport of 83ba0a9 from master.

This deprecation occurs after the alpha, but it's a prerequisite
for fixing decently #18194 which is a release blocker.