Opened 10 years ago

Closed 6 years ago

#24726 closed Bug (fixed)

ArrayField with IntegerRangeField as base field generates invalid SQL on Insert operations

Reported by: Ederson Mota Owned by: vinay karanam
Component: contrib.postgres Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

I'm with problems trying to create a model object that contains an ArrayField with IntegerRangeField as base.

I asked about this on #django, and for @knbk this seems a bug. I believe it may be right.

Model:

class PostgreSQLModel(models.Model):
    class Meta:
        abstract = True


class IntegerRangeArrayModel(PostgreSQLModel):
    field = ArrayField(IntegerRangeField())

The initial migration an the table are created fine.

                           Table "public.core_integerrangearraymodel"
 Column |    Type     |                                Modifiers
--------+-------------+--------------------------------------------------------------------------
 id     | integer     | not null default nextval('core_integerrangearraymodel_id_seq'::regclass)
 field  | int4range[] | not null
Indexes:
    "core_integerrangearraymodel_pkey" PRIMARY KEY, btree (id)

However, when I try to create an object, it fails with a SQL error.

integer_range_list = [
    (10, 20),
    (30, 40),
]

IntegerRangeArrayModel.objects.create(
    field=integer_range_list
)

======================================================================
ERROR: test_create_with_tuples (core.tests.TestIntegerRangeArray)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/eder/.virtualenvs/integerrange-array/lib/python3.4/site-packages/django/db/backends/utils.py", line 64, in execute
    return self.cursor.execute(sql, params)
psycopg2.ProgrammingError: column "field" is of type int4range[] but expression is of type text[]
LINE 1: ...O "core_integerrangearraymodel" ("field") VALUES (ARRAY['[10...
                                                             ^
HINT:  You will need to rewrite or cast the expression.


The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/home/eder/devel/integerrange-array/core/tests.py", line 17, in test_create_with_tuples
    field=integer_range_list
  File "/home/eder/.virtualenvs/integerrange-array/lib/python3.4/site-packages/django/db/models/manager.py", line 127, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
  File "/home/eder/.virtualenvs/integerrange-array/lib/python3.4/site-packages/django/db/models/query.py", line 348, in create
    obj.save(force_insert=True, using=self.db)
  File "/home/eder/.virtualenvs/integerrange-array/lib/python3.4/site-packages/django/db/models/base.py", line 710, in save
    force_update=force_update, update_fields=update_fields)
  File "/home/eder/.virtualenvs/integerrange-array/lib/python3.4/site-packages/django/db/models/base.py", line 738, in save_base
    updated = self._save_table(raw, cls, force_insert, force_update, using, update_fields)
  File "/home/eder/.virtualenvs/integerrange-array/lib/python3.4/site-packages/django/db/models/base.py", line 822, in _save_table
    result = self._do_insert(cls._base_manager, using, fields, update_pk, raw)
  File "/home/eder/.virtualenvs/integerrange-array/lib/python3.4/site-packages/django/db/models/base.py", line 861, in _do_insert
    using=using, raw=raw)
  File "/home/eder/.virtualenvs/integerrange-array/lib/python3.4/site-packages/django/db/models/manager.py", line 127, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
  File "/home/eder/.virtualenvs/integerrange-array/lib/python3.4/site-packages/django/db/models/query.py", line 920, in _insert
    return query.get_compiler(using=using).execute_sql(return_id)
  File "/home/eder/.virtualenvs/integerrange-array/lib/python3.4/site-packages/django/db/models/sql/compiler.py", line 963, in execute_sql
    cursor.execute(sql, params)
  File "/home/eder/.virtualenvs/integerrange-array/lib/python3.4/site-packages/django/db/backends/utils.py", line 64, in execute
    return self.cursor.execute(sql, params)
  File "/home/eder/.virtualenvs/integerrange-array/lib/python3.4/site-packages/django/db/utils.py", line 97, in __exit__
    six.reraise(dj_exc_type, dj_exc_value, traceback)
  File "/home/eder/.virtualenvs/integerrange-array/lib/python3.4/site-packages/django/utils/six.py", line 658, in reraise
    raise value.with_traceback(tb)
  File "/home/eder/.virtualenvs/integerrange-array/lib/python3.4/site-packages/django/db/backends/utils.py", line 64, in execute
    return self.cursor.execute(sql, params)
