Deleting an object does not delete it's activity about django-activity-stream HOT 22 CLOSED

good idea. just not sure whether it should be hooked up by default. needs more pondering time

from django-activity-stream.

On Sat, Jul 2, 2011 at 5:20 PM, justquick <
[email protected]>wrote:

good idea. just not sure whether it should be hooked up by default. needs
more pondering time

Thanks!

I'm curious how you expect to maintain the activity database in a sane state
without such a hook.

FTR I learned the lesson the hard way - in production. If you hook it by
default: i don't think that will seriously break any BC.

Thanks again for your great work on actstream, it works like a charm with
django-endless-pagination!

Cheers

from django-activity-stream.

https://docs.djangoproject.com/en/dev/ref/contrib/contenttypes/#reverse-generic-relations

Trying to add relevance and context here but isn't it the normal behavior of Django to delete? Is it that the GenericForeignKey is missing?

from django-activity-stream.

i think the default behavior is to nuke fk relations when an obj is deleted but im not sure if the same is true for gfks since it could be anything. now im gonna have to do some testing...

btw - got the example project deployed on gondor.io: http://yu713.o1.gondor.io/

from django-activity-stream.

The feed looks like:

Two started following CoolGroup 10 months, 2 weeks ago
Two joined CoolGroup 10 months, 2 weeks ago

Which means that there are at least two activity objects with a generic
relation to group object "CoolGroup".

I deleted group CoolGroup via the admin. (It didn't mention that it would
cascade delete any action object). Now the actions have broken relations and
the feed looks like:

Two started following 10 months, 2 weeks ago
Two joined 10 months, 2 weeks ago

So, the database now contains broken relations ... Is this the excepted
default behaviour ?

Regards

from django-activity-stream.

looks like from reading the docs that django does this by default iff you implement the GenericRelation class. ill give it a try and see what happens in the example project

from django-activity-stream.

Do you mean that Django should check each database table of each models with a generic relation at each single and multiple object delete on its own ?

from django-activity-stream.

This change causes a major regression: deleting models with non-integer primary keys no longer works. This includes django.contrib.sessions, for example:

Traceback:

[...]
File "[...]/django/contrib/auth/__init__.py" in login
  80.         request.session.cycle_key()
File "[...]/django/contrib/sessions/backends/base.py" in cycle_key
  273.         self.delete(key)
File "[...]/django/contrib/sessions/backends/db.py" in delete
  76.             Session.objects.get(session_key=session_key).delete()
[...]
File "[...]/actstream/models.py" in delete_orphaned_actions
  273.         Q(target_object_id=pk, target_content_type=ctype)
File "[...]/django/db/models/manager.py" in filter
  141.         return self.get_query_set().filter(*args, **kwargs)
[...]
File "[...]/django/db/models/fields/__init__.py" in get_prep_value
  876.         return int(value)

Exception Type: ValueError at /accounts/signin/
Exception Value: invalid literal for int() with base 10: '403b0f3b305ff792b87ad66016cab4ef'

from django-activity-stream.

In general, this approach is not safe by default: it makes too many assumptions about other models, and will probably end up failing in more ways than the above. The performance cost of adding multiple extra queries to every single object delete is also potentially dire: this is one reason Django does not do this by default already.

A better way to approach this, probably, is using a django-admin management command to perform the cleanup (which may be scheduled using cron, and so on). If a developer wants cleanup to be more real-time than that, they can still declare a delete hook like the above, but hopefully only on the models they're interested in, rather than all Django deletions.

from django-activity-stream.

i think this is an underlying data problem with the app as all gfk pk fields in models.py are PositiveIntegerField. looking to django comments, they use TextField for pks.

https://code.djangoproject.com/browser/django/trunk/django/contrib/comments/models.py

a pk field in reality could be any number of types, but TextField seems the friendliest on the db in my experience. changing this would unfortunately require a schema migration to change the field type and then probably a data migration to recast all the pks as text. yuck. but after this is done, orphan pruning should happen on the post_delete signal and not need to be done in cron.

