SQLInsertCompiler.as_sql() failure

[Mark Lavin]

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).

[Alex Gaynor]

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

[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.

[Tobias McNulty]

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.

[Tobias McNulty]

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

[Alex Gaynor]

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.

[Tobias McNulty]

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.

[Tobias McNulty]

Moving to DDN per Alex's comment.

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

[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.

[Jacob]

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.

[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.

[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.

[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

[Carl Meyer <carl@…>]

In 161c4da588d4cc757da44bcbb5875a84a7b8a7e6:

Fixed #14019 -- Initialize SQLInsertCompiler.return_id attribute.