classification
Title: sqlite3 iterdump fails on column with reserved name
Type: behavior Stage: resolved
Components: Extension Modules Versions: Python 3.2, Python 3.3, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: ghaering Nosy List: Marko.Kohtala, eric.araujo, ghaering, petri.lehtinen, python-dev, r.david.murray, xapple
Priority: normal Keywords: patch

Created on 2010-09-03 06:15 by Marko.Kohtala, last changed 2013-01-10 16:31 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
issue9750.patch xapple, 2011-11-08 08:43 Patch with tests and fixes review
sqlite3dump.patch Marko.Kohtala, 2012-02-05 21:33 Test cases and patch to fix problems review
Messages (19)
msg115420 - (view) Author: Marko Kohtala (Marko.Kohtala) * Date: 2010-09-03 06:15
Sqlite3 fails to dump a database with column names that are keywords.
msg115421 - (view) Author: Marko Kohtala (Marko.Kohtala) * Date: 2010-09-03 06:17
Here is a patch that may resolve the bug.
msg115423 - (view) Author: Marko Kohtala (Marko.Kohtala) * Date: 2010-09-03 08:34
The second patch contains also fixes for some places where the rules described in http://www.sqlite.org/lang_keywords.html are not followed.
msg115445 - (view) Author: Marko Kohtala (Marko.Kohtala) * Date: 2010-09-03 12:09
It also fails if table or column names contain double quote.
msg116144 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2010-09-12 01:07
Thank you for the report and patch.  Some bug tracker tips:

1) It’s possible to replace a file, so that there is always one bug.py and one diff, which is easier for reviewers to understand than a long list of files (possibly all with the same name).  It’s also possible to remove obsolete files so that nobody loses time commenting on them.

2) Can you turn your bug reproducer into a patch for Lib/test/test_sqlite.py?

3) Please follow guidelines on http://www.python.org/dev/patches/ to make patches.

Regarding the code itself:

a) I don’t understand why you added quotes around names that seemed fine without them (for example in “SELECT name”).

b) I find the master line difficult to read: “q += ",".join(["'||quote(\"" + col.replace('"', '""') + "\")||'" for col in column_names])”.  You’re using backslashes since there are both single and double quotes, but I would use triple-quoted strings here.  I also find string formatting more readable than string concatenation: 'blah blah "{}" blah'.format(thing.replace('"', '""')).

c) Minor style thing: A generator expression works as fine as a list comprehension in str.join (that is, ",".join(i for i in source) == ";".join([i for i in source])).

Thanks again!
msg116155 - (view) Author: Marko Kohtala (Marko.Kohtala) * Date: 2010-09-12 08:57
Thank you for the review.

I have very limited time to use on this. So even when I'd like to make everything easy for you, have the time you give to python be as productive as possible, I can not.

But I'll respond to your comments on the patch.

a) I added the quotes to every identifier based on a comment in sqlite documentation "SQLite adds new keywords from time to time when it takes on new features. So to prevent your code from being broken by future enhancements, you should normally quote any identifier that is an English language word, even if you do not have to." While fixing one place, I fixed it to follow this recommendation in other places as well.

b) I added quotes using backslashes because it was consistent. The table name was already quoted like that. I agree it could be clearer.

c) I know. I only tried to make minimal changes targeted only to the issues at hand. I did not want to hide the fixes in middlde of changes to style.
msg131582 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-03-21 00:33
> I have very limited time to use on this.
This is a perfectly reasonable stance, and we’re grateful for the work you’re doing.  When you say that you don’t have time, someone else will take up the patch and update it to address reviews, and you will get credited.

a) Thanks for the explanation.  This would be a useful comment in the source.

b) It’s not uncommon to make cosmetic changes in the same commit as a bugfix.  Readability is valued.
msg146362 - (view) Author: Lucas Sinclair (xapple) Date: 2011-10-25 12:00
I just encountered this issue today.

So, it's been several months, will the patch be merged into the master branch ? Or will this never be fixed ?
msg146374 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-10-25 15:24
Hi Lucas.  Have you read my previous message?  The patch needs to be updated.  Would you like to do it?
msg146702 - (view) Author: Lucas Sinclair (xapple) Date: 2011-10-31 15:16
Sure, I can have a try at it and address the issues you pointed out.

The URL to the guidelines you provided gives a 404. In what form exactly would you like the patch to be ?

I wouldn't mind either adding to the test suite, but I'm not sure how to do that.
msg146710 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-10-31 16:41
> Sure, I can have a try at it and address the issues you pointed out.
Nice!

> The URL to the guidelines you provided gives a 404.
They’ve moved to http://docs.python.org/devguide meanwhile.

> I wouldn't mind either adding to the test suite, but I'm not sure how to do that.
Edit Lib/sqlite3/test/dump.py
msg147277 - (view) Author: Lucas Sinclair (xapple) Date: 2011-11-08 08:43
My patch contains two new dump tests which originally fail, and the fixes to the code that makes them pass.
msg152710 - (view) Author: Marko Kohtala (Marko.Kohtala) * Date: 2012-02-05 21:33
Here is finally an update to my patch modified according to comments received. It should apply on 2.7 and 3.3 branches.
msg152901 - (view) Author: Petri Lehtinen (petri.lehtinen) * (Python committer) Date: 2012-02-08 19:37
Marko's last patch looks good to me.
msg153088 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2012-02-11 03:59
Petri, I think you can go ahead.

