From: Mike Bayer Date: Wed, 11 Oct 2017 21:56:14 +0000 (-0400) Subject: Disallow all ambiguous boolean values for Boolean X-Git-Tag: rel_1_2_0~57^2 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=c63658973c95996b73251e0953cf4c598cdb262b;p=thirdparty%2Fsqlalchemy%2Fsqlalchemy.git Disallow all ambiguous boolean values for Boolean In release 1.1, the :class:`.Boolean` type was broken in that boolean coercion via ``bool()`` would occur for backends that did not feature "native boolean", but would not occur for native boolean backends, meaning the string ``"0"`` now behaved inconsistently. After a poll, a consensus was reached that non-boolean values should be raising an error, especially in the ambiguous case of string ``"0"``; so the :class:`.Boolean` datatype will now raise ``ValueError`` if an incoming value is not within the range ``None, True, False, 1, 0``. Change-Id: If70c4f79c266f0dd1a0306c0ffe7acb9c66c4cc3 Fixes: #4102 --- diff --git a/doc/build/changelog/migration_12.rst b/doc/build/changelog/migration_12.rst index f6ca7f8126..5eba2def56 100644 --- a/doc/build/changelog/migration_12.rst +++ b/doc/build/changelog/migration_12.rst @@ -545,6 +545,44 @@ to query across the two proxies ``A.c_values``, ``AtoB.c_value``: New Features and Improvements - Core ==================================== +.. _change_4102: + +Boolean datatype now enforces strict True/False/None values +----------------------------------------------------------- + +In version 1.1, the change described in :ref:`change_3730` produced an +unintended side effect of altering the way :class:`.Boolean` behaves when +presented with a non-integer value, such as a string. In particular, the +string value ``"0"``, which would previously result in the value ``False`` +being generated, would now produce ``True``. Making matters worse, the change +in behavior was only for some backends and not others, meaning code that sends +string ``"0"`` values to :class:`.Boolean` would break inconsistently across +backends. + +The ultimate solution to this problem is that **string values are not supported +with Boolean**, so in 1.2 a hard ``TypeError`` is raised if a non-integer / +True/False/None value is passed. Additionally, only the integer values +0 and 1 are accepted. + +To accomodate for applications that wish to have more liberal interpretation +of boolean values, the :class:`.TypeDecorator` should be used. Below +illustrates a recipe that will allow for the "liberal" behavior of the pre-1.1 +:class:`.Boolean` datatype:: + + from sqlalchemy import Boolean + from sqlalchemy import TypeDecorator + + class LiberalBoolean(TypeDecorator): + impl = Boolean + + def process_bind_param(self, value, dialect): + if value is not None: + value = bool(int(value)) + return value + + +:ticket:`4102` + .. _change_3919: Pessimistic disconnection detection added to the connection pool diff --git a/doc/build/changelog/unreleased_12/4102.rst b/doc/build/changelog/unreleased_12/4102.rst new file mode 100644 index 0000000000..1220a23cf0 --- /dev/null +++ b/doc/build/changelog/unreleased_12/4102.rst @@ -0,0 +1,16 @@ +.. change:: + :tags: bug, sql + :tickets: 4102 + + In release 1.1, the :class:`.Boolean` type was broken in that + boolean coercion via ``bool()`` would occur for backends that did not + feature "native boolean", but would not occur for native boolean backends, + meaning the string ``"0"`` now behaved inconsistently. After a poll, a + consensus was reached that non-boolean values should be raising an error, + especially in the ambiguous case of string ``"0"``; so the :class:`.Boolean` + datatype will now raise ``ValueError`` if an incoming value is not + within the range ``None, True, False, 1, 0``. + + .. seealso:: + + :ref:`change_4102` \ No newline at end of file diff --git a/lib/sqlalchemy/processors.py b/lib/sqlalchemy/processors.py index 17f7eccdab..8ee4f470fc 100644 --- a/lib/sqlalchemy/processors.py +++ b/lib/sqlalchemy/processors.py @@ -49,13 +49,6 @@ def str_to_datetime_processor_factory(regexp, type_): return process -def boolean_to_int(value): - if value is None: - return None - else: - return int(bool(value)) - - def py_fallback(): def to_unicode_processor_factory(encoding, errors=None): decoder = codecs.getdecoder(encoding) diff --git a/lib/sqlalchemy/sql/sqltypes.py b/lib/sqlalchemy/sql/sqltypes.py index 061f8de96a..2a30bf8321 100644 --- a/lib/sqlalchemy/sql/sqltypes.py +++ b/lib/sqlalchemy/sql/sqltypes.py @@ -1581,9 +1581,22 @@ class Boolean(Emulated, TypeEngine, SchemaType): """A bool datatype. - Boolean typically uses BOOLEAN or SMALLINT on the DDL side, and on + :class:`.Boolean` typically uses BOOLEAN or SMALLINT on the DDL side, and on the Python side deals in ``True`` or ``False``. + The :class:`.Boolean` datatype currently has two levels of assertion + that the values persisted are simple true/false values. For all + backends, only the Python values ``None``, ``True``, ``False``, ``1`` + or ``0`` are accepted as parameter values. For those backends that + don't support a "native boolean" datatype, a CHECK constraint is also + created on the target column. Production of the CHECK constraint + can be disabled by passing the :paramref:`.Boolean.create_constraint` + flag set to ``False``. + + .. versionchanged:: 1.2 the :class:`.Boolean` datatype now asserts that + incoming Python values are already in pure boolean form. + + """ __visit_name__ = 'boolean' @@ -1631,20 +1644,40 @@ class Boolean(Emulated, TypeEngine, SchemaType): def python_type(self): return bool + _strict_bools = frozenset([None, True, False]) + + def _strict_as_bool(self, value): + if value not in self._strict_bools: + if not isinstance(value, int): + raise TypeError( + "Not a boolean value: %r" % value) + else: + raise ValueError( + "Value %r is not None, True, or False" % value) + return value + def literal_processor(self, dialect): compiler = dialect.statement_compiler(dialect, None) true = compiler.visit_true(None) false = compiler.visit_false(None) def process(value): - return true if value else false + return true if self._strict_as_bool(value) else false return process def bind_processor(self, dialect): + _strict_as_bool = self._strict_as_bool if dialect.supports_native_boolean: - return None + _coerce = bool else: - return processors.boolean_to_int + _coerce = int + + def process(value): + value = _strict_as_bool(value) + if value is not None: + value = _coerce(value) + return value + return process def result_processor(self, dialect, coltype): if dialect.supports_native_boolean: diff --git a/test/engine/test_processors.py b/test/engine/test_processors.py index 83b5a79489..9f0055e05f 100644 --- a/test/engine/test_processors.py +++ b/test/engine/test_processors.py @@ -34,43 +34,6 @@ class _BooleanProcessorTest(fixtures.TestBase): ) -class PyBooleanProcessorTest(_BooleanProcessorTest): - @classmethod - def setup_class(cls): - from sqlalchemy import processors - cls.module = type( - "util", (object,), - dict( - (k, staticmethod(v)) - for k, v in list(processors.py_fallback().items()) - ) - ) - - def test_bool_to_int_false(self): - from sqlalchemy import processors - eq_(processors.boolean_to_int(False), 0) - - def test_bool_to_int_true(self): - from sqlalchemy import processors - eq_(processors.boolean_to_int(True), 1) - - def test_bool_to_int_positive_int(self): - from sqlalchemy import processors - eq_(processors.boolean_to_int(5), 1) - - def test_bool_to_int_negative_int(self): - from sqlalchemy import processors - eq_(processors.boolean_to_int(-10), 1) - - def test_bool_to_int_zero(self): - from sqlalchemy import processors - eq_(processors.boolean_to_int(0), 0) - - def test_bool_to_int_one(self): - from sqlalchemy import processors - eq_(processors.boolean_to_int(1), 1) - - class CBooleanProcessorTest(_BooleanProcessorTest): __requires__ = ('cextensions',) diff --git a/test/sql/test_types.py b/test/sql/test_types.py index f9b5b49454..dd799de1b5 100644 --- a/test/sql/test_types.py +++ b/test/sql/test_types.py @@ -2621,15 +2621,14 @@ class BooleanTest( def test_nonnative_processor_coerces_to_onezero(self): boolean_table = self.tables.boolean_table with testing.db.connect() as conn: - conn.execute( + assert_raises_message( + exc.StatementError, + "Value 5 is not None, True, or False", + conn.execute, boolean_table.insert(), {"id": 1, "unconstrained_value": 5} ) - eq_( - conn.scalar("select unconstrained_value from boolean_table"), - 1 - ) @testing.skip_if(lambda: testing.db.dialect.supports_native_boolean) def test_nonnative_processor_coerces_integer_to_boolean(self): @@ -2650,6 +2649,169 @@ class BooleanTest( True ) + def test_bind_processor_coercion_native_true(self): + proc = Boolean().bind_processor( + mock.Mock(supports_native_boolean=True)) + is_(proc(True), True) + + def test_bind_processor_coercion_native_false(self): + proc = Boolean().bind_processor( + mock.Mock(supports_native_boolean=True)) + is_(proc(False), False) + + def test_bind_processor_coercion_native_none(self): + proc = Boolean().bind_processor( + mock.Mock(supports_native_boolean=True)) + is_(proc(None), None) + + def test_bind_processor_coercion_native_0(self): + proc = Boolean().bind_processor( + mock.Mock(supports_native_boolean=True)) + is_(proc(0), False) + + def test_bind_processor_coercion_native_1(self): + proc = Boolean().bind_processor( + mock.Mock(supports_native_boolean=True)) + is_(proc(1), True) + + def test_bind_processor_coercion_native_str(self): + proc = Boolean().bind_processor( + mock.Mock(supports_native_boolean=True)) + assert_raises_message( + TypeError, + "Not a boolean value: 'foo'", + proc, "foo" + ) + + def test_bind_processor_coercion_native_int_out_of_range(self): + proc = Boolean().bind_processor( + mock.Mock(supports_native_boolean=True)) + assert_raises_message( + ValueError, + "Value 15 is not None, True, or False", + proc, 15 + ) + + def test_bind_processor_coercion_nonnative_true(self): + proc = Boolean().bind_processor( + mock.Mock(supports_native_boolean=False)) + eq_(proc(True), 1) + + def test_bind_processor_coercion_nonnative_false(self): + proc = Boolean().bind_processor( + mock.Mock(supports_native_boolean=False)) + eq_(proc(False), 0) + + def test_bind_processor_coercion_nonnative_none(self): + proc = Boolean().bind_processor( + mock.Mock(supports_native_boolean=False)) + is_(proc(None), None) + + def test_bind_processor_coercion_nonnative_0(self): + proc = Boolean().bind_processor( + mock.Mock(supports_native_boolean=False)) + eq_(proc(0), 0) + + def test_bind_processor_coercion_nonnative_1(self): + proc = Boolean().bind_processor( + mock.Mock(supports_native_boolean=False)) + eq_(proc(1), 1) + + def test_bind_processor_coercion_nonnative_str(self): + proc = Boolean().bind_processor( + mock.Mock(supports_native_boolean=False)) + assert_raises_message( + TypeError, + "Not a boolean value: 'foo'", + proc, "foo" + ) + + def test_bind_processor_coercion_nonnative_int_out_of_range(self): + proc = Boolean().bind_processor( + mock.Mock(supports_native_boolean=False)) + assert_raises_message( + ValueError, + "Value 15 is not None, True, or False", + proc, 15 + ) + + def test_literal_processor_coercion_native_true(self): + proc = Boolean().literal_processor( + default.DefaultDialect(supports_native_boolean=True)) + eq_(proc(True), "true") + + def test_literal_processor_coercion_native_false(self): + proc = Boolean().literal_processor( + default.DefaultDialect(supports_native_boolean=True)) + eq_(proc(False), "false") + + def test_literal_processor_coercion_native_1(self): + proc = Boolean().literal_processor( + default.DefaultDialect(supports_native_boolean=True)) + eq_(proc(1), "true") + + def test_literal_processor_coercion_native_0(self): + proc = Boolean().literal_processor( + default.DefaultDialect(supports_native_boolean=True)) + eq_(proc(0), "false") + + def test_literal_processor_coercion_native_str(self): + proc = Boolean().literal_processor( + default.DefaultDialect(supports_native_boolean=True)) + assert_raises_message( + TypeError, + "Not a boolean value: 'foo'", + proc, "foo" + ) + + def test_literal_processor_coercion_native_int_out_of_range(self): + proc = Boolean().literal_processor( + default.DefaultDialect(supports_native_boolean=True)) + assert_raises_message( + ValueError, + "Value 15 is not None, True, or False", + proc, 15 + ) + + def test_literal_processor_coercion_nonnative_true(self): + proc = Boolean().literal_processor( + default.DefaultDialect(supports_native_boolean=False)) + eq_(proc(True), "1") + + def test_literal_processor_coercion_nonnative_false(self): + proc = Boolean().literal_processor( + default.DefaultDialect(supports_native_boolean=False)) + eq_(proc(False), "0") + + def test_literal_processor_coercion_nonnative_1(self): + proc = Boolean().literal_processor( + default.DefaultDialect(supports_native_boolean=False)) + eq_(proc(1), "1") + + def test_literal_processor_coercion_nonnative_0(self): + proc = Boolean().literal_processor( + default.DefaultDialect(supports_native_boolean=False)) + eq_(proc(0), "0") + + def test_literal_processor_coercion_nonnative_str(self): + proc = Boolean().literal_processor( + default.DefaultDialect(supports_native_boolean=False)) + assert_raises_message( + TypeError, + "Not a boolean value: 'foo'", + proc, "foo" + ) + + def test_literal_processor_coercion_native_int_out_of_range(self): + proc = Boolean().literal_processor( + default.DefaultDialect(supports_native_boolean=True)) + assert_raises_message( + ValueError, + "Value 15 is not None, True, or False", + proc, 15 + ) + + class PickleTest(fixtures.TestBase):