Fix #34746: Add a size limit for variable size when pretty printing exception report #17809
Conversation
keerthivasansa
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello! Thank you for your contribution 💪
As it's your first contribution be sure to check out the patch review checklist.
If you're fixing a ticket from Trac make sure to set the "Has patch" flag and include a link to this PR in the ticket!
If you have any design or process questions then you can ask in the Django forum.
Welcome aboard ⛵️!
|
Please let me know if we can continue with this approach I will add the limits for other data structures as well if so! |
Hello! Thank you for your contribution. I checked the proposed code and it seems to be in the right direction. In order for this PR to be listed as "needing review" in the Django Development Dashboard, please remember to set the proper Trac flags in the ticket as described in the PR checklist when this is ready for review. You may also want to assign the ticket to yourself! |
| factory = RequestFactory() | ||
|
|
||
| def test_large_sizable_object(self): | ||
| lg = list(range(50 * 1024 * 1024)) |
There was a problem hiding this comment.
I was just trying to check the efficiency of library's able to handle extreme sizes of objects and rendering them in a reasonable time. 😅What could be a more optimal value?
|
Hey @nessita, thanks for the go ahead! I am assuming it's best to just retain old behavior so I'm trying to work around that. Another thing I wanted to change was the trim message. isn't it better to say: |
django/views/debug.py
Outdated
|
|
||
| if isinstance(v, Sized) and len(v) > 4096: | ||
| diff = len(v) - 4096 | ||
| trim_msg = "...<trimmed %d bytes string>" % diff |
There was a problem hiding this comment.
I have changed it to show the length that was cut instead of the total length, I can change it back if the old behaviour (total count) is preferred.
There was a problem hiding this comment.
Did you consider having both? Something like:
f"...<trimmed to {cutoff} bytes, {reminder} bytes not shown>"
|
@felixxm I think the PR is ready for review! |
Hey @keerthivasansa, thank you again for your contribution! There is no need to mention people directly when a PR is ready for a re-review, reviewers will get to this when they have some availability. As I posted in my previous comment and in the ticket, the proper procedure to get a re-review is described in these docs. Let me know if you have any question! |
|
Gotcha, thanks! I have unset the "needs test" and "needs improvement" flags assuming that's what I have to do. |
Hello @keerthivasansa! I'm starting to review this today so I will provide answers/advice as I go thru it. |
There was a problem hiding this comment.
Thank you @keerthivasansa for this work! 🥳
Below an initial set of comments:
- I'm inclined to have the custom "repr" functionality inside the existing
textmodules inutils. - In any case, this PR should provide sufficient tests for the newly added functionality (
DjangoReprnew class). - I would rename
DjangoReprto something more explicit and specific such asDebugRepr,SizeLimitedRepr, or maybeSafeRepr. - What's your view on using this as well for the
debugtemplate tag?
| d = reporter.get_traceback_data() | ||
| vars = d["lastframe"]["vars"] | ||
|
|
||
| for k, v in vars: |
There was a problem hiding this comment.
Given that we don't (can't!) test Python, we assume Python and its libraries to be correct. So we can certainly use reprlib in the tests to assert our desired results.
What we should test is that our choosing of max limits and related are correct. In fact, we should most definitely provide tests for our custom Repr class. Once we ensure that our Repr class is correct, we could also use it to write these tests.
Does that make sense? (I can expand if not, no worries, let me know)
There was a problem hiding this comment.
Adding tests to check the custom class on common data structures maybe?
Co-authored-by: Paolo Melchiorre <paolo@melchiorre.org> Co-authored-by: nessita <124304+nessita@users.noreply.github.com>
| trimmed_dict = self.repr_instance.print(long_dict) | ||
| self.assertIn("trimmed 100 bytes string", trimmed_dict) | ||
|
|
||
|
|
There was a problem hiding this comment.
@nessita Are these the kind of tests you expected for testing this class? I can create more tests for data structures if so
|
I just thought of something else. If we use |
|
@keerthivasansa Thank you for your patience. We've been busy with the Django 5.1 Alpha release but I'll go back to this PR in a few weeks. In the meantime, could you please update your work with the latest changes from /remind me about this PR in 3 weeks |
|
@nessita set a reminder for 6/21/2024 |
|
👋 @nessita, about this PR |
|
@nessita Cool, i'll merge with the latest changes and fix the test cases now! |
|
@nessita The tests were failing because the implementation of |
Introduces a maximum variable size threshold (512KB) for pretty printing in exception reports, use the defaultpprintfor variables smaller than the thresholdUsesreprfor variables exceeding the size limitSignificantly reduces the time and memory used to create an exception report when a frame with a large variable raises an exception.[UPDATE]
sys.getsizeofcall is not accurate. If we need accurate information, we can use some of the third party libraries to deeply evaluate the value of an object but that becomes really slow, I think it would be best to switch entirely toreprlibwith indentation instead of pprint.