Lucas, thanks for your help, even though your patch is not used.
msg153222 - (view) Author: Roundup Robot (python-dev) Date: 2012-02-12 19:09
New changeset 60aa7dacb605 by Petri Lehtinen in branch '2.7':
Fix sqlite3.Connection.iterdump on tables/fields with reserved names or quotes
http://hg.python.org/cpython/rev/60aa7dacb605

New changeset 4b105d328fe7 by Petri Lehtinen in branch '3.2':
Fix sqlite3.Connection.iterdump on tables/fields with reserved names or quotes
http://hg.python.org/cpython/rev/4b105d328fe7

New changeset 5d0f7b275fe9 by Petri Lehtinen in branch 'default':
Merge branch '3.2'
http://hg.python.org/cpython/rev/5d0f7b275fe9
msg163245 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-06-20 01:56
Note that the use of 'format' in the 2.7 patch caused a regression (% causes an implicit promotion to unicode, .format does not).  See issue 15109.
msg179560 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-01-10 15:34
For the record, this patch also introduced another regression (issue 15545).
msg179565 - (view) Author: Roundup Robot (python-dev) Date: 2013-01-10 16:31
New changeset 2cdb599172ab by R David Murray in branch '3.2':
#15545: fix sqlite3.iterdump regression on unsortable row_factory objects.
http://hg.python.org/cpython/rev/2cdb599172ab

New changeset 6a85894c428f by R David Murray in branch '3.3':
merge #15545: fix sqlite3.iterdump regression on unsortable row_factory objects.
http://hg.python.org/cpython/rev/6a85894c428f

New changeset 7a62b5ee32ec by R David Murray in branch 'default':
merge #15545: fix sqlite3.iterdump regression on unsortable row_factory objects.
http://hg.python.org/cpython/rev/7a62b5ee32ec

New changeset bb4e4f0cec2e by R David Murray in branch '2.7':
#15545: sort iterdump via SQL instead of in python code
http://hg.python.org/cpython/rev/bb4e4f0cec2e
History
Date User Action Args
2013-01-10 16:31:33python-devsetmessages: + msg179565
2013-01-10 15:34:56r.david.murraysetmessages: + msg179560
2012-06-20 01:56:36r.david.murraysetnosy: + r.david.murray
messages: + msg163245
2012-02-12 19:09:42python-devsetstatus: open -> closed

nosy: + python-dev
messages: + msg153222

resolution: fixed
stage: patch review -> resolved
2012-02-11 03:59:52eric.araujosetmessages: + msg153088
2012-02-08 19:37:43petri.lehtinensetmessages: + msg152901
2012-02-05 21:34:35Marko.Kohtalasetfiles: - sqlite3bug.py
2012-02-05 21:34:30Marko.Kohtalasetfiles: - sqlite3bug2.py
2012-02-05 21:34:06Marko.Kohtalasetfiles: - sqlite3dump.patch
2012-02-05 21:33:25Marko.Kohtalasetfiles: + sqlite3dump.patch

messages: + msg152710
2012-02-03 09:19:00petri.lehtinensetnosy: + petri.lehtinen
2011-11-19 13:58:18ezio.melottisetstage: patch review
2011-11-08 08:43:08xapplesetfiles: + issue9750.patch

messages: + msg147277
2011-10-31 16:41:23eric.araujosetmessages: + msg146710
2011-10-31 15:16:55xapplesetmessages: + msg146702
2011-10-25 15:24:14eric.araujosetmessages: + msg146374
versions: + Python 3.3, - Python 3.1
2011-10-25 12:00:50xapplesetnosy: + xapple
messages: + msg146362
2011-03-21 00:33:21eric.araujosetnosy: ghaering, eric.araujo, Marko.Kohtala
messages: + msg131582
2010-09-12 08:57:20Marko.Kohtalasetmessages: + msg116155
2010-09-12 08:30:00Marko.Kohtalasetfiles: - sqlite3ident.patch
2010-09-12 08:29:52Marko.Kohtalasetfiles: - sqlite3bug.patch
2010-09-12 01:07:11eric.araujosetnosy: + eric.araujo
messages: + msg116144
2010-09-03 12:18:31Marko.Kohtalasetfiles: + sqlite3dump.patch
2010-09-03 12:09:10Marko.Kohtalasetfiles: + sqlite3bug2.py

messages: + msg115445
2010-09-03 08:35:00Marko.Kohtalasetfiles: + sqlite3ident.patch

messages: + msg115423
2010-09-03 08:16:57ghaeringsetassignee: ghaering
2010-09-03 08:12:11ned.deilysetnosy: + ghaering

versions: + Python 3.2
2010-09-03 06:17:35Marko.Kohtalasetfiles: + sqlite3bug.patch
keywords: + patch
messages: + msg115421
2010-09-03 06:15:25Marko.Kohtalacreate