-
-
Notifications
You must be signed in to change notification settings - Fork 980
feat(request): new method get_param_as_dict #2544
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
base: master
Are you sure you want to change the base?
Changes from all commits
f945b51
b3c4129
48677fc
28a57cc
1f85fb3
5782d5c
bae1d32
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| New method :func:`falcon.Request.get_param_as_dict` has been added |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2395,6 +2395,90 @@ def get_param_as_json( | |
|
|
||
| return val | ||
|
|
||
| def get_param_as_dict( | ||
| self, | ||
| name: str, | ||
| required: bool = False, | ||
| deep_object: bool = False, | ||
| store: StoreArg = None, | ||
| default: Any | None = None, | ||
| ) -> Any: | ||
| """Retrieve a query parameter as a dictionary. | ||
|
|
||
| Supports OpenAPI's deep object style (`param[key]=value`) | ||
| and list-based key/value pairs (e.g., `param=k1,v1,k2,v2`). | ||
|
|
||
| Args: | ||
| name (str): Parameter name, case-sensitive (e.g, 'sort') | ||
|
|
||
| Keyword Args: | ||
| required (bool): Set to ``True`` to raise ``HTTPBadRequest`` | ||
| instead of returning ``None`` when the parameter is not found | ||
| (default ``False``). | ||
| deep_object (bool): Set to True to use the deepObject (as in OAS) | ||
| format. | ||
| store: A ``dict``-like object in which to place the value | ||
| of the param, but only if the param is found (default ``None``). | ||
| default (any): If the param is not found returns the | ||
| given value instead of ``None`` | ||
|
|
||
| Returns: | ||
| dict: The value of the param if it is found. Otherwise, returns | ||
| ``None`` unless required is ``True``. | ||
|
|
||
| Raises: | ||
| HTTPBadRequest: A required param is missing from the request, or | ||
| the value could not be parsed from the parameter. | ||
|
|
||
| .. versionadded:: 4.1.0 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This needs to be the current development version, Falcon 4.2.0, not 4.1.0 (which is already released). |
||
| """ | ||
|
|
||
| output: dict[str, Any] | None = None | ||
|
|
||
| if deep_object: | ||
| oc: dict[str, Any] = {} | ||
| for key, value in self._params.items(): | ||
| if not key.startswith(f'{name}[') and key.endswith(']'): | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess we could optimize this a bit by caching the format, but it's likely not that important
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another thing to try would be to benchmark this flow vs using a regex, but I doubt a regex would be faster. |
||
| continue | ||
| inner = key[len(name) + 1 : -1] | ||
|
|
||
| if isinstance(value, (list, tuple)): | ||
| oc[inner] = value[0] if value else None | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not sure if we want to use
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Q: does this case ever happen (empty list)?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. probably not in actual requests... left check in to be safe but can take out
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. from my point of view we could keep this check just to be paranoid, but I would use |
||
| else: | ||
| oc[inner] = value | ||
|
|
||
| if not oc: | ||
| if required: | ||
| msg = 'Missing deep object parameter' | ||
| raise errors.HTTPMissingParam(msg) | ||
| output = default | ||
| else: | ||
| output = oc | ||
|
|
||
| else: | ||
| try: | ||
| values_list = self.get_param_as_list(name) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you add a |
||
| except errors.HTTPBadRequest: | ||
| msg = 'It could not parse the query parameter' | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This "It" here sounds slightly odd, could we try to make it more similar to other error messages? |
||
| raise errors.HTTPInvalidParam(msg, name) from None | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does raising |
||
|
|
||
| if values_list is None: | ||
| if required: | ||
| msg = 'Missing query parameter' | ||
| raise errors.HTTPMissingParam(msg) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| output = default | ||
|
|
||
| else: | ||
| if len(values_list) % 2 != 0: | ||
| msg = 'Invalid parameter format, list elements must be even' | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To be precise list elements cannot be even per se unless they are integer numbers. Suggested rewording: "...the number of list elements must be even". |
||
| raise errors.HTTPInvalidParam(msg, name) | ||
| output = dict(zip(values_list[::2], values_list[1::2])) | ||
|
|
||
| if store is not None and isinstance(output, dict): | ||
| store.update(output) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should do like in the other methods if store is not None:
store[name] = output |
||
|
|
||
| return output | ||
|
|
||
| def has_param(self, name: str) -> bool: | ||
| """Determine whether or not the query string parameter already exists. | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,118 @@ | ||
| from __future__ import annotations | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we already have a file for these test, called tests/test_query_params.py. could you move the tests there? also you can probably check there how those methods are tested |
||
|
|
||
| from typing import Any | ||
|
|
||
| import pytest | ||
|
|
||
| from falcon import errors | ||
| from falcon.request import Request | ||
|
|
||
|
|
||
| class DummyRequestParams: | ||
| def __init__(self, params: dict[str, Any]): | ||
| self._params = params | ||
|
|
||
| def get_param_as_list(self, name: str): | ||
| if name == 'bad': | ||
| raise errors.HTTPBadRequest() | ||
| return self._params.get(name) | ||
|
|
||
| def get_param_as_dict( | ||
| self, | ||
| name: str, | ||
| required: bool = False, | ||
| deep_object: bool = False, | ||
| store: dict[str, Any] | None = None, | ||
| default: Any | None = None, | ||
| ) -> Any: ... | ||
|
|
||
|
|
||
| # NOTE(StepanUFL): If a better way to make this play well with mypy exists... | ||
| DummyRequestParams.get_param_as_dict = Request.get_param_as_dict # type: ignore[method-assign, assignment] | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Defining a new fake Maybe we could simulate request, or simply construct a normal To that end, we also have a |
||
|
|
||
|
|
||
| @pytest.mark.parametrize( | ||
| 'params,expected', | ||
| [ | ||
| ({'user[name]': 'Ash', 'user[age]': '36'}, {'name': 'Ash', 'age': '36'}), | ||
| ({'user[empty]': ''}, {'empty': ''}), | ||
| ], | ||
| ) | ||
| def test_deep_object_success(params, expected): | ||
| req = DummyRequestParams(params) | ||
| result = req.get_param_as_dict('user', deep_object=True) | ||
| assert result == expected | ||
|
|
||
|
|
||
| def test_deep_object_missing_required(): | ||
| req = DummyRequestParams({}) | ||
| with pytest.raises(errors.HTTPMissingParam): | ||
| req.get_param_as_dict('user', deep_object=True, required=True) | ||
|
|
||
|
|
||
| def test_deep_object_default_used_when_missing(): | ||
| req = DummyRequestParams({}) | ||
| default = {'fallback': 123} | ||
| result = req.get_param_as_dict('user', deep_object=True, default=default) | ||
| assert result == default | ||
|
|
||
|
|
||
| def test_regular_param_success_even_length(): | ||
| req = DummyRequestParams({'pair': ['a', '1', 'b', '2']}) | ||
| result = req.get_param_as_dict('pair') | ||
| assert result == {'a': '1', 'b': '2'} | ||
|
|
||
|
|
||
| def test_regular_param_missing_required(): | ||
| req = DummyRequestParams({}) | ||
| with pytest.raises(errors.HTTPMissingParam): | ||
| req.get_param_as_dict('pair', required=True) | ||
|
|
||
|
|
||
| def test_regular_param_odd_length_raises_invalid(): | ||
| req = DummyRequestParams({'pair': ['a', 'b', 'c']}) | ||
| with pytest.raises(errors.HTTPInvalidParam): | ||
| req.get_param_as_dict('pair') | ||
|
|
||
|
|
||
| def test_regular_param_default_used_when_missing(): | ||
| req = DummyRequestParams({}) | ||
| result = req.get_param_as_dict('pair', default={'x': 'y'}) | ||
| assert result == {'x': 'y'} | ||
|
|
||
|
|
||
| def test_regular_param_bad_request_raises_invalid(): | ||
| req = DummyRequestParams({'ok': ['1', '2']}) | ||
| with pytest.raises(errors.HTTPInvalidParam): | ||
| req.get_param_as_dict('bad') | ||
|
|
||
|
|
||
| def test_store_argument_is_updated(): | ||
| req = DummyRequestParams({'pair': ['a', '1', 'b', '2']}) | ||
| store = {} | ||
| result = req.get_param_as_dict('pair', store=store) | ||
| assert result == {'a': '1', 'b': '2'} | ||
| assert store == {'a': '1', 'b': '2'} | ||
|
|
||
|
|
||
| def test_deep_object_with_list_values(): | ||
| req = DummyRequestParams({'user[name]': ['Bond'], 'user[id]': ['007']}) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also test an with a list of multiple objects and if we decide that an empty list can indeed happen, also an empty list |
||
| result = req.get_param_as_dict('user', deep_object=True) | ||
| assert result == {'name': 'Bond', 'id': '007'} | ||
|
|
||
|
|
||
| @pytest.mark.skip(reason='Delimiter functionality not implemented') | ||
| def test_regular_param_with_delimiter_argument_is_ignored_but_accepted(): | ||
| req = DummyRequestParams({'pair': ['a', '1', 'b', '2']}) | ||
| result = req.get_param_as_dict('pair') | ||
| assert result == {'a': '1', 'b': '2'} | ||
|
|
||
|
|
||
| def test_deep_object_skips_non_matching_bracketed_keys(): | ||
| params = { | ||
| 'user[name]': 'Ash', | ||
| 'weird]': 'looking', | ||
| } | ||
| req = DummyRequestParams(params) | ||
| result = req.get_param_as_dict('user', deep_object=True) | ||
| assert result == {'name': 'Ash'} | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is likely
dict[str, str | None] | Noneeven if we don't want to do the overload like the other ones