django.db.utils.ProgrammingError: column "field" is of type int4range[] but expression is of type text[]
LINE 1: ...O "core_integerrangearraymodel" ("field") VALUES (ARRAY['[10...
                                                             ^
HINT:  You will need to rewrite or cast the expression.

There are no difference if a NumericRange object is used instead.

integer_range_list = [
    NumericRange(10, 20),
    NumericRange(30, 40),
]

IntegerRangeArrayModel.objects.create(
    field=integer_range_list
)

The generated SQL is:

INSERT INTO "core_integerrangearraymodel" ("field") VALUES (ARRAY['[10,20)', '[30,40)']) RETURNING "core_integerrangearraymodel"."id"

It should be something like this:

INSERT INTO "core_integerrangearraymodel" ("field") VALUES (ARRAY['[10,20)'::int4range, '[30,40)'::int4range]) RETURNING "core_integerrangearraymodel"."id"

Or this:

INSERT INTO "core_integerrangearraymodel" ("field") VALUES ('{"[10,20)","[30,40)"}') RETURNING "core_integerrangearraymodel"."id";

I created a test project, with more details, and some test cases to help illustrate the problem.

https://github.com/edrmp/integerrange-array

There is Travis-CI too

https://travis-ci.org/edrmp/integerrange-array

Thanks in advance.

Attachments (1)

ticket_24726.diff (4.3 KB ) - added by Ederson Mota 10 years ago.
Diff between django/django@master and edrmp/django@ticket_24726

Download all attachments as: .zip

Change History (16)

comment:1 by Tim Graham, 10 years ago

Triage Stage: UnreviewedAccepted

comment:2 by Ederson Mota, 10 years ago

I forked the Django repository and added some tests:

https://github.com/edrmp/django/tree/ticket_24726

by Ederson Mota, 10 years ago

Attachment: ticket_24726.diff added

Diff between django/django@master and edrmp/django@ticket_24726

comment:3 by Ederson Mota, 10 years ago

Taking a closer look at the exception generated by one of tests created mentioned above, lead me to some things that I would like to share.

The exception is trowed at db/backends/utils.py, line 64, method execute(), when trying to:

return self.cursor.execute(sql, params)

With the SQL:

INSERT INTO "postgres_tests_integerrangesarraymodel" ("int_ranges", "bigint_ranges") VALUES (%s, %s) RETURNING "postgres_tests_integerrangesarraymodel"."id"

And params:

([NumericRange(10, 20, '[)'), NumericRange(30, 40, '[)')], [NumericRange(7000000000, 10000000000, '[)'), NumericRange(50000000000, 70000000000, '[)')])

At this time, it's a method of psycopg2.extensions.cursor.

I wondered if this error will also occur using pycopg2 directly, and the answer is yes.

import psycopg2
from psycopg2.extras import NumericRange

# Connect to an existing database
conn = psycopg2.connect("dbname=integerrange-array user=postgres")

# Open a cursor to perform database operations
cur = conn.cursor()


# Pass data to fill a query placeholders and let Psycopg perform
# the correct conversion (no more SQL injections!)
cur.execute("INSERT INTO core_integerrangearraymodel (field) VALUES (%s)",
            ([NumericRange(10, 20, '[)'), NumericRange(30, 40, '[)')],)
            )

Traceback (most recent call last):
  File "test_psycopg2.py", line 14, in <module>
    ([NumericRange(10, 20, '[)'), NumericRange(30, 40, '[)')],)
psycopg2.ProgrammingError: column "field" is of type int4range[] but expression is of type text[]
LINE 1: ... INTO core_integerrangearraymodel (field) VALUES (ARRAY['[10...
                                                             ^
HINT:  You will need to rewrite or cast the expression.

I think that this issue on psycopg2 is somehow related:

https://github.com/psycopg/psycopg2/issues/231

It's not a bug, but he describe a way to circumvent this behavior:

https://github.com/psycopg/psycopg2/issues/231#issuecomment-53741200

Thanks.

comment:4 by Marc Tamlyn, 10 years ago

This is a tricky one to fix. My inclination is that half the fault lies with postgres, and perhaps it could be fixed with psycopg2 as well.

The nicest solution is to add a ::int4range[] at the end of the parameter, but doing that is tricky to implement. It should be possible to register a custom typecaster for arrays of ranges with psycopg2. It feels like it should just work though...

I have raised https://github.com/psycopg/psycopg2/issues/313 with psycopg2 to get their feedback.

comment:5 by Daniele Varrazzo, 9 years ago

Hello,

I think your best shot is to use a NumericRange subtype with an adapter adding ::int4range. Adding ::int4range[] after the array is impractical as you should know subclass the list instead.

An example implementation is in this comment.

I'll think whether we should introduce these objects in the library, I'm not so sure, because usually end users are able to specify a placeholder like %s::int4range[] and work around the problem without introducing too many details of the Postgres data system into the Python objects.

comment:6 by Adam Zapletal, 8 years ago

Marc,

How would you like to proceed with this ticket? I'm willing to help close this with some guidance.

Fixing this would help my use case!

Thanks!

comment:7 by Adam Zapletal, 8 years ago

Owner: set to Adam Zapletal
Status: newassigned

comment:8 by Adam Zapletal, 8 years ago

https://github.com/Crossway/django/commit/718bfaa18a12fb059d9e5cf8c4c716402ac8cb4d

This patch integrates and updates the contributions from edrmp and dvarrazzo above. The tests pass, but I'm not sure I put the psycopg2 adapter bits in the right place. I guess this is more a proof of concept.

I think there needs to be more discussion about how arrays of range fields should work. Lookups don't work right now, and I'm not sure how they would.

Also, dvarrazzo's suggestion to subclass NumericRange works, but wouldn't that require range objects destined for the database to be created with the adapted subclasses? I'm not sure if that's desired for Django. I'm interested to hear what the core team thinks.

Anyway, I hope this patch is helpful!

I'm unassigning this ticket for now as I don't think I can make progress without some design discussion taking place.

comment:9 by Adam Zapletal, 8 years ago

Owner: Adam Zapletal removed
Status: assignednew

comment:10 by Adam Zapletal, 8 years ago

I interacted with mjtamlyn over email, and he asked me to open a PR with my patch.

PR is here: https://github.com/django/django/pull/7099

comment:11 by Adam Zapletal, 8 years ago

Has patch: set

comment:12 by Tim Graham, 8 years ago

Patch needs improvement: set

I guess this isn't ready for review since it's marked "WIP".

comment:13 by Tim Graham, 8 years ago

#27808 is related or a duplicate.

comment:14 by vinay karanam, 6 years ago

Owner: set to vinay karanam
Status: newassigned

PR for #28291 also fixes this issue.

comment:15 by Tim Graham <timograham@…>, 6 years ago

Resolution: fixed
Status: assignedclosed

In 3af695e:

Fixed #28291, #24726 -- Fixed ArrayField with JSONField and RangeFields.

Note: See TracTickets for help on using tickets.
Back to Top