New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show that create_content_type modifies current context #528

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@meshy
Contributor

meshy commented May 30, 2014

I am trying to register a content type on two models:

ModelOne.register_regions(('main', 'Main'))
ModelOne.create_content_type(MyContent)
ModelTwo.register_regions(('main', 'Main'))
ModelTwo.create_content_type(MyContent)

But I am receiving an error from the final line:

ImproperlyConfigured: Cannot create content type from non-abstract model (yet).

This is because MyContent is getting redefined in my models.py on the second line.

I must admit I am finding this one very hard to debug, but I suspect it is caused by one of the calls to type() in feincms.models.

The attached diff has a failing test for the problem.

Do you know what may be causing this? I would love to help if I can, but I am a little stumped as to what action to take.

meshy added some commits May 30, 2014

Set up test scenario
Show that creating content type on custom model throws no error.
@meshy

This comment has been minimized.

Show comment
Hide comment
@meshy

meshy May 30, 2014

Contributor

FYI the type call that appears to be creating the mischief is models.py#L628-L643

Contributor

meshy commented May 30, 2014

FYI the type call that appears to be creating the mischief is models.py#L628-L643

@frog32

This comment has been minimized.

Show comment
Hide comment
@frog32

frog32 May 30, 2014

Contributor

I think feincms is handling type()the right way. Maybe it has something todo with the old (django<1.7) app loading stuff. Django is doing some nasty stuff there using the metaclass. This example from Aymeric Augustins talk at djangocon 2013 shows something quite similar to this problem:

https://static.myks.org/data/20140513-DjangoCon-App-loading.pdf (page 23)

When classes which subclass models.Model are created they try to register them self in the app registry. If they are already registered no new class is created and instead the already registered class is returned https://github.com/django/django/blob/stable/1.6.x/django/db/models/base.py#L137-L140

I think you should first check with django 1.7 if this is still a problem. If no we have a problem with the old app loading stuff.

Contributor

frog32 commented May 30, 2014

I think feincms is handling type()the right way. Maybe it has something todo with the old (django<1.7) app loading stuff. Django is doing some nasty stuff there using the metaclass. This example from Aymeric Augustins talk at djangocon 2013 shows something quite similar to this problem:

https://static.myks.org/data/20140513-DjangoCon-App-loading.pdf (page 23)

When classes which subclass models.Model are created they try to register them self in the app registry. If they are already registered no new class is created and instead the already registered class is returned https://github.com/django/django/blob/stable/1.6.x/django/db/models/base.py#L137-L140

I think you should first check with django 1.7 if this is still a problem. If no we have a problem with the old app loading stuff.

@matthiask

This comment has been minimized.

Show comment
Hide comment
@matthiask

matthiask May 31, 2014

Member

If I understand this report correctly, this should not happen because of the name clash check above (

other_model = get_model(cls._meta.app_label, class_name)
).

Unfortunately we cannot continue using this code with Django 1.7 because it requires the app cache which isn't ready at this point yet.

@meshy, does it work if you add the class_name argument to create_content_type? This should help if ModelOne and ModelTwo belong to the same app.

