Unable to delete pages containing custom content types

[chrisspen]

I have several custom page content types of the form:

class MyWidget(models.Model):

    ...fields...

    class Meta:
        abstract = True
        verbose_name = _('my widget')
        verbose_name_plural = _('my widgets')

Page.create_content_type(MyWidget)

If I add these to a page, and later try and delete the page I get the IntegrityError exception:

IntegrityError at /admin/page/page/82/delete/
(1451, 'Cannot delete or update a parent row: a foreign key constraint fails (mydatabase.page_page_mywidget, CONSTRAINT parent_id_refs_id_54aa763d FOREIGN KEY (parent_id) REFERENCES page_page (id))')

I'm assuming this is due to an inappropriate Django ondelete handler on FK field for the page_page_mywidget table autogenerated by Page.create_content_type().

Ideally, when a page is deleted, any rows in content type tables linked to the deleted page should also be deleted. I believe this corresponds to the CASCADE ondelete behavior.

A slow and frustrating workaround is to manually delete all content types on the page, save the page, and then delete the page.

[matthiask]

Hi Chris

Which (exact) Django and FeinCMS version are you using? The code in feincms/__init__.py should prevent the bug you're seeing from happening, but obviously something does not quite work.

https://github.com/feincms/feincms/blob/master/feincms/__init__.py#L42

[chrisspen]

I'm using Django 1.4 and FeinCMS 1.5.4. I'll retest with 1.6.2.

[jonasvp]

This has been plaguing me as well. It seemed at first that django-reversion was to blame but removing it did not solve the issue. It only triggers on PostgreSQL, works fine with SQLite. Probably because the latter does not check constraints... I'm trying to develop a minimal test case, with no luck so far.

@chrisspen If you found a fix or workaround, do check back!

[matthiask]

This sounds more and more like a failure of feincms.ensure_completely_loaded. I'd really like to see this issue resolved, but don't know where to start right now.

[wojas]

This is a workaround for the problem: https://gist.github.com/4059168

It registers a pre-delete handler for your content container that will delete all attached content types. This fix could be integrated in FeinCMS, if it does not break other stuff.

[jonasvp]

Thanks @wojas, works like a charm!

[cogat]

We experienced this issue (FeinCMS@914eb0, Django 1.4.3). Our workaround is to add the following to urls.py:

from feincms.module.page.models import Page
Page._meta._fill_related_objects_cache()

[matthiask]

@cogat Interesting. This means that the code in https://github.com/feincms/feincms/blob/master/feincms/__init__.py#L53 does not do the right thing in this case. Now that we do not monkey-patch django.core.urlresolvers.reverse anymore, that's the ugliest thing left in FeinCMS.

Does anyone have a good idea how this issue could be fixed for good? Unfortunately it has a long history already...

[cogat]

A log of the calls to ensure_completely_loaded and Page.create_content_type indicates that ensure_completely_loaded is called (and 'ensured') before the page content types are all registered.

in ensure_completely_loaded (COMPLETELY_LOADED = False)
creating Page content types
in ensure_completely_loaded (COMPLETELY_LOADED = True)

the first call is from another app that uses feincms.models.Base, so it looks like the underlying problem is that there are several Base models in play, and ensure_completely_loaded isn't in a position to do what it claims while models are still being discovered by Django.

Commenting out the lines

   if COMPLETELY_LOADED:
       return True

solves the problem, but at the expense of performance - the cache has to be repopulated once per model per request.

I suggest a parameter force that bypasses the COMPLETELY_LOADED check, and a call to ensure_completely_loaded(force=True) when a content type is registered.

Pull request coming...

EDIT:

By Django design, ensure_completely_loaded cannot do what it claims to be able to do, since models can (theoretically) be imported at any time, and it's only the app_cache that attempts to pick them up at load time. This is why there is no Django signal for models loaded [1]. I suggest the function is renamed to clear_app_cache, but will leave that to @matthiask.

[1] https://code.djangoproject.com/ticket/7538

[matthiask]

@cogat Sounds good. As long as the performance hit only happens on application startup (whatever that is given the current state of things) that's fine. I'm still having some doubts, mainly: Does deleting the cache repeatedly really have the same consequence as calling Page._meta._fill_related_objects_cache() directly?

Regarding clear_app_cache: Good idea, I'll rename it. clear_model_meta_cache or something might be a better name though. What do you think?

[cogat]

Does deleting the cache repeatedly really have the same consequence as calling Page._meta._fill_related_objects_cache() directly?

Deleting the cache will cause Model._meta._fill_related_objects_cache() to be called the next time the cache is accessed (which only happens within Model._meta.get_all_related_objects_with_model() [1]).

Two things to note, however: Firstly, _fill_related_objects_cache() populates _related_objects_cache AND _related_objects_proxy_cache. The latter is not explicitly invalidated by ensure_completely_loaded() (it doesn't make a difference now, but may in future, or if a 3rd party uses _related_objects_proxy_cache for some reason).

Secondly, ensure_completely_loaded() also clears many other caches, and on all models, which is perhaps overkill. As far as I can tell, only the Base model's _related_objects_cache needs to be flushed at create_content_type() time. I'm tempted to revise my patch in that case.

Side note: calling Page._meta._fill_related_objects_cache() in urls.py is insufficient as well as inelegant, because the only sensible place to put the calls is in urls.py, which means that until urls.py is imported, the _related_objects_cache is potentially incomplete. This would have repercussions if migrations or shell scripts were to try to delete Feincms model instances - the delete wouldn't cascade to content_type objects for any except the first feincms.Base model.

Regarding clear_app_cache: Good idea, I'll rename it. clear_model_meta_cache or something might be a better name though. What do you think?

Yes - I mistook which cache gets cleared (I somehow thought it was django.db.models.loading.cache, but it's the model._meta caches, you're right). I would put a plural in there to better describe the action: clear_model_meta_caches.

[1] https://github.com/django/django/blob/master/django/db/models/options.py#L399

[cogat]

After trying this in a project, I have found that the patch in ixc@3ceb1a3 can cause problems in which not all reverse-related fields are registered by Django. I'm not sure of the exact cause (in a Django shell, calling _fill_related_objects_cache followed by init_name_map() fixes the related fields, although doing the same in ensure_completely_loaded() doesn't work). Reverting the patch (to the 'sledgehammer' approach in ixc@a51670d) fixes the problem.

