db.dropcolumn: Circumvent length limit for sqlite3 SQL STDIN string by mmacata · Pull Request #3273 · OSGeo/grass

mmacata · 2023-11-29T17:43:25Z

This PR supports a more recent sqlite3 command to drop a column within db.dropcolumn.
In sqlite3 3.35.0, the DROP COLUMN command was added. This PR makes use of the command if the sqlite3 version is high enough.

Background is a failing db.in.ogr command for a CSV file containing many columns:

GRASS cttest/PERMANENT:~ > db.in.ogr input=migr_emi2_L0.csv output=migr_emi2_L0
Traceback (most recent call last):
  File "/usr/local/grass83/scripts/db.in.ogr", line 189, in <module>
    main()
  File "/usr/local/grass83/scripts/db.in.ogr", line 172, in main
    grass.run_command(
  File "/usr/local/grass83/etc/python/grass/script/core.py", line 466, in run_command
    return handle_errors(returncode, result=None, args=args, kwargs=kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/local/grass83/etc/python/grass/script/core.py", line 345, in handle_errors
    raise CalledModuleError(module=module, code=code, returncode=returncode)
grass.exceptions.CalledModuleError: Module run `db.dropcolumn --q -f table=migr_emi2_L0 column=cat` ended with an error.
The subprocess ended with a non-zero return code: 1. See errors above the traceback or in the error output.

And with further debugging:

GRASS cttest/PERMANENT:~ > db.dropcolumn --q -f table=migr_emi2_L0 column=cat
WARNING: Deleting <cat> column which may be needed to keep table connected
         to a vector map
DBMI-SQLite driver error:
Error in sqlite3_prepare():
incomplete input

DBMI-SQLite driver error:
Error in sqlite3_prepare():
incomplete input

ERROR: Error while executing: 'CREATE TEMPORARY TABLE
       migr_emi2_L0_backup(NUTS_ID TEXT, TOTAL_COMPLET_NR_F_2021 DOUBLE
       PRECISION, TOTAL_COMPLET_NR_F_2020 DOUBLE PRECISION,
       TOTAL_COMPLET_NR_F_2019 DOUBLE PRECISION, TOTAL_COMPLET_NR_F_2018
       DOUBLE PRECISION, TOTAL_COMPLET_NR_F_2017 DOUBLE PRECISION,
...    
       Y43_REACH_NR_T_2017 DOUBLE PRECISION, Y43_REACH_NR_T_2016 DOUBLE
       PRECISION, Y43_REACH_NR_T_2015 DOUBLE PRECISION,
       Y44_COMPLET_NR_F_2021 DOUBLE PRECISION, Y4'
ERROR: Cannot continue (problem deleting column)

So the created SQL string becomes too long to be passed via stdin in "db.execute", input="-", database=database, driver=driver, stdin=sql command. This PR circumvents this by using the newer sqlite3 syntax if available.

mmacata · 2023-11-29T17:46:45Z

(Backport to 8.3 would be nice - I am not allowed to add labels)

scripts/db.dropcolumn/db.dropcolumn.py

wenzeslaus

This is really great upgrade!

wenzeslaus · 2023-11-30T23:02:46Z

scripts/db.dropcolumn/db.dropcolumn.py

+            driver=driver,
+        )
+
+        if sqlite3_version >= "3.35.0":


Testing versions is a pain:

>>> "10.0.0" >= "3.35.0" False >>> [int(i) for i in "10.0.0".split(".")] >= [int(i) for i in "3.35.0".split(".")] True

but:

>>> [int(i) for i in "4.0.0-dev".split(".")] >= [int(i) for i in "3.35.0".split(".")] Traceback (most recent call last): File "<stdin>", line 1, in <module> File "<stdin>", line 1, in <listcomp> ValueError: invalid literal for int() with base 10: '0-dev'

Testing only major and minor version (3.35) should be enough and skip the patch/minor part which is not really relevant to this.

Now comparing major and minor versions as arrays of INTs in recent commit.

echoix · 2023-12-01T00:29:41Z

In a little concerned about the old and new code in there. I never did SQL in Python, but if a code equivalent to that was done in C# and PHP, what looks like concatenation of strings to create a SQL query, would be vulnerable to SQL injection.

I would expect that placeholders would be used and the parameters would be binded then executed.

Co-authored-by: Vaclav Petras <[email protected]>

mmacata · 2023-12-01T13:41:22Z

In a little concerned about the old and new code in there. I never did SQL in Python, but if a code equivalent to that was done in C# and PHP, what looks like concatenation of strings to create a SQL query, would be vulnerable to SQL injection.

I would expect that placeholders would be used and the parameters would be binded then executed.

How could this look like? I read in some stackoverflow posts (no documentation quote at hand) that binding table or column names is not possible (for sqlite3), only values.
To secure this I can only think of adding the following, maybe even before the column check:

     if table not in gscript.read_command("db.tables"):
        gscript.fatal(_("Table <%s> not found in db.tables") % column)

but I am not sure if this is how db.dropcolumn should behave.

wenzeslaus · 2023-12-01T14:22:03Z

scripts/db.dropcolumn/db.dropcolumn.py

+            flags="c",
+            database=database,
+            driver=driver,
+        ).split(".")[0:2]


See maxsplit parameter which avoids the need for indexing later.

I don't see the option to select only the first items when using maxsplit, or am I overseeing something? It will split until the given index but then adding all following into one item. This will cause the problem you described above:

>>> sqlite3_version = "4.0.0-dev" >>> sqlite3_version.split('.',1) ['4', '0.0-dev'] >>> [int(i) for i in sqlite3_version.split('.',1)] Traceback (most recent call last): File "<stdin>", line 1, in <module> File "<stdin>", line 1, in <listcomp> ValueError: invalid literal for int() with base 10: '0.0-dev' >>> sqlite3_version.split('.',2) ['4', '0', '0-dev'] >>> [int(i) for i in sqlite3_version.split('.',2)] Traceback (most recent call last): File "<stdin>", line 1, in <module> File "<stdin>", line 1, in <listcomp> ValueError: invalid literal for int() with base 10: '0-dev'

@mmacata You're right, maxsplit might be used to limit the resulting list to three parts, eg:

>>> sqlite3_version = "4.0.0-dev" >>> sqlite3_version.split('.', maxsplit=3)[:2] ['4', '0']

but the index is needed. Although that seems to me to be over doing things. Index should be enough.

@wenzeslaus do you also agee? Can this be merged as is?

nilason · 2023-12-01T18:00:05Z

In a little concerned about the old and new code in there. I never did SQL in Python, but if a code equivalent to that was done in C# and PHP, what looks like concatenation of strings to create a SQL query, would be vulnerable to SQL injection.
I would expect that placeholders would be used and the parameters would be binded then executed.

How could this look like? I read in some stackoverflow posts (no documentation quote at hand) that binding table or column names is not possible (for sqlite3), only values. To secure this I can only think of adding the following, maybe even before the column check:
     if table not in gscript.read_command("db.tables"):
        gscript.fatal(_("Table <%s> not found in db.tables") % column)
but I am not sure if this is how db.dropcolumn should behave.

I think this question on sql safety is out of scope for this PR.

nilason

Looks good to me!

neteler · 2023-12-14T15:43:45Z

I'll merge this if no objections.

…3273) * db.dropcolumn: support more recent sqlite3 drop column command * use sqlite_version() * Update scripts/db.dropcolumn/db.dropcolumn.py * enhance version comparison Co-authored-by: Vaclav Petras <[email protected]>

