Skip to content

Fix SQL selected / SQL explain for gis queries - #1426

Merged
auvipy merged 3 commits into
django-commons:mainfrom
jieter:patch-1
Oct 26, 2021
Merged

Fix SQL selected / SQL explain for gis queries#1426
auvipy merged 3 commits into
django-commons:mainfrom
jieter:patch-1

Conversation

@jieter

@jieter jieter commented Dec 22, 2020

Copy link
Copy Markdown
Contributor

Without this fix, pushing the 'sel' or 'explain' button for a query containing some EWKB-encoded geometry
as parameter results in this crash:

Internal Server Error: /__debug__/sql_explain/
Traceback (most recent call last):
  File "/Users/jieter/.pyenv/versions/obs/lib/python3.8/site-packages/django/db/backends/utils.py", line 86, in _execute
    return self.cursor.execute(sql, params)
psycopg2.errors.InternalError_: parse error - invalid geometry
LINE 1: ...ure" IN (0, 1, 5)) AND "waarneming"."geo_point" @ 'ST_GeomFr...
                                                             ^
HINT:  "ST" <-- parse error at position 2 within geometry

I'm not sure if this is the appropriate location in the code, but with this fix, both sql_select and sql_explain
work without flaws.

Previous PR adding a similar fix: #1130
Fixes: #423

@codecov

codecov Bot commented Dec 22, 2020

Copy link
Copy Markdown

Codecov Report

Merging #1426 (740c872) into main (6fae2a9) will decrease coverage by 0.11%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1426      +/-   ##
==========================================
- Coverage   85.89%   85.78%   -0.12%     
==========================================
  Files          35       35              
  Lines        1893     1899       +6     
  Branches      276      279       +3     
==========================================
+ Hits         1626     1629       +3     
- Misses        187      190       +3     
  Partials       80       80              
Impacted Files Coverage Δ
debug_toolbar/panels/sql/tracking.py 88.80% <100.00%> (+0.56%) ⬆️
debug_toolbar/panels/profiling.py 86.60% <0.00%> (-2.68%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6fae2a9...740c872. Read the comment docs.

@matthiask matthiask left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This looks very hacky. I think I understand why this may be necessary but I fear the complexity a bit. Also, this change would definitely need a test to prevent regressions in the future.

@jieter

jieter commented Dec 22, 2020

Copy link
Copy Markdown
Contributor Author

Thanks! This looks very hacky. I think I understand why this may be necessary but I fear the complexity a bit. Also, this change would definitely need a test to prevent regressions in the future.

Yes, fully agreed! I only tried to make the existing solution a bit more readable by avoiding the numerical offsets.

I just looked into adding tests, which would require another test matrix entry with DB_BACKEND=postgis. Do you mind if I add that @matthiask?

@matthiask

Copy link
Copy Markdown
Member

@jieter Regarding DB_BACKEND=postgis: Not at all, I think that would be great.

@jieter

jieter commented Dec 23, 2020

Copy link
Copy Markdown
Contributor Author

I've added postgis to the tests (not yet added to github actions).

I also added a test, but this still crashes with the same error, and (for as far as I can see) the same input as captured while running debug-toolbar with the fix in my development environment. I'll try to revisit this later.

Base automatically changed from master to main February 11, 2021 15:01

@auvipy auvipy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would you mind fix the merge conflicts?

Without this fix, pushing the 'sel' or 'explain' button for a query containing some EWKB-encoded geometry 
as parameter results in this crash:
```
Internal Server Error: /__debug__/sql_explain/
Traceback (most recent call last):
  File "/Users/jieter/.pyenv/versions/obs/lib/python3.8/site-packages/django/db/backends/utils.py", line 86, in _execute
    return self.cursor.execute(sql, params)
psycopg2.errors.InternalError_: parse error - invalid geometry
LINE 1: ...ure" IN (0, 1, 5)) AND "waarneming"."geo_point" @ 'ST_GeomFr...
                                                             ^
HINT:  "ST" <-- parse error at position 2 within geometry
```

I'm not sure if this is the appropriate location in the code, but with this fix, both `sql_select` and `sql_explain` 
work without flaws.

Previous PR adding a similar fix: django-commons#1130
Fixes: django-commons#423
@auvipy
auvipy merged commit 857993c into django-commons:main Oct 26, 2021
@auvipy

auvipy commented Oct 26, 2021

Copy link
Copy Markdown
Contributor

thanks you! you can provide aditional tests if needed in another PR

@auvipy auvipy added the Bug label Oct 26, 2021
@jieter
jieter deleted the patch-1 branch October 26, 2021 06:53
@jieter

jieter commented Oct 26, 2021

Copy link
Copy Markdown
Contributor Author

@auvipy no problem!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Error debugging GeoDjango queries

4 participants