Warning: Can't synchronize with repository "(default)" (Unsupported version control system "svn": No module named svn). Look in the Trac log for more information.

Ticket #1185 (closed defect: fixed)

Opened 11 years ago

Last modified 7 years ago

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

Reported by: sanjay Owned by: anonymous
Priority: normal Milestone: 1.0.x bugfix
Component: SQLAlchemy Version: 1.0b1
Severity: normal Keywords:
Cc:

Description

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.

Attachments

demoexc.tar.gz Download (88.2 KB) - added by sanjay 11 years ago.
trans.patch Download (450 bytes) - added by alberto 11 years ago.
demoexc.tar.2.gz Download (87.3 KB) - added by alberto 11 years ago.

Change History

Changed 11 years ago by sanjay

comment:1 Changed 11 years ago by 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

Changed 11 years ago by alberto

Changed 11 years ago by alberto

comment:2 Changed 11 years ago by 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?
  1. 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

comment:3 Changed 11 years ago by sanjay

thanks sanjay

I meant

thanks

sanjay

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

comment:4 Changed 11 years ago by 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.

  1. 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

comment:5 Changed 11 years ago by jorge.vargas

  • Component changed from unassigned to Docs
  • Milestone set to 1.1

comment:6 Changed 11 years ago by alberto

  • Milestone changed from 1.1 to __unclassified__

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

comment:7 Changed 10 years ago by paj

  • Component changed from Docs to SQLAlchemy

comment:8 Changed 10 years ago by paj

  • Status changed from new to closed
  • Resolution set to fixed

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]

comment:9 Changed 7 years ago by chrisz

  • Milestone changed from __unclassified__ to 1.0.x bugfix
Note: See TracTickets for help on using tickets.