Skip to content

feat: handle request.GET or request.POST being None + a few other improvements @W-18128971 #173

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

Merged
merged 2 commits into from
Apr 8, 2025
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
97 changes: 73 additions & 24 deletions django_declarative_apis/machinery/attributes.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,13 @@

from django.db import models as django_models
import pydantic
import logging

from . import errors
from . import tasks

logger = logging.getLogger(__name__)


class EndpointAttribute(metaclass=abc.ABCMeta):
_next_attribute_number = 0
Expand Down Expand Up @@ -124,23 +127,54 @@ def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)

def coerce_value_to_type(self, raw_value):
"""Coerce a raw value to the expected field type.

Args:
raw_value: The value to coerce

Returns:
The coerced value of the expected type

Raises:
ClientErrorInvalidFieldValues: If the value cannot be coerced to the expected type
"""
# handle tricksy quickly right off the bat
if raw_value is None:
return None

try:
if self.field_type == bool and not isinstance(raw_value, self.field_type):
return "rue" in raw_value
elif issubclass(self.field_type, pydantic.BaseModel):
if self.field_type == bool:
if isinstance(raw_value, bool):
return raw_value
if isinstance(raw_value, str):
return raw_value.lower() in ("true", "1", "yes", "on")
# handle ints and floats too
if isinstance(raw_value, (int, float)):
return bool(raw_value)
raise ValueError(f"Cannot convert {raw_value} to boolean")

if issubclass(self.field_type, pydantic.BaseModel):
return self.field_type.parse_obj(raw_value)
else:
if isinstance(raw_value, collections.abc.Iterable) and not isinstance(
raw_value, (str, dict)
):
return list(self.field_type(r) for r in raw_value)
else:
return self.field_type(raw_value)
except Exception as e: # noqa

if isinstance(raw_value, collections.abc.Iterable) and not isinstance(
raw_value, (str, dict)
):
return list(self.field_type(r) for r in raw_value)

return self.field_type(raw_value)
except Exception as e:
name = self.name or self.api_name
logger.info(
'ev=dda, loc=coerce_value_to_type, name=%s, raw="%s", error="%s"',
name,
raw_value,
e,
)
raise errors.ClientErrorInvalidFieldValues(
[self.name],
[name],
"Could not parse {val} as type {type}".format(
val=raw_value, type=self.field_type.__name__
val=raw_value,
type=self.field_type.__name__,
),
)

Expand Down Expand Up @@ -288,31 +322,46 @@ def documentation(self):
return result

def get_without_default(self, owner_instance, request):
if request.method == "POST":
query_dict = request.POST
else:
query_dict = request.GET
"""Get the field value from the request without applying default value.

Args:
owner_instance: The instance of the endpoint definition
request: The Django request object

Returns:
The coerced value of the field, or None if not found
"""
if not request:
return None

query_dict = request.POST if request.method == "POST" else request.GET
# handle errors during MIME type translation or data deserialization
if not query_dict:
return None

if (self.api_name or self.name) in query_dict:
field_name = self.api_name or self.name
if not field_name:
return None

if field_name in query_dict:
if not self.multivalued:
raw_value = query_dict.get(self.api_name or self.name)
raw_value = query_dict.get(field_name)
else:
raw_value = query_dict.getlist(self.api_name or self.name)
raw_value = query_dict.getlist(field_name)
typed_value = self.coerce_value_to_type(raw_value)
else:
typed_value = None

if self.post_processor:
return self.post_processor(owner_instance, typed_value)
else:
return typed_value

return typed_value

def get_field(self, owner_instance, request):
raw_value = self.get_without_default(owner_instance, request)
if raw_value is not None:
return raw_value
else:
return self.default_value
return self.default_value


class ResourceField(RequestField):
Expand Down
7 changes: 5 additions & 2 deletions django_declarative_apis/resources/resource.py
Original file line number Diff line number Diff line change
Expand Up @@ -165,8 +165,11 @@ def authenticate(self, request, rm): # noqa: C901
if auth_header.startswith(hint.header):
potential_authenticators.extend(authenticators)
continue
except KeyError:
logger.exception("ev=dda_resource method=authenticate state=KeyError")
except KeyError as ke:
logger.info(
"ev=dda loc=resource method=authenticate state=no_auth_header key_error=%s",
ke,
)
pass

