[sqlalchemy] Re: Feature request: Session.get_local()
A patch containing tests and cleaned up identity key is attached.
~ Daniel
Michael Bayer wrote:
committed, r2409.
the row needs to have a class and entity_name present to determine
what mapper you want to use to extract from the row, so i put those as
keyword arguments for now.
also, I notice the usage of plain old assert for argument checking.
should we make this change across the board and get rid of
exceptions.ArgumentError ? i feel like we have to go one way or the
other with that.
also i didnt do any testing of this, we might want to add some tests to
test/orm/session.py .
On Mar 12, 2007, at 9:21 PM, Daniel Miller wrote:
def identity_key(self, *args, **kwargs):
Get an identity key
Valid call signatures:
identity_key(class_, ident, entity_name=None)
class_ - mapped class
ident - primary key, if the key is composite this is a tuple
entity_name - optional entity name. May be given as a
positional arg or as a keyword arg.
identity_key(instance=instance)
instance - object instance (must be given as a keyword arg)
identity_key(row=row)
row - result proxy row (must be given as a keyword arg)
if args:
kw = {}
if len(args) == 2:
class_, ident = args
entity_name = kwargs.pop(entity_name, None)
assert not kwargs, (unknown keyword arguments: %s
% (kwargs.keys(),))
else:
assert len(args) == 3, (two or three positional args are
accepted, got %s % len(args))
class_, ident, entity_name = args
mapper = _class_mapper(class_, entity_name=entity_name)
return mapper.instance_key_from_primary_key(ident,
entity_name=entity_name)
else:
try:
instance = kwargs.pop(instance)
except KeyError:
row = kwargs.pop(row)
assert not kwargs, (unknown keyword arguments: %s
% (kwargs.keys(),))
mapper = # not sure how to get the mapper form a row
return mapper.identity_key_from_row(row)
else:
assert not kwargs, (unknown keyword arguments: %s
% (kwargs.keys(),))
mapper = _object_mapper(instance)
return mapper.identity_key_from_instance(instance)
--~--~-~--~~~---~--~~
You received this message because you are subscribed to the Google Groups
sqlalchemy group.
To post to this group, send email to sqlalchemy@googlegroups.com
To unsubscribe from this group, send email to [EMAIL PROTECTED]
For more options, visit this group at
http://groups.google.com/group/sqlalchemy?hl=en
-~--~~~~--~~--~--~---
Index: lib/sqlalchemy/orm/session.py
===
--- lib/sqlalchemy/orm/session.py (revision 2432)
+++ lib/sqlalchemy/orm/session.py (working copy)
@@ -452,54 +452,57 @@
identity_key(class\_, ident, entity_name=None)
class\_
-mapped class
-
+mapped class (must be a positional argument)
+
ident
primary key, if the key is composite this is a tuple
entity_name
-optional entity name. May be given as a
-positional arg or as a keyword arg.
+optional entity name
identity_key(instance=instance)
instance
object instance (must be given as a keyword arg)
-identity_key(row=row, class=class\_, entity_name=None)
+identity_key(class\_, row=row, entity_name=None)
+class\_
+mapped class (must be a positional argument)
+
row
result proxy row (must be given as a keyword arg)
-
+
+entity_name
+optional entity name (must be given as a keyword arg)
if args:
-kw = {}
-if len(args) == 2:
+if len(args) == 1:
+class_ = args[0]
+try:
+row = kwargs.pop(row)
+except KeyError:
+ident = kwargs.pop(ident)
+entity_name = kwargs.pop(entity_name, None)
+elif len(args) == 2:
class_, ident = args
entity_name = kwargs.pop(entity_name, None)
-assert not kwargs, (unknown keyword arguments: %s
-% (kwargs.keys(),))
+elif len(args) == 3:
+class_, ident, entity_name = args
else:
-assert len(args) == 3, (two or three positional args are
-
[sqlalchemy] Re: Feature request: Session.get_local()
Michael Bayer wrote:
id rather not have people needing to deal with an actual identity
key tuple most of the time. they should be able to say
session.identity_map.something(class, pk) and get an instance out of
it.
What's the use of exposing the identity map if you don't want people to deal
with the keys with which it's indexed? You might as well expose an
identity_set, except that would be really unhandy as it would be expensive or
impossible to do the exact kind of find that I'm asking for.
the reason for those big names on mapper is because all of them are
used, we need identity keys from instances, rows, primary keys, all of
it, and i was myself getting confused since their names were not
clear...so i changed the names to be absolutely clear. but also, i
dont want people generally dealing with the methods off of
Mappers...all the methods you need should be off of Session and
Query. so maybe we can put a keyword-oriented identity_key method
on Session, or something (identity_key(primary_key=None,
instance=None, row=None, etc)). or maybe it can use some kind of
multiple-dispatch that detects scalar, tuple, object instance,
ResultProxy. but also, i dont think the get an SA identity key step
as an interim step to accomplishing something else should really be
needed in the usual case.
If the session got an identity_key() method that would be great (and eliminate
the need for a find method). Here's an implementation:
def identity_key(self, *args, **kwargs):
Get an identity key
Valid call signatures:
identity_key(class_, ident, entity_name=None)
class_ - mapped class
ident - primary key, if the key is composite this is a tuple
entity_name - optional entity name. May be given as a
positional arg or as a keyword arg.
identity_key(instance=instance)
instance - object instance (must be given as a keyword arg)
identity_key(row=row)
row - result proxy row (must be given as a keyword arg)
if args:
kw = {}
if len(args) == 2:
class_, ident = args
entity_name = kwargs.pop(entity_name, None)
assert not kwargs, (unknown keyword arguments: %s
% (kwargs.keys(),))
else:
assert len(args) == 3, (two or three positional args are
accepted, got %s % len(args))
class_, ident, entity_name = args
mapper = _class_mapper(class_, entity_name=entity_name)
return mapper.instance_key_from_primary_key(ident,
entity_name=entity_name)
else:
try:
instance = kwargs.pop(instance)
except KeyError:
row = kwargs.pop(row)
assert not kwargs, (unknown keyword arguments: %s
% (kwargs.keys(),))
mapper = # not sure how to get the mapper form a row
return mapper.identity_key_from_row(row)
else:
assert not kwargs, (unknown keyword arguments: %s
% (kwargs.keys(),))
mapper = _object_mapper(instance)
return mapper.identity_key_from_instance(instance)
Note that I didn't implement the part to get a mapper from a row because I'm
not sure how to do that. I imagine that's trivial though.
~ Daniel
On Mar 10, 8:27 pm, Daniel Miller [EMAIL PROTECTED] wrote:
Michael Bayer wrote:
my only concern is that you now have more than one way to do it. i
need to deal with things in the identity map. do i go look at the
session.identity_map ? (which is documented, its part of the public
API) oh no, i dont have the exact kind of key to use, now i have to
go use a method called find() (which again, does that mean, find it
in the database ? where is it looking ?)
These are good points. Maybe the problem is in my brain--I've always had a
disconnect between the session.identity_map and the mapper.identity_key()
function. I guess it's clearly documented that they are compatible and can
be used like this:
key = MyClass.mapper.identity_key(pk_value)
itm = session.identity_map.get(key)
It just seemed like that was digging a bit too deep into what I thought were
implementation details of the mapper and the session. If those things (i.e.
mapper.identity_key and session.identity_map) are clearly documented as part
of the interface of SA, and they are meant to work together like that then
maybe this proposal isn't even necessary. After all, it's just two lines
instead of one. However, upon looking at the documentation, this is all I
find on the identity_key method of the Mapper class:
def identity_key(self, primary_key)
deprecated. a synonym for identity_key_from_primary_key.
Now I thought identity_key was OK (if a bit obscure due to lack of
documentation), but identity_key_from_primary_key is not so great IMHO. This
is not a method name that will come to mind when I'm trying to get the
identity key of a given
[sqlalchemy] Re: Feature request: Session.get_local()
Michael Bayer wrote: my only concern is that you now have more than one way to do it. i need to deal with things in the identity map. do i go look at the session.identity_map ? (which is documented, its part of the public API) oh no, i dont have the exact kind of key to use, now i have to go use a method called find() (which again, does that mean, find it in the database ? where is it looking ?) These are good points. Maybe the problem is in my brain--I've always had a disconnect between the session.identity_map and the mapper.identity_key() function. I guess it's clearly documented that they are compatible and can be used like this: key = MyClass.mapper.identity_key(pk_value) itm = session.identity_map.get(key) It just seemed like that was digging a bit too deep into what I thought were implementation details of the mapper and the session. If those things (i.e. mapper.identity_key and session.identity_map) are clearly documented as part of the interface of SA, and they are meant to work together like that then maybe this proposal isn't even necessary. After all, it's just two lines instead of one. However, upon looking at the documentation, this is all I find on the identity_key method of the Mapper class: def identity_key(self, primary_key) deprecated. a synonym for identity_key_from_primary_key. Now I thought identity_key was OK (if a bit obscure due to lack of documentation), but identity_key_from_primary_key is not so great IMHO. This is not a method name that will come to mind when I'm trying to get the identity key of a given object. It's just too long. Would it be OK to un-deprecate identity_key and just state clearly in the documentation that it requires a primary key as it's argument? Change it like this: def identity_key(self, pk=None, instance=None) Return the identity key given a primary key OR an instance Either the pk or instance keyword argument must be supplied. An error will be raised if both instance and pk are given or if both are None. Note that this is backward-compatible with the previous version of identity_key, which took a primary key as its first argument. Then do this: identity_key_from_primary_key - deprecated identity_key_from_instance - deprecated Finally, we also need to clearly document in the section that talks about the identity_map that the keys used in that map may be obtained directly from the mapper of the object by using mapper.identity_key(). If those things were cleared up I would see no reason why we need a session.get_local() or session.find() or whatever... And we have one clear way to do it. What do you think of this? ~ Daniel --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups sqlalchemy group. To post to this group, send email to sqlalchemy@googlegroups.com To unsubscribe from this group, send email to [EMAIL PROTECTED] For more options, visit this group at http://groups.google.com/group/sqlalchemy?hl=en -~--~~~~--~~--~--~---
[sqlalchemy] Re: Feature request: Session.get_local()
my only concern is that you now have more than one way to do it. i need to deal with things in the identity map. do i go look at the session.identity_map ? (which is documented, its part of the public API) oh no, i dont have the exact kind of key to use, now i have to go use a method called find() (which again, does that mean, find it in the database ? where is it looking ?) the identity_map accessor on Session is pretty much only used by the public API so yes we'd make a wrapper around the underlying dictionary/ weak dictionary. i think its more pythonic to use property-style access rather than methods (which in javaland feels very dangerous...but in python, its not ! really ! heres the article that convinced me http://dirtsimple.org/2004/12/python-is-not-java.html ). On Mar 8, 8:13 pm, Daniel Miller [EMAIL PROTECTED] wrote: Michael Bayer wrote: fine, but the name get_local is not so descriptive and also attracts the focus away from the existing identity_map accessor. how about we jack up the identity_map dictionary to be smarter ? so you could say: session.identity_map.find(class_, ident, entity_name='foo') Are you going to make the identity_map a subclass of dict then? What about the weak_identity_map option--will there be a subclass of weakref.WeakValueDictionary too? That smells a bit. I guess the right way to do it is to make a wrapper around the real identity_map, but then you're starting to add more overhead to accessing the identity_map... Your suggestion is fine from a functional point of view, it just seems to add more complexity than it's worth (i.e. it would require an entire subclass (or two) or a wrapper instead of just a single function). The identity_map has always seemed like more of an implementation detail than an exposed part of the session interface (to me at least). The Session class is a facade to the UnitOfWork/identity_map. When I start to write code that depends heavily on those inner parts of the session I feel a bit nervous--like I'm writing fragile code. The other thing is documenting this in a way that people will be able to easily find it. The first place I would go to look for a feature like this is on the session--identity_map has always had the semantics of a plain dict. Would it be OK to name it session.find() and clearly document it's behavior with the other similar functions like session.get() and session.load()? The only possible issue is that it definitely does does NOT belong on Query unlike get() and load(). I expect it will be used more with expire() and expunge() so maybe that doesn't matter since neither of those methods are part of the query interface either. I don't really care, I'm just giving the first thoughts that come to mind here. ~ Daniel On Mar 8, 10:07 am, Daniel [EMAIL PROTECTED] wrote: Could we add this new method on sqlalchemy.orm.Session? def get_local(self, class_, ident, entity_name=None): Get an object from the identity map (do not hit the database) Returns None if the object does not exist in this session's identity map. idkey = self.mapper(class_, entity_name=entity_name).identity_key(ident) return self.identity_map.get(idkey) This is handy for doing tasks such as expiring/expunging/etc. an instance but only if it exists in the session. I'm not sure if it makes sense to have the entity_name parameter because I've never used it before, but I added it just in case. Thanks, ~ Daniel --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups sqlalchemy group. To post to this group, send email to sqlalchemy@googlegroups.com To unsubscribe from this group, send email to [EMAIL PROTECTED] For more options, visit this group at http://groups.google.com/group/sqlalchemy?hl=en -~--~~~~--~~--~--~---
[sqlalchemy] Re: Feature request: Session.get_local()
fine, but the name get_local is not so descriptive and also attracts the focus away from the existing identity_map accessor. how about we jack up the identity_map dictionary to be smarter ? so you could say: session.identity_map.find(class_, ident, entity_name='foo') On Mar 8, 10:07 am, Daniel [EMAIL PROTECTED] wrote: Could we add this new method on sqlalchemy.orm.Session? def get_local(self, class_, ident, entity_name=None): Get an object from the identity map (do not hit the database) Returns None if the object does not exist in this session's identity map. idkey = self.mapper(class_, entity_name=entity_name).identity_key(ident) return self.identity_map.get(idkey) This is handy for doing tasks such as expiring/expunging/etc. an instance but only if it exists in the session. I'm not sure if it makes sense to have the entity_name parameter because I've never used it before, but I added it just in case. Thanks, ~ Daniel --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups sqlalchemy group. To post to this group, send email to sqlalchemy@googlegroups.com To unsubscribe from this group, send email to [EMAIL PROTECTED] For more options, visit this group at http://groups.google.com/group/sqlalchemy?hl=en -~--~~~~--~~--~--~---