[cogat]

Reverted in 5632c1c

[matthiask]

Alright, thanks!

Can we close this issue now?

[cogat]

Yes, I believe so.

On 15 February 2013 19:03, Matthias Kestenholz notifications@github.comwrote:

Alright, thanks!

Can we close this issue now?

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/323#issuecomment-13595985.

Dr Greg Turner
Chief Executive Officer
the Interaction Consortium

http://interaction.net.au
Phone: 1300 43 78 99
skype: gregturner
Follow us on twitter:
@theIC http://twitter.com/_theIC_
@Gsta http://twitter.com/gsta

Be green! Read from the screen.

[matthiask]

Ok, thanks!

[smacker]

This patch broke my application.
I didn't have time to deal well where problem is, but ensure_completely_loaded(force=False) in create_content_type fixed my problem.

I have big application, where my modules located in package. So, I have validation errors like:

idecoapp.productaddition: 'product' has a relation with model <class 'idecoapp.models.products.ProductEdition'>, which has either not been installed or is abstract.

Maybe later I'll have time to investigate, but this patch breaks some applications for sure.

[cogat]

I've been able to reproduce this with Django 1.5. Django 1.4.1 doesn't exhibit this problem.

[cogat]

The commit above should address this issue @smacker @jphalip though our fork isn't at the current HEAD revision. Maybe wait for #398 to be merged.

[smacker]

@cogat Great! Thanks!

[matthiask]

Very nice. Thanks!

[DrMeers]

Unfortunately this hasn't gone away; I'm using the latest FeinCMS next branch with Django 1.5.4 and:

>>> import feincms
>>> feincms.VERSION
(1, 8, 0, 'pre')
>>> django.VERSION
(1, 5, 4, 'final', 0)
>>> def count_related_objects(model):
...     return len(
...         model._meta.get_all_related_objects(
...             include_hidden=True,
...             include_proxy_eq=True,
...         )
...     )
... 
>>> from content.models import Page
>>> count_related_objects(Page)
5
>>> feincms.ensure_completely_loaded()
True
>>> count_related_objects(Page)
5
>>> feincms.ensure_completely_loaded(force=True)
True
>>> count_related_objects(Page)
33

[matthiask]

So at least ensure_completely_loaded(force=True) seems to do the right thing. Should we try to run the code in ensure_completely_loaded two times instead of only once? Once to load all models, then once again to make sure that we restart from an environment where everything is already loaded?

...

[matthiask]

Something like that (hack alarm):

matthiask@d0725b0

[DrMeers]

Not sure if I understood you correctly, just I tried removing the early return statement and duplicating the content of ensure_completely_loaded so that it is effectively forced to run twice upon every call, but there was no improvement.

[DrMeers]

Let me try your hack commit too to confirm ;-)

[DrMeers]

Nope, problem persists :(

[DrMeers]

The problem seems to be that the Page model isn't actually returned by loading.get_models when ensure_completely_loaded is called (e.g. after each content type is registered), so its _related_objects_cache is not cleared. This can be checked by loading.app_cache_ready() which returns False at content type registration time. Would it be a problem to return False if not loading.app_cache_ready() within ensure_completely_loaded? This seems to resolve the issue for me, but I'm not familiar enough with ensure_completely_loaded to know whether this is appropriate.

So I think you can remove your hack branch above (and I think > 0 should be == 0?).

[matthiask]

Ah stupid me. This should have been if _completely_loaded_counter <= 0 and not force:

Yes, using app_cache_ready() sounds good. It should probably be if loading.app_cache_ready(): COMPLETELY_LOADED = True then?

How is it even possible that get_models() would not return the Page model, though?

[DrMeers]

Sounds good to me.

If you want to find out more, django.db.models.loading makes for "interesting" reading. During population of the cache, apart from threadsafe locking issues etc, module additions can be postponed if Python hasn't finished importing them (which is likely the case while we're still calling create_content_type etc within that module).

[DrMeers]

Mind you we still have the issue of finding a way of automatically calling ensure_completely_loaded once the app cache is ready...

[matthiask]

ensure_completely_loaded is called in several places; for example when the item editor is initialized, when accessing Page.template etc.

This should hopefully be sufficient...

[DrMeers]

No, unfortunately not:

>>> from content.models import Page
>>> len(Page._meta.get_all_related_objects())
1
>>> feincms.ensure_completely_loaded()
True
>>> len(Page._meta.get_all_related_objects())
27

As far as I'm aware there is no app_cache_ready signal or similar; something like that would be ideal.

[matthiask]

What if you access Page.template in the shell? Does that change anything?

from content.models import Page
page = Page.objects.all()[0]
page.template
len(Page._meta.get_all_related_objects())

[matthiask]

... or access page.content, this should work as well.

[DrMeers]

Not Page.template -- that's a property object -- but yes page_instance.template does the trick.

See https://code.djangoproject.com/ticket/19212 for Russ' overview of the long-standing "app loading" issues in relation to this.

[matthiask]

Good to hear. Yes, I know about the app loading / app refactor issues. Unfortunately, the migration work also touched a lot of code in that area (app cache and everything) so a solution seems to be far away again.