-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
There was a problem hiding this 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.
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 |
@jieter Regarding |
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. |
There was a problem hiding this 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
for more information, see https://pre-commit.ci
thanks you! you can provide aditional tests if needed in another PR |
@auvipy no problem! |
Without this fix, pushing the 'sel' or 'explain' button for a query containing some EWKB-encoded geometry
as parameter results in this crash:
I'm not sure if this is the appropriate location in the code, but with this fix, both
sql_select
andsql_explain
work without flaws.
Previous PR adding a similar fix: #1130
Fixes: #423