django-prometheus icon indicating copy to clipboard operation
django-prometheus copied to clipboard

Is CursorWrapper.executemany assuming len on param_list correct?

Open daniel5gh opened this issue 4 years ago • 1 comments

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

daniel5gh avatar Jul 23 '20 09:07 daniel5gh

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.

AntonOfTheWoods avatar Nov 02 '20 02:11 AntonOfTheWoods