Proposal for refactoring the way models are handled in Django-Shop.

[jrief]

Currently there are concrete and abstract models: BaseProductand Product, BaseCartand Cart, BaseCartItem and CartItem, BaseOrderand Order and finally, BaseOrderItem and OrderItem.

The concrete models are stored in shop.models, whereas abstract models are stored in shop.models_bases. This is quite confusing and makes it difficult to find the right model definition whenever one has to access the definition of one of the models. Additionally, if someone wants to override a model, he must use a configuration directive, say PRODUCT_MODEL, ORDER_MODEL, ORDER_MODEL_ITEM from the projects settings.py. This makes configuration quite complicate and causes other drawbacks:

• Unless someone overrides all models, the native ones appear in the administration backend below the category Show, while the customized ones appear under the given application name. To a customer, this backend inconsistency is quite difficult to explain.
• In the past, having overriden and native models caused some issues with circular dependencies.

Therefore my proposal is to remove all the configuration settings PRODUCT_MODEL, ORDER_MODEL, ORDER_MODEL_ITEM, etc. from django-shop. Additionally all concrete models, Product, Order, OrderItem, Cart, CartItem are removed from django-shop. These model definitions are just stubs, so removing them should not cause any harm. All abstract models are moved into the directory shop/models/; this is where a programmer expects them.

For an application using django-shop this should not be a big deal. Each application simply derives each model from one of the abstract base models and thus creates its concrete model. This also avoids another problem, one might easily encounter: Say, one starts with django-shop and uses the built-in Cart model. In the database this is handled by a table named shop_cart. Sometimes later, he decides to add an extra field to the model Cart. He then has to create a customized cart model in his own application, but now the database table will have a different name and it is not possible to override this using app_name in the model's Meta class. These kinds of migration are not handled by South and therefore he has to write a migration for renaming the table by hand.

Mapping of Foreign Keys

One might argue, that this can't work, since foreign keys must refer to a real model, not to abstract ones! Therefore one can not add a field ForeignKey, OneToOneField or ManyToManyField which refers an abstract model in the Django-Shop project. However, these relations are fundamental for a properly working software. Imagine a CartItem without a foreign relation to Cart.

Fortunately there is a neat trick to solve this problem. By deferring the mapping onto a real model, instead of using a real ForeignKey, one can use a special placeholder field defining a relation with an abstract model. Now, whenever the models are “materialized”, then these abstract relations are converted into real foreign keys. The only drawback for this solution is, that one may derive from an abstract model only once, but that's a non-issue and doesn't differ from the current situation, where one for instance, also can override Cart only once.

I therefore would strongly encourage to refactor Django-Shop, since this will hugely simplify the current model system.

Technical Details

Instead of using models.ForeignKey, models.OneToOneField or models.ManyToManyField, I use special pseudo-fields: DeferredForeignKey, DeferredOneToOneField and DeferredManyToManyField. During model instantiation, these pseudo-fields are replaced against real foreign keys.

Have a look at this meta class: https://github.com/jrief/django-shop/blob/refactor-models/shop/deferred.py

Now an abstract model can be defined as: https://github.com/jrief/django-shop/blob/refactor-models/shop/models/abstract.py

and the materialized model then is just as simple as:

from shop.models import abstract

class RealModel1(abstract.AbstractModel1):
    pass

class RealModel2(abstract.AbstractModel2):
    pass

class RealModel3(abstract.AbstractModel3):
    pass

Gone are the days with dozens of configurations settings to hard wire the real models in Django-Shop.

[chrisglass]

This is brilliant!

You raise points I always had issues with from a conceptual point of view, but never could find a suitable solution for (well, except the one currently implemented, which does have some problems, as you pointed out).

I fully agree this is the way to go - it's actually the way I would have liked to write the code in the first place, but couldn't.

This needs quite a lot of refactoring however. I can certainly contribute some help (hey, I want to participate doing this so my brain child can evolve into a better looking "teenager" project!), but I'm certain it's way beyond my current availability if I were to do this on my own. I would furthermore probably make this a "django-shop-2" project/package, since it will likely not be backwards-compatible.

What do you think? Can you help?

[chrisglass]

Feel free to ping me on #django-shop on freenode to discuss this further, too.

[mkoistinen]

I agree. I think this is the way forward, but I was worried about migration paths for existing installations. I think a hard fork, or at least a new major version number, is the way to get there.

[jrief]

I now have a fully working version of Django-SHOP which uses the proposed deferred model pattern. All models from django shop must be materialized in your app. There is no more implicit model creation in Django-SHOP. From now on in Django-SHOP all models will be abstract.

