Skip to content
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

'<' not supported between instances of 'int' and 'str' #1668

Closed
OdifYltsaeb opened this issue Aug 31, 2022 · 28 comments
Closed

'<' not supported between instances of 'int' and 'str' #1668

OdifYltsaeb opened this issue Aug 31, 2022 · 28 comments

Comments

@OdifYltsaeb
Copy link

OdifYltsaeb commented Aug 31, 2022

If session keys are not all strings, then this line (https://github.com/jazzband/django-debug-toolbar/blob/6a34e007e773d1243e3c7ba0b0ee07e4b45514ba/debug_toolbar/panels/request.py#L67) generate a TypeError:

TypeError at /myurl

'<' not supported between instances of 'int' and 'str'

because:

>>> l = ['a', 1, 'x', 22]
>>> sorted(l)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: '<' not supported between instances of 'int' and 'str'

@matthiask
Copy link
Member

How can session keys NOT all be strings? Are you using the PickleSerializer by chance? It is deprecated and will be removed in Django 5.0.

@OdifYltsaeb
Copy link
Author

Not sure. Going through someone else's code, and Yes I know that the Django session guidelines say they should be all strings.. but apparently, someone found a way to bypass it :)

So I don't know... perhaps it makes sense to ignore this report, because it appears someone is trying to use Django the way it is not supposed to be used.

@matthiask
Copy link
Member

Thank you!

django-debug-toolbar shouldn't crash anyway, but this makes it a normal priority issue for me. @tim-schilling Maybe something for the Djangocon sprints as well, if nobody gets to it earlier?

@tim-schilling
Copy link
Member

I'm thinking we sort the session by strings, then anything else goes in at a random order. I think trying to stringify the keys could have averse effects. For example, if someone put a model as the key for the session, it would then call the str function which could be accessing other properties, generating DB requests.

@matthiask
Copy link
Member

I think sorting is fine and helpful if it works. Otherwise we should just bail out and show keys and values in the order they appear in the dict.

@ritiksoni00
Copy link
Contributor

ritiksoni00 commented Sep 9, 2022

Hy, is anybody working on this issue? @tim-schilling if not then id like to work on it

@matthiask
Copy link
Member

@ritiksoni00 The djangocon US sprints will take place in October, see https://2022.djangocon.us/sprints/

AFAIK nobody is working on this issue currently. Feel free to jump in!

@ritiksoni00
Copy link
Contributor

Thanks, Matthias for pointing this out but I already got the virtual(😞) ticket for the conf.

for this task according to my understanding, I need to convert all the items into str right? or I understand it wrong.

@matthiask
Copy link
Member

My comment from above still applies :) I think the code should just bail out and skip sorting if it fails.

@ritiksoni00
Copy link
Contributor

ritiksoni00 commented Sep 9, 2022

i try to understand it and what tim says converting them into str can make DB queries for no reason. and according to your comment

i should just use try/except block for handling the error smt like this? Please correct me if I'm wrong.

try:
    (k, request.session.get(k))
    for k in sorted(request.session.keys())
except TypeError:
     pass

edit:1 or smt like this

try:
    (k, request.session.get(k))
    for k in sorted(request.session.keys())
except TypeError:
     (k, request.session.get(k))
     for k in request.session.keys()

@matthiask
Copy link
Member

Yes, something like this. Please also spend some time constructing a test so that we do not regress in this area.

@ritiksoni00
Copy link
Contributor

i tried to run the project on codespace and this is how my INTERNAL_IPS looks like

INTERNAL_IPS = ["127.0.0.1", "::1", 'https://ritiksoni00-django-debug-toolbar-5x5q59v6p5jcp4xw.github.dev' , 'https://ritiksoni00-django-debug-toolbar-5x5q59v6p5jcp4xw-8000.githubpreview.dev']

but it still fails and says :-

    Origin checking failed - https://ritiksoni00-django-debug-toolbar-5x5q59v6p5jcp4xw-8000.githubpreview.dev does not match any trusted origins.

what could be the issue? its in the /workspaces/django-debug-toolbar/example/settings.py

@tim-schilling
Copy link
Member

@ritiksoni00 I believe that's unrelated to the toolbar. That's part of Django's CSRF protection. This is likely the setting you need to change.

@ritiksoni00
Copy link
Contributor

        if hasattr(request, "session"):
            try:
                session_list = [(k, request.session.get(k))
                                for k in sorted(request.session.keys())]
            except TypeError:
                session_list = [(k, request.session.get(k))
                                for k in request.session.keys()]
            self.record_stats(
                {
                    "session": {
                        "list": session_list
                    }
                }
            )

and for the test I'm confused about how should i write it

def test_session_list_sorted_or_not(self):
        """
        Test varifies that session_list is sorted if all session key is a string, 
        otherwise, it remains unsorted

        See  https://github.com/jazzband/django-debug-toolbar/issues/1668
        """
        self.request.session = {'list': ['foo','bar', 1]}
        response = self.panel.process_request(self.request)
        self.panel.generate_stats(self.request, response)
        record_stats = self.panel.record_stats

Please guide me if the name for the test function is reasonable. is doc string good? and yeah as i said I don't have any idea how I should write test for that case.

@ritiksoni00
Copy link
Contributor

@tim-schilling i need a lil bit of guidance here. (sorry for the ping) but I don't know how will I proceed ahead from here

@tim-schilling
Copy link
Member

@ritiksoni00 a good test here is one that will break the code before the change and pass after the change succeeds. Since the original issue was that the session couldn't sort the keys because they weren't all strings, I'd suspect you'll need to change the session to be:

session = {
    1: "some value",
    'list': ['foo', 'bar', 1],
    (2, 3): "tuple key",
}

If using a session with the above structure breaks the code and then works with your changes, then you should be in the clear. Feel free to open a PR with your changes as well. It's easier for Matthias and I to provide feedback there than in an issue when it comes to actual code.

@tim-schilling
Copy link
Member

Rather than:

test varifies that session_list is sorted if all session key is a string, otherwise, it remains unsorted

Try:

Verify the session is sorted when all keys are strings.

But you'll need to include in the test both cases, one where all keys are strings and one where there's at least one key that's not (see previous comment).

@tim-schilling
Copy link
Member

Fixed by #1673

@Andras1000
Copy link

This issue seems to be happening again in version 4.4.6 and at least 4.4.2 and possibly earlier.

@matthiask
Copy link
Member

@Andras1000 A full traceback would be helpful!

@Andras1000
Copy link

Andras1000 commented Aug 21, 2024

@Andras1000 A full traceback would be helpful!

Traceback (most recent call last):
  File ".../venv/lib/python3.12/site-packages/debug_toolbar/panels/request.py", line 64, in generate_stats
    (k, request.session.get(k)) for k in sorted(request.session.keys())
                                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

During handling of the above exception ('<' not supported between instances of 'int' and 'str'), another exception occurred:
...

It seems I have some int in session.

EDIT: Rest of the traceback

During handling of the above exception ('<' not supported between instances of 'int' and 'str'), another exception occurred:
  File ".../venv/lib/python3.12/site-packages/django/core/handlers/exception.py", line 55, in inner
    response = get_response(request)
               ^^^^^^^^^^^^^^^^^^^^^
  File ".../venv/lib/python3.12/site-packages/sentry_sdk/integrations/django/middleware.py", line 169, in __call__
    return f(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^
  File ".../venv/lib/python3.12/site-packages/debug_toolbar/middleware.py", line 92, in __call__
    panel.generate_stats(request, response)
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".../venv/lib/python3.12/site-packages/debug_toolbar/panels/request.py", line 67, in generate_stats
    session_list = [(k, request.session.get(k)) for k in request.session]
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".../venv/lib/python3.12/site-packages/django/contrib/sessions/backends/base.py", line 53, in __getitem__
    return self._session[key]
           ^^^^^^^^^^^^^^^^^^

Exception Type: KeyError at /example/url/path

@matthiask
Copy link
Member

So that's expected; but, the TypeError which happens should be catched and the panel retries without sorting:
https://github.com/jazzband/django-debug-toolbar/blob/6fc5ce868da102b8d3206552925a513b2f26cb75/debug_toolbar/panels/request.py#L66-L67

So, I still don't really see what's going on here.

@matthiask
Copy link
Member

So, maybe you still didn't post the full traceback. The exception is to be expected when you have ints in the session, but the code should handle it just fine. If you have more to share, I'm happy to look at it again.

@Andras1000
Copy link

Andras1000 commented Aug 21, 2024

So, maybe you still didn't post the full traceback. The exception is to be expected when you have ints in the session, but the code should handle it just fine. If you have more to share, I'm happy to look at it again.

I've updated my comment to include the full traceback, #1668 (comment)

@matthiask
Copy link
Member

I wonder if the fallback is faulty.

Could you try the following?

[21.08. 15:49:40] ~/projects/django-debug-toolbar$ git diff
diff --git a/debug_toolbar/panels/request.py b/debug_toolbar/panels/request.py
index f9375b38..b9a3a51f 100644
--- a/debug_toolbar/panels/request.py
+++ b/debug_toolbar/panels/request.py
@@ -66,5 +66,5 @@ class RequestPanel(Panel):
                     (k, request.session.get(k)) for k in sorted(request.session.keys())
                 ]
             except TypeError:
-                session_list = [(k, request.session.get(k)) for k in request.session]
+                session_list = [(k, request.session.get(k)) for k in request.session.keys()]
             self.record_stats({"session": {"list": session_list}})
[21.08. 15:49:41] ~/projects/django-debug-toolbar$ 

matthiask added a commit to matthiask/django-debug-toolbar that referenced this issue Aug 21, 2024
@matthiask
Copy link
Member

@Andras1000 Here's a branch or pull request if that's easier for you to test: #1994

matthiask added a commit that referenced this issue Aug 21, 2024
* Refs #1668: Fixed the unsortable session keys fallback
* Disable the flake8-simplify ruleset
@Andras1000
Copy link

I wonder if the fallback is faulty.

Could you try the following?

[21.08. 15:49:40] ~/projects/django-debug-toolbar$ git diff
diff --git a/debug_toolbar/panels/request.py b/debug_toolbar/panels/request.py
index f9375b38..b9a3a51f 100644
--- a/debug_toolbar/panels/request.py
+++ b/debug_toolbar/panels/request.py
@@ -66,5 +66,5 @@ class RequestPanel(Panel):
                     (k, request.session.get(k)) for k in sorted(request.session.keys())
                 ]
             except TypeError:
-                session_list = [(k, request.session.get(k)) for k in request.session]
+                session_list = [(k, request.session.get(k)) for k in request.session.keys()]
             self.record_stats({"session": {"list": session_list}})
[21.08. 15:49:41] ~/projects/django-debug-toolbar$ 

This change makes the error go away for me.

@matthiask
Copy link
Member

Thanks for confirming!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants