#122 closed enhancement (fixed)
[patch] Build models using fieldname=FieldClass
Reported by: | anonymous | Owned by: | Adrian Holovaty |
---|---|---|---|
Component: | Core (Other) | Version: | |
Severity: | normal | Keywords: | |
Cc: | mmarshall@… | Triage Stage: | Unreviewed |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
This is a patch to make fieldname=FieldClass type model definitions possible. It is fully backwards-compatible with the fields=(...) method.
The example given in the tutorial could be re-written as such:
from django.core import meta class Poll(meta.Model): question = meta.CharField(maxlength=200) pub_date = meta.DateTimeField('date published') class Choice(meta.Model): ForeignKey = Poll choice = meta.CharField(maxlength=200) votes = meta.IntegerField()
Other ways of defining a ForeignKey:
#multiple keys can be used with tuples: ForeignKey = Poll, OtherKey #options can also be used: ForeignKey = Poll, {'edit_inline':True, 'num_in_admin':3} #the attribute name is irrelevant here: anything = ForeignKey(Poll, edit_inline=True, num_in_admin=3)
The implementation is quite simple. (or horribly hackish, depending on how you see things.) It simply goes through all of the attributes of the class, and adds all of the instances of 'Field' to the fields list. (after updating their names with the attribute name.) It also processes the ForeignKey field, producing ForeignKeys as needed.
Provisions are in place to maintain the order of the Fields.
Here is the patch:
Index: django/core/meta.py =================================================================== --- django/core/meta.py (revision 253) +++ django/core/meta.py (working copy) @@ -368,6 +368,40 @@ if not bases: return type.__new__(cls, name, bases, attrs) + # We must refactor the attributes around a little. All Field class instances will be given + # names (as needed) and moved to the fields list. + + attrs["fields"] = attrs.has_key("fields") and list(attrs["fields"]) or [] + + def handle_ForeignKey(obj): + if isinstance(obj, Model): + attrs["fields"].append(ForeignKey(obj)) + elif type(obj) in (list, tuple): + if isinstance(obj[0], ModelBase) and type(obj[1]) == dict: + attrs["fields"].append(ForeignKey(obj[0], **obj[1])) + else: + for i in obj: handle_ForeignKey(i) + else: + raise ValueError("Cannot make ForeignKey from "+str(obj)) + + def handle_field(obj, name): + if isinstance(obj, Field): #Check if this is a field + if not isinstance(obj,ForeignKey): obj.set_name(name) + attrs["fields"].append(obj) + return True + elif name == "ForeignKey": + handle_ForeignKey(obj) + return True + return False + + # loop through the attributes, and take out the fields. + for key in attrs.keys()[:]: + if handle_field(attrs[key], key): + del attrs[key] # If the attr was added to fields, we want to delete it. + + # Sort the fields, so that they appear in the correct order. + attrs["fields"].sort(lambda x,y: x.creation_counter - y.creation_counter) + # If this model is a subclass of another Model, create an Options # object by first copying the base class's _meta and then updating it # with the overrides from this class. @@ -1551,14 +1585,19 @@ # database level. empty_strings_allowed = True - def __init__(self, name, verbose_name=None, primary_key=False, + # Will be increaced each time a Field object is instanced. Used to + # retain the order of fields. + creation_counter = 0 + + def __init__(self, name=None, verbose_name=None, primary_key=False, maxlength=None, unique=False, blank=False, null=False, db_index=None, core=False, rel=None, default=NOT_PROVIDED, editable=True, prepopulate_from=None, unique_for_date=None, unique_for_month=None, unique_for_year=None, validator_list=None, choices=None, radio_admin=None, help_text=''): self.name = name - self.verbose_name = verbose_name or name.replace('_', ' ') + self.original_verbose_name = verbose_name + self.verbose_name = verbose_name or name and name.replace('_', ' ') self.primary_key = primary_key self.maxlength, self.unique = maxlength, unique self.blank, self.null = blank, null @@ -1583,6 +1622,24 @@ else: self.db_index = db_index + # Increace the creation counter, and save our local copy. + self.creation_counter = Field.creation_counter + Field.creation_counter += 1 + + def set_name(self, name ): + """ + Sets the name, and generates the verbose_name from it if needed. + This function is here for when the name needs to be set later, + (such as if it needs to be obtained from the attribute name that + stores the Field instance.) + """ + if not self.original_verbose_name: + # If the verbose name was originally not specified, we will assume that + # the user intends for the first argument passed to __init__ to be the verbose_name. + self.verbose_name = self.name + + self.name = name + self.verbose_name = self.verbose_name or name and name.replace('_', ' ') + def pre_save(self, obj, value, add): """ Hook for altering the object obj based on the value of this field and
Attachments (9)
Change History (53)
comment:1 by , 19 years ago
comment:2 by , 19 years ago
Oh, there is something I forgot to mention. Notice how in the examples I included "module_name" in the examples? It seems that the automatic name generation gets messed up if you put a class inside of your model class. (I think this also happens if you use method names with upper case.)
I also just realized that I didn't do anything for ManyToManyField. Although you can simply do fields=(meta.ManyToManyField(...),), it probably should be implemented the same as ForeignKey.
comment:3 by , 19 years ago
The following case should be taken into consideration. This code could not work because there is a mutual relationship where both tables have a FK, relating to each other.
sqlobject does this by passing table names as a string, not an object.
from django.core import meta class Person(meta.Model): name = meta.CharField(maxlength=200) place_of_residence = ForeignKey(Address) # Error Occurs Here class Address(meta.Model): owner = ForeignKey(Person) suburb = meta.CharField(maxlength=200) postcode = meta.IntegerField()
comment:4 by , 19 years ago
hmm... Wouldn't it be better to do this?
from django.core import meta class Person(meta.Model): name = meta.CharField(maxlength=200) class Address(meta.Model): owner = meta.OneToOneField(Person) suburb = meta.CharField(maxlength=200) postcode = meta.IntegerField()
BTW, looking at this line:
owner = ForeignKey(Person)
I guess it would be best for the attribute name (owner in this case) to be used as the rel_name for ForeignKey? (Right now in this patch it is simply discarded)
by , 19 years ago
Attachment: | meta3.diff added |
---|
A third version of the patch. Better support for ForeignKey, ManyToManyField, and OneToOneField.
comment:5 by , 19 years ago
Ok, I just attached a new version of the patch.
Something like this:
owner = ForeignKey(Person)
... now works the same as this:
fields = ( ForeignKey(Person, rel_name="owner"), )
ManyToManyField and OneToOneField should work in this way as well. (I haven't tested...)
comment:6 by , 19 years ago
mmarshall:
nope, A person can own many properties, and might live in a property he doesn't own.
Also, throwing away the name of the relationship causes problems, as the following kind of situation may occur:
from django.core import meta class Person(meta.Model): name = meta.CharField(maxlength=200) class CourtCase(meta.Model): plaintiff = meta.ForeignKey(Person) defendant = meta.ForeignKey(Person)
- the name of the relationship is very important.
- it should be possible to have foreignkeys refer to tables via their name, not by passing the object, to avoid the chicken/egg problem detailed in my previous code example.
A tried and tested python ORM API is sqlobject, which handles both these cases, maybe we should take some lessons from there:
comment:7 by , 19 years ago
Oh, just reread comments. The one previous to my last one talks about a patch that doesn't throw away the relationship name. I didn't realise that.
Sorry. The only remaining issue I see is the mutual FK relationship. We need to be able to have a FK in a class that doesn't yet exist.
This will also cause problems when automatically generating the tables in the database. You have to define the tables first, then add the FKs, to avoid database constraints from blowing up.
comment:9 by , 19 years ago
I like the fieldname = meta.Field(...)
syntax in general, but here are a few issues I have with this particular implementation:
- It's too flexible, to the point of confusion. Making it possible to do either
class Meta
orclass Field
or plain class attributes just smacks of "there's more than one way to do it." There should be one, clear, obvious way to do it. If we decide to change model syntax, let's haveclass Meta
for non-field info, and all fields are just attributes of the class. - The current
ForeignKey
solution rubs me the wrong way.ForeignKey = Poll, {'edit_inline':True, 'num_in_admin':3}
just doesn't feel natural -- or look good. I still haven't seen a good way to designateForeignKey
s in class attribute syntax without reverting to cruft.
I'd love to see ideas/patches that use the nicer fieldname = meta.Field(...)
syntax but avoid these two issues.
comment:10 by , 19 years ago
I like adrian's suggestion of having just class Meta for non-field info and all other fields are just written as attributes of the class. +1.
One concern, in the remote-possibility range: what if someone absolutely must have a field named "Meta", with that initial capital, and can't change it? Is there a way to do this even with a class.Meta in the equation? Perhaps (strawman syntax): "othername = meta.Field(override_name="Meta")?"
This probably isn't too terribly important, though, and I suspect if it really becomes necessary it would be possible to shoehorn it in.
comment:11 by , 19 years ago
Thanks for the feedback!
"""
either class Meta or class Field or plain class attributes just smacks of "there's more than one way to do it."
"""
I agree. I implemented all three ways because there were people who like each one. I didn't know which one I liked best, and it took very little more code to implement them all. If you want just the 'class Meta:' method, I can implement that just by deleting a little code.
"""
The current ForeignKey solution rubs me the wrong way. ForeignKey = Poll, {'edit_inline':True, 'num_in_admin':3} just doesn't feel natural -- or look good.
"""
I agree. So, perhaps it should be limited to "ForeignKey = Poll" for when no options are needed? and use "rel_name = meta.ForeignKey(RelatedObject)" for when more options are needed? Or just scrap the former?
MWM
comment:12 by , 19 years ago
rmunn:
If someone absolutely _must_ have a field named "Meta", they can use "field=(meta.ForeignKey(RelatedObject, rel_name='Meta'),)" and use the new way for all the other fields.
MWM
comment:13 by , 19 years ago
I say stay consistent: treat ForeignKeys like any other field type, and make it "rel_name = meta.ForeignKey(RelatedObject)" even if no other options are needed. Switching between "ForeignKey = Poll" and "poll = ForeignKey(Poll, null=True, blank=True)" would be very jarring otherwise.
I believe there's been a suggestion on the django-developers list to allow something like "poll = ForeignKey('Poll')" and do a name-based lookup later, to allow for circular references of ForeignKeys, but maybe that should be a separate patch.
comment:14 by , 19 years ago
Regarding ForeignKey
s --
The small problem with "rel_name = meta.ForeignKey(RelatedObject)
" is that users shouldn't have to type the rel_name
unless they specifically want to have control over it. That's the sort of thing that should be automated unless absolutely necessary.
Any way to use "meta.ForeignKey(RelatedObject)
" without having to specify the rel_name
?
comment:15 by , 19 years ago
Suggestion for the latest version of the patch: the line "attrs += getattr(attrsMeta, name)" (after "if name in ("fields",):") should probably be "attrs.extend(getattr(attrsMeta, name))". I don't like extending lists with the += operator, since its operation on (mutable) lists is different from its operation on other, immutable values like strings or ints or even tuples. That's too "magical" for my taste; list.extend is better because it's explicit about what's going on.
comment:16 by , 19 years ago
To use "meta.ForeignKey(RelatedObject)
" without having to specify the rel_name
will probably have to involve some trickery with stack frames. I think it would be possible, though: I'm playing with some ideas. I'll post my results once I have 'em.
comment:17 by , 19 years ago
Allright, I have a new version of the patch attached. I cut out a bunch of the options. 'ForeignKey = RelatedObject' is no longer implemented, and the 'class Fields:' is no longer there. You can, however, still specify meta data in either 'class Meta:' or as top level attributes. (This is for backwards compatibility.)
@Adrian
Yes, this is why I came up with the 'ForeignKey = RelatedObject' form. And yes, I think that "meta.ForeignKey(RelatedObject)" without assigning it to anything is possible, but it would require a bit of magic. Should I work on that? (And, I assume, do the same for ManyToManyField and OneToOneField?)
@rmunn
I think that the way I had it looks cleaner, but whatever. It's changed :)
MWM
comment:18 by , 19 years ago
mmarshall -- Sure, if you can get meta.ForeignKey(RelatedObject)
to work without assigning it to anything, and it works, and it doesn't have too much overhead, that'll be the final step before we integrate this into Django, as far as I'm concerned. I'll need to talk about it with Jacob, but I believe he thinks it's an improvement.
For backwards compatibility, what do you think of these proposals:
- We could call the new syntax
meta.NewModel
instead ofmeta.Model
. That waymeta.Model
would still work, until the first official release, when we'd renamemeta.Model
tometa.OldModel
andmeta.NewModel
tometa.Model
. Eventually we'd removemeta.OldModel
. - Or, we could immediately rename
meta.Model
tometa.OldModel
and use the newmeta.Model
immediately. For people who don't have time to convert their models to the new syntax, they could usemeta.OldModel
for a quick/easy way to fix their code in the interim. Thenmeta.OldModel
would be removed, probably at first release.
Other than that, don't worry about backwards compatibility within your code -- make the code as fast and clean as possible.
comment:19 by , 19 years ago
I'm experimenting with ways to do meta.ForeignKey(RelatedObject)
without assigning it; I'll post a proof-of-concept when I'm done. (Soon).
comment:20 by , 19 years ago
About backwords compatibility... I have been making it backwards compatible because that is the easy way out. In case if you haven't looked carefully at the patch, all that I am doing is:
- Taking all of the attributes that inherent from Field, assigning the attribute name to them, and adding them to the fields list.
- A little hack to make the first arg passed to a field be the verbose_name when applicable.
- Copying the attributes from the Meta class, and placing them in the main class attributes.
So, the only time my code comes into play is when python parses the class. From then on, it's all just the same as it was before.
I chose this route because I knew that the old method with the 'fields' attribute has been well tested. If I revamp all of that, I am afraid I would introduce all sorts of bugs that won't pop up for a while.
However, I suppose that this also is another layer of complexity. Having the Model classes go through this conversion could be confusing for someone else trying to get into the code.
So, Adrian, what do you think? Would it be worth it to delve into refactoring meta.py? Or should I just give some clear documentation of what is going on with the conversion? (I am inclined to the latter.)
by , 19 years ago
Attachment: | proof_of_concept.py added |
---|
Proof-of-concept for ForeignKeys without assignment
comment:21 by , 19 years ago
Okay, I've got proof-of-concept code up. Look at the definition of SampleRecord, then run it and notice that the ForeignKeys know what their name is, and that SampleRecord got assigned the right attributes.
by , 19 years ago
Attachment: | meta5.diff added |
---|
Added support for relationship fields not needing to be assigned to an attribute.
comment:22 by , 19 years ago
I just attached a new version of the patch that supports relationship fields not being assigned to an attribute. Great idea rmunn!
This works with ForeignKey, ManyToManyField, and OneToOneField.
MWM
by , 19 years ago
Attachment: | meta6.diff added |
---|
Fixed a bug with relationship fields in the 'fields' attribute being duplicated. (oops)
comment:23 by , 19 years ago
Hey Adrian,
I was just thinking more about my comment concerning backwards compatibility, and trying to avoid refactoring a large amount of the code. There might be some good reasons for refactoring.
First of all, I would like to see everything that has to do with a field inside of that field's class. I don't think that stuff like ""isinstance(f, FileField):"" belongs in Model.new. (Relationship fields might be a good exception for this, however.) It might also be good to put the Field classes in their own module, cutting a fair bit off of the 2200+ line beast of meta.py ;-) This should also make it easier for a user to create a custom Field subclass, without having to modify django itself.
Also related to Fields, I am currently doing some dirty hacks to get name/verbose_name sorted out. With this new method, verbose_name should be made the first argument, in order to clean stuff up.
Ok, I have convinced myself at least that this is the better way to go.
Thoughts?
MWM
comment:24 by , 19 years ago
Oh, and when I said "There might be some good reasons for refactoring.", I meant more along the lines of, "There might be some other reasons for refactoring that I hadn't thought of before."
MWM
comment:26 by , 19 years ago
milestone: | → Version 1.0 |
---|
by , 19 years ago
Attachment: | model_api.diff added |
---|
Implements the new model api. Breaks compatibility with old models.
by , 19 years ago
Attachment: | model_api.r.389.diff added |
---|
Same as before, only resolves confilicts with r.389
comment:27 by , 19 years ago
Allright, so these latest patches break compatibility. Here are some notes, in the order that they come to mind:
First two args for fields are now reversed. (verbose_name before name)
add_fields is not supported. Fields are inhereted like normal attributes. (Come to think of it, there wasn't a test for this, so it might not work.)
All fields can be added without assigning to an attribute. (Opposed to just the relationship fields in previous patches.) This was done because it was the easiest and cleanest way to do it, not to have a gazzillion ways of doing something. ;)
I had wanted to get rid of many of those 'isinstance(f, SomeFieldClass)'s, but I didn't. This is enough code change for one patch :)
I ported all of the test models. They all pass for me.
It is thread-safe. (Or at least it should be.)
Thoughts?
MWM
comment:28 by , 19 years ago
Summary: | Build models using fieldname=FieldClass → [patch] Build models using fieldname=FieldClass |
---|
comment:29 by , 19 years ago
As Syndrome might put it, I'm completely geeking out over the technical wizardry behind not having to actually assign an attribute to get a field. I'm just as worried, though, that there's perhaps a litle *too* much magic in there. I wouldn't be at all surprised if the five seconds saved by not having to give an attribute name would be wiped out several times over in confusion amongst code maintainers that, whilst Python-aware, might not necessarily know Django.
It's not just a matter of readability-to-humans, either. I can't imagine all IDEs, code checkers, documentation generators, and API browsers will cope. If they can't, then you're making a whole bunch of people's lives a whole lot more difficult just to save others a few keystrokes.
What's the problem with making the model look like Python classes that makes it preferable to implement some weird bucket of expressions that build a class in a way unlike any other class a Python programmer has ever encountered?
comment:30 by , 19 years ago
Another hazard is that stackframe wizard is vulnerable to changes in the stackframes. You'll need to widen your test footprint to -O, Psyco, PyPy, IronPython, Jython, every new version of Python, running under wxPython, and goodness knows what else if you rely on it. I had massive problems with my lock debugging code in PyDS just from -O alone.
comment:31 by , 19 years ago
Cc: | added |
---|
With the way that django pieces together classes using metaclassing, (and modules for that matter,) I don't think that this patch will break IDEs and code checkers any more than they are already broken. :-D
As for messing with the stack, I hadn't thought about there being difficulties with other python implementations. I don't imagine anyone trying doing it on PyPy, IronPython, and Jython in the near future, but -O, and maybe Psyco would be worth testing with. Even then, IMHO Psyco takes up more memory than it's worth in a webapp, and AFAIK -O is considered unstable anyway.
Have you taken a close look at my patch? Inspecting the stack is mainly just there for multithreading. (Although there will be a couple of trivial issues that pop up if I remove it.)
MWM
comment:32 by , 19 years ago
I just tested it with -O and Psyco. All tests pass fine with -O, but Psyco has problems with the inspect module. However, with Psyco you cannot dynamically modify member functions, so I don't think it would work anyway.
MWM
comment:33 by , 19 years ago
Resolution: | → wontfix |
---|---|
Status: | new → closed |
Jacob and I have talked this over at length, and we've decided the model syntax shouldn't change. Using a fieldname=FieldClass
syntax would require too much "magic" behind the scenes for minimal benefit.
comment:35 by , 19 years ago
Resolution: | wontfix |
---|---|
Status: | closed → reopened |
Reopening after IRC discussion.
by , 19 years ago
Attachment: | modeldiscuss_cleanedup.txt added |
---|
IRC discussion with tabs cleaned up so it's easier to read
comment:36 by , 19 years ago
The tabs in the previous file were making text from me (rmunn) and hugo- on a different indentation level than everyone else's text, so I cleaned up the file to use spaces instead. Now it should be a lot easier to read. :-)
comment:37 by , 19 years ago
Assuming I'm reading the transcript correctly: are we back to something like the following?
from django.core import meta class Person(meta.Model): name = meta.CharField(maxlength=200) class CourtCase(meta.Model): plaintiff = meta.ForeignKey(Person) defendant = meta.ForeignKey(Person)
I'm definitely +1 on that.
Do we need a separate ticket for direct assignment and reference of objects in related tables, or can we squeeze it in on the same change as this ticket?
me = persons.get_object(name__exact="garthk") you = persons.get_object(name__exact="hugo-") case = courtcases.CourtCase() case.defendant = me case.plaintiff = you case.save() print case.defendant.name
You'd also want to retain support for case.defendant_id = me.id
of course, but I keep stumbling over not being able to do it the obvious way.
comment:38 by , 19 years ago
garthk: I'd say we would use a different ticket for that, I concur with your views (although I haven't checked the code to see how hard an implementation would be).
comment:39 by , 19 years ago
Just had a chat to hugo, and I'd also like to +1 his suggestion that there also be some model creation API through which he can generate his thousand-field tables programmatically. It makes sense that the new model syntax invoke that API.
Whilst I'm posting, I suspect the current convention would be that the way to let Django know about the administration model would be:
class Poll(meta.Model): __admin__ = meta.Admin() question = meta.CharField(maxlength=30) pub_date = meta.DateTimeField() sites = meta.ManyToManyField(core.Site)
rather than:
class Poll(meta.Model): question = meta.CharField(maxlength=30) pub_date = meta.DateTimeField() sites = meta.ManyToManyField(core.Site) class Meta: admin = meta.Admin()
comment:40 by , 19 years ago
I actually, I think the convention for metadata would still be a class Meta, although I suggested this:
class Poll(meta.Model): question = meta.CharField(maxlength = 30) pub_date = meta.DateTimeField() sites = meta.ManyToManyField(core.Site) class META: admin = meta.Admin()
I'm not sure what the contents for the meta-class would look like, though.
comment:41 by , 19 years ago
Following Manuzhai's suggestion, I've moved my related field access idea to #359.
comment:42 by , 19 years ago
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
(In [549]) Fixed #122 -- BIG, BACKWARDS-INCOMPATIBLE CHANGE. Changed model syntax to use fieldname=FieldClass() syntax. See ModelSyntaxChangeInstructions for important information on how to change your models
Allright, I have an updated version here.
Now, you can optionally place your fields under "class Fields:" and your meta data under "class Meta:". Note that you can still put everything directly in your Model class, or you can put some {Fields|Meta} info under "class {Fields|Meta}:" and put some directly under your Model class.
Here are some examples of what you can do: (Previously given example still works.)
Here is the patch.