Request Line is too large (400) sometimes #347

Merged
merged 12 commits into from Apr 24, 2013

Conversation

Projects
None yet
4 participants
Contributor

nuklea commented Dec 27, 2012

  • Request Line is too large now excempted, cuz using POST
  • Template now is more simple

usefull pull request

Owner

jezdez commented Mar 2, 2013

This seems related to #357 to be honest, I'm closing this here since it doesn't merge cleanly anyway.

@jezdez jezdez closed this Mar 2, 2013

Contributor

nuklea commented Mar 5, 2013

@jezdez but my pull request has easer template, than #357!

Owner

jezdez commented Mar 5, 2013

@midiotthimble Feel free to collaborate with the author of #357 then :)

Contributor

nuklea commented Mar 5, 2013

@jezdez just don't understand why #357 better =( I can improve my code, if need.

Owner

jezdez commented Mar 5, 2013

@midiotthimble I assumed that while both pull requests tackle the same problem that it would be worthwhile to just work on one version. It wasn't a judgement on the quality. You have my attention now though. Can you explain why you think yours is better?

@jezdez jezdez reopened this Mar 5, 2013

Contributor

nuklea commented Mar 5, 2013

@jezdez

  • More simple template (form uses)
  • Faster (has no urlencode filters)
  • JS already minified
  • Don't understand how #357 works without csrf_exempt
Contributor

nuklea commented Apr 18, 2013

@jezdez what I can do for approval this pull request?

dkrnl commented Apr 22, 2013

@jezdez This pull request works fine. In my project I have very long queries and now I can't debug that. Please, merge this pull request.

Owner

jezdez commented Apr 22, 2013

@midiotthimble Not quite yet, lemme make some comments on the code.

debug_toolbar/views.py
- sql = request.GET.get('sql', '')
- params = request.GET.get('params', '')
- alias = request.GET.get('alias', 'default')
+ sql = request.REQUEST.get('sql', '')
@jezdez

jezdez Apr 22, 2013

Owner

As far as I understand it's not required anymore to accept those parameters via GET, could you please change to using request.POST instead?

- <a class="remoteCall" href="/__debug__/sql_profile/?sql={{ query.raw_sql|urlencode }}&amp;params={{ query.params|urlencode }}&amp;duration={{ query.duration|floatformat:"2"|urlencode }}&amp;hash={{ query.hash }}&amp;alias={{ query.alias|urlencode }}">Prof</a>
- {% endifequal %}
+
+ <form method="post">
@jezdez

jezdez Apr 22, 2013

Owner

Please stay consistent with tabs (since the rest of the file uses them).

- {% endifequal %}
+
+ <form method="post">
+ <input type="hidden" name="sql" value="{{ query.raw_sql }}" />
@jezdez

jezdez Apr 22, 2013

Owner

What would you think about using a Django form for rendering those hidden fields? That would make the view code much cleaner and the rendering more robust.

Contributor

nuklea commented Apr 23, 2013

@jezdez complete!

debug_toolbar/forms.py
+ return hash
+
+ def reformat_sql(self):
+ from debug_toolbar.panels.sql import reformat_sql
@jezdez

jezdez Apr 23, 2013

Owner

Let's move this to a third module, e.g. debug_toolbar.utils or something match.

@nuklea

nuklea Apr 23, 2013

Contributor

Move what? reformat_sql function?

@nuklea

nuklea Apr 23, 2013

Contributor

@jezdez please, be exactly :)

@jezdez

jezdez Apr 23, 2013

Owner

Yeah, the reformat_sql function so that the import can be moved at the top of the file. I assume you put the import here to prevent a circular import.

- <a class="remoteCall" href="/__debug__/sql_profile/?sql={{ query.raw_sql|urlencode }}&amp;params={{ query.params|urlencode }}&amp;duration={{ query.duration|floatformat:"2"|urlencode }}&amp;hash={{ query.hash }}&amp;alias={{ query.alias|urlencode }}">Prof</a>
- {% endifequal %}
+ <form method="post">
+ {% for field in query.form.hidden_fields %}
@jezdez

jezdez Apr 23, 2013

Owner

Why only the hidden fields? I think {{ query.form }} should suffice.

Owner

jezdez commented Apr 23, 2013

@midiotthimble Looks great, thank you!

Contributor

nuklea commented Apr 24, 2013

@jezdez circular import was resolved.

Owner

jezdez commented Apr 24, 2013

Hm, getting lots of merge conflicts when pull, can you update the pull request with the recent master please?

Merge branch 'master' of https://github.com/django-debug-toolbar/djan…
…go-debug-toolbar into sql-panel-refactor

Conflicts:
	debug_toolbar/static/debug_toolbar/css/toolbar.min.css
	debug_toolbar/views.py
Contributor

nuklea commented Apr 24, 2013

@jezdez ready!

Contributor

nuklea commented Apr 24, 2013

Don't know how to fix Django 1.5 tests.

Owner

jezdez commented Apr 24, 2013

Okay, it was an issue with the tests_require setting in setup.py that required Django 1.3-1.5 but not 1.5.x. Can you merge with master again and see if that fixes it?

Owner

jezdez commented Apr 24, 2013

Oh and definitely add yourself to the AUTHORS file!

Contributor

nuklea commented Apr 24, 2013

I'm not sure that it's my guilty.

Owner

jezdez commented Apr 24, 2013

Awesome, the failing test was already failing in master!

jezdez added a commit that referenced this pull request Apr 24, 2013

Merge pull request #347 from midiotthimble/sql-panel-refactor
Request Line is too large (400) sometimes

@jezdez jezdez merged commit 1390c40 into jazzband:master Apr 24, 2013

1 check failed

default The Travis build failed
Details
Owner

jezdez commented Apr 24, 2013

Thanks for you help @midiotthimble :)

ryneeverett pushed a commit to ryneeverett/django-debug-toolbar that referenced this pull request Oct 2, 2016

Merge pull request #347 from midiotthimble/sql-panel-refactor
Request Line is too large (400) sometimes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment