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

Support order_by with None value #96

Closed
vuongdv-spinshell opened this issue Aug 12, 2022 · 6 comments · Fixed by #97
Closed

Support order_by with None value #96

vuongdv-spinshell opened this issue Aug 12, 2022 · 6 comments · Fixed by #97
Labels

Comments

@vuongdv-spinshell
Copy link
Contributor

I try to sort with an annotate field that possible None value. But look like django-querysetsequence doesn't suport it

Reproduce step:

room_options is a JSON field that contains branding 
room = Room.objects.all().annotate(brand=F('room_options__branding'))
ticket = Ticket.objects.all().annotate(brand=F('room_options__branding'))
room_ticket =  QuerySetSequence(room, ticket, model=Room).order_by('brand')
room_ticket[0]

=> TypeError: '>' not supported between instances of 'NoneType' and 'NoneType'

@clokep
Copy link
Owner

clokep commented Aug 16, 2022

Hmm, I wonder if this doesn't work for any nullable field? Definitely seems like an oversight, thanks for the bug report!

Do you have a fuller stack trace of the error? I suspect some special handling is needed near:

return cmp(value1, value2)

But I'm not 100% sure.

@clokep clokep added the bug label Aug 16, 2022
@vuongdv-spinshell
Copy link
Contributor Author

Yes, You're right. This is the full stack trace for it.

Environment:


Request Method: GET
Request URL: http://localhost:8000/api/calls2/?sort=brand

Django Version: 3.2.11
Python Version: 3.8.13
Installed Applications:
['django.contrib.admindocs',
 'server.apps.ServerConfig',
 'django.contrib.auth',
 'django.contrib.contenttypes',
 'django.contrib.sessions',
 'django.contrib.messages',
 'django.contrib.staticfiles',
 'rest_framework',
 'corsheaders',
 'zappa_django_utils',
 'pushevent.apps.PushEventConfig',
 'rest_framework.authtoken',
 'guardian',
 'django_filters',
 'drf_spectacular',
 'imagekit',
 'reservations',
 'questionnaires',
 'django.contrib.admin',
 'oauth2_provider',
 'social_django',
 'rest_framework_social_oauth2',
 'authentication.apps.AuthenticationConfig',
 'monotonic_counter.apps.MonotonicCounterConfig',
 'parler',
 'anymail',
 'ordered_model',
 'dynamo_cache',
 'drf_multiple_model',
 'django.contrib.humanize',
 'puml_generator',
 'debug_toolbar',
 'channels',
 'channels_websocket.apps.ChannelsWebsocketConfig']
Installed Middleware:
['debug_toolbar.middleware.DebugToolbarMiddleware',
 'django.middleware.security.SecurityMiddleware',
 'django.contrib.sessions.middleware.SessionMiddleware',
 'corsheaders.middleware.CorsMiddleware',
 'django.middleware.common.CommonMiddleware',
 'django.middleware.csrf.CsrfViewMiddleware',
 'django.contrib.auth.middleware.AuthenticationMiddleware',
 'django.contrib.messages.middleware.MessageMiddleware',
 'django.middleware.clickjacking.XFrameOptionsMiddleware',
 'server.permissions.StandardiseErrorMiddleware',
 'server.permissions.MissingTOSAgreementResponseMiddleware']



