[patch] Tighten subclass check at top of db.models.base.ModelBase.__new__.

[phil.h.smith@…]

The "not bases or bases == (object,)" check at the top of ModelBase.new assumes that if the class-to-be has any bases, that one of them will be Model.

It cropped up while writing a mixin superclass for models that provided some new service-ish functionality for my app. The mixin requires a metaclass of its own. To satisfy guido's metaclass resolution rules, my mixin's metaclass extends ModelBase. The catch: when evaluating my mixin definition, ModelBase is invoked. My mixin has a superclass, so it slips past the check, and things explode.

My own metaclass can opt to check the Model subclass condition itself, and not call super(..).new, but that is hackish. Please consider the attached patch instead.

Thanks for listening!

[phil.h.smith@…]

The need that led me to file the issue will be addressed if/when model inheritance is finished. Particularly, if it supports multiple abstract base classes for mixing in columns.

[Malcolm Tredinnick]

This is a bug in our implementation of meta-classes and respecting sub-classing. Regardless of particular use-cases we know about, we should fix it. I'll take care of it.

[Malcolm Tredinnick]

(In [4607]) Fixed #2363 -- Fixed subclass checking in ModelBase to allow for mixin
superclasses. Thanks phil.h.smith@….

[Chris.Wesseling@…]

Checking on name == "Model" is even 'hackisher' IMHO.

I have a Model named Model.. This check doesn't take the difference between django.db.models.Model and project.app.models.Model in account and django.db.models.ForeignKey(Model) breaks.

from django.db import models
class Measures(models.Model):
    top = models.IntegerField()
    waist = models.IntegerField()
    hip = models.IntegerField()
    def is_major_turnon():
       import random
       return random.choice((True, False)) #Mind over matter ;)

class Model(models.Model):
    name = models.CharField(maxlength=25)
    measures = models.ForeignKey(Measures)

class AmericasTopModel(models.Model):
    season = models.IntegerField()
    model = models.ForeingKey(Model)
    def program_sucks():
       return True

This is not my real app, just some witty example. My real app records devices and their makes and models.

[phil.h.smith@…]

That name check is there to prevent a chicken-and-egg problem when the definition of (django's) Model itself triggers metaclass evaluation. I don't like it either, but there needs to be a guard against the actual reference later in the line.

A better way may be to just try resolving the reference to Model in a try. If the metaclass truly is evaluating django's Model class, a NameError will be raise, and we can know to ignore this type. (I'll put up a patch when I get home...)

[Chris.Wesseling@…]

Even tighter check. With test.

[Chris.Wesseling@…]

Replying to phil.h.smith@gmail.com:

I've uploaded a slight mod of your patch with some added testcode harnassing my situation. I don't know if "invalid_models" is the right place for such a test, but I saw some other valid models in the file.

[phil.h.smith@…]

I'm kinda leery of adding the module check: if the name check is hackish, wouldn't it be more of the same? I concede that the only cases that would defeat it are pathological.

I'm sorry about not getting a patch up yesterday; (a tapas bar viciously leaped on me, I barely escaped with my life.) I've got an (untested) one here just to show what I meant a few comments ago. (I can't run the new test (or the others) atm, but maybe Chris would give it a crack?)

[phil.h.smith@…]

now with less name checks

[Chris.Wesseling@…]

Merge of phil.h.smith@… cleaner code and my test.

[Chris.Wesseling@…]

Replying to phil.h.smith@gmail.com:

I'm kinda leery of adding the module check: if the name check is hackish, wouldn't it be more of the same? I concede that the only cases that would defeat it are pathological.

I agree, it's just what the first patch meant to say: "Is this Model which is defined elsewhere in this module?"
It's arguably more explicit then catching the NameError, but the comment solves that.

I'm sorry about not getting a patch up yesterday; (a tapas bar viciously leaped on me, I barely escaped with my life.) I've got an (untested) one here just to show what I meant a few comments ago. (I can't run the new test (or the others) atm, but maybe Chris would give it a crack?)

Tapas bars can be very verocious. I have one living around the corner, which makes my residence in it's hunting grounds. ;)

The patch tested out fine. I've merged the code with my testcode. Maybe you can add some of your mix-in code to the test too?
I've never written a mix-in in Python, but could this be a start for a test?

class PurrMixin():
    """Working Cars"""
    def make_noise(self):
        return "VROOOM"

class RacketMixin():
    """Car Wrecks"""
    def make_noise(self):
        return "RRRRRR crack BANG RRRR"

class HummingMixin():
    """Fancy Hybrid Cars"""
    running_motor = None
    def make_noise(self):
        if self.running_motor == 'Combustion':
            return "VROOOM"
        else:
            return "mmmmmm"

[Malcolm Tredinnick]

In principle, this looks like the right solution. Thanks, guys. :-)

A shame that Python doesn't let use access the thing raising the name error more easily for tighter checking (by including it as an attribute in the NameError class), but I don't want to start matching against the string version of the exception, so we'll leave that.

I'll check this in when I get a chance.

[Malcolm Tredinnick]

(In [4881]) Fixed #2363 -- Improved base class checking in ModelBase metaclass. Thanks to
combined work from phil.h.smith@… and Chris.Wesseling@….