Opened 6 years ago

Closed 3 years ago

#14019 closed Bug (fixed)

SQLInsertCompiler.as_sql() failure

Reported by: Mark Lavin Owned by: Tobias McNulty
Component: Database layer (models, ORM) Version: 1.2
Severity: Normal Keywords:
Cc: Michael 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 Mark Lavin 6 years ago.

Download all attachments as: .zip

Change History (19)

Changed 6 years ago by Mark Lavin

Attachment: 13448.diff added

comment:1 Changed 6 years ago by Mark Lavin

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

comment:2 Changed 6 years ago by Michael Manfre

Cc: Michael Manfre added

comment:3 Changed 6 years ago by Alex Gaynor

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

comment:4 Changed 6 years ago by Mark Lavin

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 6 years ago by Tobias McNulty

Triage Stage: UnreviewedAccepted

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 6 years ago by Tobias McNulty

Owner: changed from nobody to Tobias McNulty
Status: newassigned

comment:7 Changed 5 years ago by Julien Phalip

Needs tests: set
Severity: Normal
Type: Bug

comment:8 Changed 5 years ago by Tobias McNulty

Easy pickings: unset
Triage Stage: AcceptedReady 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 5 years ago by Alex Gaynor

Triage Stage: Ready for checkinAccepted

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 5 years ago by Tobias McNulty

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 5 years ago by Tobias McNulty

Triage Stage: AcceptedDesign decision needed

Moving to DDN per Alex's comment.

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

Last edited 5 years ago by Tobias McNulty (previous) (diff)

comment:12 Changed 5 years ago by Tobias McNulty

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 3 years ago by Jacob

Resolution: wontfix
Status: assignedclosed

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 3 years ago by Karen Tracey

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 3 years ago by Carl Meyer

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 3 years ago by Carl Meyer

Resolution: wontfix
Status: closednew

comment:17 Changed 3 years ago by Tobias McNulty

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 3 years ago by Carl Meyer <carl@…>

Resolution: fixed
Status: newclosed

In 161c4da588d4cc757da44bcbb5875a84a7b8a7e6:

Fixed #14019 -- Initialize SQLInsertCompiler.return_id attribute.

Note: See TracTickets for help on using tickets.
Back to Top