django-prometheus
django-prometheus copied to clipboard
Is CursorWrapper.executemany assuming len on param_list correct?
Looking at
def executemany(self, query, param_list, *args, **kwargs):
execute_total.labels(alias, vendor).inc(len(param_list))
execute_many_total.labels(alias, vendor).inc(len(param_list))
with query_duration_seconds.labels(**labels).time(), (
ExceptionCounterByType(errors_total, extra_labels=labels)
According to PEP-249 [1], param_list
should be a "sequences or mappings", additionally footnote 5 in PEP-249 [1] says:
The module will use the
__getitem__
method of the parameters object to map either positions (integers) or names (strings) to parameter values. This allows for both sequences and mappings to be used as input.
The cursor wrapper expects __len__
on the param_list
.
I have a situation where I am passing in an itertools.chain
object as param_list
, it doesn't have __len__
or __getitem__
. A generator wouldn't have them either, but people to use generators here, for example psycopg2
tests do [2].
I am going to apply Postel's Law and change my implementation to pass in a tuple instead of the itertools.chain
, but maybe django-prometheus
should be more liberal in what it accepts and do something like:
def executemany(self, query, param_list, *args, **kwargs):
if not hasattr(param_list, '__len__'):
# to cater for param_list not having len, (generator or itertools.chain for example)
param_list = list(param_list)
By adding this slight overhead, robustness is increased. The only downside I see to this is that someone may not expect a generator to be evaluated in one go.
I'm happy to raise a PR if you think this is a good idea.
[1] https://www.python.org/dev/peps/pep-0249/ [2] https://github.com/psycopg/psycopg2/blob/2bee47efacd3c114fd93ce84880f91799b0b72af/tests/test_cursor.py#L68
Hi @asherf , I have just wasted quite a bit of time on this (I'm a little blind but the point remains!) and it would be good to get the maintainer's view on this. I can obviously make sure to never use generators again in django
db raw queries but... the fix seems solid-ish and it would be good to get an opinion on whether one of us should submit a PR or not.