Traceback (most recent call last):
  File "/Users/vuongdo/Library/Caches/pypoetry/virtualenvs/livecall-5JrKXylz-py3.8/lib/python3.8/site-packages/django/core/handlers/exception.py", line 47, in inner
    response = get_response(request)
  File "/Users/vuongdo/Library/Caches/pypoetry/virtualenvs/livecall-5JrKXylz-py3.8/lib/python3.8/site-packages/django/core/handlers/base.py", line 181, in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
  File "/Users/vuongdo/Library/Caches/pypoetry/virtualenvs/livecall-5JrKXylz-py3.8/lib/python3.8/site-packages/django/views/decorators/csrf.py", line 54, in wrapped_view
    return view_func(*args, **kwargs)
  File "/Users/vuongdo/Library/Caches/pypoetry/virtualenvs/livecall-5JrKXylz-py3.8/lib/python3.8/site-packages/rest_framework/viewsets.py", line 125, in view
    return self.dispatch(request, *args, **kwargs)
  File "/Users/vuongdo/Library/Caches/pypoetry/virtualenvs/livecall-5JrKXylz-py3.8/lib/python3.8/site-packages/rest_framework/views.py", line 509, in dispatch
    response = self.handle_exception(exc)
  File "/Users/vuongdo/Library/Caches/pypoetry/virtualenvs/livecall-5JrKXylz-py3.8/lib/python3.8/site-packages/rest_framework/views.py", line 469, in handle_exception
    self.raise_uncaught_exception(exc)
  File "/Users/vuongdo/Library/Caches/pypoetry/virtualenvs/livecall-5JrKXylz-py3.8/lib/python3.8/site-packages/rest_framework/views.py", line 480, in raise_uncaught_exception
    raise exc
  File "/Users/vuongdo/Library/Caches/pypoetry/virtualenvs/livecall-5JrKXylz-py3.8/lib/python3.8/site-packages/rest_framework/views.py", line 506, in dispatch
    response = handler(request, *args, **kwargs)
  File "/Users/vuongdo/Library/Caches/pypoetry/virtualenvs/livecall-5JrKXylz-py3.8/lib/python3.8/site-packages/rest_framework/mixins.py", line 40, in list
    page = self.paginate_queryset(queryset)
  File "/Users/vuongdo/Library/Caches/pypoetry/virtualenvs/livecall-5JrKXylz-py3.8/lib/python3.8/site-packages/rest_framework/generics.py", line 171, in paginate_queryset
    return self.paginator.paginate_queryset(queryset, self.request, view=self)
  File "/Users/vuongdo/Library/Caches/pypoetry/virtualenvs/livecall-5JrKXylz-py3.8/lib/python3.8/site-packages/rest_framework/pagination.py", line 216, in paginate_queryset
    return list(self.page)
  File "/opt/homebrew/Cellar/[email protected]/3.8.13/Frameworks/Python.framework/Versions/3.8/lib/python3.8/_collections_abc.py", line 874, in __iter__
    v = self[i]
  File "/Users/vuongdo/Library/Caches/pypoetry/virtualenvs/livecall-5JrKXylz-py3.8/lib/python3.8/site-packages/django/core/paginator.py", line 188, in __getitem__
    self.object_list = list(self.object_list)
  File "/Users/vuongdo/Library/Caches/pypoetry/virtualenvs/livecall-5JrKXylz-py3.8/lib/python3.8/site-packages/queryset_sequence/__init__.py", line 549, in __iter__
    self._fetch_all()
  File "/Users/vuongdo/Library/Caches/pypoetry/virtualenvs/livecall-5JrKXylz-py3.8/lib/python3.8/site-packages/queryset_sequence/__init__.py", line 526, in _fetch_all
    self._result_cache = list(self._iterable_class(self))
  File "/Users/vuongdo/Library/Caches/pypoetry/virtualenvs/livecall-5JrKXylz-py3.8/lib/python3.8/site-packages/queryset_sequence/__init__.py", line 197, in _ordered_iterator
    iterables = sorted(iterables, key=comparator)
  File "/Users/vuongdo/Library/Caches/pypoetry/virtualenvs/livecall-5JrKXylz-py3.8/lib/python3.8/site-packages/queryset_sequence/__init__.py", line 181, in comparator
    return _comparator(tuple_1[2], tuple_2[2])
  File "/Users/vuongdo/Library/Caches/pypoetry/virtualenvs/livecall-5JrKXylz-py3.8/lib/python3.8/site-packages/queryset_sequence/__init__.py", line 136, in comparator
    return cls._cmp(v1, v2) * reverses[0]
  File "/Users/vuongdo/Library/Caches/pypoetry/virtualenvs/livecall-5JrKXylz-py3.8/lib/python3.8/site-packages/queryset_sequence/__init__.py", line 101, in _cmp
    return cmp(value1, value2)
  File "/Users/vuongdo/Library/Caches/pypoetry/virtualenvs/livecall-5JrKXylz-py3.8/lib/python3.8/site-packages/queryset_sequence/__init__.py", line 26, in cmp
    return (a > b) - (a < b)

Exception Type: TypeError at /api/calls2/
Exception Value: '>' not supported between instances of 'str' and 'NoneType'

@clokep
Copy link
Owner

clokep commented Aug 16, 2022

I think that code needs special handling for null, unfortunately not all databases order null the same way, but pretty much we would want to return 1 is value1 is None and -1 if value2 is None (or perhaps the opposite depending how you want nulls sorted). (And maybe 0 if they're both null.)

@vuongdv-spinshell
Copy link
Contributor Author

I'm currently using PostgreSQL, and look like your above-defined rule is the default of Postgres sorting with null. Please fixed it as you mention. Thank you.
For more improvements, you probably want to add nulls_last and nulls_first when you implement this update #92

@clokep
Copy link
Owner

clokep commented Aug 16, 2022

I'm currently using PostgreSQL, and look like your above-defined rule is the default of Postgres sorting with null.

It should work on everything except SQLite, I think.

Please fixed it as you mention. Thank you.

I unfortunately won't have time to implement this anytime soon, but would appreciate a pull request for it! Let me know if you have any questions!

For more improvements, you probably want to add nulls_last and nulls_first when you implement this update #92

Good catch! 👍

@vuongdv-spinshell
Copy link
Contributor Author

@clokep I created #97 pull request to fix this issue, please kindly check it. Thank you.

@clokep clokep linked a pull request Sep 9, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants