Repository: marshmallow-code/marshmallow

Issue:
Is it intentional that setting parameter unknown=<any non-empty string> behaves the same as unknown=EXCLUDE?
I was playing around with some of the unknown options and noticed that if you set unknown to a random string when instantiating a schema, it behaves the same as setting `unknown=EXCLUDE`.

Is this intentional? 

```
class TestSchema(Schema):
    schema_field = fields.Str(required=True)

test = dict(schema_field="random", non_schema_field="also random")
schema = TestSchema(unknown=EXCLUDE)
print(schema.load(test))
# {'schema_field': 'random'}
```

```
schema = TestSchema(unknown=INCLUDE)
print(schema.load(test))
# {'schema_field': 'random', 'non_schema_field': 'also random'}
```

```
schema = TestSchema(unknown="1234")
print(schema.load(test))
# {'schema_field': 'random'}
```

Discussion:
lafrech: Thanks for this.

> In cases where unknown or self.unknown is used to support None, do the validation after the or.

I suppose it would be slightly more efficient to only validate `unknown` in this case as `self.unknown` is supposed to be valid since it was validated earlier (thanks to this PR).

The code would be a little longer, though. Not a one-liner anymore.

Not sure this is relevant. The validation itself is just one statement after all:

```py
    if obj not in (INCLUDE, EXCLUDE, RAISE):
```

(Would a (precomputed at import) `set` be marginally faster?)
sirosen: > I suppose it would be slightly more efficient to only validate `unknown` in this case as `self.unknown` is supposed to be valid since it was validated earlier (thanks to this PR).
> 
> The code would be a little longer, though. Not a one-liner anymore.

I was thinking the same thing and then I realized that there's a nice way to write this:
```python
unknown = self.unknown if unknown is None else validate_unknown_parameter_value(unknown)
```

I'll make that change since it should be slightly faster when `load` is called without the `unknown` arg (which is where I think it really matters to try to make things fast).

I don't think it really matters -- once these changes are tweaked as above -- to use a module-scoped set, but I'll make that change too. We're talking about it and I see no reason _not_ to do it that way.

Patch:
diff --git a/CHANGELOG.rst b/CHANGELOG.rst
index 77922a0a..73008f7b 100644
--- a/CHANGELOG.rst
+++ b/CHANGELOG.rst
@@ -4,6 +4,11 @@ Changelog
 (unreleased)
 ************
 
+Features:
+
+- Raise `ValueError` if an invalid value is passed to the ``unknown`` argument (:issue:`1721`, :issue:`1732`).
+  Thanks :user:`sirosen` for the PR.
+
 Other changes:
 
 - Set lower bound for `packaging` requirement (:issue:`1957`).
diff --git a/src/marshmallow/schema.py b/src/marshmallow/schema.py
index ff119a46..00e697e1 100644
--- a/src/marshmallow/schema.py
+++ b/src/marshmallow/schema.py
@@ -34,6 +34,7 @@ from marshmallow.utils import (
     get_value,
     is_collection,
     is_instance_or_subclass,
+    validate_unknown_parameter_value,
 )
 from marshmallow.warnings import RemovedInMarshmallow4Warning
 
@@ -227,7 +228,7 @@ class SchemaOpts:
         self.include = getattr(meta, "include", {})
         self.load_only = getattr(meta, "load_only", ())
         self.dump_only = getattr(meta, "dump_only", ())
-        self.unknown = getattr(meta, "unknown", RAISE)
+        self.unknown = validate_unknown_parameter_value(getattr(meta, "unknown", RAISE))
         self.register = getattr(meta, "register", True)
 
 
@@ -389,7 +390,11 @@ class Schema(base.SchemaABC, metaclass=SchemaMeta):
         self.load_only = set(load_only) or set(self.opts.load_only)
         self.dump_only = set(dump_only) or set(self.opts.dump_only)
         self.partial = partial
-        self.unknown = unknown or self.opts.unknown
+        self.unknown = (
+            self.opts.unknown
+            if unknown is None
+            else validate_unknown_parameter_value(unknown)
+        )
         self.context = context or {}
         self._normalize_nested_options()
         #: Dictionary mapping field_names -> :class:`Field` objects
@@ -833,7 +838,11 @@ class Schema(base.SchemaABC, metaclass=SchemaMeta):
         error_store = ErrorStore()
         errors = {}  # type: dict[str, list[str]]
         many = self.many if many is None else bool(many)
