KeyError in util.HistoryArraySet.added_items()

[Former user]

OS: Mac 10.3.9

Python: 2.4.1

SQLAlchemy: 0.1.4

This may not be a bug, but rather a misunderstanding of how to use SQLAlchemy. At any rate, the code I present below is likely a better choice.

Scenario: I had two independent tables which were parents in a many-to-many relationship with a third table. The third table could have records created on it independently, and then later queried and added via table1.table3_assoc.append(table3_record). However, when I tried using this, I consistently got the following error:

Traceback (most recent call last):
[snip](snip)
  File "[snip](snip)/sqlalchemy/util.py", line 388, in added_items
    return [for key in self.data if self.records[key](key) is True]
  File "[snip](snip)/sqlalchemy/util.py", line 217, in __getitem__
    return dict.__getitem__(self, key)
KeyError: [snip](snip)

Some debugging revealed that most of the times that added_items() was called, self.data and self.records were both none. However, one of the times it was called, there was something in self.data but nothing in self.records.

I simply changed line util.py 384 (333 in current svn) in the following manner:

-- return [for key in self.data if self.records[key](key) is True]
++ return [for key in self.data if self.records.get(key) is True](key)

I noticed there were a few other lines like this in the same file; I didn't mess with those, as I haven't run into any issues... it's worth considering, though: if you're doing a test on truth, you probably want None instead of KeyError.

So, regardless of whether this is a bug or not, the second option is likely a better bet. If I am doing something wrong, then perhaps an exception should be raised at an appropriate point before the KeyError occurs?

d

P.S. It took all day for me to "decide" (incorrectly?) that this was a bug in SQLAlchemy... don't really have the energy to put together a minimal example with code. If you think that this might be a bug, and that I am using SQLAlchmey correctly, just let me know, and I'll knock something up real quick on another day. I've got a ''lot'' of catching up to do now. Thanks.

[Mike Bayer]

I know the get() seems like a better option at face value but in fact this would indicate that you somehow have "broken the rules" with the HistoryArraySet; if you have an element in the "data" array which is not in the "records" dictionary, the HistoryArraySet is in an invalid state; all records in "data" must have a corresponding entry in "records" at all times.

so the "bug" is that you somehow managed to get that to happen, and for that I definitely need to see what youre doing.

[Former user]

Yeah, I was afraid of something like that.

Even with the heavy delay from yesterday, SQLAlchmey saved me so much time and work, I managed to actually get ahead last night. I will put together a minimal example today and post that.

Might I make a suggestion though? Instead of having syntax play the role of prevention (without informtion), maybe add a try/except around {{{self.recordskey}}} and on {{{KeyError}}}, raise {{{InvalidHistoryState}}} (with an appropriate message/Error docstring)?

[Mike Bayer]

oh absolutely, stuff like that should be caught, ill see if I can throw that in. I think you will find though that theres like hundreds of different scenarios all throughout the code where stuff like that will happen, and most of them will eventually need try/excepts as they are identified...such is the nature of Python...

[Former user]

That's great to hear -- thanks!

I will shortly attach an example, as promised.

[Former user]

An example that runs from the command line and exhibits the described behaviour. Useful docstring at the top of the file.

[Mike Bayer]

my 2 second impression is that the HistoryArraySet object doesnt have an extend() method (its falling back on the one thats part of UserList). I am running out for a bit, but if you want to try tacking on an extend() method to HistoryArraySet which performs the requisite setrecord() calls in a manner simlar to the other methods, i think the example will get much further.

[Former user]

Hmm... I think I'm missing your first impression -- I'll wait to get that, so that I have a full context :-)

[Mike Bayer]

lines 157 and 166 of your script are using artists.extend(data). HistoryArraySet does not properly implement this method. so you can either: dont use extend() on those two lines; fix the extend() method yourself in HistoryArraySet; or wait for me to get around to it.

[Former user]

Ooooo! I read "2 second" as "2nd", that's why I said the bit about your "first" impression. Heh.

Got it! Thanks :-) I will work on that tonight and submit a patch.

[Former user]

Corrected a few bugs in the script.

[Former user]

Okay, you were right on. It's so trivial a fix to {{{sqlalchemy.util.HistoryArraySet}}}, that it would take more effort to download and apply the patch that to simply edit it by hand. Here's all I did:

@@ -376,6 +376,9 @@
     def insert(self, i, item): 
         if self._setrecord(item):
             self.data.insert(i, item)
+    def extend(self, item_list):
+        for item in item_list:
+            self.append(item)
     def pop(self, i=-1):
         item = self.data[i](i)
         if self._delrecord(item):
@@ -463,4 +466,4 @@

This approach is based on my limited understanding of SQLAlchemy internals. My reasoning goes like this:

Looking at {{{HistoryArraySet.append()}}}, before an item gets inserted into {{{self.data}}}, there needs to be a check that the same item exists in {{{self._setrecord}}}. So it seems iteration over the list is required, and one can't simply use {{{super(HistoryArraySet, self).extend(item)}}}.

With the application of this change, the {{{KeyError}}} issue is addressed and the (updated) example runs without problems.

[Mike Bayer]

great, its committed in changeset:1308changeset:1309, and we're fixed.