From a0636067814a481da33ee877c2cbe0dc62165bd8 Mon Sep 17 00:00:00 2001 From: Daniele Varrazzo Date: Mon, 24 Jun 2024 01:53:26 +0200 Subject: [PATCH] fix: avoid to dump [multi]range as text when binary is requested If a range has no type information (e.g. is empty and it is a base class, not a specific range subtype), `upgrade()` would have returned a text dumper. This would have resulted in protocol violation, such as requesting unhealthy amount of memory to the backend... and it was tested too! Restoring sanity has a drawback. If we have an array of ranges, none of which has type information, we now fail binary dumping. Previously this worked, provided that a cast `::int8range[]` was provided, but I think with an extreme stretch of the protocol: we were producing binary arrays containing text representation of the ranges. I don't think the latter is quite valid. So I have a preference for dropping the feature in exchange of not violating the protocol. We trade a better dumping of the basic type (an xfail less there) in exchange of losing the ability of dumping an array of ranges, all empty, using a cast, in binary format. Binary dump works using a subclass such as `Int8Range` instead of `Range`. The lost use case is so marginal that I don't feel the need to document it. We don't even have documentation for the `Range` subclasses. --- psycopg/psycopg/types/multirange.py | 2 +- psycopg/psycopg/types/range.py | 2 +- tests/types/test_multirange.py | 46 ++++++++++++----------------- tests/types/test_range.py | 42 +++++++++++--------------- 4 files changed, 38 insertions(+), 54 deletions(-) diff --git a/psycopg/psycopg/types/multirange.py b/psycopg/psycopg/types/multirange.py index 735333049..1bf98ed34 100644 --- a/psycopg/psycopg/types/multirange.py +++ b/psycopg/psycopg/types/multirange.py @@ -206,7 +206,7 @@ class BaseMultirangeDumper(RecursiveDumper): item = self._get_item(obj) if item is None: - return MultirangeDumper(self.cls) + return self dumper: BaseMultirangeDumper if type(item) is int: diff --git a/psycopg/psycopg/types/range.py b/psycopg/psycopg/types/range.py index 665c9a6de..3327a8411 100644 --- a/psycopg/psycopg/types/range.py +++ b/psycopg/psycopg/types/range.py @@ -310,7 +310,7 @@ class BaseRangeDumper(RecursiveDumper): item = self._get_item(obj) if item is None: - return RangeDumper(self.cls) + return self dumper: BaseRangeDumper if type(item) is int: diff --git a/tests/types/test_multirange.py b/tests/types/test_multirange.py index 4175eebe1..fc074a419 100644 --- a/tests/types/test_multirange.py +++ b/tests/types/test_multirange.py @@ -5,7 +5,6 @@ from decimal import Decimal import pytest from psycopg import pq, sql -from psycopg import errors as e from psycopg.adapt import PyFormat from psycopg.types.range import Range from psycopg.types import multirange @@ -196,22 +195,21 @@ def test_dump_builtin_empty_wrapper(conn, wrapper, fmt_in): assert rec[2] == dumper.oid -@pytest.mark.parametrize("pgtype", mr_names) -@pytest.mark.parametrize( - "fmt_in", - [ - PyFormat.AUTO, - PyFormat.TEXT, - # There are many ways to work around this (use text, use a cast on the - # placeholder, use specific Range subclasses). - pytest.param( - PyFormat.BINARY, - marks=pytest.mark.xfail( - reason="can't dump array of untypes binary multirange without cast" - ), +pyformat_array = [ + PyFormat.AUTO, + PyFormat.TEXT, + # There are ways to work around this (use text, use specific Range subclasses). + pytest.param( + PyFormat.BINARY, + marks=pytest.mark.xfail( + reason="can't dump array of untyped binary multirange without cast" ), - ], -) + ), +] + + +@pytest.mark.parametrize("pgtype", mr_names) +@pytest.mark.parametrize("fmt_in", pyformat_array) def test_dump_builtin_array(conn, pgtype, fmt_in): mr1 = Multirange() # type: ignore[var-annotated] mr2 = Multirange([Range(bounds="()")]) # type: ignore[var-annotated] @@ -223,7 +221,7 @@ def test_dump_builtin_array(conn, pgtype, fmt_in): @pytest.mark.parametrize("pgtype", mr_names) -@pytest.mark.parametrize("fmt_in", PyFormat) +@pytest.mark.parametrize("fmt_in", pyformat_array) def test_dump_builtin_array_with_cast(conn, pgtype, fmt_in): mr1 = Multirange() # type: ignore[var-annotated] mr2 = Multirange([Range(bounds="()")]) # type: ignore[var-annotated] @@ -241,8 +239,8 @@ def test_dump_builtin_array_with_cast(conn, pgtype, fmt_in): @pytest.mark.parametrize("fmt_in", PyFormat) def test_dump_builtin_array_wrapper(conn, wrapper, fmt_in): wrapper = getattr(multirange, wrapper) - mr1 = Multirange() # type: ignore[var-annotated] - mr2 = Multirange([Range(bounds="()")]) # type: ignore[var-annotated] + mr1 = wrapper() + mr2 = wrapper([Range(bounds="()")]) cur = conn.execute( f"""select '{{"{{}}","{{(,)}}"}}' = %{fmt_in.value}""", ([mr1, mr2],) ) @@ -315,14 +313,8 @@ def test_copy_in(conn, min, max, bounds, format): r = Range(empty=True) mr = Multirange([r]) - try: - with cur.copy(f"copy copymr (mr) from stdin (format {format.name})") as copy: - copy.write_row([mr]) - except e.InternalError_: - if not min and not max and format == pq.Format.BINARY: - pytest.xfail("TODO: add annotation to dump multirange with no type info") - else: - raise + with cur.copy(f"copy copymr (mr) from stdin (format {format.name})") as copy: + copy.write_row([mr]) rec = cur.execute("select mr from copymr order by id").fetchone() if not r.isempty: diff --git a/tests/types/test_range.py b/tests/types/test_range.py index 08b2387ab..bb90fcd8b 100644 --- a/tests/types/test_range.py +++ b/tests/types/test_range.py @@ -5,7 +5,6 @@ from decimal import Decimal import pytest from psycopg import pq, sql -from psycopg import errors as e from psycopg.adapt import PyFormat from psycopg.types import range as range_module from psycopg.types.range import Range, RangeInfo, register_range @@ -78,22 +77,21 @@ def test_dump_builtin_empty_wrapper(conn, wrapper, fmt_in): assert cur.fetchone()[0] is True -@pytest.mark.parametrize("pgtype", range_names) -@pytest.mark.parametrize( - "fmt_in", - [ - PyFormat.AUTO, - PyFormat.TEXT, - # There are many ways to work around this (use text, use a cast on the - # placeholder, use specific Range subclasses). - pytest.param( - PyFormat.BINARY, - marks=pytest.mark.xfail( - reason="can't dump an array of untypes binary range without cast" - ), +pyformat_array = [ + PyFormat.AUTO, + PyFormat.TEXT, + # There are ways to work around this (use text, use specific Range subclasses). + pytest.param( + PyFormat.BINARY, + marks=pytest.mark.xfail( + reason="can't dump an array of untyped binary range without cast" ), - ], -) + ), +] + + +@pytest.mark.parametrize("pgtype", range_names) +@pytest.mark.parametrize("fmt_in", pyformat_array) def test_dump_builtin_array(conn, pgtype, fmt_in): r1 = Range(empty=True) # type: ignore[var-annotated] r2 = Range(bounds="()") # type: ignore[var-annotated] @@ -105,7 +103,7 @@ def test_dump_builtin_array(conn, pgtype, fmt_in): @pytest.mark.parametrize("pgtype", range_names) -@pytest.mark.parametrize("fmt_in", PyFormat) +@pytest.mark.parametrize("fmt_in", pyformat_array) def test_dump_builtin_array_with_cast(conn, pgtype, fmt_in): r1 = Range(empty=True) # type: ignore[var-annotated] r2 = Range(bounds="()") # type: ignore[var-annotated] @@ -211,14 +209,8 @@ def test_copy_in(conn, min, max, bounds, format): else: r = Range(empty=True) - try: - with cur.copy(f"copy copyrange (r) from stdin (format {format.name})") as copy: - copy.write_row([r]) - except e.ProtocolViolation: - if not min and not max and format == pq.Format.BINARY: - pytest.xfail("TODO: add annotation to dump ranges with no type info") - else: - raise + with cur.copy(f"copy copyrange (r) from stdin (format {format.name})") as copy: + copy.write_row([r]) rec = cur.execute("select r from copyrange order by id").fetchone() assert rec[0] == r -- 2.47.2