Since this is a rather radical code refactoring, this for any further development means, that from now on one must decide whether to go with the old way of specifying models or using my deferred model pattern.

Concretely speaking, this change will fork the code base. It now is my default branch: https://github.com/jrief/django-shop

[ghost]

I gave this a go, and sadly it is not a 'fully working version of Django-SHOP which uses the proposed deferred model pattern' for me.

I'm a bit of a simpleton, so I'm probably missing something obvious.

I've added in the stubs in shopmodels.py, but that is insufficient to get the SHOP going.

Following the docs as close as possible ends up with "cannot import name Cart" and this traceback.

1. from shop.payment.api import PaymentAPI File "/home/django-cms/shop-testing/s-env/local/lib/python2.7/site-packages/shop/payment/api.py" in
2. from shop.models import Cart

Exception Type: ImportError at /

Exception Value: cannot import name Cart

Fixing that Cart import then gives many Order import problems.

(s-env)django-cms@dmz:~/shop-testing/s-env/local/lib/python2.7/site-packages/shop$ grep -r import . | grep ' Order' | grep -v '/tests' ./shop_api.py:from shop.models.ordermodel import OrderExtraInfo, Order ./admin/order.py:from shop.models.order import BaseOrder, BaseOrderItem, OrderExtraInfo, ExtraOrderPriceField, OrderPayment ./util/order.py:from shop.models.ordermodel import Order ./util/decorators.py:from shop.models.ordermodel import Order ./urls/order.py:from shop.views.order import OrderListView, OrderDetailView ./shipping/api.py:from shop.models.ordermodel import Order ./views/checkout.py:from shop.models import AddressModel, OrderExtraInfo ./views/checkout.py:from shop.models import Order ./views/order.py:from shop.models import Order ./payment/backends/prepayment.py:from shop.models.ordermodel import Order, OrderPayment ./payment/api.py:from shop.models.ordermodel import OrderPayment ./payment/api.py:from shop.models.ordermodel import Order

With the deferred model pattern how should one import Order?

Thanks .

[jrief]

Did you try my fork of djangoSHOP at https://github.com/jrief/django-shop I agree that I should add this as separate branch to the official repo and add some docs.

# -*- coding: utf-8 -*-
from django.utils.encoding import force_text
from django.utils.translation import ugettext_lazy as _
from shop.models.deferred import ForeignKeyBuilder
ForeignKeyBuilder.app_label = 'my_shop'
from shop.models import cart, order, product

class Cart(cart.BaseCart):
    pass

class CartItem(cart.BaseCartItem):
    pass

class Order(order.BaseOrder):
    pass

class OrderItem(order.BaseOrderItem):
    pass

class Product(product.BaseProduct):
    class Meta(product.BaseProduct.Meta):
        app_label = 'my_shop'

Im my_shop/models/shopmodels.py this is my current model defer code. The real product is derived from Product and adds all specific fields.

[ghost]

Yes, I'm studying your version of Django-SHOP.

I see the shopmodels.py you've just posted above is a little different from that in the samples directory.

I suppose that shop/models/__init__.py needs some adjustments to expose Cart, Order and Product ??? Or is that s'posed to happen automagically?

Anyway, it just seemed more future-proof to follow your branch.

Thanks again .

[jrief]

You have to include

# -*- coding: utf-8 -*-
from . import shopmodels

into your my_shop/models/__init__.py

To Order, OrderItem, Cart and CartItem add whatever field is required for your own shop. In Product add only fields which are required by all of your products, otherwise put them into model derived from it.

[ghost]

Yes, I have done that.

That is not enough for me?

For example, for me ./views/order.py:from shop.models import Order raises an Exception Value: cannot import name Order

I must be missing something fundamental. I'm quite new at this.

[ghost]

I've changed shop/models/__init__.py to

from shop.order_signals import *

from myshop.models.shopmodels import Cart, CartItem, Order, OrderItem, Product

And now its going better ... seems I just now need to adjust the shop.model.ordermodel imports

  3. from shop.payment.api import PaymentAPI
File "/home/django-cms/shop-testing/s-env/local/lib/python2.7/site-packages/shop/payment/api.py" in <module>
  9. from shop.models.ordermodel import OrderPayment

Exception Type: ImportError at /
Exception Value: No module named ordermodel

[jrief]

This "defer" pattern is in proof of concept stage. I'am currently building a real shop around it. So, at the moment please do not expect that this django app runs smoothly and out of the box. As soon as everything works fine, I'll create a develop branch in Divio's repo.

[ghost]

OK. I've fixed up the legacy imports and got it going.

Is there a cleaner idiomatic way to achieve this effect?

