-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Binary parameter handling for GeoDjango #2169
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
base: main
Are you sure you want to change the base?
Conversation
for more information, see https://pre-commit.ci
…f docs/changes.rst.
debug_toolbar/panels/sql/forms.py
Outdated
def _get_query_params(self): | ||
"""Get reconstructed parameters for the current query""" | ||
query = self.cleaned_data["query"] | ||
return _reconstruct_params(json.loads(query["params"])) |
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.
Do you think this would be improved by making use of the json.loads(..., cls)
argument?
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.
If I understand you correctly, @tim-schilling, you're suggesting that we create our own DebugToolbarJSONDecoder and use it like this: json.loads(query["params"], cls=DebugToolbarJSONDecoder), so we can encapsulate all the base64 logic inside of _reconstruct_params
in custom JSON decoder class?
updates: - [github.com/astral-sh/ruff-pre-commit: v0.12.4 → v0.12.5](astral-sh/ruff-pre-commit@v0.12.4...v0.12.5) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
for more information, see https://pre-commit.ci
@tim-schilling I added some commits following what you were pointing out, let me know what do you think! |
Description
hey folks, this is my very first PR in the django-commons. I did this because I faced the issue myself in one project that I'm working on. I don't know how I can test this in that project. So I will wait for some of you to tell me how I can do that... but in the meantime this is the fix for the SQL Explain functionality for GeoDjango queries with binary parameters.
Fixes #423
SQL Explain feature throws a 500 error when used with GeoDjango queries containing binary parameters. This affects any query using PostGIS functions like ST_GeomFromEWKB(), ST_Distance_Sphere(), etc.
I'm trying to create a robust binary parameter handling system:
Checklist:
docs/changes.rst
.