Code

Opened 8 years ago

Closed 7 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: mtredinnick
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 8 years ago.
tighter_subclasscheck.patch (1.6 KB) - added by Chris.Wesseling@… 7 years ago.
Even tighter check. With test.
base.py.2.patch (961 bytes) - added by phil.h.smith@… 7 years ago.
now with less name checks
tighter_subclasscheck.2.patch (1.8 KB) - added by Chris.Wesseling@… 7 years ago.
Merge of phil.h.smith@… cleaner code and my test.

Download all attachments as: .zip

Change History (15)

Changed 8 years ago by anonymous

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

  • Triage Stage changed from Unreviewed to Design decision needed

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

  • Owner changed from adrian to mtredinnick
  • Triage Stage changed from Design decision needed to Ready 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 7 years ago by mtredinnick

  • Resolution set to fixed
  • Status changed from new to closed

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

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

  • Patch needs improvement set
  • Resolution fixed deleted
  • Status changed from closed to reopened

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 follow-up: Changed 7 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 7 years ago by Chris.Wesseling@…

Even tighter check. With test.

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

  • Patch needs improvement unset
  • Triage Stage changed from Ready for checkin to Unreviewed

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 follow-up: Changed 7 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 7 years ago by phil.h.smith@…

now with less name checks

Changed 7 years ago by Chris.Wesseling@…

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

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

  • Triage Stage changed from Unreviewed to Ready 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 7 years ago by mtredinnick

  • Resolution set to fixed
  • Status changed from reopened to closed

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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.