Repository: marshmallow-code/webargs

Issue:
Dealing with empty values in `DelimitedFieldMixin`
`DelimitedList(String())` deserializes "a,,c" as `["a", "", "c"]`.

I guess this meets user expectations.

My expectation with integers would be that

`DelimitedList(Integer(allow_none=True))` deserializes `"1,,3"` as `[1,None,3]`

but it errors.

The reason for this is that when the string is split, it is turned into `["1", "", "3"]`. This is why it works in the string case.

I'm not sure this was really intended. It may be a side effect of the `split` function that happens to do well with strings.

We could change that to replace empty values with `None`. But it would break the string use case, unless the user overloads `String` field to deserialize `None` as `""`.

Likewise, users may already overload `Integer` to deserialize `""` as `None` and no modification is required to `DelimitedFieldMixin`.

Just been caught by this and wondering out loud. Advice welcome.

In any case, there is an intrinsic limitation in the delimited string format: one can't distinguish empty string from missing value (as opposed to a JSON payload). It is not clear to me how OpenAPI (for instance) deals with the case of an empty element in an array (in a query argument).

Discussion:
lafrech: While having a look at how to document this, I came to think that maybe we're doing this the wrong way.

The `DelimitedTuple` case is a hint that solving this at list level rather than nested schema level might be a bit wrong. Indeed, one could imagine a tuple with mixed strings and integers:

```python
    DelimitedLTuple(String(load_default=""), Integer(load_default=0)
```

What if we decided that `DelimitedList` passes `missing` to the nested schema rather than the "empty" value, then we'd just rely on nested schema default?

I'm not sure this would work as-is, and I'm done for tonight, but we should think about this before merging.

Also this would break the `DelimitedList(String)` use case because users would have to set a default explicitly, but maybe that's the way it is. 

```python
    DelimitedList(String(load_default=""))
```

sirosen: I'm not sure I like the idea of requiring `load_default` for the current behavior. `DelimitedList(String())` parsing `"a,b,,c"` into `["a", "b", "", "c"]` seems to me like the most intuitive thing.

But you're also right that passing `missing` allows the nested field to decide how to handle this...

What about adding `empty=""` as the default, but documenting `empty=ma.missing` in the advanced docs as a way of getting more power out of the delimited fields? I think there's an argument in favor of the default being `missing` from a "purity" perspective, but that `""` is more practical because it's what more users will want.
lafrech: I agree about the need for a practical default.

Yet the hybrid tuple case bothers me.

Note that I didn't check that passing missing actually works. It might silently hide empty items. I dunno, I didn't try anything.

Perhaps practicality and purity could be achieved by using subclasses or mixins to make the choice explicit without requiring an "empty" argument.
sirosen: Just to quickly answer the question, passing `missing` works as you'd hope:
```python
>>> import webargs as wa
>>> f = wa.fields.DelimitedList(wa.fields.String(), empty=wa.missing)
>>> f.deserialize("a,,b")
['a', <marshmallow.missing>, 'b']
>>> f = wa.fields.DelimitedList(wa.fields.String(load_default="foo"), empty=wa.missing)
>>> f.deserialize("a,,b")
['a', 'foo', 'b']
```

Taking a little inspiration from pathlib's naming, I'm wondering if we'd be okay with this?
```python
# passes missing to subfields
PureDelimitedList
PureDelimitedTuple
# passes "" to subfields
DelimitedList
DelimitedTuple
```
with the implementation being that `empty_value` (longer name to be a little more explicit) is a classvar which each subclass can set, and which is `missing` on `DelimitedFieldMixin`.

I like, as a user of `pathlib`, that `PureX` vs `X` indicates clearly to the user that there's a subtle distinction between classes which you _might_ need to understand depending on your use-case.
sirosen: I looked at my comment again and don't like `Pure` for naming. It looks _too much_ like `PurePath`, which has a different meaning from this.

If we do it with classes, maybe `UnfilteredDelimitedList`.

Also, it may make sense in v9 for tuple to default to missing while list defaults to `""`.

I'm still trying to find a good clean solve, so thoughts are welcome.
sirosen: @lafrech, I have an idea for v9, but it would be backwards incompatible. Would you be okay with the following plan?

- set `empty_value` as a class variable in v8, equal to `""`, rework this PR to implement that
- in v9, `empty_value` will default to `missing`
- in v9, the default behavior of `DelimitedList` _if given no field as an argument_ is to insert a field `String(load_default="")`
- we document these changes and the rationale as more controlled handling of empty values

I would want to work more on this and maybe tinker with related ideas when we prepare v9, but this way we can close this issue out today and have a plan for the future. Having `DelimitedList()` produce a list of strings (including empty strings) satisfies my desire to make the easy case easy.
sirosen: I refactored this and rebased. It now uses `empty_value` as a class attribute (no support for `empty=...` in the field constructor) and the changelog has been updated to list the change.

The implementation barely changes in this, but it will set us up for the desired change for 9.0 .