try:
Expand Down
100 changes: 100 additions & 0 deletions tests/machinery/test_attributes.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
import unittest
from django.test import RequestFactory
from django.http import QueryDict
from django_declarative_apis import machinery
from django_declarative_apis.machinery import errors


class TestRequestField(unittest.TestCase):
def setUp(self):
self.request_factory = RequestFactory()

def test_get_without_default_none_request(self):
field = machinery.field(type=str)
actual = field.get_without_default(None, None)
self.assertIsNone(actual)

def test_get_without_default_empty_query_dict(self):
request = self.request_factory.get("/")
request.GET = QueryDict()
field = machinery.field(type=str)
actual = field.get_without_default(None, request)
self.assertIsNone(actual)

Copy link
Collaborator

Choose a reason for hiding this comment

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

we could maybe add a test with request with query_dict but without name or api_name

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that would fail in a different way because the there's a meta class that has to fill in that name and in the tests it would need to be mocked

def test_get_without_default_get_request(self):
field = machinery.field(type=str, name="test_field")
expected = "test_value"
request = self.request_factory.get(f"/?test_field={expected}")
actual = field.get_without_default(None, request)
self.assertEqual(actual, expected)

def test_get_without_default_post_request(self):
spaghetti_code_field = machinery.field(type=str, name="spaghetti_code_field")
spaghetti_code_value = "italiano"
request = self.request_factory.post(
"/",
{"test_field": "test_value", "spaghetti_code_field": spaghetti_code_value},
)
actual = spaghetti_code_field.get_without_default(None, request)
self.assertEqual(actual, spaghetti_code_value)

def test_get_without_default_multivalued(self):
name = "vibe"
field = machinery.field(type=str, multivalued=True, name=name)
v1 = "cool"
v2 = "good"
request = self.request_factory.get(f"/?{name}={v1}&{name}={v2}")
actual = field.get_without_default(None, request)
self.assertEqual(actual, [v1, v2])

def test_get_without_default_different_types(self):
int_field = machinery.field(type=int, name="age")
expected = 5
request = self.request_factory.get(f"/?age={expected}")
actual = int_field.get_without_default(None, request)
self.assertEqual(actual, expected)

# boolean type
bool_field = machinery.field(type=bool, name="is_cool")
request = self.request_factory.get("/?is_cool=true")
actual = bool_field.get_without_default(None, request)
self.assertTrue(actual)

# float type
float_field = machinery.field(type=float, name="pi")
short_pi = 3.14
request = self.request_factory.get(f"/?pi={short_pi}")
actual = float_field.get_without_default(None, request)
self.assertEqual(actual, short_pi)

def test_get_without_default_invalid_type(self):
name = "chad"
antagonistical_value = "chad_is_not_an_int"
int_field = machinery.field(type=int, name=name)
request = self.request_factory.get(f"/?{name}={antagonistical_value}")

with self.assertRaises(errors.ClientErrorInvalidFieldValues) as cm:
int_field.get_without_default(None, request)

self.assertIn(
f"Could not parse {antagonistical_value} as type int", str(cm.exception)
Copy link
Collaborator

Choose a reason for hiding this comment

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

we could also check for the presence of new log ev=dda, loc=coerce_value_to_type

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't see much benefit from asserting the log line is there, also makes the logging updates difficult after (tests would need update too)

)

def test_get_without_default_with_post_processor(self):
def post_processor(owner_instance, value):
return f"spicy_{value}"

field = machinery.field(type=str, name="bird")
field_value = "duck"
expected = post_processor(None, field_value)
field.post_processor = post_processor

request = self.request_factory.get(f"/?bird={field_value}")
actual = field.get_without_default(None, request)
self.assertEqual(actual, expected)

def test_get_without_default_field_not_present(self):
field = machinery.field(type=str, name="nemo")
request = self.request_factory.get("/?dory=keeps_on_swimming")
actual = field.get_without_default(None, request)
self.assertIsNone(actual)