Define a custom collection

[Luca Wehrstedt]

Hello everyone,

I'm posting here because I don't know if the issues I'm having are bugs or just improper use of the library.

I'd like to define a custom collection class since I'm not completely satisfied by the default ones. In particular I'd like to subclass MappedCollection and make it provide more consistency. That is, if on a `parent` object I have a `children` attribute which is an instance of my new collection class and contains objects of the `Child` class indexed by their `column` attribute, I want:

• `parent['new_key'] = existing_child` to do `del parent['old_key']` (if the child was already in the collection) and `existing_child.column = 'new_key'` (and also remove the object previously associated with 'new_key', if any, from the collection, as MappedCollection already does [1])
  
• `existing_child.column = 'new_key'` to have the same effects as `parent['new_key'] = existing_child`, described above.

[1] I forgot to mention that I assume there to be a UniqueConstraint on the value of `Child.column`

My plan to do all of this was:

• to pass the name of the `column` attribute as a parameter to the constructor of the class
• to have the class figure out by itself which object/classes/relationships it was bound to, using the collection adapter
• to have the collection listen on 'set' events of the `column` property of the `Child` class
• do all the "magic" inside the event listener and the __setitem__ method

Additionally, I wanted to override the converter to make it accept any sequence (list, set, etc.) of Child objects, since the collection knows how to extract their keys and make it become a dictionary.

While trying to do it I faced the following issues:

