diff --git a/AUTHORS.rst b/AUTHORS.rst index e4161f924..0bf611761 100644 --- a/AUTHORS.rst +++ b/AUTHORS.rst @@ -189,3 +189,4 @@ Contributors (chronological) - Kadir Can Ozden `@bysiber `_ - Dhruvil Darji `@dhruvildarji `_ - Haïm Dimer `@hdimer `_ +- `@ChrisJr404 `_ diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 2c2b60c7e..e3101cf81 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -1,6 +1,16 @@ Changelog ========= +(unreleased) +------------ + +Features: + +- `marshmallow.validate.OneOf` now emits a warning when a string is passed as + ``choices``, since each character of the string is then treated as a separate + choice. Wrap the string in a list to silence the warning (:issue:`2074`). + Thanks :user:`lafrech` for the suggestion. + 4.3.1 (2026-08-08) ------------------ diff --git a/src/marshmallow/validate.py b/src/marshmallow/validate.py index 35c0e8bce..170d7e7d8 100644 --- a/src/marshmallow/validate.py +++ b/src/marshmallow/validate.py @@ -4,6 +4,7 @@ import re import typing +import warnings from abc import ABC, abstractmethod from operator import attrgetter @@ -604,6 +605,18 @@ def __init__( *, error: str | None = None, ): + # A string is a valid iterable, but passing one as ``choices`` is + # almost always a mistake (e.g. ``OneOf("one", "two")`` where "one" + # ends up being the choices and "two" the labels). Each character is + # then treated as a separate choice. ``ContainsOnly`` deliberately + # accepts strings, so only warn for plain ``OneOf`` instances. + if type(self) is OneOf and isinstance(choices, str): + warnings.warn( + "Passing a string as 'choices' to OneOf is likely a mistake: " + "each character of the string is treated as a separate choice. " + "Wrap it in a list to silence this warning.", + stacklevel=2, + ) self.choices = choices self.choices_text = ", ".join(str(choice) for choice in self.choices) self.labels = labels if labels is not None else [] diff --git a/tests/test_validate.py b/tests/test_validate.py index 752e544e1..039cbfdac 100644 --- a/tests/test_validate.py +++ b/tests/test_validate.py @@ -1,6 +1,7 @@ """Tests for marshmallow.validate""" import re +import warnings import pytest @@ -759,15 +760,20 @@ def test_noneof_repr(): def test_oneof(): assert validate.OneOf([1, 2, 3])(2) == 2 - assert validate.OneOf("abc")("b") == "b" - assert validate.OneOf("")("") == "" + # A string still works as choices, but now emits a warning (see below). + with pytest.warns(UserWarning, match="each character"): + assert validate.OneOf("abc")("b") == "b" + with pytest.warns(UserWarning, match="each character"): + assert validate.OneOf("")("") == "" assert validate.OneOf(dict(a=0, b=1))("a") == "a" assert validate.OneOf((1, 2, None))(None) is None with pytest.raises(ValidationError, match="Must be one of: 1, 2, 3."): validate.OneOf([1, 2, 3])(4) + with pytest.warns(UserWarning, match="each character"): + abc_validator = validate.OneOf("abc") with pytest.raises(ValidationError): - validate.OneOf("abc")("d") + abc_validator("d") with pytest.raises(ValidationError): validate.OneOf((1, 2, 3))(None) with pytest.raises(ValidationError): @@ -776,8 +782,25 @@ def test_oneof(): validate.OneOf(())(()) with pytest.raises(ValidationError): validate.OneOf(dict(a=0, b=1))(0) + with pytest.warns(UserWarning, match="each character"): + digits_validator = validate.OneOf("123") with pytest.raises(ValidationError): - validate.OneOf("123")(1) + digits_validator(1) + + +def test_oneof_string_choices_warns(): + with pytest.warns(UserWarning, match="Passing a string as 'choices' to OneOf"): + validate.OneOf("abc") + + # Wrapping the string in a list silences the warning. + with warnings.catch_warnings(): + warnings.simplefilter("error") + validate.OneOf(list("abc")) + + # Subclasses that legitimately accept strings (ContainsOnly) do not warn. + with warnings.catch_warnings(): + warnings.simplefilter("error") + validate.ContainsOnly("abc") def test_oneof_options():