feat(request): add delimiter support to get_param_as_list #2540
Conversation
Add a new delimiter keyword argument to eq.get_param_as_list to support splitting query parameter values on characters other than a comma. Closes falconry#2538
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2540 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 64 64
Lines 7859 7865 +6
Branches 1076 1079 +3
=========================================
+ Hits 7859 7865 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -0,0 +1 @@ | |||
| req.get_param_as_list() now supports a delimiter argument for splitting values. No newline at end of file | |||
There was a problem hiding this comment.
It would be nicer to actually link to the request method in question, probably the below could do (untested)
:meth:`falcon.Request.get_param_as_list`| Keyword Args: | ||
| delimiter(str): An optional character for splitting a parameter | ||
| value into a list. Useful for styles like ``spaceDelimited`` | ||
| or ``pipeDelimited``. If not provided, default list parsing |
There was a problem hiding this comment.
It might be useful to link to the OpenAPI spec in the parentheses (see also: ...).
| transform: Callable[[str], _T] | None = None, | ||
| required: bool = False, | ||
| store: StoreArg = None, | ||
| delimiter: str | None = None, |
There was a problem hiding this comment.
Let's add the delimiter parameter last. We haven't enforced keyword-only arguments in this method yet (maybe we should 🤔), so this could otherwise theoretically be perceived as a breaking change if one passes everything as positional arguments.
| items = params[name] | ||
|
|
||
| # If a delimiter is specified AND the param is a single string, split it. | ||
| if delimiter and isinstance(items, str): |
There was a problem hiding this comment.
Let's only allow the delimiters from the OpenAPI spec in the first iteration, so ',', ' ', and '|'.
This will make this change easier to reason about in the terms of possible edge cases and side effects.
There was a problem hiding this comment.
it may makes sense to use a dict like similar to the one below to validate delimiter
{' ': ' ', 'spaceDelimited': ' ', ',': ',', 'commaDelimited': ',', '|': '|', 'pipeDelimited': '|'}(we could likely define this at module level)
There was a problem hiding this comment.
(not sure if we want to also support comma, space, pipe as valid keys. I'm not against it, but they are not in the spec)
There was a problem hiding this comment.
Personally I would vote for keeping just the bare minimum of supported delimiters needed for the OAS in the first iteration (so just like your mapping, maybe even sans commaDelimited since it is not in the spec either, but comma is already default in some compact styles there).
We can add more later if there is need/popular demand.
| items = items.split('|') | ||
| else: | ||
| # For commas and others, also strip whitespace for convenience. | ||
| items = [v.strip() for v in items.split(delimiter)] |
There was a problem hiding this comment.
I don't think we want undocumented side effects like stripping whitespace, moreover, depending on the value of the delimiter.
|
Sounds good. I'm away from my computer for the next 2 days but I'll be able to take care of those adjustments afterwards. |
Thanks, have a nice weekend! |
| items = params[name] | ||
|
|
||
| # If a delimiter is specified AND the param is a single string, split it. | ||
| if delimiter and isinstance(items, str): |
There was a problem hiding this comment.
it may makes sense to use a dict like similar to the one below to validate delimiter
{' ': ' ', 'spaceDelimited': ' ', ',': ',', 'commaDelimited': ',', '|': '|', 'pipeDelimited': '|'}(we could likely define this at module level)
| assert _parse_etags(header_value) is None | ||
|
|
||
| @pytest.mark.parametrize('asgi', [False, True]) | ||
| def test_get_param_as_list_space_delimited(self, asgi): |
There was a problem hiding this comment.
I think we already a fixture called asgi that automatically does true/false
Add a new delimiter keyword argument to
req.get_param_as_list to support splitting query parameter values on characters other than a comma.
Closes #2538
Summary of Changes
This PR introduces a new delimiter keyword argument to the req.get_param_as_list() method.
This enhancement allows for parsing list-style query parameters that use delimiters other than the default comma (e.g., spaces, pipes). The primary motivation is to better support spaceDelimited and pipeDelimited styles as described in the OpenAPI specification.
The splitting logic is only applied when the parameter value is a single string, which preserves the framework's existing behavior for handling multiple instances of the same parameter in a query string.
Related Issues
Closes #2538
Pull Request Checklist
This is just a reminder about the most common mistakes. Please make sure that you tick all appropriate boxes. Reading our contribution guide at least once will save you a few review cycles!
If an item doesn't apply to your pull request, check it anyway to make it apparent that there's nothing to do.
tox.docs/.docs/.versionadded,versionchanged, ordeprecateddirectives.docs/_newsfragments/, with the file name format{issue_number}.{fragment_type}.rst. (Runtowncrier --draftto ensure it renders correctly.)If you have any questions to any of the points above, just submit and ask! This checklist is here to help you, not to deter you from contributing!
PR template inspired by the attrs project.