possible bug in InstrumentedAttribute.__delete__?

28 views
Skip to first unread message

lars van gemerden

unread,
Jul 12, 2013, 12:16:19 PM7/12/13
to sqlal...@googlegroups.com
Hi all,

I had an arror in my code and i think i have reconstructed it as follows:

-----------------------------------------------------------------------------------------------

        from sqlalchemy import Column, Integer, String
        from sqlalchemy import create_engine
        from sqlalchemy.orm import sessionmaker
        from sqlalchemy.ext.declarative import declarative_base

        Base = declarative_base()        
        engine = create_engine('sqlite:///:memory:', echo=True)
        Session = sessionmaker(bind=engine)
        
        class User(Base):
            __tablename__ = 'users'
            id = Column(Integer, primary_key=True)
            name = Column(String)
            
        Base.metadata.create_all(engine) 
            
        session = Session()
        user = User(name = 'bob')
        session.add(user)
        session.commit()
        #user.name
        del user.name #error in sqlalchemy file attributes.py line 529
        #user.name
        session.commit()
        assert user.name == None #error: user.name is still 'bob'

------------------------------------------------------------------------------------------------------------

These two errors do not occur if i access the attributes before the delete or the commit (i.e. uncomment the #user.name lines).

I am using version 7.5; are these errors solved by the latest version?

 I would like to avoid upgrading at this point, but if i could be reasonably sure that upgrading solves the problem, then no problem ..

Cheers, Lars


Michael Bayer

unread,
Jul 12, 2013, 12:28:29 PM7/12/13
to sqlal...@googlegroups.com
We've never supported "del obj.attrname" as a means of setting an attribute to None (which translates to NULL in SQL).   Setting the value to None explicitly is preferred.

--
You received this message because you are subscribed to the Google Groups "sqlalchemy" group.
To unsubscribe from this group and stop receiving emails from it, send an email to sqlalchemy+...@googlegroups.com.
To post to this group, send email to sqlal...@googlegroups.com.
Visit this group at http://groups.google.com/group/sqlalchemy.
For more options, visit https://groups.google.com/groups/opt_out.
 
 

Lars van Gemerden

unread,
Jul 12, 2013, 5:36:55 PM7/12/13
to sqlal...@googlegroups.com
Can i just override __delattr__ in a subclass of a declarative base to achieve this anyway?

Cheers, Lars

====================================
Lars van Gemerden
+31 6 26 88 55 39
====================================
You received this message because you are subscribed to a topic in the Google Groups "sqlalchemy" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/sqlalchemy/TRUmFTaDCF8/unsubscribe.
To unsubscribe from this group and all its topics, send an email to sqlalchemy+...@googlegroups.com.

Lars van Gemerden

unread,
Jul 12, 2013, 6:11:02 PM7/12/13
to sqlal...@googlegroups.com
After thinking on it some more, should InstrumentedAttribute.__delete__ even exist then, or maybe raise a NotImplementedError?

CL

====================================
Lars van Gemerden
+31 6 26 88 55 39
====================================

On 12 jul. 2013, at 18:28, Michael Bayer <mik...@zzzcomputing.com> wrote:

You received this message because you are subscribed to a topic in the Google Groups "sqlalchemy" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/sqlalchemy/TRUmFTaDCF8/unsubscribe.
To unsubscribe from this group and all its topics, send an email to sqlalchemy+...@googlegroups.com.

Michael Bayer

unread,
Jul 12, 2013, 7:52:24 PM7/12/13
to sqlal...@googlegroups.com
There's all kinds of things that __delete__ should possibly do, most likely just do what you expect, i.e. set the value to NULL, but this would be changing its behavior in a very backwards-incompatible way; such a change could only be considered for 0.9.    

Whats not clear is how exactly an entire application was built around 0.7, relying on "del" in some way such that this can no longer be changed, and suddenly it's an issue?  doesn't make sense, I don't see why you can't just get replace those "del" statements on your end.  It's not supported, and if it were, it would not be in 0.7 or 0.8.

Lars van Gemerden

unread,
Jul 13, 2013, 10:24:43 AM7/13/13
to sqlal...@googlegroups.com
I think i didn't explain the error enough; just calling del user.name in the example below causes an exception (except when i already accessed (say print user.name) the attribute, which seemed odd to me). This is independent of whether the attr becomes None. 

Otherwise it isn't a real problem in my code, just something i ran into while cleaning up some internal api, as you say i can easily work around it.

Cheers, Lars



====================================
Lars van Gemerden
+31 6 26 88 55 39
====================================

Michael Bayer

unread,
Jul 13, 2013, 2:02:06 PM7/13/13
to sqlal...@googlegroups.com

On Jul 13, 2013, at 10:24 AM, Lars van Gemerden <la...@rational-it.com> wrote:

> I think i didn't explain the error enough; just calling del user.name in the example below causes an exception (except when i already accessed (say print user.name) the attribute, which seemed odd to me). This is independent of whether the attr becomes None.
>
> Otherwise it isn't a real problem in my code, just something i ran into while cleaning up some internal api, as you say i can easily work around it.

Great then, yes we don't do a good job with "del" but there is a logic to it which I can't be 100% sure some apps aren't relying upon. I can possibly make it behave intuitively for 0.9, I guess it should mean, "set the attribute to None" - this because as you might have noticed an instrumented column attribute defaults itself to "None" when first accessed.

Lars van Gemerden

unread,
Jul 13, 2013, 3:21:17 PM7/13/13
to sqlal...@googlegroups.com
Just trying to help; i am not sure, but what you might also consider is to initialise attributes (or on first access) with a default if a default value as Column argument is given (i am already doing this in my own code) and also reset to this default in case of del, but maybe there are disadvantages.

Cheers, Lars



====================================
Lars van Gemerden
la...@rational-it.com
+31 6 26 88 55 39
====================================

Michael Bayer

unread,
Jul 13, 2013, 4:59:08 PM7/13/13
to sqlal...@googlegroups.com

On Jul 13, 2013, at 3:21 PM, Lars van Gemerden <la...@rational-it.com> wrote:

> Just trying to help; i am not sure, but what you might also consider is to initialise attributes (or on first access) with a default if a default value as Column argument is given (i am already doing this in my own code) and also reset to this default in case of del, but maybe there are disadvantages.

I can see why that might be reasonable, but at the same time the "default" for Column is at the level of Core; the "default" could be a Python callable, and this callable could be relying upon the fact that it is invoked within the context of executing the INSERT statement. By pulling out the Core-level "default" upon attribute access and just populating at the ORM level means we bypass the Core in doing its normal job with "default", which violates ..... there's a term for this I can't recall, but the idea is, "A does job X using mechanism P" therefore if, based on some very non-deterministic system, once in awhile "B does job X using mechanism Q" instead, it makes the code much harder to understand and debug. So to really do it this way, the contract of "default" in terms of the ORM would need to be modified completely, such that *all* Column "default" args are applied at the ORM level. But even then, your Column() is still behaving in two different ways based on if you use Session.add() or engine.execute(insert()).

The original intent of "default" for Column is that it acts exactly like "server_default", except it runs on the Python side for the case that your schema wasn't created with this default, or the functionality of your default is not supported by the database. But otherwise, just like a "server_default", the value is None on the Python side until the INSERT occurs.

Really, a better solution here would be an ORM-level "default", that's what you've implemented anyway.


Wichert Akkerman

unread,
Jul 14, 2013, 4:49:40 AM7/14/13
to sqlal...@googlegroups.com

On Jul 13, 2013, at 22:59, Michael Bayer <mik...@zzzcomputing.com> wrote:
> The original intent of "default" for Column is that it acts exactly like "server_default", except it runs on the Python side for the case that your schema wasn't created with this default, or the functionality of your default is not supported by the database. But otherwise, just like a "server_default", the value is None on the Python side until the INSERT occurs.

For what it's worth I forget that all the time and end up flushing objects out just to get the default values right.

> Really, a better solution here would be an ORM-level "default", that's what you've implemented anyway.

In order to keep things pythonic I wonder if this should just be left as default arguments for the constructor. If everyone would do this:

class Foo(Base):
start_end = sa.Column(sa.Timestamp())

def __init__(self, start_end=datetime(2001, 2, 3), **kw):
Base.__init__(self, start_end. **kw)

This is standard python and easy to read. For association tables you already need to do this, either this way or by supplying a factory function, so the step to doing it for other objects is not that big. The downside is that the default value is now at a different place than the column definition, but it does make it very clear that this operates at the Python object level instead of the core SQL level.

Wichert.


lars van gemerden

unread,
Jul 14, 2013, 5:09:41 AM7/14/13
to sqlal...@googlegroups.com
Now i was doing both, but you're right, i can remove the default from the column definition, and just initialise the object with the default in __init__ (from metadata i created elsewhere, but will remain available). Setting the default ASAP (on object creation) seems like the way of least surprise and it's not hard to do, as Wichert mentioned.

Thanks again, Michael, my code improves ...

Lars
Reply all
Reply to author
Forward
0 new messages