Call for feedback: First GSoC patch ready to land
Russell Keith-Magee
I've just finished the polishing the first part of Alex's GSoC work to
add Multi-db, which I'd like to commit. This is a call for feedback.
This first patch isn't directly related to multi-db, but it is a
prerequisite that will allow the multi-db code to function. The goal
of this patch is to remove all uses of raw SQL from related.py. This
means that all M2M and FK operations are performed using Django
queries, rather than using cursors and manually constructed SQL. This
is needed so that multi-db queries have complete control over the SQL
that is generated by Django. As an aside, this is also a prerequisite
for the non-SQL backend work, as non-SQL backends obviously can't use
a SQL cursor.
This is being tracked as ticket #10109. I've uploaded the most recent
patch to that ticket. Code is also available from Alex's github branch
(details on the ticket).
My intention is to commit this mid next week. If you have any
objections (either to the patch, or my intended commit schedule), now
is the time to raise them.
Here are the fine details for anyone interested:
First - an example set of models:
class Author(Model):
name = CharField()
class Book
name = CharField()
authors = ManyToManyField(Author)
Historically, accessing mybook.authors resulted in the creation of a
cursor and the execution of some hand-crafted SQL to retrieve data
from the myapp_book_authors table.
Using this patch, the authors m2m relation is now backed by an
automatically generated m2m model. You don't have to do anything to
get this model - it is created during the contribute_to_class
processing for the m2m field.
The automatically generated m2m model is the equivalent of the following:
class Book_authors:
book = ForeignKey(Book)
author = ForeignKey(Author)
This model is then used as a 'through' model in the m2m-intermediate
sense. All requests to add/remove from the m2m relation are handled by
creating/destroying instances of the m2m model.
The auto-created model is available in the model index if you ask for
it explicitly, but it won't be returned by loader.get_models() unless
you explicitly ask for the auto-created models (using
loader.get_models(include_auto_created=True) )
Signals for the m2m model have been silenced - you won't get a
create/destroy signal every time you add/remove an m2m object.
The other interesting feature is that the reverse relations (the _set
objects that would normally exist on Book and Author) have been
hidden. This is achieved by appending '+' to the related names. The
use of '+' to hide relations has been in Django for a while (to mask
the reverse relation for symmetric m2m relations with self) - this
patch expands the use of '+' on a field name to a more general
field-hiding capability.
As a result of using normal Django models for the m2m relation, we can
deprecate a lot of the special handling for m2m models in the model
creation code in the backends. With this patch, an m2m table is just a
normal model with two foreign keys, so we can use normal table
creation code.
Some interesting fallout onto other tickets:
* #11795 (allowing admin inlines for m2m objects) becomes a 2 line
fix, plus documentation. Since this patch makes m2m tables fully
fledged Django objects, you can reference them in the models= line of
a TabularInline. m2m inlines are actually possible without any change
on top of this patch - but with two extra lines, it becomes a lot
easier to use.
* #5537 becomes a purely documentation fix. If you use a related_name
of '+' on your model, the related object is hidden. The only question
here is whether we want to document this change - so far the '+' thing
has been an internal implementation issue, so we need to decide if we
want to make this official.
So - that's the patch. Feedback welcome.
Yours,
Russ Magee %-)
Russell Keith-Magee
On Oct 30, 4:33 pm, Russell Keith-Magee <freakboy3...@gmail.com>
wrote:
>
> I've just finished the polishing the first part of Alex's GSoC work to
> add Multi-db, which I'd like to commit. This is a call for feedback.
compatible, and require no changes to any existing code.
Yours
Russ Magee %-)
Johannes Dollinger
"AssertionError: ForeignKey cannot define a relation with abstract
class A"
{{{
class R(models.Model): pass
class A(models.Model):
r_set = models.ManyToManyField(R, related_name="%(class)s_set")
class Meta:
abstract = True
class B(A): pass
class C(A): pass
}}}
Disabling `create_many_to_many_intermediary_model()` if
`cls._meta.abstract` fixed it for me.
__
Johannes
Russell Keith-Magee
<johannes....@einfallsreich.net> wrote:
>
> The m2m-refactor patch breaks ManyToManyFields on abstract models:
> "AssertionError: ForeignKey cannot define a relation with abstract
> class A"
Thanks for the report, Johannes. I'll take a look at this and see what
I can do about a fix.
Yours,
Russ Magee %-)
Russell Keith-Magee
I've just pushed a fix to the github branch, and uploaded a new patch
based on your analysis and proposed fix. Thanks for the help.
Yours,
Russ Magee %-)
Jacob Kaplan-Moss
I'm +1 on merging the patch immediately. I have some feedback on the
couple of issues you raised below, but I see no reason they can't be
addressed after merging.
On Fri, Oct 30, 2009 at 3:33 AM, Russell Keith-Magee
<freakb...@gmail.com> wrote:
> The automatically generated m2m model is the equivalent of the following:
>
> class Book_authors:
> book = ForeignKey(Book)
> author = ForeignKey(Author)
I can't quite see how you'd "get at" this model class, should I want
it. I guess I could go through `django.db.models.loading.get_model`,
right? Is the intention to make this class an internal implementation
detail? If so, then it's just fine that it's not directly available...
but if this model is supposed to be available to public code then do
we need a better way to expose it?
I ask because...
> * #11795 (allowing admin inlines for m2m objects) becomes a 2 line
> fix, plus documentation. Since this patch makes m2m tables fully
> fledged Django objects, you can reference them in the models= line of
> a TabularInline. m2m inlines are actually possible without any change
> on top of this patch - but with two extra lines, it becomes a lot
> easier to use.
... it seems that to make this fix you'd need to somehow "get at"
those related models. Right?
> * #5537 becomes a purely documentation fix. If you use a related_name
> of '+' on your model, the related object is hidden. The only question
> here is whether we want to document this change - so far the '+' thing
> has been an internal implementation issue, so we need to decide if we
> want to make this official.
`related_name="+"` makes me wince. It's entirely unclear (to me) that
"+" means "hide the reverse relation", so I'm -1 on documenting that
fact.
However, it's useful behavior, so why not just add a constant to
`django.db.models` for that? It'd then look like::
fk = models.ForeignKey(ToModel, related_name=models.HIDE_RELATED)
Jacob
Russell Keith-Magee
Correct - and that's the 2 line fix.
As is, there are four ways to get at the through model via various
convoluted channels:
1. get_model('myapp','Book_authors')
2. Book._meta.get_field('authors').rel.through
3. Book.authors.field.rel.through
4. Author.book_set.related.field.rel.through
None of these are particularly pretty, but they work. However, 3 and 4
can be made humane with very little effort. If we put a readonly
'through' property on the m2m descriptors (forward and reverse) that
completes the rest of the lookup, it becomes possible to say:
3. Book.authors.through
4. Author.book_set.through
So, your admin declaration becomes:
class AuthorInline(TabularInline):
model = Book.authors.through
class Book(ModelAdmin):
inlines = [AuthorInline]
exclude = ['authors']
This technique works for _any_ m2m intermediate table - explicit or implicit.
>> * #5537 becomes a purely documentation fix. If you use a related_name
>> of '+' on your model, the related object is hidden. The only question
>> here is whether we want to document this change - so far the '+' thing
>> has been an internal implementation issue, so we need to decide if we
>> want to make this official.
>
> `related_name="+"` makes me wince. It's entirely unclear (to me) that
> "+" means "hide the reverse relation", so I'm -1 on documenting that
> fact.
>
> However, it's useful behavior, so why not just add a constant to
> `django.db.models` for that? It'd then look like::
>
> fk = models.ForeignKey(ToModel, related_name=models.HIDE_RELATED)
Sounds reasonable to me. The important part is actually deciding that
it's useful behavior that we want to encourage - the discussion on
#5537 hasn't got consensus that it is.
Yours
Russ Magee %-)
jedie
On syncdb i get this error:
"""
Error: One or more models did not validate:
pylucid.design: 'headfiles' has a manually-defined m2m relation
through model Design_headfiles, which does not have foreign keys to
EditableHtmlHeadFile and Design
"""
It works fine with r11709.
My models are a package. This seems to be related to this behaviour.
My model code is here:
http://trac.pylucid.net/browser/branches/0.9/pylucid_project/apps/pylucid/models?rev=2347
Russell Keith-Magee
Are you still getting this error with a more recent checkout? There
have been some updates to trunk since r11709 that directly affect the
validation process.
Yours.
Russ Magee %-)
jedie
> Withhttp://code.djangoproject.com/changeset/11710i have a problem.
>
> """
> Error: One or more models did not validate:
> pylucid.design: 'headfiles' has a manually-defined m2m relation
> through model Design_headfiles, which does not have foreign keys to
> EditableHtmlHeadFile and Design
> """
headfiles = models.ManyToManyField("EditableHtmlHeadFile")
I must add the appname:
headfiles = models.ManyToManyField("pylucid.EditableHtmlHeadFile")
But IMHO the error message is not good. Should it be added to:
http://code.djangoproject.com/wiki/BetterErrorMessages ?
Russell Keith-Magee
>
> On 5 Nov., 08:53, jedie <google.gro...@jensdiemer.de> wrote:
>> Withhttp://code.djangoproject.com/changeset/11710i have a problem.
>> On syncdb i get this error:
>>
>> """
>> Error: One or more models did not validate:
>> pylucid.design: 'headfiles' has a manually-defined m2m relation
>> through model Design_headfiles, which does not have foreign keys to
>> EditableHtmlHeadFile and Design
>> """
>
> I found the error! This line is wrong:
> headfiles = models.ManyToManyField("EditableHtmlHeadFile")
>
> I must add the appname:
> headfiles = models.ManyToManyField("pylucid.EditableHtmlHeadFile")
So... is this a regression or not? Your original message said that
this worked previously.
> But IMHO the error message is not good. Should it be added to:
> http://code.djangoproject.com/wiki/BetterErrorMessages ?
Well, sure, if you like. That wiki page isn't a formal part of the
development process, though, so adding your use case to that page
won't inherently result in the problem getting fixed. I, for one,
don't spend any time trawling the wiki looking for suggestions of
things to fix - that's what Trac is for.
If people want to use the wiki to organize complex proposals, that's
fine. However, if you actually want to see problems fixed, at some
point we (the core) need tickets in Trac (with patches) that describes
the work that needs to be done.
Yours,
Russ Magee %-)
jedie
> On Thu, Nov 5, 2009 at 3:53 PM, jedie <google.gro...@jensdiemer.de> wrote:
>
> > Withhttp://code.djangoproject.com/changeset/11710i have a problem.
>
> > """
> > Error: One or more models did not validate:
> > pylucid.design: 'headfiles' has a manually-defined m2m relation
> > through model Design_headfiles, which does not have foreign keys to
> > EditableHtmlHeadFile and Design
> > """
>
> > It works fine with r11709.
>
> > My models are a package. This seems to be related to this behaviour.
> > My model code is here:
> Are you still getting this error with a more recent checkout? There
> have been some updates to trunk since r11709 that directly affect the
> validation process.
But a M2M to django.contrib.sites.models.Site still doesn't work :(
I used the last SVN version.
I tried:
from django.contrib.sites.models import Site
...
sites = models.ManyToManyField(Site)
And i tried:
sites = models.ManyToManyField("sites.Site")
The M2M table doesn't created.
jedie
> But a M2M to django.contrib.sites.models.Site still doesn't work :(
> I used the last SVN version.
>
Russell Keith-Magee
I don't mean to be rude, but you're not really helping me here. I'm
having difficulty filtering the useful debugging information out of
the multitude of messages you have posted.
What I need is a single, _complete_ example of the simplest set of
models you can build that that fails validation, and the _specific_
SVN revision on which it fails.
Yours,
Russ Magee %-)
jedie
> What I need is a single, _complete_ example of the simplest set of
> models you can build that that fails validation, and the _specific_
> SVN revision on which it fails.
I tried to create a django test case for this, but i never have
written a django "self test" before. So i don't know if it's the right
way...
I created http://code.djangoproject.com/ticket/12168 and attach a
Patch with the "model_package" test.
If you run the test you will see, that the M2M tables are not created.
Russell Keith-Magee
Thanks - that's exactly what I needed. I've fixed the problem in r11724.
Yours,
Russ Magee %-)
Schmilblick
http://code.djangoproject.com/ticket/5390 ?
I'm currently using django with the patch provided in that ticket, and
the latest commits regarding the gsoc-project created a diff im not
sure how to handle.
Let me know if you need any more information, or if i should stay in
django-users :)
/stark
On Nov 5, 12:59 pm, Russell Keith-Magee <freakboy3...@gmail.com>
wrote:
>
> > On 5 Nov., 11:30, Russell Keith-Magee <freakboy3...@gmail.com> wrote:
> >> What I need is a single, _complete_ example of the simplest set of
> >> models you can build that that fails validation, and the _specific_
> >> SVN revision on which it fails.
>
> > Sorry for confusion ;)
>
> > I tried to create a django test case for this, but i never have
> > written a django "self test" before. So i don't know if it's the right
> > way...
Russell Keith-Magee
> Sorry for crashing the list, but how does this affect this bug:
> http://code.djangoproject.com/ticket/5390 ?
>
> I'm currently using django with the patch provided in that ticket, and
> the latest commits regarding the gsoc-project created a diff im not
> sure how to handle.
m2m-refactor doesn't introduce any new signalling behavior.
Time permitting, this ticket is something I want to look at for v1.2.
If someone were to take the time to update the current patch, that is
one less job I will need to do, which will increase the likelihood
that I will be able to find the time to finish the job and commit the
ticket.
Yours,
Russ Magee %-)