Repository: marshmallow-code/marshmallow
Pull request #2044 (2022-09-04 16:28:24)

Merge EnumSymbol and EnumValue into Enum field
On second thought, this fits more with our fields naming rationale. Generally, the field name represents the type in object world and the serialization type is defined at init (with an argument such as format, etc.).

I'd like to merge this before publishing 3.18. If this needs more thought and we want to ship a bugfix before, we can revert the merge of #2017.

sloria: i agree that `Enum` fits our field naming convention better. would it make more sense to rename `EnumSymbol`->`Enum` and keep `EnumValue` as is?

i'm lukewarm on merging the two together and using a parameter to determine the internal type, as this makes it more difficult to add correct typings. wdyt?
deckar01: The only difference between these two use cases is how the members are accessed from the enum, which seems congruent with the `data_key` and `attribute` as constructor arguments. Switching the behavior from symbol to value based on the presence of the field argument seems cryptic. I can understand the desire to make one of the use cases the default to simplify the interface. I am not sure which behavior should be the default.

```py
foo = Enum()
bar = Enum(from_symbol=True)
```
lafrech: Thanks for the feedback.

> The only difference between these two use cases is how the members are accessed from the enum, which seems congruent with the data_key and attribute as constructor arguments.

Yes, exactly, the object is always the same (an `Enum` member). Hence the will to call the field `Enum`. This plus the fact that is "looks" simpler (not really a great argument). A non-great argument the other way is that the field name may collide with the Python `Enum` class but that shouldn't be an issue.

> i'm lukewarm on merging the two together and using a parameter to determine the internal type, as this makes it more difficult to add correct typings. wdyt?

In object world it is always an `Enum` member, so no problem. In serialized world, it is either a string (symbol) or whatever the field serializes to (value). I don't see this as an issue. Splitting the two fields would guarantee the output is a string in the symbol case but leave it open in the value case, IIUC. I see little benefit to the split in terms of typing and I wouldn't trade simplicity for that. But maybe I'm missing something.

> Switching the behavior from symbol to value based on the presence of the field argument seems cryptic.

I understand that. OTOH, the parameter seems redundant with the field and accepting both means we'd need to manage all cases.