Patch:
diff --git a/CHANGELOG.rst b/CHANGELOG.rst
index d02eb12..3ba8020 100644
--- a/CHANGELOG.rst
+++ b/CHANGELOG.rst
@@ -1,9 +1,34 @@
 Changelog
 ---------
 
-8.3.1 (Unreleased)
+8.4.0 (Unreleased)
 ******************
 
+Features:
+
+* Add a new class attribute, ``empty_value`` to ``DelimitedList`` and
+  ``DelimitedTuple``, with a default of ``""``.
+  This controls the value deserialized when an empty string is seen.
+
+``empty_value`` can be used to handle types other than strings more gracefully, e.g.
+
+.. code-block:: python
+
+    from webargs import fields
+
+
+    class IntList(fields.DelimitedList):
+        empty_value = 0
+
+
+    myfield = IntList(fields.Int())
+
+.. note::
+
+    ``empty_value`` will be changing in webargs v9.0 to be ``missing`` by
+    default. This will allow use of fields with ``load_default`` to specify
+    handling of the empty value.
+
 Changes:
 
 * Type annotations for ``FlaskParser`` have been improved
diff --git a/src/webargs/fields.py b/src/webargs/fields.py
index 5598b54..96590a3 100644
--- a/src/webargs/fields.py
+++ b/src/webargs/fields.py
@@ -15,6 +15,8 @@ tells webargs where to parse the request argument from.
 """
 from __future__ import annotations
 
+import typing
+
 import marshmallow as ma
 
 # Expose all fields from marshmallow.fields.
@@ -64,6 +66,8 @@ class DelimitedFieldMixin:
     delimiter: str = ","
     # delimited fields set is_multiple=False for webargs.core.is_multiple
     is_multiple: bool = False
+    # NOTE: in 8.x this defaults to "" but in 9.x it will be 'missing'
+    empty_value: typing.Any = ""
 
     def _serialize(self, value, attr, obj, **kwargs):
         # serializing will start with parent-class serialization, so that we correctly
@@ -77,6 +81,8 @@ class DelimitedFieldMixin:
         if not isinstance(value, (str, bytes)):
             raise self.make_error("invalid")
         values = value.split(self.delimiter) if value else []
+        # convert empty strings to the empty value; typically "" and therefore a no-op
+        values = [v or self.empty_value for v in values]
         return super()._deserialize(values, attr, data, **kwargs)
 
 
@@ -117,6 +123,12 @@ class DelimitedTuple(DelimitedFieldMixin, ma.fields.Tuple):
 
     default_error_messages = {"invalid": "Not a valid delimited tuple."}
 
-    def __init__(self, tuple_fields, *, delimiter: str | None = None, **kwargs):
+    def __init__(
+        self,
+        tuple_fields,
+        *,
+        delimiter: str | None = None,
+        **kwargs,
+    ):
         self.delimiter = delimiter or self.delimiter
         super().__init__(tuple_fields, **kwargs)


Test patch:
diff --git a/tests/test_core.py b/tests/test_core.py
index c24ac1b..39b8284 100644
--- a/tests/test_core.py
+++ b/tests/test_core.py
@@ -11,6 +11,7 @@ from marshmallow import (
     INCLUDE,
     RAISE,
     Schema,
+    missing,
     post_load,
     pre_load,
     validates_schema,
@@ -1106,6 +1107,47 @@ def test_delimited_tuple_passed_invalid_type(web_request, parser):
     assert excinfo.value.messages == {"json": {"ids": ["Not a valid delimited tuple."]}}
 
 
+def test_delimited_list_custom_empty_value(web_request, parser):
+    class ZeroList(fields.DelimitedList):
+        empty_value = 0
+
+    web_request.json = {"ids": "1,,3"}
+    schema_cls = Schema.from_dict({"ids": ZeroList(fields.Int())})
+    schema = schema_cls()
+
+    parsed = parser.parse(schema, web_request)
+    assert parsed["ids"] == [1, 0, 3]
+
+
+def test_delimited_tuple_custom_empty_value(web_request, parser):
+    class ZeroTuple(fields.DelimitedTuple):
+        empty_value = 0
+
+    web_request.json = {"ids": "1,,3"}
+    schema_cls = Schema.from_dict(
+        {"ids": ZeroTuple((fields.Int, fields.Int, fields.Int))}
+    )
+    schema = schema_cls()
+
+    parsed = parser.parse(schema, web_request)
+    assert parsed["ids"] == (1, 0, 3)
+
+
+def test_delimited_list_using_missing_for_empty(web_request, parser):
+    # this is "future" because we plan to make this the default for webargs v9.0
+    class FutureList(fields.DelimitedList):
+        empty_value = missing
+
+    web_request.json = {"ids": "foo,,bar"}
+    schema_cls = Schema.from_dict(
+        {"ids": FutureList(fields.String(load_default="nil"))}
+    )
+    schema = schema_cls()
+
+    parsed = parser.parse(schema, web_request)
+    assert parsed["ids"] == ["foo", "nil", "bar"]
+
+
 def test_missing_list_argument_not_in_parsed_result(web_request, parser):
     # arg missing in request
     web_request.json = {}