i do like the idea of a user being able to select which models they want to watch for orphan action pruning. maybe a setting?

from django-activity-stream.

but after this is done, orphan pruning should happen on the post_delete signal and not need to be done in cron.

I'm not so sure... a global post_delete handler is very aggressive, and the kind of potential complication that should be left to the user, rather than being a library default. At most, i think it should be an optional feature that you could enable using a Django setting, for example.

i do like the idea of a user being able to select which models they want to watch for orphan action pruning. maybe a setting?

One way of doing this is simply to declare a GenericRelation on the target models: https://docs.djangoproject.com/en/dev/ref/contrib/contenttypes/#reverse-generic-relations

This is Django's way of declaring it, and should give the user reasonable control over where they want this feature, without impacting unrelated models.

from django-activity-stream.

It's true, the slot has this in my production code:

if sender.__name__ == 'Session':
      return None

Sorry i should have notified you about the update. Maybe it's better to change it a PK check or even some counts.

By leaving the complication to the user, he can make errors like I did and learn the hard way ... Maybe a settings like ACTSTREAM_KEEP_DATABASE_SANE could be added to control the activation of this feature.

from django-activity-stream.

Would anyone object if i remove the default post_delete hook, and implement a management command?

This is currently a show-stopper for us.

from django-activity-stream.

I really don't mind. But if you're suggesting to implement it in a management command, i can only believe that you haven't experienced dead relations in activities in production ... else you'd want it live.

Replace a "show-stopper" now by a "show-stopper" tomorrow. What can I say ... You've been warned ;)

from django-activity-stream.

turned off by default in v0.3.9 now on pypi

from django-activity-stream.

justquick: Thanks!

jpic: I certainly have experienced it: #31 was a direct result of crashes on our production server. :)

The underlying issue here is that GenericForeignKey fields do not provide the same validity guarantee that ForeignKey does: it is always possible for generic reference fields to become stale, and return None when their target is gone. Code using generic references must be prepared to handle these None values appropriately (which should not be hard: for example, in #31, the stale Follows are simply ignored / filtered out).

Once the code is generic-relation-safe, there should be no show-stoppers: a management command would only be there to free up space, à la expired session cleanup.

from django-activity-stream.

Of course, you can hack all around to ignore broken generic relations. If that works for you then you should keep it this way. Call me a maniac, or hardcore sysadmin, i prefer to just preserve my database's sanity. There's a reason Django provides constraints and cascade delete on FKs by default. I think this debate is over, it's a matter of taste and it's an option now :)

from django-activity-stream.

no management commands and no signals. ive gotta find a sane way to create GenericRelations so i dont have to deal with this mess anymore

from django-activity-stream.

justquick wrote:

no management commands and no signals.

I tend to agree: although i suggested a management command as a lesser-evil alternative post_delete signal, neither should be really be necessary.

ive gotta find a sane way to create GenericRelations so i dont have to deal with this mess anymore

I think the documentation should just recommend that users declare reverse GenericRelation fields on the models they intend to use as targets for Action and Follow most often, if they want those records to be reclaimed automatically on delete. This should be enough (assuming the code is otherwise GFK-safe), and is pretty much in line with Django's GenericRelation documentation.

from django-activity-stream.

the app will be able to create those relationships for you on the fly. so is the way of the django. basically any model (eg User) will have the following GenericRelations

User.actions - Actions where user is actor
User.target_actions - Actions where user is target
User.action_object_actions - Actions where user is action_object

i know the naming sucks. any better ideas? im also thinking about making the names configurable so if you already had a model w/ an actions attr you could change it to use another name.

from django-activity-stream.

In reply to your suggestion call:

User.actions_as_actor
User.actions_as_target
User.actions_as_action_object
User.related_actions # all of the above ?

That said I directly use the ORM to select actions and it works great ... Is the public Action select API the feature that has the most room for improvement in actstream ?

from django-activity-stream.

yea, that and letting people create/register/use custom streams by name (eg User.my_custom_action_stream)

from django-activity-stream.