Skip to content

Fix SQL selected / SQL explain for gis queries #1426

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Oct 26, 2021

Conversation

jieter
Copy link
Contributor

@jieter jieter commented Dec 22, 2020

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
Copy link

codecov bot commented Dec 22, 2020

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.

Copy link
Member

@matthiask matthiask left a comment

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
Copy link
Contributor Author

jieter commented Dec 22, 2020

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
Member

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

@jieter
Copy link
Contributor Author

jieter commented Dec 23, 2020

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
Copy link
Contributor

@auvipy auvipy left a comment

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
Copy link
Contributor

auvipy commented Oct 26, 2021

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
Copy link
Contributor Author

jieter commented Oct 26, 2021

@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