-        unknown = unknown or self.unknown
+        unknown = (
+            self.unknown
+            if unknown is None
+            else validate_unknown_parameter_value(unknown)
+        )
         if partial is None:
             partial = self.partial
         # Run preprocessors
diff --git a/src/marshmallow/utils.py b/src/marshmallow/utils.py
index 2aa6dfe1..4e123a5c 100644
--- a/src/marshmallow/utils.py
+++ b/src/marshmallow/utils.py
@@ -20,6 +20,7 @@ from marshmallow.warnings import RemovedInMarshmallow4Warning
 EXCLUDE = "exclude"
 INCLUDE = "include"
 RAISE = "raise"
+_UNKNOWN_VALUES = {EXCLUDE, INCLUDE, RAISE}
 
 
 class _Missing:
@@ -333,3 +334,11 @@ def timedelta_to_microseconds(value: dt.timedelta) -> int:
     https://github.com/python/cpython/blob/bb3e0c240bc60fe08d332ff5955d54197f79751c/Lib/datetime.py#L665-L667  # noqa: B950
     """
     return (value.days * (24 * 3600) + value.seconds) * 1000000 + value.microseconds
+
+
+def validate_unknown_parameter_value(obj: typing.Any) -> str:
+    if obj not in _UNKNOWN_VALUES:
+        raise ValueError(
+            f"Object {obj!r} is not a valid value for the 'unknown' parameter"
+        )
+    return obj


Test patch:
diff --git a/tests/test_schema.py b/tests/test_schema.py
index c00189df..4943fbd8 100644
--- a/tests/test_schema.py
+++ b/tests/test_schema.py
@@ -1,50 +1,47 @@
 import datetime as dt
 import decimal
-import random
 import math
-from collections import namedtuple, OrderedDict
-
-import simplejson as json
+import random
+from collections import OrderedDict, namedtuple
 
 import pytest
-
+import simplejson as json
 from marshmallow import (
+    EXCLUDE,
+    INCLUDE,
+    RAISE,
     Schema,
+    class_registry,
     fields,
     utils,
     validates,
     validates_schema,
-    EXCLUDE,
-    INCLUDE,
-    RAISE,
-    class_registry,
 )
 from marshmallow.exceptions import (
-    ValidationError,
-    StringNotCollectionError,
     RegistryError,
+    StringNotCollectionError,
+    ValidationError,
 )
 
 from tests.base import (
-    UserSchema,
-    UserMetaSchema,
-    UserRelativeUrlSchema,
-    ExtendedUserSchema,
-    UserIntSchema,
-    UserFloatStringSchema,
-    BlogSchema,
-    BlogUserMetaSchema,
+    Blog,
     BlogOnlySchema,
-    UserExcludeSchema,
-    UserAdditionalSchema,
+    BlogSchema,
     BlogSchemaExclude,
     BlogSchemaMeta,
+    BlogUserMetaSchema,
+    ExtendedUserSchema,
     User,
+    UserAdditionalSchema,
+    UserExcludeSchema,
+    UserFloatStringSchema,
+    UserIntSchema,
+    UserMetaSchema,
+    UserRelativeUrlSchema,
+    UserSchema,
     mockjson,
-    Blog,
 )
 
-
 random.seed(1)
 
 
@@ -2912,3 +2909,27 @@ def test_class_registry_returns_schema_type():
 
     SchemaClass = class_registry.get_class(DefinitelyUniqueSchema.__name__)
     assert SchemaClass is DefinitelyUniqueSchema
+
+
+@pytest.mark.parametrize("usage_location", ["meta", "init", "load"])
+def test_unknown_parameter_value_is_validated(usage_location):
+    class MySchema(Schema):
+        foo = fields.String()
+
+    with pytest.raises(
+        ValueError,
+        match="Object 'badval' is not a valid value for the 'unknown' parameter",
+    ):
+        # Meta.unknown setting gets caught at class creation time, since that's when
+        # metaclass __new__ runs
+        if usage_location == "meta":
+
+            class SubSchema(MySchema):
+                class Meta:
+                    unknown = "badval"
+
+        # usages in init and load are caught at call time, as expected
+        elif usage_location == "init":
+            MySchema(unknown="badval")
+        else:
+            MySchema().load({"foo": "bar"}, unknown="badval")