- as value + field => OK
- as symbol + no field => OK
- as value + no field => error or default to String (I'm fine with `String` as default, both in terms of usage and implementation)
- as symbol + field => error or actually try to serialize symbol name (string) with the field, which sounds wrong in most cases, although it might allow twisted stuff like passing a `String` field with validators to apply between symbol string deserialization and `Enum` member instantiation (I don't see the point of this).

> I can understand the desire to make one of the use cases the default to simplify the interface. I am not sure which behavior should be the default.

From my perspective (usage), default would be symbol, but YMMV. Symbol is generally a snake-case Python variable name which suits an API (some like them camel-case but oh well), while value may be anything. Typically, I'll have

```py
class Enum:
    item_one = "The first item"
    item_two = "The second item"
```

Although both are strings, I serialize as symbol (code) rather than value (human readable string).

I can add an `as_value` parameter (defaulting to `False`) but I'm not sure how to deal with odd `field` / `as_value` combinations. `as_symbol` + `field` defaulting to `String` is fine, nice actually. `as_value` + `field` not `String` sounds definitely wrong and should probably error.

Or we state explicitly that `field` is only used if `as_value` is set. We may even warn or error otherwise.
deckar01: `Enum` allows arbitrary value types like lists and dicts for member values and the types don't have to be homogeneous. The default could be `Field`.
deckar01: As far as I can tell the current code handles exotic enums fine. I am suggesting that strict value type validation does not need to be the default, because enum membership validation works regardless of value type. The error message providing the allowed values seems like a more useful default than an error message telling me the expected value type. Actually, even if the value field is specialized, it might be more useful show the member options error message.
lafrech: I just pushed an updated version introducing `by_value` argument. If `True`, `field` defaults to `Field`. Otherwise an exception is raised if passing a field.

Tests are incomplete (I didn't test the exception and the default value of field) but at least it gives us an idea of what it would look like.

I'm not really fan of the signature, but I'm afraid it is either this or the two classes.

```py
    def __init__(
        self,
        enum: type[EnumType],
        by_value: bool = False,
        field: Field | type | None = None,
        **kwargs,
    ):
```

```py
# Call using kwargs
field = fields.Enum(HairColorEnum, by_value=True, field=fields.String)

# Call using args. I don't like True arguments in function calls. Not explicit enough.
# (Yet, I didn't prevent it in the signature by making it kwargs only.)
field = fields.Enum(HairColorEnum, True, fields.String)

# Field would be explicit enough to be passed as positional but we can't do this.
# And we don't want to put field first because one may pass only by_value and rely on default Field.
field = fields.Enum(HairColorEnum, by_value=True, fields.String)
```

deckar01: > Switching the behavior from symbol to value based on the presence of the field argument seems cryptic.

My main concern was that `field` did not imply value. What if the `by_value` was keyword only and could be a boolean or a field?

```py
    def __init__(
        self,
        enum: type[EnumType],
        *,
        by_value: bool | Field | type = False,
        **kwargs,
    ):
```

```py
# Default: by_value=False, field=String
field = fields.Enum(HairColorEnum)

# Convenience: field=Field
field = fields.Enum(HairColorEnum, by_value=True)

# Custom
field = fields.Enum(HairColorEnum, by_value=fields.Int)
```
lafrech: This is technically better as it makes one less argument and avoids the forbidden case of by_symbol + field.

I'm not a huge fan of arguments that can be multiple types (strings or list, boolean or field) but I still think it is better.

Yet, `True` is just a shortcut for `fields.Field` and I while I understand the intent, I believe this convenience will almost never be used because in practice users will want to pass a field, most of the time `String` or `Integer`, right? So is it worth keeping the multiple types argument if it (almost) always receives a field anyway?

I also understand the argument about the name conveying the meaning, and `by_value` expecting only a field (not a boolean) would sound weird, while `field` implying by value is not obvious.

OK, I just tested and now I get your point. If I set `Field` instead of `String` or `Integer`, it works the same. The purpose of using  those fields generally is to validate the input (plus marginal stuff like ensure type), which is useless in `fields.Enum` because `enum.Enum` does it just as well if not better (better error message). So your point is that in most cases people may leave the field empty and rely on `enum.Enum` to do the job. Now I agree.

Note that there may be slight differences. For instance, for enum integer values, `Field` will let `enum.Enum` choke on integers passed as strings, while `Integer` will cast them (it is not strict by default). I don't think that's a problem, though.

OK, lemme try to do that.



Patch:
diff --git a/README.rst b/README.rst
index d979160c..ed07dd05 100644
--- a/README.rst
+++ b/README.rst
@@ -145,6 +145,12 @@ while seamlessly integrating with existing tools. [`Get professional support`_]
     :target: https://tidelift.com/subscription/pkg/pypi-marshmallow?utm_source=pypi-marshmallow&utm_medium=readme
     :alt: Get supported marshmallow with Tidelift
 
+Security Contact Information
+============================
+
+To report a security vulnerability, please use the
+`Tidelift security contact <https://tidelift.com/security>`_.
+Tidelift will coordinate the fix and disclosure.
 
 Project Links
 =============
diff --git a/SECURITY.md b/SECURITY.md
deleted file mode 100644
index 92ec2bb6..00000000
--- a/SECURITY.md
+++ /dev/null
@@ -1,5 +0,0 @@
-# Security Contact Information
-
-To report a security vulnerability, please use the
-[Tidelift security contact](https://tidelift.com/security).
-Tidelift will coordinate the fix and disclosure.
diff --git a/setup.py b/setup.py
index 286ec759..739ce945 100644
--- a/setup.py
+++ b/setup.py
@@ -6,7 +6,7 @@ EXTRAS_REQUIRE = {
     "lint": [
         "mypy==0.971",
         "flake8==5.0.4",
-        "flake8-bugbear==22.9.11",
+        "flake8-bugbear==22.8.23",
         "pre-commit~=2.4",
     ],
     "docs": [
diff --git a/src/marshmallow/fields.py b/src/marshmallow/fields.py
index 1dc74714..f153b5f9 100644
--- a/src/marshmallow/fields.py
+++ b/src/marshmallow/fields.py
@@ -11,7 +11,7 @@ import decimal
 import math
 import typing
 import warnings
-from enum import Enum
+from enum import Enum as EnumType
 from collections.abc import Mapping as _Mapping
 
 from marshmallow import validate, utils, class_registry, types
@@ -60,8 +60,7 @@ __all__ = [
     "IPInterface",
     "IPv4Interface",
     "IPv6Interface",
-    "EnumSymbol",
-    "EnumValue",
+    "Enum",
     "Method",
     "Function",
     "Str",
@@ -1856,43 +1855,16 @@ class IPv6Interface(IPInterface):
     DESERIALIZATION_CLASS = ipaddress.IPv6Interface
 
 
-class EnumSymbol(String):
-    """An Enum field (de)serializing enum members by symbol (name) as string.
+class Enum(Field):
+    """An Enum field (de)serializing enum members by symbol (name) or by value.
 
     :param enum Enum: Enum class
+    :param boolean|Schema|Field by_value: Whether to (de)serialize by value or by name,
+        or Field class or instance to use to (de)serialize by value. Defaults to False.
 
-    .. versionadded:: 3.18.0
-    """
-
-    default_error_messages = {
-        "unknown": "Must be one of: {choices}.",
-    }
-
-    def __init__(self, enum: type[Enum], **kwargs):
-        self.enum = enum
-        self.choices = ", ".join(enum.__members__)
-        super().__init__(**kwargs)
-
-    def _serialize(self, value, attr, obj, **kwargs):
-        if value is None:
-            return None
-        return value.name
-
-    def _deserialize(self, value, attr, data, **kwargs):
-        value = super()._deserialize(value, attr, data, **kwargs)
-        try:
-            return getattr(self.enum, value)
-        except AttributeError as exc:
-            raise self.make_error("unknown", choices=self.choices) from exc
-
-
-class EnumValue(Field):
-    """An Enum field (de)serializing enum members by value.
-
-    A Field must be provided to (de)serialize the value.
-
-    :param cls_or_instance: Field class or instance.
-    :param enum Enum: Enum class
+    If `by_value` is `False` (default), enum members are (de)serialized by symbol (name).
+    If it is `True`, they are (de)serialized by value using :class:`Field`.
+    If it is a field instance or class, they are (de)serialized by value using this field.
 
     .. versionadded:: 3.18.0
     """
@@ -1901,31 +1873,59 @@ class EnumValue(Field):
         "unknown": "Must be one of: {choices}.",
     }
 
-    def __init__(self, cls_or_instance: Field | type, enum: type[Enum], **kwargs):
+    def __init__(
+        self,
+        enum: type[EnumType],
+        *,
+        by_value: bool | Field | type = False,
+        **kwargs,
+    ):
         super().__init__(**kwargs)
-        try:
-            self.field = resolve_field_instance(cls_or_instance)
-        except FieldInstanceResolutionError as error:
-            raise ValueError(
-                "The enum field must be a subclass or instance of "
-                "marshmallow.base.FieldABC."
-            ) from error
         self.enum = enum
-        self.choices = ", ".join(
-            [str(self.field._serialize(m.value, None, None)) for m in enum]
-        )
+        self.by_value = by_value
+
+        # Serialization by name
+        if by_value is False:
+            self.field: Field = String()
+            self.choices_text = ", ".join(
+                str(self.field._serialize(m, None, None)) for m in enum.__members__
+            )
+        # Serialization by value
+        else:
+            if by_value is True:
+                self.field = Field()
+            else:
+                try:
+                    self.field = resolve_field_instance(by_value)
+                except FieldInstanceResolutionError as error:
+                    raise ValueError(
+                        '"by_value" must be either a bool or a subclass or instance of '
+                        "marshmallow.base.FieldABC."
+                    ) from error
+            self.choices_text = ", ".join(
+                str(self.field._serialize(m.value, None, None)) for m in enum
+            )
 
     def _serialize(self, value, attr, obj, **kwargs):
         if value is None:
             return None
-        return self.field._serialize(value.value, attr, obj, **kwargs)
+        if self.by_value:
+            val = value.value
+        else:
+            val = value.name
+        return self.field._serialize(val, attr, obj, **kwargs)
 
     def _deserialize(self, value, attr, data, **kwargs):
-        value = self.field._deserialize(value, attr, data, **kwargs)
+        val = self.field._deserialize(value, attr, data, **kwargs)
+        if self.by_value:
+            try:
+                return self.enum(val)
+            except ValueError as error:
+                raise self.make_error("unknown", choices=self.choices_text) from error
         try:
-            return self.enum(value)
-        except ValueError as exc:
-            raise self.make_error("unknown", choices=self.choices) from exc
+            return getattr(self.enum, val)
+        except AttributeError as error:
+            raise self.make_error("unknown", choices=self.choices_text) from error
 
 
 class Method(Field):


Test patch:
diff --git a/tests/base.py b/tests/base.py
index 4dbbcd5f..fea1e1b4 100644
--- a/tests/base.py
+++ b/tests/base.py
@@ -54,9 +54,9 @@ ALL_FIELDS = [
     fields.IPInterface,
     fields.IPv4Interface,
     fields.IPv6Interface,
-    functools.partial(fields.EnumSymbol, GenderEnum),
-    functools.partial(fields.EnumValue, fields.String, HairColorEnum),
-    functools.partial(fields.EnumValue, fields.Integer, GenderEnum),
+    functools.partial(fields.Enum, GenderEnum),
+    functools.partial(fields.Enum, HairColorEnum, by_value=fields.String),
+    functools.partial(fields.Enum, GenderEnum, by_value=fields.Integer),
 ]
 
 
diff --git a/tests/test_deserialization.py b/tests/test_deserialization.py
index b922d6a5..c80a2581 100644
--- a/tests/test_deserialization.py
+++ b/tests/test_deserialization.py
@@ -1097,54 +1097,82 @@ class TestFieldDeserialization:
 
         assert excinfo.value.args[0] == "Not a valid IPv6 interface."
 
-    def test_enumsymbol_field_deserialization(self):
-        field = fields.EnumSymbol(GenderEnum)
+    def test_enum_field_by_symbol_deserialization(self):
+        field = fields.Enum(GenderEnum)
         assert field.deserialize("male") == GenderEnum.male
 
-    def test_enumsymbol_field_invalid_value(self):
-        field = fields.EnumSymbol(GenderEnum)
+    def test_enum_field_by_symbol_invalid_value(self):
+        field = fields.Enum(GenderEnum)
         with pytest.raises(
             ValidationError, match="Must be one of: male, female, non_binary."
         ):
             field.deserialize("dummy")
 
-    def test_enumsymbol_field_not_string(self):
-        field = fields.EnumSymbol(GenderEnum)
+    def test_enum_field_by_symbol_not_string(self):
+        field = fields.Enum(GenderEnum)
         with pytest.raises(ValidationError, match="Not a valid string."):
             field.deserialize(12)
 
-    def test_enumvalue_field_deserialization(self):
-        field = fields.EnumValue(fields.String, HairColorEnum)
+    def test_enum_field_by_value_true_deserialization(self):
+        field = fields.Enum(HairColorEnum, by_value=True)
         assert field.deserialize("black hair") == HairColorEnum.black
-        field = fields.EnumValue(fields.Integer, GenderEnum)
+        field = fields.Enum(GenderEnum, by_value=True)
         assert field.deserialize(1) == GenderEnum.male
-        field = fields.EnumValue(fields.Date(format="%d/%m/%Y"), DateEnum)
+
+    def test_enum_field_by_value_field_deserialization(self):
+        field = fields.Enum(HairColorEnum, by_value=fields.String)
+        assert field.deserialize("black hair") == HairColorEnum.black
+        field = fields.Enum(GenderEnum, by_value=fields.Integer)
+        assert field.deserialize(1) == GenderEnum.male
+        field = fields.Enum(DateEnum, by_value=fields.Date(format="%d/%m/%Y"))
         assert field.deserialize("29/02/2004") == DateEnum.date_1
 
-    def test_enumvalue_field_invalid_value(self):
-        field = fields.EnumValue(fields.String, HairColorEnum)
+    def test_enum_field_by_value_true_invalid_value(self):
+        field = fields.Enum(HairColorEnum, by_value=True)
         with pytest.raises(
             ValidationError,
             match="Must be one of: black hair, brown hair, blond hair, red hair.",
         ):
             field.deserialize("dummy")
-        field = fields.EnumValue(fields.Integer, GenderEnum)
+        field = fields.Enum(GenderEnum, by_value=True)
         with pytest.raises(ValidationError, match="Must be one of: 1, 2, 3."):
             field.deserialize(12)
-        field = fields.EnumValue(fields.Date(format="%d/%m/%Y"), DateEnum)
+
+    def test_enum_field_by_value_field_invalid_value(self):
+        field = fields.Enum(HairColorEnum, by_value=fields.String)
+        with pytest.raises(
+            ValidationError,
+            match="Must be one of: black hair, brown hair, blond hair, red hair.",
+        ):
+            field.deserialize("dummy")
+        field = fields.Enum(GenderEnum, by_value=fields.Integer)
+        with pytest.raises(ValidationError, match="Must be one of: 1, 2, 3."):
+            field.deserialize(12)
+        field = fields.Enum(DateEnum, by_value=fields.Date(format="%d/%m/%Y"))
         with pytest.raises(
             ValidationError, match="Must be one of: 29/02/2004, 29/02/2008, 29/02/2012."
         ):
             field.deserialize("28/02/2004")
 
-    def test_enumvalue_field_wrong_type(self):
-        field = fields.EnumValue(fields.String, HairColorEnum)
+    def test_enum_field_by_value_true_wrong_type(self):
+        field = fields.Enum(HairColorEnum, by_value=True)
+        with pytest.raises(
+            ValidationError,
+            match="Must be one of: black hair, brown hair, blond hair, red hair.",
+        ):
+            field.deserialize("dummy")
+        field = fields.Enum(GenderEnum, by_value=True)
+        with pytest.raises(ValidationError, match="Must be one of: 1, 2, 3."):
+            field.deserialize(12)
+
+    def test_enum_field_by_value_field_wrong_type(self):
+        field = fields.Enum(HairColorEnum, by_value=fields.String)
         with pytest.raises(ValidationError, match="Not a valid string."):
             field.deserialize(12)
-        field = fields.EnumValue(fields.Integer, GenderEnum)
+        field = fields.Enum(GenderEnum, by_value=fields.Integer)
         with pytest.raises(ValidationError, match="Not a valid integer."):
             field.deserialize("dummy")
-        field = fields.EnumValue(fields.Date(format="%d/%m/%Y"), DateEnum)
+        field = fields.Enum(DateEnum, by_value=fields.Date(format="%d/%m/%Y"))
         with pytest.raises(ValidationError, match="Not a valid date."):
             field.deserialize("30/02/2004")
 
diff --git a/tests/test_serialization.py b/tests/test_serialization.py
index 8e751e5c..51671ddf 100644
--- a/tests/test_serialization.py
+++ b/tests/test_serialization.py
@@ -255,20 +255,29 @@ class TestFieldSerialization:
             == ipv6interface_exploded_string
         )
 
-    def test_enumsymbol_field_serialization(self, user):
+    def test_enum_field_by_symbol_serialization(self, user):
         user.sex = GenderEnum.male
-        field = fields.EnumSymbol(GenderEnum)
+        field = fields.Enum(GenderEnum)
         assert field.serialize("sex", user) == "male"
 
-    def test_enumvalue_field_serialization(self, user):
+    def test_enum_field_by_value_true_serialization(self, user):
         user.hair_color = HairColorEnum.black
-        field = fields.EnumValue(fields.String, HairColorEnum)
+        field = fields.Enum(HairColorEnum, by_value=True)
         assert field.serialize("hair_color", user) == "black hair"
         user.sex = GenderEnum.male
-        field = fields.EnumValue(fields.Integer, GenderEnum)
+        field = fields.Enum(GenderEnum, by_value=True)
         assert field.serialize("sex", user) == 1
         user.some_date = DateEnum.date_1
-        field = fields.EnumValue(fields.Date(format="%d/%m/%Y"), DateEnum)
+
+    def test_enum_field_by_value_field_serialization(self, user):
+        user.hair_color = HairColorEnum.black
+        field = fields.Enum(HairColorEnum, by_value=fields.String)
+        assert field.serialize("hair_color", user) == "black hair"
+        user.sex = GenderEnum.male
+        field = fields.Enum(GenderEnum, by_value=fields.Integer)
+        assert field.serialize("sex", user) == 1
+        user.some_date = DateEnum.date_1
+        field = fields.Enum(DateEnum, by_value=fields.Date(format="%d/%m/%Y"))
         assert field.serialize("some_date", user) == "29/02/2004"
 
     def test_decimal_field(self, user):