Code

Opened 4 years ago

Closed 12 months ago

#14019 closed Bug (fixed)

SQLInsertCompiler.as_sql() failure

Reported by: mlavin Owned by: tobias
Component: Database layer (models, ORM) Version: 1.2
Severity: Normal Keywords:
Cc: manfre Triage Stage: Design decision needed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I came across a problem with SQLInsertCompiler.as_sql function while trying to get a stacktrace printed on insert errors (using django-db-log). I found that the as_sql function has an implicit condition that execute_sql must be called first because execute_sql sets the return_id attribute. This simple sequence shows the issue using any core db-backend:

from django.contrib.auth.models import User
from django.db import router
from django.db.models.sql import InsertQuery

db = router.db_for_write(User)
query = InsertQuery(User)
query.insert_values([(User._meta.fields[0], 1)], raw_values=False)
print query.get_compiler(using=db).as_sql()

yields

AttributeError: 'SQLInsertCompiler' object has no attribute 'return_id'

I've attached a patch which checks for the existence of the return_id attribute and defaults to False if not found (as is the convention in execute_sql).

Attachments (1)

13448.diff (849 bytes) - added by mlavin 4 years ago.

Download all attachments as: .zip

Change History (19)

Changed 4 years ago by mlavin

comment:1 Changed 4 years ago by mlavin

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

comment:2 Changed 4 years ago by manfre

  • Cc manfre added

comment:3 Changed 4 years ago by Alex

The Query classes are an internal API, is there a way to reproduce this with the public API?

comment:4 Changed 4 years ago by mlavin

I don't have a simple example using the public API yet. The scenario that brought me to this issue was I have a set of unmanaged models (from a legacy database). An app was built to create these models but was violating a table constraint. When the insert failed the exception was caught and django-db-log tried to print out the failing SQL statement. Calling Query.__str__ called SQLInsertCompiler.as_sql which failed because return_id was never set.

comment:5 Changed 4 years ago by tobias

  • Triage Stage changed from Unreviewed to Accepted

Internal API or not, it seems like a generally good idea to make the code as fool-proof as possible, and this is a fairly simple change. Two thoughts:

1) Does as_sql() return anything useful if execute was not called first?

2) It's often cleaner just to set self.return_id in init, instead of spreading hasattr() throughout the code. I see no other cases where return_id is used within or outside the class, so setting it in init should be fine.

comment:6 Changed 4 years ago by tobias

  • Owner changed from nobody to tobias
  • Status changed from new to assigned

comment:7 Changed 3 years ago by julien

  • Needs tests set
  • Severity set to Normal
  • Type set to Bug

comment:8 Changed 2 years ago by tobias

  • Easy pickings unset
  • Triage Stage changed from Accepted to Ready for checkin
  • UI/UX unset

A pull request with an updated patch is here: https://github.com/django/django/pull/76

And the corresponding diff: https://github.com/django/django/pull/76.diff

comment:9 Changed 2 years ago by Alex

  • Triage Stage changed from Ready for checkin to Accepted

This isn't RFC, because I'm not even sure it should be accepted, there's no issue of resiliency, if you're using the code wrong it's ok that it breaks. This isn't a public API.

comment:10 Changed 2 years ago by tobias

  • Needs tests unset

Unit test added to the pull request before I saw Alex's comment.

It seems like good development practice to me to initialize variables before they can potentially be used.

comment:11 Changed 2 years ago by tobias

  • Triage Stage changed from Accepted to Design decision needed

Moving to DDN per Alex's comment.

If it's accepted the patch should be ready for review.

Last edited 2 years ago by tobias (previous) (diff)

comment:12 Changed 2 years ago by tobias

Ensuring that calling str() on an object will not raise an exception seems like a pretty basic improvement to me as well. I'm not sure why this trivial fix is getting so much push back.

comment:13 Changed 13 months ago by jacob

  • Resolution set to wontfix
  • Status changed from assigned to closed

Per Alex's comments, I'm marking this wontfix. It's not a public API, so making it work for anyone other than Django itself.

comment:14 Changed 13 months ago by kmtracey

Really? Sigh. This is a real problem hit by people trying to use Django and a couple of external packages (mssql backend, django db log). Why the incredible pushback to initializing an attribute in init so as to ensure that a utility routine won't fall over dead if this "internal api" is used "incorrectly"? What's the harm? Maybe it would give a clue as to what the heck has gone wrong with use of this internal api by whatever package is in error...the current Django code makes it impossible to figure out who has gone wrong when, because you get a totally useless exception. Why are we being so user-unfriendly? Sure, it's an internal API but I'm not exactly surprised if an externally-maintained DB backend uses it, and given we don't exactly document this stuff very well I think it would be far more friendly of us to make the small effort to make this code a bit more foolproof.

comment:15 Changed 12 months ago by carljm

Reopening after checking with Jacob on IRC. This is a code quality fix, and code quality matters in private APIs too.

I don't like the patch using hasattr, but I think initializing return_id in __init__ is a clear win.

comment:16 Changed 12 months ago by carljm

  • Resolution wontfix deleted
  • Status changed from closed to new

comment:17 Changed 12 months ago by tobias

The original pull request was against the unofficial Django repo on GitHub (and hence the link is broken), so I created a new one against the new repo that should apply cleanly (and the test passes, etc.):

https://github.com/django/django/pull/1002

comment:18 Changed 12 months ago by Carl Meyer <carl@…>

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

In 161c4da588d4cc757da44bcbb5875a84a7b8a7e6:

Fixed #14019 -- Initialize SQLInsertCompiler.return_id attribute.

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.