from myshop.models.shopmodels import Cart, CartItem, Order, OrderItem, Product

Not liking that out-of-scope(ish) import.

[jhagege]

I'm trying to understand the difference between django-polymorphic and DeferredForeignKey? It was proposed to refactor the code using DeferredForeignKey, DeferredOneToOneField etc..., but from what I see this is currently using django-polymorphic. It seems conceptually similar to me, and I'd be happy to understand it a bit better...

Also, is it ok performance-wise to rely on Django-Polymorphic? Is it stable enough to be production ready?

Thanks.

[jrief]

Currently, https://github.com/jrief/django-shop is the release I'm working on.

Just for clarity, DeferredForeignKey and django-polymorphic are completely different. A polymorphic product has one table for its base product and additional tables for the implemented product specialization.

On the other side, the deferred pattern does nothing else, than postponing the foreign key mapping until the materialized model is known. This avoids some nasty issues, we had with circular dependencies and it allows to move the whole model tree from django-shop into your shop implementation.

[jhagege]

Thanks for your fast answer. From my understanding, the Deferred model doesn't allow the same polymorphic behavior, or does it?

class Comment(models.Model):
    commenter = models.ForeignKey('auth.User')
    content = DeferredForeignKey()
    body = models.TextField()
class BlogPost(models.Model):
    title = models.CharField(max_length=100)
    slug = models.SlugField()
    body = models.TextField()
point(Comment, 'content', BlogPost)

It looks like from now on, all the content fields inside Comment will be of type BlogPost. i.e: We cannot assign dynamically for each instance of Comment. It's either they are all of type BlogPost, or all of type Image (for example)

Thanks a lot for your help.

[jrief]

From my understanding, the Deferred model doesn't allow the same polymorphic behavior, or does it?

no, not at all

The deferred patterns only make sense for abstract models. DeferredForeignKey requires a foreign model. A class using deferred keys must inherit like this

class Comment(with_metaclass(deferred.ForeignKeyBuilder, models.Model)):
    content = deferred.ForeignKey(BlogPost)
    otherfield = ...

    class Meta:
        abstract = True

[jhagege]

Thanks a lot for your help, it's clearer now.

[febsn]

Disclaimer: I've just started to use django-shop and thus I'm not quite familiar with the code yet. I've just run into an issue trying to introduce django migrations using django-shop with django 1.7 and a custom Address model (unsurprisingly, Django creates a migration in the shop app itself for whatever Address model is currently being used; I created the migration with the default Address model in place and then tried to swap in my own, which threw a reverse accessor clash for the User model). I started digging and found that most of the django-shop models can be swapped by setting SHOP_ADDRESS_MODEL (using Address as an example). This already sounds a bit like django swappable models, which atm is used for the Auth model, but intentionally undocumented (here's why: https://code.djangoproject.com/ticket/19103; see also http://stackoverflow.com/questions/22025476/what-is-swappable-in-model-meta-for, and https://github.com/wq/django-swappable-models for a light wrapper where also the concept is explained). If the latter is to be trusted, swappable models are already quite safe to use and I assume they'll be documented some time soon. Honestly I haven't tried, but I don't know how @jrief's solution would play with (django) migrations, which I consider important for people who want to use the default models. Also, although I bet it's a brilliant solution, I'd rather use django-builtins than reinventing the wheel with deferred fields. As mentioned, I haven't gone into detail yet, I just have the feeling that we are moving towards a dead end (or at least a non-djangoish end). What are your thoughts on it? I'm not a code guru, but I'd be glad to help refactoring django-shop to using swappable models and django migrations (I'm about to start some experiments on that).

[jrief]

@febsn Thanks for the hint. Last year I was not aware of it. Anyway, the deferred models work quite well with migrations. The only caveat it that migrations then have to be done inside the application, but since these are models defined by the customer of that library, I can live with it.

I need a deeper look at swappable models. Currently my version of django-shop is almost ready for being released.

[febsn]

@jrief no wonder you weren't, it's still a bit of a mystery. Well of course migrations can be implemented at a per-project basis, but that makes it impossible to release more complicated migrations along with the code… and as a django-shop is also suitable for smaller use cases, default out-of-the-box usable models would be an advantage, I guess. I only realized you are member of the core team after I had written my thoughts above. Are you planning to bring your refactored version into trunk, or what exactly is the long-term plan? If there's a way for me to contribute to this issue without abandoning all your good work, let me know.

[jrief]

That would be great! Can we schedule a Google Hangout, then I can present some features. What is missing? Mainly docs and tests.

[jrief]

Closed. Please check version 0.9.