Opened 10 years ago

Closed 10 years ago

#2363 closed enhancement (fixed)

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

Reported by: phil.h.smith@… Owned by: Malcolm Tredinnick
Component: Core (Other) Version:
Severity: normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

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!

Attachments (4)

base.py.patch (651 bytes) - added by anonymous 10 years ago.
tighter_subclasscheck.patch (1.6 KB) - added by Chris.Wesseling@… 10 years ago.
Even tighter check. With test.
base.py.2.patch (961 bytes) - added by phil.h.smith@… 10 years ago.
now with less name checks
tighter_subclasscheck.2.patch (1.8 KB) - added by Chris.Wesseling@… 10 years ago.
Merge of phil.h.smith@… cleaner code and my test.

Download all attachments as: .zip

Change History (15)

Changed 10 years ago by anonymous

Attachment: base.py.patch added

comment:1 Changed 10 years ago by Simon G. <dev@…>

Triage Stage: UnreviewedDesign decision needed

comment:2 Changed 10 years ago by 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.

comment:3 Changed 10 years ago by Malcolm Tredinnick

Owner: changed from Adrian Holovaty to Malcolm Tredinnick
Triage Stage: Design decision neededReady for checkin

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.

comment:4 Changed 10 years ago by Malcolm Tredinnick

Resolution: fixed
Status: newclosed

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

comment:5 Changed 10 years ago by Chris.Wesseling@…

Patch needs improvement: set
Resolution: fixed
Status: closedreopened

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.

comment:6 Changed 10 years ago by 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...)

Changed 10 years ago by Chris.Wesseling@…

Attachment: tighter_subclasscheck.patch added

Even tighter check. With test.

comment:7 in reply to:  6 Changed 10 years ago by Chris.Wesseling@…

Patch needs improvement: unset
Triage Stage: Ready for checkinUnreviewed

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.

comment:8 Changed 10 years ago by 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?)

Changed 10 years ago by phil.h.smith@…

Attachment: base.py.2.patch added

now with less name checks

Changed 10 years ago by Chris.Wesseling@…

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

comment:9 in reply to:  8 Changed 10 years ago by 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"

comment:10 Changed 10 years ago by Malcolm Tredinnick

Triage Stage: UnreviewedReady for checkin

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.

comment:11 Changed 10 years ago by Malcolm Tredinnick

Resolution: fixed
Status: reopenedclosed

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

Note: See TracTickets for help on using tickets.
Back to Top