Opened 3 years ago

Closed 2 years ago

#18978 closed Cleanup/optimization (fixed)

Move cleanup management command to contrib.sessions

Reported by: rasca Owned by: aaugustin
Component: Core (Management commands) Version: master
Severity: Normal Keywords:
Cc: tomas.ehrlich@…, crodjer@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

Right now it's in core.management

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

Attachments (2)

18978-1.diff (4.4 KB) - added by Elvard 3 years ago.
Cleansessions command and documentation
18978.patch (6.1 KB) - added by aaugustin 2 years ago.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 3 years ago by ptone

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

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.

comment:2 Changed 3 years ago by Elvard

  • Has patch set

comment:3 Changed 3 years ago by ptone

  • Needs documentation set
  • Needs tests set
  • Patch needs improvement set
  • Version changed from 1.4 to master

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.

comment:4 Changed 3 years ago by Elvard

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…

Changed 3 years ago by Elvard

Cleansessions command and documentation

comment:5 Changed 3 years ago by Elvard

  • Cc tomas.ehrlich@… added

comment:6 Changed 3 years ago by ptone

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.

comment:7 Changed 3 years ago by Elvard

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.

comment:8 Changed 3 years ago by ptone

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

comment:9 Changed 2 years ago by Elvard

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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

comment:10 Changed 2 years ago by ptone

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

comment:11 Changed 2 years ago by Elvard

  • Needs tests set

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…

comment:12 Changed 2 years ago by Elvard

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.

comment:13 Changed 2 years ago by crodjer

  • Cc crodjer@… added

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.

comment:14 Changed 2 years ago by Elvard

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.

comment:15 Changed 2 years ago by aaugustin

  • Owner changed from rasca to aaugustin

comment:16 Changed 2 years ago by aaugustin

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.
Last edited 2 years ago by aaugustin (previous) (diff)

comment:17 Changed 2 years ago by aaugustin

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

Changed 2 years ago by aaugustin

comment:18 Changed 2 years ago by Aymeric Augustin <aymeric.augustin@…>

  • Resolution set to fixed
  • Status changed from new to closed

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.

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