Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RealDictCursor No longer works with v2.8 #886

Closed
scottiris opened this Issue Apr 5, 2019 · 9 comments

Comments

Projects
None yet
4 participants
@scottiris
Copy link

scottiris commented Apr 5, 2019

psycopg2-birnary

Hello, I have been using this code to return a list of python dictionaries

conn = psycopg2.connect(connection_creds, cursor_factory=RealDictCursor)
c = conn.cursor()
c.execute(_sql, _data)
 rows = c.fetchall()
 c.close()
 return rows

Previous to 2.8, I get back a list of python dictionaries. Which is what I want.
Now, the same code returns a dictionary like object. Which breaks things downstream.

Am I missing something that needs to update?

Thanks

@laurikoobas

This comment has been minimized.

Copy link

laurikoobas commented Apr 5, 2019

pop() function on it is broken as well. It does remove the keys and values, but the values() iterator continues to function on top of _column_mapping internal list where the keys is not dropped from.

{'a':1, 'b':2}
# use pop('b') on it
{'a':1}
# but the following fails due to _column_mapping still containing the 'b' key
for i in blaah.values():
    print(i)

"solution" is this:

            d.pop('local_timezone', None)
            d._column_mapping = [c for c in d._column_mapping if c != 'local_timezone']

I expect it to break soon, but oh well.

@dvarrazzo

This comment has been minimized.

Copy link
Member

dvarrazzo commented Apr 5, 2019

Thank you for the report, I'll look into those.

@dvarrazzo

This comment has been minimized.

Copy link
Member

dvarrazzo commented Apr 5, 2019

@scottiris

It is not true that previous version of dict cursors were pure Python dictionaries: they have always been a subclass.

import psycopg2
import psycopg2.extras
cnn = psycopg2.connect("")
cur = cnn.cursor(cursor_factory=psycopg2.extras.RealDictCursor)
cur.execute("select 1 as a")
r = cur.fetchone()
r 
# {'a': 1}

type(r)
# psycopg2.extras.RealDictRow

psycopg2.__version__
# '2.7.2.dev0 (dt dec mx pq3 ext lo64)'

so what is that "breaks things downstream"?

Among the differences I see between 2.7 and 2.8:

git diff 2_7_7..2_8 -- lib/extras.py

 class RealDictRow(dict):
     """A `!dict` subclass representing a data record."""
 
-    __slots__ = ('_column_mapping')
+    __slots__ = ('_column_mapping',)
 
     def __init__(self, cursor):
-        dict.__init__(self)
+        super(RealDictRow, self).__init__()
         # Required for named cursors
         if cursor.description and not cursor.column_mapping:
             cursor._build_index()
@@ -273,15 +269,41 @@ class RealDictRow(dict):
     def __setitem__(self, name, value):
         if type(name) == int:
             name = self._column_mapping[name]
-        return dict.__setitem__(self, name, value)
+        super(RealDictRow, self).__setitem__(name, value)
 
     def __getstate__(self):
-        return (self.copy(), self._column_mapping[:])
+        return self.copy(), self._column_mapping[:]

In the past, the constructor was just wrong, I defend this change. So what is the change in behaviour you expect?

dvarrazzo added a commit that referenced this issue Apr 5, 2019

Fixed RealDictCursor.pop()
Addresses #886, but there might be something else broken there.
@dvarrazzo

This comment has been minimized.

Copy link
Member

dvarrazzo commented Apr 5, 2019

@laurikoobas pop() fixed, thank you for reporting 👍

@scottiris

This comment has been minimized.

Copy link
Author

scottiris commented Apr 5, 2019

I am running the code on Ubuntu 18

The issue that I discovered involves modifying the returned dictionary. Here is an example with some code.

recordset = {'a': 'b'}

for r in recordset:
    print(type(r)
    # <class 'psycopg2.extras.RealDictRow'>
    r['b'] = "c"
    print(r)
   # {'a': 'b', 'b': 'c'}

OK, at this point everything looks ok. However, the place where I saw the issue is passing this "RealDictRow" into a flask wtf form (FieldList) which is expecting a dictionary

form.myfieldlist.append_entry(r)

I am not sure I understand exactly what is happening, but that form record is changed during this append

for f in form.myfieldlist: 
    print(f.data)
    # 2.7.7
    #{'a': 'b', 'b', 'c'}

    # 2.8
    #{'a': 'b', 'b': 'None'}

Maybe you could make the argument that this is a problem with werkzeug (0.15.2), but I expect that there are other libraries where this is a problem.

@scottiris scottiris closed this Apr 5, 2019

@scottiris scottiris reopened this Apr 5, 2019

@dvarrazzo

This comment has been minimized.

Copy link
Member

dvarrazzo commented Apr 5, 2019

I don't want to make an argument: I want to fix a potential bug :)

So if you can investigate better and suggest how this row object behaves differently from a plain dictionary, I'm happy to correct it.

I'll take another look at it later, to verify all the standard dictionary methods behave as expected. However if you can verify what happens to that dict within werkzeug I would be more reassured in closing the bug.

@Rosuav

This comment has been minimized.

Copy link

Rosuav commented Apr 6, 2019

The part that breaks is that iterating always uses self._column_mapping even if some of those aren't in the dictionary. This also means that adding something to the dict will show up in some places and not others.

Complete test case:

postgres = psycopg2.connect("") # Need any legal connection for testing
cur = postgres.cursor(cursor_factory=psycopg2.extras.RealDictCursor)
cur.execute("select 1 as foo, 2 as bar")
data = cur.fetchone()
print(data) # Looks like a dict
print("foo:", data.pop("foo"))
data["quux"] = 3
print(data) # Looks fine still
print(*data) # Shows the wrong keys
print({**data}) # Boom! Can't iterate.

The solution, for me, was to flip the RealDict into, well, a real dict, before doing any mutation: Rosuav/MustardMine@45e8040

For Python versions 3.7 or newer (maybe 3.6 or newer), can RealDictCursor actually give us a real dict? Iteration order is guaranteed on those versions. Alternatively, RealDictRow can simply lose iter and friends on those versions, as dict.iter will do the right thing.

@dvarrazzo

This comment has been minimized.

Copy link
Member

dvarrazzo commented Apr 6, 2019

I guess those objects weren't supposed to be mutable in first place.

For any supported Python version, OrderedDict is available and it does pretty much what the RealDictRow is meant to. The constructor is different (rows take a cursor), however, with a bit of gymnastic it's possible to have an almost unmodified OrderedDict used as a row.

Committing to check if tests pass on all the grid...

dvarrazzo added a commit that referenced this issue Apr 6, 2019

Fixed RealDictCursor.pop()
Addresses #886, but there might be something else broken there.

@dvarrazzo dvarrazzo closed this in cc815e8 Apr 6, 2019

@scottiris

This comment has been minimized.

Copy link
Author

scottiris commented Apr 8, 2019

Just to follow up on this, the 2.8.1 fix to "pop" also fixed my issue. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.