• I tried to define a method "def link(self, adapter):" with the "@collection.link" decorator but it didn't work (it wasn't called by SA). By looking at the code I guessed that a workaround would be to call the method `_sa_on_link` and, indeed, it worked. What's the proper way to use the "collection.link" decorator?
• I was able to add a listener for 'set' events but not to remove it. A comment in the code said that this feature is not implemented and gave suggestions on how to do it. No big issue, since the collection is almost never unlinked and thus it's hardly needed to remove the listener, yet it's an undocumented behavior.
• I tried to define a method "def convert(self, collection):" with the "@collection.converter" decorator but it didn't work. Even renaming it to "_sa_converter" didn't help. How can I override the converter of MappedCollection (that I'm subclassing)?

Thanks for any help you can provide,

Luca

[Michael Bayer]

making new MappedCollections and especially trying to work with the very old "on link" hooks, which to my knowledge have never been used (but of course we will accept patches to get those fixed/tested if you need), is a hard road to travel.   Your two primary use cases seem like they could more easily be accomplished using attribute set events and leaving the collection class alone, have you tried that ?   In particular, detecting "existing_child.column = value" has to be through an attribute event since that's a scalar set.

Otherwise, the collection mechanics were written by someone else years ago, I can navigate them fairly well but I've not attempted to use the @collection.converter decorator myself, and I see that it isn't present in the test suite test/orm/test_converter.py.    So I'd characterize the level of customization you're doing as possibly not possible right now, unless you'd like to help us get all these codepaths implemented and tested, which would be great, we really could use the help.   But the collection module is tricky to work with beyond the use cases we've documented.

--
You received this message because you are subscribed to the Google Groups "sqlalchemy" group.
To view this discussion on the web visit https://groups.google.com/d/msg/sqlalchemy/-/SuJLukrnfg4J.
To post to this group, send email to sqlal...@googlegroups.com.
To unsubscribe from this group, send email to sqlalchemy+...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/sqlalchemy?hl=en.

[Luca Wehrstedt]

Thanks for the reply.

I started diving into the code to debug the issues and, perhaps, write a patch to fix them and I think I discovered their causes.

The problem with the "link" decorator is the following:

• the "@collection.link"[1] decorator sets the "_sa_instrument_role" of the method to "link";
  
• the "_instrument_class" function correctly recognizes this role and, after doing all its magic, sets the "_sa_link" attribute of the class to be the linker method;
  
• the "link_to_self"[3] and "unlink"[4] methods of "CollectionAdapter" check the "_sa_on_link" attribute for existence and, in case, call it.
  

As you can see the issue is a name mismatch, therefore the solution is to change all names to "_sa_link", to "_sa_on_link" or, my favorite, "_sa_linker", for consistency with the other roles (appender, remover, iterator, converter).

Can you fix it right away or do I have to open a bug and provide a patch on the bug tracker?

The problem with the converter is more subtle. It's caused by the fact that I am deriving my class from MappedCollection, which gets instrumented before my class. Thus, after instrumentation, MappedCollection has two methods marked with "_sa_instrument_role" of "converter": the "_convert" method defined by MappedCollection itself and the "_sa_converter" method added by "_instrument_class". When I define my class I override at most one of these (or possibly none, if I call my converter, for example, "convert"). This causes my class to have many different methods marked as converters and, for some strange reasons, SA seems to always choose the ones I inherit from MappedCollection (and not the one that I define, that never gets called).

Once I discovered the problem the workaround easily followed: I define my converter, called "convert", and then I set "_convert = convert" and "_sa_converter = convert". Sure, this is not an optimal solution, but it only happens when one derives from MappedCollection (not when one creates a new collection from scratch) and fixing it properly would require large changes to the collection instrumentation mechanism and therefore I think it's an acceptable solution. It may be worth to say something about it in the documentation.

Luca

[1] https://bitbucket.org/sqlalchemy/sqlalchemy/src/1a0cfc426ba23842ddf0c6f14ec4b51de98cb7b7/lib/sqlalchemy/orm/collections.py?at=default#cl-422

[2] https://bitbucket.org/sqlalchemy/sqlalchemy/src/1a0cfc426ba23842ddf0c6f14ec4b51de98cb7b7/lib/sqlalchemy/orm/collections.py?at=default#cl-877

[3] https://bitbucket.org/sqlalchemy/sqlalchemy/src/1a0cfc426ba23842ddf0c6f14ec4b51de98cb7b7/lib/sqlalchemy/orm/collections.py?at=default#cl-609

[4] https://bitbucket.org/sqlalchemy/sqlalchemy/src/1a0cfc426ba23842ddf0c6f14ec4b51de98cb7b7/lib/sqlalchemy/orm/collections.py?at=default#cl-615

[Michael Bayer]

On Jan 15, 2013, at 2:58 PM, Luca Wehrstedt wrote:

Thanks for the reply.

I started diving into the code to debug the issues and, perhaps, write a patch to fix them and I think I discovered their causes.

The problem with the "link" decorator is the following:

• the "@collection.link"[1] decorator sets the "_sa_instrument_role" of the method to "link";
  
• the "_instrument_class" function correctly recognizes this role and, after doing all its magic, sets the "_sa_link" attribute of the class to be the linker method;
  
• the "link_to_self"[3] and "unlink"[4] methods of "CollectionAdapter" check the "_sa_on_link" attribute for existence and, in case, call it.
  

As you can see the issue is a name mismatch, therefore the solution is to change all names to "_sa_link", to "_sa_on_link" or, my favorite, "_sa_linker", for consistency with the other roles (appender, remover, iterator, converter).

Can you fix it right away or do I have to open a bug and provide a patch on the bug tracker?

an expedient way to work here is to provide pull requests at https://bitbucket.org/sqlalchemy/sqlalchemy .

The problem with the converter is more subtle. It's caused by the fact that I am deriving my class from MappedCollection, which gets instrumented before my class.

Now that is kind of interesting, because at some point I found a bug related to the fact that MappedCollection was not being instrumented soon enough, so I added some explicit module-level _instrument_class() calls at the bottom of the module (you can see that was ticket 2406).   I was unsure if there was some reason for that deferred instrumentation, and perhaps this was it.  But it's not something that should be relied upon in any case.

Thus, after instrumentation, MappedCollection has two methods marked with "_sa_instrument_role" of "converter": the "_convert" method defined by MappedCollection itself and the "_sa_converter" method added by "_instrument_class". When I define my class I override at most one of these (or possibly none, if I call my converter, for example, "convert"). This causes my class to have many different methods marked as converters and, for some strange reasons, SA seems to always choose the ones I inherit from MappedCollection (and not the one that I define, that never gets called).

Yeah these are not mechanics I'm intimately familiar with.    But there should be a way to fix this.

Once I discovered the problem the workaround easily followed: I define my converter, called "convert", and then I set "_convert = convert" and "_sa_converter = convert". Sure, this is not an optimal solution, but it only happens when one derives from MappedCollection (not when one creates a new collection from scratch) and fixing it properly would require large changes to the collection instrumentation mechanism and therefore I think it's an acceptable solution. It may be worth to say something about it in the documentation.

I'd rather find a way to make it work more naturally without workarounds.   One advantage to the fact that the methods aren't working, is that we can be sure nobody's using them.  So if we need to change the API a bit, that's not a terrible thing.

There probably should be a ticket for all of this regardless of pull requests so feel free to open something up on the tracker.

[Luca Wehrstedt]

Done.

https://bitbucket.org/sqlalchemy/sqlalchemy/pull-request/36/fix-the-collectionlink-decorator

http://www.sqlalchemy.org/trac/ticket/2653

http://www.sqlalchemy.org/trac/ticket/2654