controller method trying to flush the database session(sqlalchemy) even if exception occours

[sanjay]

The problem is discussed at  http://groups.google.co.in/group/turbogears/browse_thread/thread/3fd38e7c12bd5e2a/8d2dac787cd05a9b?lnk=gst&q=sanjay&rnum=1&hl=en#8d2dac787cd05a9b

Attaching a sample project depicting the issue. Database uri has to be set in dev.cfg, and database has to be created by tg-admin sql create.

Needs sqlalchemy.

[alberto]

This is related to the auto-saving behavior of  assign_mapper and  sessioncontext.

Let me explain:

When you create an instance of a mapped class which is handled by assignmapper it is automatically attached to the current session on construction. So, even if an exception occurs afterwards (in assignName) the object is already in the session.

The exception is raised, catched by errorhandling and execution branches to index where it is displayed *and* the session finally flushed (as no exceptions occur inside index).

There are 3 solutions possible (that I can think of ATM):

1. Clearing the session inside index() if an error or exception occurs so when it's flushed nothing is saved.
2. Don't use activemapper and manually save "person" in the session if assignName succeds.
3. re-raise tg_exceptions inside index() so the transaction is rolled-back

1. is probably what you want, I believe. See attached files to see how to implement it

There's room for improvent in TG in this regard I believe... Probably when an exception is trapped by errorhandling a flag should be set in the request to inform run_with_transaction (the wrapper that expose uses to begin/end transactions) of the fact so it rolls back (if execution has branched by errorhandling). I'm not sure, however, if this might cause unwanted side-effects as someone might be depending on this behaviour to recover from the exception in the excpetion handler and flushing anyway. Probably not a change we should make in 1.0bX due it's risks IMO.

I believe that for the time-being your best bet is to explicitly clear the session if tg_excpetions is not None.

We could also make the transaction object available at the request so you can manually roll it back instead of clearing the session (looks cleaner and more explicit to me).

def index(self, ...):
    if tg_excpetions is not None:
        cherrypy.request.tg_sa_transaction.rollback()
        ...

Try the attached patch see if it helps and if so I can commit it (I can't think of any imaginable way it could break existing (users) code (apart from the obvious non-probable name clash of tg_sa_transaction ;) ).

Alberto

[sanjay]

Hi Alberto,

Thanks a lot for the elaborative explaination and the patch.

I tried session.clear() like this:

def index(self, ...):
    if tg_excpetions:
        flash("Exception: " + str(tg_exceptions))
        session.clear()
        cherrypy.request.my_exc = tg_exceptions

It worked perfectly.

I have two layman doubts, though:

1. Will not clearing the session affect other requests, i.e. is session not shared with other requests?

2. Could not make out the use of adding this line: cherrypy.request.my_exc = tg_exceptions

Then, after applying the patch, while I tried

def index(self, ...):
    if tg_excpetions:
        flash("Exception: " + str(tg_exceptions))
        cherrypy.request.tg_sa_transaction.rollback()

it did not seem working. The behaviour remained unchanged.

thanks sanjay

[sanjay]

thanks sanjay

I meant

thanks

sanjay

(my unfamiliarity with trac put both words in the same line!)

[alberto]

1. Will not clearing the session affect other requests, i.e. is session not shared with other requests?

Nope. The session is wrapped by SessionContext? which gives each thread it's own session.

2. Could not make out the use of adding this line: cherrypy.request.my_exc = tg_exceptions

Take a look at tests/test_controllers.py. I used it to check if the exception raised was the expected one

... it did not seem working. The behaviour remained unchanged.

Hmmm. I'll need to take a closer look at what is exactly happening when I get a chance...

Alberto

[jorge.vargas]

I put a request for this on  http://docs.turbogears.org/1.0/RoughDocs/DocumentationWishList

[alberto]

Batch moved into unclassified from 1.1 to properly track progress on the later

[paj]

I looked into this some more, and someone had already had a go, I guess some time previously. I hadn't noticed it before as it's in errorhandling.py. So this isn't a behaviour change after all. I fixed up the previous attempt a little, so you can now use the database in an exception handler, and I've added a unit test.

This is different behaviour to SQLObject, but I suggest leaving the SO code as it is, for compatibility.

Committed in [3432] and [3433]