(I also have a different, new idea what could be the cause. I think that content type models cannot be declared in the same app as the feincms.models.Base-inheriting class because create_content_type puts the derived class in the same package as the feincms.models.Base-inheriting class. Where is MyContent declared? In the same package as ModelOne and/or ModelTwo? That's probably really hard to support. That being said, set class_name to something different and it should just work. Of course the class name and database table for the content type etc. would be different.)

Member

matthiask commented May 31, 2014

If I understand this report correctly, this should not happen because of the name clash check above (

other_model = get_model(cls._meta.app_label, class_name)
).

Unfortunately we cannot continue using this code with Django 1.7 because it requires the app cache which isn't ready at this point yet.

@meshy, does it work if you add the class_name argument to create_content_type? This should help if ModelOne and ModelTwo belong to the same app.

(I also have a different, new idea what could be the cause. I think that content type models cannot be declared in the same app as the feincms.models.Base-inheriting class because create_content_type puts the derived class in the same package as the feincms.models.Base-inheriting class. Where is MyContent declared? In the same package as ModelOne and/or ModelTwo? That's probably really hard to support. That being said, set class_name to something different and it should just work. Of course the class name and database table for the content type etc. would be different.)

@meshy

This comment has been minimized.

Show comment
Hide comment
@meshy

meshy Jun 3, 2014

Contributor

@frog32 I do not believe type is being misused, nor that this has anything to do with the metaclassing in django, despite the apparent similarity. I am unable to run the tests against 1.7, presumably because of the app loading issues in 1.7 that @matthiask has mentioned.

@matthiask That check only checks for models defined in the app cache, not for classes imported into the scope.

I believe that the problem occurs because type defines a new class in __module__ that has the same name as a variable that is already in that scope, and the old variable gets overridden.

Contributor

meshy commented Jun 3, 2014

@frog32 I do not believe type is being misused, nor that this has anything to do with the metaclassing in django, despite the apparent similarity. I am unable to run the tests against 1.7, presumably because of the app loading issues in 1.7 that @matthiask has mentioned.

@matthiask That check only checks for models defined in the app cache, not for classes imported into the scope.

I believe that the problem occurs because type defines a new class in __module__ that has the same name as a variable that is already in that scope, and the old variable gets overridden.

@meshy

This comment has been minimized.

Show comment
Hide comment
@meshy

meshy Jun 3, 2014

Contributor

...Sorry, I hit ENTER too early, then. I wanted to add that passing class_name does indeed work around the issue, but does not solve the underlying problem.

Contributor

meshy commented Jun 3, 2014

...Sorry, I hit ENTER too early, then. I wanted to add that passing class_name does indeed work around the issue, but does not solve the underlying problem.

@mjl

This comment has been minimized.

Show comment
Hide comment
@mjl

mjl Jun 3, 2014

Contributor

...Sorry, I hit ENTER too early, then. I wanted to add that passing class_name does indeed work around the issue, but does not solve the underlying problem.

Can we just make the generated class name more unique or do we need to refer to it by name later on?

mjl
Contributor

mjl commented Jun 3, 2014

...Sorry, I hit ENTER too early, then. I wanted to add that passing class_name does indeed work around the issue, but does not solve the underlying problem.

Can we just make the generated class name more unique or do we need to refer to it by name later on?

mjl
@frog32

This comment has been minimized.

Show comment
Hide comment
@frog32

frog32 Jun 3, 2014

Contributor

I bet by commenting out the mentioned lines in the __metaclass__ of models.Model you can also resolve this issue. This is of course not a valid solution but django <1.7 does not allow twice the same model name in the same application because it uses this "magic" to make shure that the same model imported using two different import paths (e.g. from mypacket.models import X and mypacket.models.x import X when you splitt up your models file into multiple files, is not resulting in two different classes.

@mjl: giving them a unique classname would of course solve the problem because django uses the app_label and name for it's metaclass magic.

Contributor

frog32 commented Jun 3, 2014

I bet by commenting out the mentioned lines in the __metaclass__ of models.Model you can also resolve this issue. This is of course not a valid solution but django <1.7 does not allow twice the same model name in the same application because it uses this "magic" to make shure that the same model imported using two different import paths (e.g. from mypacket.models import X and mypacket.models.x import X when you splitt up your models file into multiple files, is not resulting in two different classes.

@mjl: giving them a unique classname would of course solve the problem because django uses the app_label and name for it's metaclass magic.

@matthiask

This comment has been minimized.

Show comment
Hide comment
@matthiask

matthiask Jun 3, 2014

Member

I'm a bit hesitant to change the class name because the class name defines database table names, related names, permission names etc. This would probably make it hard to upgrade FeinCMS.

Maybe we could check the module whether a class / variable with the name exists and raising ImproperlyConfigured if that's the case (and also point users towards the class_name argument).

Member

matthiask commented Jun 3, 2014

I'm a bit hesitant to change the class name because the class name defines database table names, related names, permission names etc. This would probably make it hard to upgrade FeinCMS.

Maybe we could check the module whether a class / variable with the name exists and raising ImproperlyConfigured if that's the case (and also point users towards the class_name argument).

@mjl

This comment has been minimized.

Show comment
Hide comment
@mjl

mjl Jun 3, 2014

Contributor

I'm a bit hesitant to change the class name because the class name defines database table names, related names, permission names etc. This would probably make it hard to upgrade FeinCMS.

Agreed, it might break stuff out there.

Maybe we could check the module whether a class / variable with the name exists and raising ImproperlyConfigured if that's the case (and also point users towards the class_name argument).

That's a good first step, yes, so people can continue and not get held up by "you must know you need to use class_name", which nobody actually knows. I like explicit and helpful error messages a lot.

Just food for thought: Do we actually need to store the class where we put it? Could we do some name swizzling after creation?

Contributor

mjl commented Jun 3, 2014

I'm a bit hesitant to change the class name because the class name defines database table names, related names, permission names etc. This would probably make it hard to upgrade FeinCMS.

Agreed, it might break stuff out there.

Maybe we could check the module whether a class / variable with the name exists and raising ImproperlyConfigured if that's the case (and also point users towards the class_name argument).

That's a good first step, yes, so people can continue and not get held up by "you must know you need to use class_name", which nobody actually knows. I like explicit and helpful error messages a lot.

Just food for thought: Do we actually need to store the class where we put it? Could we do some name swizzling after creation?

@meshy

This comment has been minimized.

Show comment
Hide comment
@meshy

meshy Jun 3, 2014

Contributor

Perhaps, rather than allowing the class_name to be set, it would make more sense to allow the table/related/permission names to be set without creating the class in that module?

Contributor

meshy commented Jun 3, 2014

Perhaps, rather than allowing the class_name to be set, it would make more sense to allow the table/related/permission names to be set without creating the class in that module?

@frog32

This comment has been minimized.

Show comment
Hide comment
@frog32

frog32 Jun 3, 2014

Contributor

I was digging deeper into this and i realized that the id() of CustomContentType changes after create_content_type returns. Then i checked the id() of the model attribute during the create_content_type call and it's the same address as CustomContentType has before the call.

I went on and tried to find out which part of create_content_type has an influence on the address. When i remove this line the assertion passes https://github.com/feincms/feincms/blob/master/feincms/models.py#L706

Any ideas what the installation of the newly created content type could do to a CustomContentType in the models.py?

Contributor

frog32 commented Jun 3, 2014

I was digging deeper into this and i realized that the id() of CustomContentType changes after create_content_type returns. Then i checked the id() of the model attribute during the create_content_type call and it's the same address as CustomContentType has before the call.

I went on and tried to find out which part of create_content_type has an influence on the address. When i remove this line the assertion passes https://github.com/feincms/feincms/blob/master/feincms/models.py#L706

Any ideas what the installation of the newly created content type could do to a CustomContentType in the models.py?

@meshy

This comment has been minimized.

Show comment
Hide comment
@meshy

meshy Jun 3, 2014

Contributor

Looks like you're absolutely right! It wasn't the call to type after all!

Contributor

meshy commented Jun 3, 2014

Looks like you're absolutely right! It wasn't the call to type after all!

@matthiask

This comment has been minimized.

Show comment
Hide comment
@matthiask

matthiask Jun 3, 2014

Member

We could probably remove this line without consequences. The test suite still runs through, and the documented way to access the concrete content type is Base.content_type_for, not importing it from some location.

https://feincms-django-cms.readthedocs.org/en/latest/contenttypes.html#obtaining-a-concrete-content-type-model

Member

matthiask commented Jun 3, 2014

We could probably remove this line without consequences. The test suite still runs through, and the documented way to access the concrete content type is Base.content_type_for, not importing it from some location.

https://feincms-django-cms.readthedocs.org/en/latest/contenttypes.html#obtaining-a-concrete-content-type-model

@mjl

This comment has been minimized.

Show comment
Hide comment
@mjl

mjl Jun 3, 2014

Contributor

We could probably remove this line without consequences. The test suite still runs through, and the documented way to access the concrete content type is Base.content_type_for, not importing it from some location.

I remember adding this. The commit message says

When creating a content type, also install it in the module.
This helps django-admin-tools which directly looks up model paths and
crashes if it can't find some model (arguably a bug).

I can try to reproduce that problem (by removing that line and see what django-admin-tools does nowadays).

Contributor

mjl commented Jun 3, 2014

We could probably remove this line without consequences. The test suite still runs through, and the documented way to access the concrete content type is Base.content_type_for, not importing it from some location.

I remember adding this. The commit message says

When creating a content type, also install it in the module.
This helps django-admin-tools which directly looks up model paths and
crashes if it can't find some model (arguably a bug).

I can try to reproduce that problem (by removing that line and see what django-admin-tools does nowadays).

@matthiask

This comment has been minimized.

Show comment
Hide comment
@matthiask

matthiask Jun 3, 2014

Member

Ah thanks, I didn't remember.

Member

matthiask commented Jun 3, 2014

Ah thanks, I didn't remember.

@matthiask

This comment has been minimized.

Show comment
Hide comment
@matthiask

matthiask Jun 12, 2014

Member

@mjl Any news regarding django-admin-tools? Do you have any objections to removing the setattr line?

Member

matthiask commented Jun 12, 2014

@mjl Any news regarding django-admin-tools? Do you have any objections to removing the setattr line?

@matthiask matthiask closed this Jun 15, 2014

@matthiask matthiask reopened this Jun 15, 2014

@mjl

This comment has been minimized.

Show comment
Hide comment
@mjl

mjl Jun 30, 2014

Contributor

I just commented out the line mentioned above and a project with admin tools still works, so I guess it has been corrected there. No objections from my side.

Contributor

mjl commented Jun 30, 2014

I just commented out the line mentioned above and a project with admin tools still works, so I guess it has been corrected there. No objections from my side.

@meshy

This comment has been minimized.

Show comment
Hide comment
@meshy

meshy Jun 30, 2014

Contributor

Thanks for getting back, @mjl. I've now added the removal of those lines to this PR.

Contributor

meshy commented Jun 30, 2014

Thanks for getting back, @mjl. I've now added the removal of those lines to this PR.

@matthiask

This comment has been minimized.

Show comment
Hide comment
@matthiask

matthiask Jul 8, 2014

Member

Merged to next. Thanks!

(FeinCMS 1.10 is not far away.)

Member

matthiask commented Jul 8, 2014

Merged to next. Thanks!

(FeinCMS 1.10 is not far away.)

@matthiask matthiask closed this Jul 8, 2014

@meshy

This comment has been minimized.

Show comment
Hide comment
@meshy

meshy Jul 8, 2014

Contributor

\o/

Contributor

meshy commented Jul 8, 2014

\o/

@meshy meshy deleted the incuna:blitzing-existing branch Jul 8, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment