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 #1452 (closed defect: fixed)

Opened 12 years ago

Last modified 11 years ago

Cannot change user data with SA in TG 1.0.3

Reported by: chrisz Owned by: anonymous
Priority: high Milestone: 1.0.4
Component: SQLAlchemy Version: 1.0.3
Severity: blocker Keywords: SA identity session
Cc:

Description

Sometimes you need to change attributes of the current user in the database. For instance, assume a user 'bob' wants to change his display name to 'Robert'.

This worked nicely with TG < 1.0.3, but after a change in r3290 (see ticket #1419), this stopped working with SQLAlchemy.

Here is how you can reproduce the problem:

Quickstart a dummy project:

tg-admin quickstart foo --sqlalchemy --identity
cd foo
tg-admin sql create

Create a user:

tg-admin shell
User(user_name='bob',password='bob',
email_address='bob',display_name='bob')
^Z
Do you wish to commit? yes

Change the index method in controller.py like this:

    @expose(template="foo.templates.welcome")
    @identity.require(identity.not_anonymous())
    def index(self):
        user = identity.current.user
        display_name = user.display_name
        user.display_name='Robert'
        return dict(now='User = ' + display_name)

Start the application with start-foo.py, go to  http://localhost:8080 and log in as "bob/bob".

You will see "Welcome Robert" in the first line, but further down, where normally the date is displayed, you see "User = bob" instead of "Robert". This is ok so far. However, when you reload, it should become "Robert" in both places. But this does not happen with TG 1.0.3 (or 1.0.3.2); the display name in the database is never updated. Even if you intersperse the code with session.flush() ad nauseam.

If you use TG < 1.0.3 or SQLObject instead of SQLAlchemy, everything works as expected.

It seems we need to reconsider ticket #1419.

Attachments

idmap3.patch Download (683 bytes) - added by chrisz 11 years ago.
Alternative patch for #1419

Change History

comment:1 Changed 12 years ago by paj

  • Version changed from 1.0.2 to trunk

Oh dear, and there was me hoping #1419 would be a simple fix! I think the change in #1419 is needed, but it has shown up that there is a problem in how it interacts with identity. I will have a go at reproducing your problem and preparing a fix for that.

comment:2 Changed 12 years ago by paj

  • Version changed from trunk to 1.0.3
  • Milestone changed from 1.1 to 1.0.4

comment:3 Changed 12 years ago by paj

Ok, the per-request identity hook is called from the visit plugin, which is a CherryPy? filter. So it is called before we hit the @expose decorator, and before we get to sa_rwt. So when my change deletes the SA session, it disconnects the user object from the database.

The root cause of these problems is the ad-hoc mix of filters and decorators. This is a long-acknowledged problem with TG and not something we're aiming to solve before TG2.

However, we can put some band-aid on this. I'll try moving the delete session until the end of the request.

comment:4 Changed 12 years ago by chrisz

Yes, I hope in TG2 things will be more easy to see through. Currently, there are so many hardly documented mechanisms involved (objectstore, sessioncontext, filters, decorators, sa's unit of work paradigm, ...) so it's really difficult to understand what's going on. Plus, there is a lack of unit tests for TG+SA (see issue #1453). There should be tests for both issues #1419 and #1452.

Would it be a solution to put it the del session.context.current in EndTransactionsFilter after or instead of the session.clear()? At least this would solve my problem above, but I'm not sure whether it solves the issue in #1419 (see my comment there).

Changed 11 years ago by chrisz

Alternative patch for #1419

comment:5 Changed 11 years ago by chrisz

  • Severity changed from major to blocker

comment:6 Changed 11 years ago by tlg

The patch fixes the problem for me, but this is a real blocker. Can I expect a release soonish, or do I have to roll my own patched version (arghhh)?

comment:7 Changed 11 years ago by chrisz

This should be fixed in the next release (1.0.4/1.1), scheduled for September 1st.

comment:8 Changed 11 years ago by paj

  • Priority changed from normal to high

comment:9 Changed 11 years ago by paj

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

The fix for #1419 was never needed; it has been reverted and this should fix this ticket. I had a go at writing a unit test for this issue, but there are a few challenges with unit testing visit/identity (and no existing tests) and that's more work than is worth it at present.

Note: See TracTickets for help on using tickets.