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

fix: Do not try to call an ordering object's order method if it is not a decorated method #584

Merged
merged 1 commit into from
Jul 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions strawberry_django/ordering.py
Original file line number Diff line number Diff line change
Expand Up @@ -108,8 +108,10 @@ def process_order(
sequence = sequence or {}
args = []

if not skip_object_order_method and (order_method := getattr(order, "order", None)):
assert isinstance(order_method, FilterOrderFieldResolver)
if not skip_object_order_method and isinstance(
order_method := getattr(order, "order", None),
bellini666 marked this conversation as resolved.
Show resolved Hide resolved
FilterOrderFieldResolver,
):
return order_method(
order, info, queryset=queryset, prefix=prefix, sequence=sequence
)
Expand Down
29 changes: 29 additions & 0 deletions tests/test_ordering.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
# ruff: noqa: TRY002, B904, BLE001, F811, PT012
from typing import Any, List, Optional, cast
from unittest import mock

import pytest
import strawberry
from django.db.models import Case, Count, Value, When
from pytest_mock import MockFixture
from strawberry import auto
from strawberry.annotation import StrawberryAnnotation
from strawberry.exceptions import MissingArgumentsAnnotationsError
Expand Down Expand Up @@ -331,6 +333,33 @@ def custom_order(self, root, info, prefix, value: auto, sequence, queryset):
process_order(_order, _info, _queryset, prefix="ROOT", sequence=_sequence)


def test_order_method_not_called_when_not_decorated(mocker: MockFixture):
bellini666 marked this conversation as resolved.
Show resolved Hide resolved
@strawberry_django.ordering.order(models.Fruit)
class Order:
def order(self, root, info, prefix, value: auto, sequence, queryset):
pytest.fail("Should not have been called")

mock_order_method = mocker.spy(Order, "order")

process_order(
bellini666 marked this conversation as resolved.
Show resolved Hide resolved
cast(WithStrawberryObjectDefinition, Order()), mock.Mock(), mock.Mock()
)

mock_order_method.assert_not_called()


def test_order_field_not_called(mocker: MockFixture):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (testing): Test name could be more descriptive.

Consider renaming the test to something like test_order_field_not_called_as_method to make it clearer that the test is ensuring the field is not called as a method.

Suggested change
def test_order_field_not_called(mocker: MockFixture):
def test_order_field_not_called_as_method(mocker: MockFixture):

@strawberry_django.ordering.order(models.Fruit)
class Order:
order: Ordering = Ordering.ASC

# Calling this and no error being raised is the test, as the wrong behavior would
# be for the field to be called like a method
bellini666 marked this conversation as resolved.
Show resolved Hide resolved
process_order(
cast(WithStrawberryObjectDefinition, Order()), mock.Mock(), mock.Mock()
)


def test_order_object_method():
@strawberry_django.ordering.order(models.Fruit)
class Order:
Expand Down
Loading