Code

Opened 6 years ago

Closed 6 years ago

Last modified 3 years ago

#7315 closed (fixed)

Forcing Inner join to a nullable foreignkey when it is inherited from an abstract class

Reported by: marinho Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Keywords: qsrf-cleanup
Cc: Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

I am having a similar problem to #6981 ticket, but in my case the queryset doesn't uses select_related, and the column is in the inherited abstract class.

models.py:

class AbstractCrane(models.Model):
    manufacturer = models.ForeignKey(
            'Company',
            null=True,
            blank=True,
            db_column='new_manufacturer_id',
            limit_choices_to={'is_manufacturer': True},
            )

class WishListCrane(AbstractCrane):
    pass

queryset (there is 1 row, but it doesn't is found):

>>> WishListCrane.objects.all()
[]

>>> connection.queries[-1]['sql']
u'SELECT "cranes_wishlistcrane"."id", "cranes_wishlistcrane"."new_manufacturer_id", "cranes_wishlistcrane"."other_manufacturer", "cranes_wishlistcrane"."category_id", "cranes_wishlistcrane"."other_category", "cranes_wishlistcrane"."model", "cranes_wishlistcrane"."lift_capacity", "cranes_wishlistcrane"."boom_length", "cranes_wishlistcrane"."jib_length", "cranes_wishlistcrane"."description", "cranes_wishlistcrane"."user_id", "cranes_wishlistcrane"."require_manufacturer", "cranes_wishlistcrane"."require_categories", "cranes_wishlistcrane"."require_model", "cranes_wishlistcrane"."require_lift_capacity", "cranes_wishlistcrane"."require_boom_length", "cranes_wishlistcrane"."require_jib_length", "cranes_wishlistcrane"."require_description" FROM "cranes_wishlistcrane" INNER JOIN "companies_company" ON ("cranes_wishlistcrane"."new_manufacturer_id" = "companies_company"."id") ORDER BY "companies_company"."name" ASC, "cranes_wishlistcrane"."model" ASC'

I found this part of django/db/models/sql/query.py, and tought maybe it is wrong:

840:         elif promote or outer_if_first:
841:            join_type = self.LOUTER

I modified to:

840:         elif promote or outer_if_first or nullable:
841:            join_type = self.LOUTER

and worked.

I'm nor sure if this is correct for the architecture, because I had no made a better checking, but worked for me

Attachments (1)

7315-fix.diff (568 bytes) - added by marinho 6 years ago.

Download all attachments as: .zip

Change History (9)

Changed 6 years ago by marinho

comment:1 Changed 6 years ago by marinho

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 6 years ago by Alex

Do all the tests still pass with this modification?

comment:3 Changed 6 years ago by gav

  • Keywords qsrf-cleanup added

comment:4 Changed 6 years ago by marinho

@Alex

I have two sites working on this change since I did it. They has, each one, about 10k page views per day, and they are working well.

But I'm not sure if this change is effective for other situations

comment:5 Changed 6 years ago by jacob

  • milestone set to 1.0

comment:6 Changed 6 years ago by mtredinnick

The example you give here isn't complete (e.g. abstract=True is missing), but looking at the query, it seems there is some default ordering being applied in the Company model. So I suspect this is #7181 in disguise and might be fixed in [7761] (since it's only the ordering table that is incorrectly joined). Please check against that revision and, if the problem still occurs, provide the missing details in the example: right now, nobody but you can recreate the example, since we don't have all the information necessary (and you can probably remove stuff like db_column and limit_choices_to, I suspect, without changing the result, which will make things simpler).

The patch you supply isn't correct because it goes against the very definition of the nullable parameter (the join can be promoted in the future). A join that should be outer right now has promote=True already.

comment:7 Changed 6 years ago by mtredinnick

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

Closing on the grounds that it's almost certainly a dupe of #7181. Please reopen with a self-contained example if the problem still exists.

comment:8 Changed 3 years ago by jacob

  • milestone 1.0 deleted

Milestone 1.0 deleted

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.