…SGeo#3273) * db.dropcolumn: support more recent sqlite3 drop column command * use sqlite_version() * Update scripts/db.dropcolumn/db.dropcolumn.py * enhance version comparison Co-authored-by: Vaclav Petras <[email protected]>

db.dropcolumn: support more recent sqlite3 drop column command

68f025b

nilason added bug Something isn't working Python Related code is in Python database Related to database management backport to 8.3 labels Nov 29, 2023

nilason added this to the 8.4.0 milestone Nov 29, 2023

neteler reviewed Nov 29, 2023

View reviewed changes

scripts/db.dropcolumn/db.dropcolumn.py Outdated Show resolved Hide resolved

use sqlite_version()

160389c

wenzeslaus reviewed Nov 30, 2023

View reviewed changes

scripts/db.dropcolumn/db.dropcolumn.py Outdated Show resolved Hide resolved

wenzeslaus reviewed Nov 30, 2023

View reviewed changes

mmacata and others added 2 commits December 1, 2023 13:09

Update scripts/db.dropcolumn/db.dropcolumn.py

cd8c349

Co-authored-by: Vaclav Petras <[email protected]>

enhance version comparison

2f2715d

wenzeslaus reviewed Dec 1, 2023

View reviewed changes

griembauer mentioned this pull request Dec 4, 2023

v.db.join: speed up processing by using fewer db.execute commands #3286

Merged

nilason approved these changes Dec 5, 2023

View reviewed changes

neteler merged commit 045f43b into OSGeo:main Dec 15, 2023

neteler removed the backport to 8.3 label Dec 15, 2023

neteler modified the milestones: 8.4.0, 8.3.2 Dec 15, 2023

Uh oh!

Comments

Conversation

mmacata commented Nov 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mmacata commented Nov 29, 2023

Uh oh!

Uh oh!

Uh oh!

wenzeslaus left a comment

Choose a reason for hiding this comment

Uh oh!

wenzeslaus Nov 30, 2023

Choose a reason for hiding this comment

Uh oh!

nilason Dec 1, 2023

Choose a reason for hiding this comment

Uh oh!

mmacata Dec 1, 2023

Choose a reason for hiding this comment

Uh oh!

echoix commented Dec 1, 2023

Uh oh!

mmacata commented Dec 1, 2023

Uh oh!

wenzeslaus Dec 1, 2023

Choose a reason for hiding this comment

Uh oh!

mmacata Dec 5, 2023

Choose a reason for hiding this comment

Uh oh!

nilason Dec 5, 2023

Choose a reason for hiding this comment

Uh oh!

mmacata Dec 7, 2023

Choose a reason for hiding this comment

Uh oh!

nilason commented Dec 1, 2023

Uh oh!

nilason left a comment

Choose a reason for hiding this comment

Uh oh!

neteler commented Dec 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mmacata commented Nov 29, 2023 •

edited

Loading