Opened 5 years ago
Closed 2 years ago
#14019 closed Bug (fixed)
SQLInsertCompiler.as_sql() failure
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)
Change History (19)
Changed 5 years ago by mlavin
comment:1 Changed 5 years ago by mlavin
- Has patch set
- Needs documentation unset
- Needs tests unset
- Patch needs improvement unset
comment:2 Changed 5 years ago by manfre
- Cc manfre added
comment:3 Changed 5 years ago by Alex
comment:4 Changed 5 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 5 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 5 years ago by tobias
- Owner changed from nobody to tobias
- Status changed from new to assigned
comment:7 Changed 4 years ago by julien
- Needs tests set
- Severity set to Normal
- Type set to Bug
comment:8 Changed 4 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:
And the corresponding diff:
comment:9 Changed 4 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 4 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.
comment:12 Changed 2 years 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 2 years 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 2 years ago by carljm
- Resolution wontfix deleted
- Status changed from closed to new
comment:17 Changed 2 years 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.):
comment:18 Changed 2 years ago by Carl Meyer <carl@…>
- Resolution set to fixed
- Status changed from new to closed
The Query classes are an internal API, is there a way to reproduce this with the public API?